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

Add workaround for sporadic kills when building on Macos #10196

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

charlesroussel
Copy link
Contributor

This is the workaround for the issue #10060

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2021
@ehuss
Copy link
Contributor

ehuss commented Dec 13, 2021

Thanks for the PR! Can you say why this is specific to arm? I have been experiencing this on x86_64 myself.

@charlesroussel
Copy link
Contributor Author

Thanks for the PR! Can you say why this is specific to arm? I have been experiencing this on x86_64 myself.

I thought it was linked to ARM and the fact that all applications requiring to being signed. Apparently it is only due to APFS.
I'll update the review to include macos version from 11+ instead of arm only.

@charlesroussel charlesroussel changed the title Add workaround for sporadic kills when building on Apple silicon Add workaround for sporadic kills when building on Macos Dec 13, 2021
@alexcrichton
Copy link
Member

This seems like a particularly heavy hammer for this issue since file copying can have a pretty significant performance impact on Cargo. It sounds like this is just intentionally slowing down Cargo to let other processes in the system settle which also would mean that this isn't actually solving the issue, it's just making it more rare. I don't know what a "proper" fix would look like since I'm not sure what we could synchronize on, but otherwise I think it would be useful to quantify what kind of a slowdown this will look like.

@hkratz
Copy link
Contributor

hkratz commented Dec 13, 2021

@alexcrichton fs::copy() tries to use the CoW fclonefileat on macOS, which appears to be not slower than hardlinking on APFS filesystems.

@alexcrichton
Copy link
Member

Oh nice! To confirm, have you benchmarked a bit before and after and seen no difference?

@hkratz
Copy link
Contributor

hkratz commented Dec 14, 2021

On a Macbook Air M1, Monterey 12.1, cargo 1.58.0-nightly (40dc281 2021-12-06) I tested it with the already existing __CARGO_COPY_DONT_LINK_DO_NOT_USE_THIS env variable on cargo itself - difference not measurable.

With hardlinks:

$ cargo clean; cargo -q build
# confirm paths
$ ls -i target/debug/cargo target/debug/deps/cargo-90dea0042da6ad93
4196983 target/debug/cargo
4196983 target/debug/deps/cargo-90dea0042da6ad93
# confirm test methodology
$ rm target/debug/cargo; cargo -q build
$ ls -i target/debug/cargo target/debug/deps/cargo-90dea0042da6ad93
4196983 target/debug/cargo
4196983 target/debug/deps/cargo-90dea0042da6ad93
# !! Note same inodes -> hardlinking used

# benchmark
$ hyperfine -w 3 "rm target/debug/cargo; cargo -q build"
Benchmark #1: rm target/debug/cargo; cargo -q build
  Time (mean ± σ):      98.1 ms ±   0.4 ms    [User: 65.3 ms, System: 31.3 ms]
  Range (min … max):    97.3 ms …  99.3 ms    29 runs

With CoW copies:

# 2) CoW copy performance
$ export __CARGO_COPY_DONT_LINK_DO_NOT_USE_THIS=1
# confirm test methodology
$ rm target/debug/cargo; cargo -q build
$ ls -i target/debug/cargo target/debug/deps/cargo-90dea0042da6ad93
4197238 target/debug/cargo
4196983 target/debug/deps/cargo-90dea0042da6ad93
# !! Note different inodes -> clone/copy used

# benchmark
$ hyperfine -w 3 "rm target/debug/cargo; cargo -q build"
Benchmark #1: rm target/debug/cargo; cargo -q build
  Time (mean ± σ):      98.5 ms ±   0.6 ms    [User: 65.4 ms, System: 31.5 ms]
  Range (min … max):    97.7 ms …  99.8 ms    29 runs

I regularly clone multiple GB files with cp -c and it is instantaneous.

@alexcrichton
Copy link
Member

I forget precisely where and when we use hard links vs copies, but once we invoke rustc all bets are off because rustc is by far always going to be slower than Cargo. Could you try benchmarking builds where Cargo doesn't actually do anything? (where it basically just determines that the project is up-to-date). I think projects with a number of medium-sized binaries would be good to benchmark for that.

@charlesroussel
Copy link
Contributor Author

I tried on rust-analyser project

Clean build without and with removing target binary. The first one is a clean build to have everything built.

