reduce threads spawned by ui-tests

[the8472]

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]

r? @Mark-Simulacrum

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

[the8472]

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. 😕

[Mark-Simulacrum]

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]

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]

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

[the8472]

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]

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]

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]

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.

[nagisa]

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]

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.

[the8472]

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]

@bors r+

[bors]

📌 Commit fe66a93be4e93d111821cc9e71bb5569c4c77d5c has been approved by Mark-Simulacrum

[petrochenkov]

@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]

⌛ Testing commit fe66a93be4e93d111821cc9e71bb5569c4c77d5c with merge b1cf3c06ea8e8c923a48b7c03ed76376881dad59...

[bors]

💔 Test failed - checks-actions

[the8472]

@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]

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

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 2786870 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 2786870 with merge da0b9b6...

[bors]

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