Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with releasing and reacquiring implicit jobserver token #858

Closed
tgnottingham opened this issue Aug 19, 2023 · 1 comment · Fixed by #878
Closed

Issues with releasing and reacquiring implicit jobserver token #858

tgnottingham opened this issue Aug 19, 2023 · 1 comment · Fixed by #878

Comments

@tgnottingham
Copy link

tgnottingham commented Aug 19, 2023

I was trying to optimize build times and found that -sys crate dependencies seemed to be taking a long time according to the cargo build --timings output. Further investigation showed that these dependencies didn't take as long to build in isolation (with concurrency controlled for with -j1), which seemed odd.

More investigation led to rust-lang/cargo#7689 and rust-lang/cargo#7344 being the culprits.

Long story short, I think that releasing and reacquiring the implicit jobserver token causes the cargo build --timings output to be misleading.

Take this example:

cargo new example
cd example
cargo add bzip2-sys nng-sys zstd-sys 
cargo build -j1 --timings

And the resulting graph:

Screenshot from 2023-08-19 15-58-31

If I build each of the -sys dependencies in isolation, bzip2-sys takes ~0.5s, nng-sys takes ~14s, and zstd-sys takes ~15s. Note the inconsistency with the graph.

What appears to be happening, roughly, in this case is:

  1. bzip2-sys build script starts
  2. cc for bzip2-sys releases its implicit token before its processing loop
  3. That allows zstd-sys build script to start
  4. cc for zstd-sys releases its implicit token before its processing loop
  5. That allows nng-sys build script to start
  6. nng-sys build script runs to completion in ~14s
  7. nng-sys starts and runs to completion
  8. zstd-sys build script resumes and runs for ~2.5s
  9. cc for zstd-sys releases a token
  10. That allows bzip2-sys build script to resume and run to completion in ~0.5s
  11. bzip2-sys starts and runs to completion
  12. zstd-sys build script resumes and runs to completion in ~12.5s
  13. zstd-sys starts and runs to completion

If you assume this is more or less what happened, the bars in the graph make sense and the timings are consistent with how long the -sys crates take to build in isolation.

Also, I don't know how cargo schedules dependency building, but I'm wondering if this issue could cause increases in compile times by delaying builds of dependents in an unexpected way. If cargo specifically chooses to compile one dependency earlier to unlock its ability to start compiling dependents, presumably with the goal of reducing overall build time, then I think this choice might be undermined by the behavior seen here, since we see other crates sort of cut in line.

I'm also wondering if, once this is fixed, the change introduced by rust-lang/cargo#7344 should be reverted or modified somehow, or if doing so would cause problems with older versions of cc on older versions of cargo.

@NobodyXu
Copy link
Collaborator

Yeah that is a really unfortunate situation.
I tried to remove this in #779 but failed.

NOTE that if any build.rs spawns multiple threads to compile in parallel, they would release even more tokens and then acquire.

I'm really not sure how to fix this.
Perhaps the solution is to not re-acquire the token at all, or add a new method jobserver::Client::try_acquire.

osiewicz added a commit to zed-industries/zed that referenced this issue Nov 13, 2023
This resolves a minor issue where build scripts could've acquired more
job server tokens from Cargo than allowed by `-j` parameter. I've filled
a PR at rust-lang/cc-rs#878 and we've iterated
on the design over there since.

TL;DR: some build scripts may complete a tad bit quicker, potentially
shaving off a few seconds off of debug/release builds. Full description
of the issue is available in
rust-lang/cc-rs#858

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants