Communicate over stderr jobserver token acquiring/releasing #67398
Conversation
Mark-Simulacrum
added
the
WG-compiler-parallel
rust-highfive
added
the
S-waiting-on-review
|
In theory we can land this as-is (it doesn't do anything except if A sample run on a tiny crate with the parallel compiler enabled looks something like this; it doesn't terminate in this case as we run out of jobserver tokens (if the -Z flag is passed, they're never released by rustc itself, as it expects Cargo to handle that). @bors try |
Communicate over stderr jobserver token acquiring/releasing This set of commits revises the jobserver communication inside rustc to always go through a shim around the real jobserver crate to allow us to inform Cargo of when tokens are intended to be acquired/released. The Cargo side of this hasn't yet been implemented at all yet.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-author
|
I think this should work for our own internal testing with Cargo and such, but I would be wary of shipping this. I think we'll want more integration with the actual json printing of rustc to ensure that it's all properly synchronized and runs no risk of being jumbled up. I suspect that will require refactorings of the emitter though. Apart from that, as a separate concern but not blocking this from being shipped, I would like to see the parallelism of the backend and frontend unified, but this is something we've always known and is extremely difficult to do so until we ship parallel-by-default! |
|
Okay, I will try and refactor this a bit to pull in the emitter as needed, though that'll likely be a pretty sizeable refactor I imagine. I think we should be fine since However, one blocker for wiring this together with the emitter is that currently the emitter is a Session-specific resource vs. the jobserver Client and such being a process-global resource. I'm not sure if there's a good way to integrate those two, particularly in a situation where we have more than one rustc flying around in one process, but maybe that's not a case we need to care about? I think we don't have anything today that does so in-tree, though maybe miri etc are doing something like that (to my knowledge, rustdoc does not do this after semi-recent refactoring). I think for proper jobserver <-> emitter support we basically need the emitter to be global but that feels not quite right either :/ |
|
Oh right so to clarify we probably don't need to do this just yet, I think it's fine to test with this. I don't think that |
Do you want to take a stab at implementing the Cargo side of this, or do you want me to try and take a look? I'll work on getting the try build working regardless.
I think we don't care about line-locking in this case since we're emitting all messages in one go (i.e., within one lock) but I'd have to check myself as well :) |
|
If you've got time to work on this in Cargo that'd be great, I'm a bit strapped at the moment! I also agree that there's not much to worry about with this exact implementation, it's largely future-proofing I'd like to handle, but can always happen at another time. |
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
0689914 to
2e0163e
Compare
|
☔ The latest upstream changes (presumably #67449) made this pull request unmergeable. Please resolve the merge conflicts. |
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
2e0163e to
49dc0bb
Compare
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
49dc0bb to
9d65d6f
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
9d65d6f to
be860a3
Compare
|
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
be860a3 to
6af074a
Compare
Mark-Simulacrum
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
2 times, most recently
from
0dc0fbc to
9d61a88
Compare
|
☔ The latest upstream changes (presumably #67763) made this pull request unmergeable. Please resolve the merge conflicts. |
We want to restrict access to the raw Client and this is the first step to doing so; the client is still initialized from the environment at the same time.
Sending an io::Result of the acquired token is a bit odd as the receiver can't productively do anything on error (other than panic) and largely if we've encountered an error when reading from the pipe it's not something that we expect to happen (i.e., likely indicates something has gone wrong outside our control or is a rustc programmer error) so a panic is appropriate.
Future commits will be changing the jobserver integration to have custom logic to enable it to appropriately talk to Cargo, so we can't have other code that directly talks to the jobserver.
This adds the -Zjobserver-token-requests debugging flag which Cargo will pass down to us, but for now that flag doesn't do anything. We don't really have a good way to deal with multiple initialize calls so for now this implementation just ignores all but the first construction's token requesting mode. It's plausible we should assert that all calls have the same mode, but it's unclear whether that really brings us any benefits (and it does have a cost of forcing people to thread things through on their end).
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
abac0a6 to
f0a6239
Compare
Previously, we were holding a global lock whenever we were waiting for a requested token, and since the only way to release a token was by acquiring that lock, we could relatively easily deadlock (at least if `-j1` was passed, or so, otherwise it's likely we'd just be much slower).
Mark-Simulacrum
force-pushed
the
no-jobserver-herds
branch
from
f0a6239 to
d88fcc1
Compare
|
Outstanding todo items:
I'm still not quite sure how to manage the first one. It feels like we want to integrate into rustc's standard error reporting, but I'm not yet sure how we can manage to make that work. rustc currently has a per-rustc-instance We can also give up on having multiple rustc's in one process, I'm not sure that's actually used today, but that seems unfortunate. |
|
☔ The latest upstream changes (presumably #67953) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I would personally prefer to go the route of as few globals as possible and tie things into |
|
Triaged |
joelpalmer
added
S-waiting-on-author
|
@alexcrichton any updates? |
|
I'm going to close this PR for now as realistically I don't have the time to work on it right now and generally speaking the problems it solves have stalled out (along with the parallelization effort). Likely to unblock we'd need to land the global stderr locked handles or something and that has similar issues to what Alex is commenting on (re:Session). |
`-Zjobserver-per-rustc` has been broken for a long while. My guess is that we didn't make `-Zjobserver-token-request` into rustc (rust-lang/rust#67398). To reduce the complexity of the current job queue implementation, I propose to remove this potion. Cargo doesn't really receive any `jobserver_event`[^1] from the offical rustc. We can always bring this back if needed. [^1]: https://github.com/rust-lang/cargo/blob/65cab34dc75587eeaff68d3c19358c4d79041452/src/cargo/core/compiler/mod.rs#L1704-L1713
`-Zjobserver-per-rustc` has been broken for a long while. My guess is that we didn't make `-Zjobserver-token-request` into rustc (rust-lang/rust#67398). To reduce the complexity of the current job queue implementation, I propose to remove this portion. Cargo doesn't really receive any `jobserver_event`[^1] from the offical rustc. We can always bring this back if needed. [^1]: https://github.com/rust-lang/cargo/blob/65cab34dc75587eeaff68d3c19358c4d79041452/src/cargo/core/compiler/mod.rs#L1704-L1713
`-Zjobserver-per-rustc` has been broken for a long while. My guess is that we didn't make `-Zjobserver-token-request` into rustc (rust-lang/rust#67398). To reduce the complexity of the current job queue implementation, I propose to remove this portion. Cargo doesn't really receive any `jobserver_event`[^1] from the offical rustc. We can always bring this back if needed. [^1]: https://github.com/rust-lang/cargo/blob/65cab34dc75587eeaff68d3c19358c4d79041452/src/cargo/core/compiler/mod.rs#L1704-L1713
`-Zjobserver-per-rustc` has been broken for a long while. My guess is that we didn't make `-Zjobserver-token-request` into rustc (rust-lang/rust#67398). To reduce the complexity of the current job queue implementation, I propose to remove this portion. Cargo doesn't really receive any `jobserver_event`[^1] from the official rustc. We can always bring this back if needed. [^1]: https://github.com/rust-lang/cargo/blob/65cab34dc75587eeaff68d3c19358c4d79041452/src/cargo/core/compiler/mod.rs#L1704-L1713
This set of commits revises the jobserver communication inside rustc to always go through a shim around the real jobserver crate to allow us to inform Cargo of when tokens are intended to be acquired/released.
The Cargo side of this hasn't yet been implemented at all yet.