Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd a GNU make jobserver implementation to Cargo #4110
Conversation
rust-highfive
assigned
brson
May 30, 2017
This comment has been minimized.
This comment has been minimized.
rust-highfive
commented
May 30, 2017
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
r? @matklad |
rust-highfive
assigned
matklad
and unassigned
brson
May 30, 2017
This comment has been minimized.
This comment has been minimized.
|
When reviewing this, it may also be worth taking a brief look at the crate's source as well |
matklad
reviewed
May 30, 2017
| // we're only sending "longer living" messages and we should also | ||
| // destroy all references to the channel before this function exits as | ||
| // the destructor for the `helper` object will ensure the associated | ||
| // thread i sno longer running. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
matklad
May 30, 2017
Member
Interesting, looks like this transmute is always safe, because Sender is contravariant.
This comment has been minimized.
This comment has been minimized.
alexcrichton
May 31, 2017
Author
Member
That's what I originally thought yeah but actually I don't think so (unfortunately). I believe Drop for Sender may run destructors for items in the internal queue (not received yet), which means that if you persist a Sender beyond the lifetime of the item I think it'll access data outside of its lifetime.
(not in this case though, the Sender should always go away with the stack frame.
| @@ -51,6 +53,9 @@ impl Config { | |||
| extra_verbose: Cell::new(false), | |||
| frozen: Cell::new(false), | |||
| locked: Cell::new(false), | |||
| // This should be called early on in the process, so in theory the | |||
| // unsafety is ok here. (taken ownership of random fds) | |||
| jobserver: unsafe { jobserver::Client::from_env() }, | |||
This comment has been minimized.
This comment has been minimized.
matklad
May 30, 2017
Member
There should be at most one job server, and it's not entirely obvious that we call Config::new exactly once. Perhaps, we can wrap jobserver creation in a jobserver_singleton function, which uses ONCE_INIT or lazy_static! internally?
This comment has been minimized.
This comment has been minimized.
alexcrichton
force-pushed the
alexcrichton:jobserver
branch
from
892e7fb
to
0e89a79
May 31, 2017
This comment has been minimized.
This comment has been minimized.
|
Ok I've pushed up w/ a |
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
force-pushed the
alexcrichton:jobserver
branch
from
0e89a79
to
3220daf
Jun 1, 2017
This comment has been minimized.
This comment has been minimized.
|
LGTM, but perhaps we want to add a test here? I understand that there's an impressive amount of tests in I am thinking of two useful test:
|
This comment has been minimized.
This comment has been minimized.
|
Oh, and what should we do if Cargo is launched with jobjserver, but also with an explicit |
alexcrichton
force-pushed the
alexcrichton:jobserver
branch
from
3220daf
to
434177c
Jun 1, 2017
This comment has been minimized.
This comment has been minimized.
|
All excellent suggestions! I've updated the PR (thanks so much for the thorough reviews!) |
This comment has been minimized.
This comment has been minimized.
|
r+, but looks like the CI failure on windows is legit: https://ci.appveyor.com/project/rust-lang-libs/cargo/build/1.0.1971#L271
Well, I learn a lot of stuff from them :) |
This comment has been minimized.
This comment has been minimized.
|
Looks like a corner case! |
alexcrichton
force-pushed the
alexcrichton:jobserver
branch
from
434177c
to
cbf25a9
Jun 2, 2017
This comment has been minimized.
This comment has been minimized.
|
@bor: r=matklad |
This comment has been minimized.
This comment has been minimized.
|
@bors: r=matklad |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jun 2, 2017
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton commentedMay 30, 2017
This commit adds a GNU make jobserver implementation to Cargo, both as a client
of existing jobservers and also a creator of new jobservers. The jobserver is
actually just an IPC semaphore which manifests itself as a pipe with N bytes
of tokens on Unix and a literal IPC semaphore on Windows. The rough protocol
is then if you want to run a job you read acquire the semaphore (read a byte on
Unix or wait on the semaphore on Windows) and then you release it when you're
done.
All the hairy details of the jobserver implementation are housed in the
jobservercrate on crates.io instead of Cargo. This should hopefully make itmuch easier for the compiler to also share a jobserver implementation
eventually.
The main tricky bit here is that on Unix and Windows acquiring a jobserver token
will block the calling thread. We need to either way for a running job to exit
or to acquire a new token when we want to spawn a new job. To handle this the
current implementation spawns a helper thread that does the blocking and sends a
message back to Cargo when it receives a token. It's a little trickier with
shutting down this thread gracefully as well but more details can be found in
the
jobservercrate.Unfortunately crates are unlikely to see an immediate benefit of this once
implemented. Most crates are run with a manual
make -jNand this overrides thejobserver in the environment, creating a new jobserver in the sub-make. If the
-jNargument is removed, however, thenmakewill share Cargo's jobserver andproperly limit parallelism.
Closes #1744