Fix Build::compile_objects perf regression and deadlock

[NobodyXu]

• Fix perf regression by using Child::try_wait and mpsc::Receiver::try_recv to wait on
  multiple processes and the channel concurrently, and
  thread::yield_now() if no progress is made.
• Fix the potential deadlock by waiting for all threads to be done and release all the jobserver tokens.

Signed-off-by: Jiahao XU [email protected]

[twistedfall]

I can confirm that this fixes the performance regression that I'm experiencing at 1.0.81

[NobodyXu]

I can confirm that this fixes the performance regression that I'm experiencing at 1.0.81

Thank you for testing!

I also wonder if it improves performance, the original PR is intended to improve performance by dramatically reducing number of threads spawned and joined.

[twistedfall]

Well, if it does, then marginally, in some of my tests I got some times with the patch that were about half a second under the times with 1.0.79, but then again in some tests it finished in about half a second slower :)

[NobodyXu]

Well, if it does, then marginally, in some of my tests I got some times with the patch that were about half a second under the times with 1.0.79, but then again in some tests it finished in about half a second slower :)

Thanks, I guess modern hardware are a bit too powerful.

@twistedfall Can you push it to GitHub and check whether it improve CI run-time?
GitHub CI are overloaded, have only two vCPUs on Linux and Windows by default, so I think it could be helpful in that scenario.

[twistedfall]

Setting up CI with a custom patched cc-rs specifically for this it a bit of a hassle to be honest, sorry :) But I did run tests locally with -j2 flag passed to cargo which resulted in the same behavior, the amount of concurrently running jobs was limited to 2. And the performance difference is similarly negligible between 1.0.79 and 1.0.81 with the patch from this PR applied.

[NobodyXu]

Setting up CI with a custom patched cc-rs specifically for this it a bit of a hassle to be honest, sorry :) But I did run tests locally with -j2 flag passed to cargo which resulted in the same behavior, the amount of concurrently running jobs was limited to 2. And the performance difference is similarly negligible between 1.0.79 and 1.0.81 with the patch from this PR applied.

Thank you very much!
I guess we would have to roll out this PR first and see whether it does speedup the CI.

cc @thomcc This PR fixed a potential deadlock and a known performance regression.
I will open a PR for new release this time so you can approve and merge it.

[thomcc]

Looks good for now. I will think about this more, but that should not block getting this fix to users.

[NobodyXu]

@twistedfall cc v1.0.82 has released!

[newpavlov]

Use of retain_mut has bumped MSRV to 1.61, it has broken CI for the kem crate. At the very least, I believe it should be mentioned in release notes.

[NobodyXu]

That's definitely not intended...I thought the msrv would check that, but it doesn't enable feature parallel.

[newpavlov]

Do you plan to remove the use of retain_mut or leave as-is? I guess we could bump MSRV of kem from 1.60 to 1.61, though, unfortunately, it will go against our MSRV policy.

[thomcc]

It's a release over a year old and was unintentional, so I wouldn't fix it on my own, but I would accept a patch fixing it if it's reasonable (with no guarantee that we won't bump the MSRV again in a future release -- it would be in the relnotes next time though).

Definitely the CI detection should be fixed, however.

EDIT: Note that I can't promise a release of cc until the weekend though.

[NobodyXu]

retain_mut can be replaced with Vec::swap_remove since we don't really care about the order of the Vec anyway, just want to iterate over all the processes and .try_wait() on them..

@newpavlov Unfortunately I have been busy recently so I won't be able to provide a PR in a short time, however I do think it can be done even if you don't have any experience with the cc codebase.

[newpavlov]

In our case cc was only part of dev dependency tree and new MSRV is just 1 version bigger than one supported by the crate, so we simply tweaked our CI.

[NobodyXu]

That's good to hear, though I will submit a PR to fix this

[NobodyXu]

@newpavlov @thomcc I've opened #888 to fix this