Page MenuHomePhabricator

copies-rust: move CPU-heavy Rust processing into a child thread
ClosedPublic

Authored by SimonSapin on Jan 6 2021, 9:12 AM.

Details

Summary

… that runs in parallel with the parent thread fetching data.
This can be disabled through a new config. CLI example:

hg --config=devel.copy-tracing.multi-thread=no

For now both threads use the GIL, later commits will reduce this.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Jan 6 2021, 9:12 AM
Alphare added inline comments.
rust/hg-cpython/src/copy_tracing.rs
54

This is a better comment, I almost said something on the last commit

78

This will not honor the RAYON_NUM_THREADS environment variable that we use to contain the number of threads in tests, but also technically for users as an implementation detail. Should we use rayon threads here so as to have the same threadpool everywhere?

SimonSapin added inline comments.Jan 22 2021, 11:48 AM
rust/hg-cpython/src/copy_tracing.rs
54

This was a pre-existing comment, the previous commit only moved it around. But yeah I could have included this change in the previous commit.

78

I feel that starting a pool of multiple thread to run a single task on it and leaving it running until process exit is counter-productive compared to the one thread started and then stopped here. Unless combine_changeset_copies_wrapper itself is called with high concurrency from many Python threads (which would contend the GIL) we’re not gonna have many of these Rust threads.

Alphare accepted this revision.Jan 24 2021, 12:23 AM
baymax updated this revision to Diff 25704.Feb 22 2021, 9:25 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25741.Feb 22 2021, 11:00 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25786.Feb 22 2021, 3:47 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.