Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only work-steal in the main loop #12

Open
wants to merge 1 commit into
base: rustc
Choose a base branch
from

Conversation

Zoxc
Copy link

@Zoxc Zoxc commented Aug 26, 2023

This PR removes work stealing from other threads, except when in the main loop. This avoids situations where we steal work we can't complete due to waiting on queries. This should fix bugs like rust-lang/rust#111521 and rust-lang/rust#115223, but it does have a negative performance impact.

Tests for 6 threads:

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check0.6502s0.7641s💔 17.52%
🟣 hyper:check0.1396s0.1508s💔 8.00%
🟣 regex:check0.4033s0.4413s💔 9.43%
🟣 syn:check0.6749s0.7749s💔 14.81%
🟣 syntex_syntax:check2.1708s2.4696s💔 13.76%
Total4.0389s4.6007s💔 13.91%
Summary1.0000s1.1270s💔 12.70%
BenchmarkBeforeAfter
TimeTime%
🟠 clap:debug2.3400s2.4645s💔 5.32%
🟠 hyper:debug0.3807s0.3919s💔 2.96%
🟠 regex:debug1.7557s1.7719s 0.92%
🟠 syn:debug1.7401s1.8426s💔 5.89%
🟠 syntex_syntax:debug6.6743s6.9144s💔 3.60%
Total12.8908s13.3854s💔 3.84%
Summary1.0000s1.0374s💔 3.74%

@SparrowLii
Copy link
Member

SparrowLii commented Aug 26, 2023

The regression looks regrettable, and it will probably prevent us from landing parallel rustc.

I would like us to decide whether to work-stealing by querying dependencies instead of prohibiting it in inner loops.

I hope we can left this change and try other implementations first.

@Zoxc Zoxc force-pushed the less-work-steal branch 2 times, most recently from 0481924 to a0a8c45 Compare August 27, 2023 17:40
@Zoxc Zoxc changed the title Only work-steal in the main loop [WIP] Only work-steal in the main loop Aug 28, 2023
@Zoxc Zoxc force-pushed the less-work-steal branch from a0a8c45 to b72936d Compare August 28, 2023 04:06
@Zoxc
Copy link
Author

Zoxc commented Jan 18, 2024

By increasing the thread count we can increase the work stealing done and thus reduce the regression:

BenchmarkBefore PR (6 threads ,-j6)After PR (12 threads ,-j6)
TimeTime%
🟣 clap:check0.6556s0.6647s💔 1.38%
🟣 hyper:check0.1485s0.1519s💔 2.32%
🟣 regex:check0.4050s0.4126s💔 1.89%
🟣 syn:check0.6795s0.6895s💔 1.47%
🟣 syntex_syntax:check2.1858s2.2038s 0.82%
Total4.0743s4.1224s💔 1.18%
Summary1.0000s1.0158s💔 1.58%

@Zoxc Zoxc changed the title [WIP] Only work-steal in the main loop Only work-steal in the main loop Jan 19, 2024
@Zoxc
Copy link
Author

Zoxc commented Jan 19, 2024

@cuviper Would you be up for reviewing this?

@zetanumbers
Copy link

Hello! Do you have any tips on currently triggering those bugs?

@cuviper
Copy link
Member

cuviper commented Aug 21, 2024

I haven't had time to review this in depth -- but seeing how many deep areas it needs to change, along with existing patches diverging from rayon, I'm starting to think that we would be better off with a bespoke rustc_threadpool that doesn't try to be a modified rayon. Most of rayon's API is not used at all in rustc #[cfg(parallel_compiler)], and I think a lot of rustc-rayon's customization would be easier under a more limited API.

@zetanumbers
Copy link

I haven't had time to review this in depth -- but seeing how many deep areas it needs to change, along with existing patches diverging from rayon, I'm starting to think that we would be better off with a bespoke rustc_threadpool that doesn't try to be a modified rayon. Most of rayon's API is not used at all in rustc #[cfg(parallel_compiler)], and I think a lot of rustc-rayon's customization would be easier under a more limited API.

I am currently working on this and other parallelization issues.

@zetanumbers
Copy link

By increasing the thread count we can increase the work stealing done and thus reduce the regression

@Zoxc wouldn't that mess up with the jobserver? It raises the limit of number cores used in parallel.

@Zoxc
Copy link
Author

Zoxc commented Sep 21, 2024

I'm starting to think that we would be better off with a bespoke rustc_threadpool that doesn't try to be a modified rayon.

Yeah, this does seem to imply a move away from tracking Rayon. This approach doesn't match well with Rayon iterators and we may want to introduce fibers too. I'd like to see some more extensive testing to verify this solves the deadlock issues before moving further in this direction though.

@Zoxc wouldn't that mess up with the jobserver? It raises the limit of number cores used in parallel.

Not sure what you mean here? The jobserver would still limit active threads across cores as before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants