feat: Use channels to maintain job tokens & reuse the implicit token without dropping it first

[osiewicz]

This is a follow-up to #779 and addressing #858. This approach uses an additional thread for polling the jobserver.
Per my local tests, it fixes #858

[osiewicz]

I am wondering if we could also attempt removal of the current wait_thread in this code, as per #829 it is there mostly to avoid a deadlock with acquire - which happens in yet another separate thread anyways as of this PR.

[NobodyXu]

I am wondering if we could also attempt removal of the current wait_thread in this code, as per #829 it is there mostly to avoid a deadlock with acquire - which happens in yet another separate thread anyways as of this PR.

JobTokenServer::acquire from this PR still blocks, so it would still be required.
However I don't think it's necessary for JobTokenServer to spawn a help thread, that's definitely not necessary.

If the help thread can be removed, then you can also replace that mpsc channel with an Arc<AtomicBool>.

[osiewicz]

Eh, I've flirted with the idea of Arc<AtomicBool>, though I like a channel approach a bit more, as it makes obtaining a token opaque - you don't care whether the token is owned by the current process or if it was obtained from the jobserver. It allows for token reuse and to obtain an implicit token the same way a jobserver token is obtained without having to release it.
I agree though that the additional thread is probably not necessary.

[osiewicz]

Eh, I've tried to get rid of helperthread, leading to a deadlock of a build on one of my test projects. I believe the issue here is similar to what you've ran into in #779 (comment) - we can't just acquire, as that might lead to the deadlock. By having a separate thread and a channel we can request a token like we do with acquire - separate thread sounds redundant, right? Well, if acquire is never going to return, then a separate thread is blocked, not the main one - and we can grab the implicit token once that becomes available.
Attaching a patch on top of 52afaf1, perhaps I've done something incorrectly.
no_helperthread.patch

[NobodyXu]

Well, if acquire is never going to return, then a separate thread is blocked, not the main one - and we can grab the implicit token once that becomes available.

I see why it doesn't work: Setting new values on Arc<AtomicBool> cannot stop jobserver::Client from blocking.

However in the current PR it calls mpsc recv, which will succeed and stop blocking when the implicit token is released.

(It's a shame there isn't built-in async executor in stdlib, otherwise this would be much, much easier.)

[NobodyXu]

Regarding the wait_thread, it can be removed by providing a non-blocking try_acquire using mpsc::Receiver::try_receive.

Since the spawning thread can consume all tokens (implicit and explicit), wait_thread is necessary unless we add try_acquire, and switch to waiting on spawned children on failure.

However this sounds so complex that I believe it should be a separate PR.

[NobodyXu]

Regading jobserver, I think that maybe it's a good idea to have a vendored implementation that never blocks.

cc only needs to:

• open a jobserver
• create a jobserver
• try acquire a token
• release a token

[NobodyXu]

LGTM, but you would need a team member's approval to merge

[thomcc]

I'll take a look later. I suspect it's mostly fine with @NobodyXu's approval.

[NobodyXu]

Thank you @osiewicz !

P.S. I was just granted the permission to approve and merge PRs in this repository.

[osiewicz]

Thanks!

[NobodyXu]

I just discovered a problem with this PR: The mpsc in the jobserver will never be dropped so the token will never be put back to jobserver.

I'm already working on a new PR which dramatically improves the parallel compilation and it will be in ready in a few days.

[osiewicz]

Uh oh, that's no good.
Off the top of my head though, does the implicit token need to be returned? We never explicitly request it so it's effectively managed by a caller of cc crate. For the acquired tokens, we should be returning them to the pool by the impl of JobToken for which .forget was called.

That's just off the top of my head though and I'm writing that reply at 3AM, so you know, I am probably wrong in some of my assumptions. :x

[NobodyXu]

Off the top of my head though, does the implicit token need to be returned?

No

For the acquired tokens, we should be returning them to the pool by the impl of JobToken for which .forget was called.

But for tokens which are still in the mpsc, they will never be returned to the jobserver.

[NobodyXu]

I've opened #889 which fixes this and completely removes usage of thread, please have a look/review if you have time.