unset __CARGO_COPY_DONT_LINK_DO_NOT_USE_THIS
cargo build
hyperfine -w 3 "cargo build"
Benchmark 1: cargo build
  Time (mean ± σ):     130.7 ms ±   4.3 ms    [User: 97.7 ms, System: 30.9 ms]
  Range (min … max):   127.1 ms … 143.7 ms    22 runs

hyperfine -w 3 "rm target/debug/rust-analyser;cargo build"
Benchmark 1: rm target/debug/rust-analyser;cargo build
  Time (mean ± σ):     130.9 ms ±   2.2 ms    [User: 97.7 ms, System: 31.4 ms]
  Range (min … max):   128.3 ms … 136.6 ms    22 runs
export __CARGO_COPY_DONT_LINK_DO_NOT_USE_THIS=1
cargo build
hyperfine -w 3 "cargo build"
Benchmark 1: cargo build
  Time (mean ± σ):     129.8 ms ±   3.8 ms    [User: 96.8 ms, System: 30.4 ms]
  Range (min … max):   126.9 ms … 145.3 ms    22 runs

hyperfine -w 3 "rm target/debug/rust-analyser;cargo build"
Benchmark 1: rm target/debug/rust-analyser;cargo build
  Time (mean ± σ):     130.3 ms ±   0.9 ms    [User: 96.9 ms, System: 31.5 ms]
  Range (min … max):   128.2 ms … 131.5 ms    22 runs

There is no apparent difference when cargo "does nothing"

@charlesroussel charlesroussel requested a review from ehuss December 16, 2021 09:32
@alexcrichton
Copy link
Member

Awesome, thanks!

Could you update the comment a bit with the performance implication where using the CoW-enabled syscall this should theoretically be just as fast as the hard linking version?

@charlesroussel
Copy link
Contributor Author

Awesome, thanks!

Could you update the comment a bit with the performance implication where using the CoW-enabled syscall this should theoretically be just as fast as the hard linking version?

Done

@alexcrichton
Copy link
Member

@bors: r+

👍

@bors
Copy link
Contributor

bors commented Dec 16, 2021

📌 Commit c9c67c0 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2021
@bors
Copy link
Contributor

bors commented Dec 16, 2021

⌛ Testing commit c9c67c0 with merge 180f599...

@bors
Copy link
Contributor

bors commented Dec 16, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 180f599 to master...

@bors bors merged commit 180f599 into rust-lang:master Dec 16, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 17, 2021
Update cargo

8 commits in a359ce16073401f28b84840da85b268aa3d37c88..fcef61230c3b6213b6b0d233a36ba4ebd1649ec3
2021-12-14 18:40:22 +0000 to 2021-12-17 02:30:38 +0000
- Minor docs change for `cargo test --help` (rust-lang/cargo#10210)
- Make clippy happy (rust-lang/cargo#10205)
- Enhance descriptions of issue templates (rust-lang/cargo#10202)
- Add workaround for sporadic kills when building on Macos (rust-lang/cargo#10196)
- Detect filesystem loop during walking the projects (rust-lang/cargo#10188)
- Error about not having any crates with documentation (rust-lang/cargo#10204)
- Don't document libs with doc=false (rust-lang/cargo#10201)
- Bumps up tar to 0.4.36 (rust-lang/cargo#10198)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2021
Update cargo

8 commits in a359ce16073401f28b84840da85b268aa3d37c88..fcef61230c3b6213b6b0d233a36ba4ebd1649ec3
2021-12-14 18:40:22 +0000 to 2021-12-17 02:30:38 +0000
- Minor docs change for `cargo test --help` (rust-lang/cargo#10210)
- Make clippy happy (rust-lang/cargo#10205)
- Enhance descriptions of issue templates (rust-lang/cargo#10202)
- Add workaround for sporadic kills when building on Macos (rust-lang/cargo#10196)
- Detect filesystem loop during walking the projects (rust-lang/cargo#10188)
- Error about not having any crates with documentation (rust-lang/cargo#10204)
- Don't document libs with doc=false (rust-lang/cargo#10201)
- Bumps up tar to 0.4.36 (rust-lang/cargo#10198)
@ehuss ehuss added this to the 1.59.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants