Skip to content

Only work-steal in the main loop for rustc_thread_pool #143035

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ywxt
Copy link

@ywxt ywxt commented Jun 26, 2025

This PR is a replica of rust-lang/rustc-rayon#12 that only retained work-steal in the main loop for rustc_thread_pool.

r? @oli-obk

cc @SparrowLii @Zoxc @cuviper

Updates #113349

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@@ -52,6 +54,12 @@ struct ScopeBase<'scope> {
/// latch to track job counts
job_completed_latch: CountLatch,

/// Jobs that have been spawned, but not yet started.
pending_jobs: Mutex<IndexSet<JobRefId>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this swapped to IndexSet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lint is "prefer FxHashSet over HashSet, it has better performance".

Should I suppress it?

Copy link
Member

@SparrowLii SparrowLii Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impact on performance needs to be measured with rustc-perf. For now, we can keep the original implementation :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been restored.

@Zoxc
Copy link
Contributor

Zoxc commented Jun 26, 2025

You should add me as a co-author for proper copyright assignment.

@ywxt
Copy link
Author

ywxt commented Jun 26, 2025

You should add me as a co-author for proper copyright assignment.

How to do it? Sorry, I'm not familiar with it.

Done

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2025
@ywxt ywxt force-pushed the less-work-steal branch from 699035a to ccd9af7 Compare June 26, 2025 02:21
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2025
@@ -796,14 +797,83 @@ impl WorkerThread {
/// stealing tasks as necessary.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this comment, which contains "stealing tasks", is still correct after we removed work-steal.

@ywxt ywxt force-pushed the less-work-steal branch from a8a202d to 273c9b6 Compare June 26, 2025 06:55
@ywxt ywxt requested a review from Zoxc June 26, 2025 12:28
@ywxt ywxt force-pushed the less-work-steal branch from 273c9b6 to a0178fd Compare June 27, 2025 02:42
@ywxt ywxt force-pushed the less-work-steal branch from 486fb86 to 36462f9 Compare June 28, 2025 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants