Skip to content

reduce threads spawned by ui-tests#81942

Merged
bors merged 1 commit intorust-lang:masterfrom
the8472:reduce-ui-test-threads
Apr 9, 2021
Merged

reduce threads spawned by ui-tests#81942
bors merged 1 commit intorust-lang:masterfrom
the8472:reduce-ui-test-threads

Conversation

@the8472
Copy link
Member

@the8472 the8472 commented Feb 9, 2021 •
edited
Loading

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
Contributor

rust-highfive commented Feb 9, 2021

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
the8472
the8472 commented Feb 9, 2021
View reviewed changes
@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
the8472
the8472 commented Feb 10, 2021
View reviewed changes
the8472
the8472 commented Feb 10, 2021
View reviewed changes
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Feb 13, 2021

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

Mark-Simulacrum commented Feb 14, 2021

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

the8472
the8472 commented Feb 14, 2021
View reviewed changes
the8472
the8472 commented Feb 14, 2021
View reviewed changes
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

Mark-Simulacrum commented Feb 20, 2021

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

Mark-Simulacrum commented Feb 20, 2021

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

Mark-Simulacrum commented Feb 20, 2021

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 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
@the8472 the8472 force-pushed the reduce-ui-test-threads branch from c7a8445 to ef9023c Compare February 20, 2021 23:58
@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...

@the8472 the8472 force-pushed the reduce-ui-test-threads branch from ef9023c to 9159029 Compare February 21, 2021 00:46
@bors
Copy link
Collaborator

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 8, 2021

Not sure what the correct way to solve this one is. Some stackoverflow answers suggest that passing mcmodel=medium or disabling PIE might help, but since that test requires hwsan which only works on aarch64 I can't test it.

Of course I can simply bump the codegen units again instead.

@the8472 the8472 mentioned this pull request Apr 8, 2021
Closed
@the8472 the8472 force-pushed the reduce-ui-test-threads branch from ccdb282 to fe66a93 Compare April 8, 2021 05:10
@the8472
Copy link
Member Author

the8472 commented Apr 8, 2021

Couldn't reproduce it on godbolt either since it supports crosscompilation but not linking for other targets.

Filed #83989 and bumped CGUs.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 8, 2021
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Apr 8, 2021

@bors r+

@bors
Copy link
Collaborator

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

petrochenkov commented Apr 8, 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?

@bors
Copy link
Collaborator

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
Collaborator

bors commented Apr 8, 2021

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label 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

Mark-Simulacrum commented Apr 9, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 9, 2021

📌 Commit 2786870 has been approved by Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Apr 9, 2021

⌛ Testing commit 2786870 with merge da0b9b6...

@bors
Copy link
Collaborator

bors commented Apr 9, 2021

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

@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