Create a jobserver with N tokens, not N-1 #7344
Conversation
I recently added `jobserver` support to the `cc` crate and ended up running afoul of a `jobserver` quirk on Windows. Due to how it's implemented, on Windows you can't actually add more than the intial number of tokens to the jobserver (it uses an IPC semaphore). On Unix, however, you can since you're just writing bytes into a pipe. In `cc`, however, I found it convenient to control parallelism by simply releasing a token before the parallel loop, then reacquiring the token after the loop. That way the loop just has to acquire a token for each job it wants to spawn and then release it when the job finishes. This is a bit simpler than trying to juggle the "implicit token" all over the place as well as coordinating its use. It's technically invalid because it allows a brief moment of `N+1` parallelism since we release a token and then do a bit of work to acquire a new token, but that's hopefully not really the end of the world. In any case this commit updates Cargo's creation of a jobserver to create it with `N` tokens instead of `N-1`. The same semantics are preserved where Cargo then immediately acquires one of the tokens, but the difference is that this "implicit token" can be released back to the jobserver pool, unlike before.
|
r? @ehuss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I'm trying to better understand the motivation for this change. I think I see how it allows I think this seems ok, I can't think of any problems, but this is always so tricky to get right. Also, I assume from an error handling standpoint in the I'm a little concerned that there's a window after all Another question: Should I update |
You can see from the release history of
This is "handled" in TBH I haven't though too too carefully about this, I was sort of hoping bugs in the wild would guide it and figured the bugs wouldn't be that bad. It's also something I assumed I could pretty easily fix in Cargo come Monday :)
I believe this is correct yeah. This is why the way This PR basically ensures that the initial release for the build script actually works, because currently it doesn't on Windows. Most initial releases work but if you run I was also wondering if it's worth reacquiring the token, but the
Yeah I think it should be safe to update, this probably only affects Cargo's dependency graph and that's already tested on CI with the latest libgit2 and such! |
|
Ok! 😄 @bors r+ |
|
📌 Commit cab8640 has been approved by |
bors
added
S-waiting-on-bors
Create a jobserver with N tokens, not N-1 I recently added `jobserver` support to the `cc` crate and ended up running afoul of a `jobserver` quirk on Windows. Due to how it's implemented, on Windows you can't actually add more than the intial number of tokens to the jobserver (it uses an IPC semaphore). On Unix, however, you can since you're just writing bytes into a pipe. In `cc`, however, I found it convenient to control parallelism by simply releasing a token before the parallel loop, then reacquiring the token after the loop. That way the loop just has to acquire a token for each job it wants to spawn and then release it when the job finishes. This is a bit simpler than trying to juggle the "implicit token" all over the place as well as coordinating its use. It's technically invalid because it allows a brief moment of `N+1` parallelism since we release a token and then do a bit of work to acquire a new token, but that's hopefully not really the end of the world. In any case this commit updates Cargo's creation of a jobserver to create it with `N` tokens instead of `N-1`. The same semantics are preserved where Cargo then immediately acquires one of the tokens, but the difference is that this "implicit token" can be released back to the jobserver pool, unlike before.
|
☀️ Test successful - checks-azure |
I recently added
jobserversupport to thecccrate and ended uprunning afoul of a
jobserverquirk on Windows. Due to how it'simplemented, on Windows you can't actually add more than the intial
number of tokens to the jobserver (it uses an IPC semaphore). On Unix,
however, you can since you're just writing bytes into a pipe.
In
cc, however, I found it convenient to control parallelism by simplyreleasing a token before the parallel loop, then reacquiring the token
after the loop. That way the loop just has to acquire a token for each
job it wants to spawn and then release it when the job finishes. This is
a bit simpler than trying to juggle the "implicit token" all over the
place as well as coordinating its use. It's technically invalid because
it allows a brief moment of
N+1parallelism since we release a tokenand then do a bit of work to acquire a new token, but that's hopefully
not really the end of the world.
In any case this commit updates Cargo's creation of a jobserver to create
it with
Ntokens instead ofN-1. The same semantics are preservedwhere Cargo then immediately acquires one of the tokens, but the
difference is that this "implicit token" can be released back to the
jobserver pool, unlike before.