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.

Sign up for GitHub

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

Jump to bottom

reduce threads spawned by ui-tests #81942

Merged
merged 1 commit into from Apr 9, 2021
Merged

reduce threads spawned by ui-tests #81942

merged 1 commit into from Apr 9, 2021

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Feb 9, 2021 •
edited

The test harness already spawns enough tests to keep all cores busy.
Individual tests should keep their own threading to a minimum to avoid context switch overhead.

When running ui tests with lld enabled this shaves about 10% off that testsuite on my machine.

Resolves #81946

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
Copy link
Member Author

the8472 commented Feb 9, 2021

That failure is concerning.

The original issue #69225 reported an out of bounds access that doesn't happen when codegen-units < 4. That issue was fixed and the reproducer at that time didn't depend on CGUs either. And yet here it is. 😕

This was referenced Feb 10, 2021
Closed
Closed
@Mark-Simulacrum
Copy link
Member

I am worried about maintaining the complexity cost here, but the 10% does sound appealing. Could you see if we get similar effects from configuring a make jobserver (via the jobserver crate) with just 1 token?

@the8472
Copy link
Member Author

the8472 commented Feb 13, 2021

As far as I can tell from documentation lld.ld does not support jobservers, only the --threads argument. Running ./x.py build compiler/rustc -j1 also results in that multi-threaded peak from lld. For compiletest that's significant because it's linking many small testcases.

@Mark-Simulacrum
Copy link
Member

Huh, that's annoying. I'll think some more about the tradeoff here, I guess.

the8472
the8472 commented Feb 14, 2021
View reviewed changes
Copy link
Member Author

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Also note that in rustc the jobserver only restricts the number of concurent threads, it doesn't have an impact on codegen unit count. But the number of threads already...

@Mark-Simulacrum
Copy link
Member

Hm, I'm not sure who might be able to check the linker flags will generally do the right thing. I'm not myself really that familiar with our linker story today. cc @nagisa perhaps?

@Mark-Simulacrum
Copy link
Member

It also looks like -no-threads might be preferable to the threads=1 option? That communicates intent a bit more clearly, maybe they have different handling internally?

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Feb 20, 2021
@Mark-Simulacrum
Copy link
Member

Otherwise this seems good to me - I think I'd like some more eyes on it for the linker stuff, so I'll nominate for a T-compiler meeting, but I think we could land after that.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021
@nagisa
Copy link
Member

nagisa commented Feb 20, 2021

My only major concern here is that there may have been tests written for ICEs or soundness issues out there that relied on absence of -Ccodegen-units flag being equivalent to "many of them" rather than "1 CU". Of course the tests are pretty poor if they made assumptions like these, but 🤷.

@the8472
Copy link
Member Author

the8472 commented Feb 20, 2021

It also looks like -no-threads might be preferable to the threads=1 option?

That only seems to be available on lld 10. 11 only has the threads argument.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
Copy link
Member Author

the8472 commented Feb 21, 2021

Can't reproduce the issue locally even with ./src/ci/docker/run.sh x86_64-gnu-llvm-9 and it didn't occur in 0e4d4d04636bce5b242accee74b0c95039957afd either. Trying again...

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 8, 2021

📌 Commit fe66a93be4e93d111821cc9e71bb5569c4c77d5c has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2021
@petrochenkov
Copy link
Contributor

@the8472

Ok, I can do that in a followup PR so it can get proper review.

Could you at least leave FIXMEs (preferably referencing the opened issues) in tests to which this PR adds -Ccodegen-units?

@bors
Copy link
Contributor

bors commented Apr 8, 2021

⌛ Testing commit fe66a93be4e93d111821cc9e71bb5569c4c77d5c with merge b1cf3c06ea8e8c923a48b7c03ed76376881dad59...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 8, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 8, 2021
@the8472
Copy link
Member Author

the8472 commented Apr 9, 2021

@the8472

Ok, I can do that in a followup PR so it can get proper review.

Could you at least leave FIXMEs (preferably referencing the opened issues) in tests to which this PR adds -Ccodegen-units?

Will do. I have looked at the test which I thought failed with a stderr diff again and it turns out it actually is another linker error. I'll file an issue for that too.

@the8472 the8472 mentioned this pull request Apr 9, 2021
Closed
@the8472
Copy link
Member Author

the8472 commented Apr 9, 2021

Added FIXMEs, bumped CGUs further for the latest failing test.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
reduce threads spawned by ui-tests
2786870 
the test harness already spawns enough tests for all cores, individual
tests should keep their own threading to a minimum to avoid context switch
overhead

some tests fail with 1 CGU, so explicit compile flags have been added
to keep their old behavior
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 9, 2021

📌 Commit 2786870 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2021
@bors
Copy link
Contributor

bors commented Apr 9, 2021

⌛ Testing commit 2786870 with merge da0b9b6...

@bors
Copy link
Contributor

bors commented Apr 9, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing da0b9b6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 9, 2021
@bors bors merged commit da0b9b6 into rust-lang:master Apr 9, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 9, 2021
@c410-f3r c410-f3r mentioned this pull request May 15, 2021
Merged
@the8472 the8472 mentioned this pull request May 15, 2021
Merged
@wesleywiser wesleywiser mentioned this pull request Jun 21, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@tmiasko tmiasko tmiasko left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.

Safe code (ui/issues/issue-69225-SCEVAddExpr-wrap-flag.rs) miscompiles under llvm9 + codegen-units=1