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

Update cargo #74923

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Update cargo #74923

merged 1 commit into from
Jul 30, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 29, 2020

14 commits in aa6872140ab0fa10f641ab0b981d5330d419e927..974eb438da8ced6e3becda2bbf63d9b643eacdeb
2020-07-23 13:46:27 +0000 to 2020-07-29 16:15:05 +0000

@rust-highfive
Copy link
Collaborator

Updates src/tools/cargo.

cc @ehuss

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2020
@ehuss
Copy link
Contributor Author

ehuss commented Jul 29, 2020

@bors p=1 rollup=never

Don't rollup, this includes several major changes.

Requesting review from @Mark-Simulacrum because this updates a dependency used by almost everything. I've run some tests, and I haven't run into any issues.

Unfortunately I don't remember why the update to termcolor was being delayed. I thought there were still some issues on Windows, but I did a few tests on Windows 8 and the colors looked OK.

@Mark-Simulacrum
Copy link
Member

Hm, so there is this: #63725 (comment)

Do you happen to have Windows 7 or 8 handy to try and see if those issues have since been solved?

@ehuss
Copy link
Contributor Author

ehuss commented Jul 29, 2020

Yes, I already tested on Windows 8, and the colors look OK to me. kennytm/fwdansi#1 got fixed (that's why it is updated here). My concern was kennytm/fwdansi#2, but I don't remember exactly why I filed that. I think Cargo (and rustc, etc.) uses simple enough colors that it isn't an issue.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 29, 2020

And once this lands on nightly, I'll do some more testing, and if everything looks good, I will close rust-lang/cargo#8089 and #55769.

@Mark-Simulacrum
Copy link
Member

Okay. That's good enough for me -- I suspect getting this landed and rolled out to the community at large is the best way to determine if this breaks anyone's workflow anyway :)

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2020

📌 Commit 89d7906 has been approved by Mark-Simulacrum

@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 Jul 29, 2020
@bors
Copy link
Contributor

bors commented Jul 29, 2020

⌛ Testing commit 89d7906 with merge 7986bf52e4c8ab2b61eb1931dbdf322b27f19b18...

@bors
Copy link
Contributor

bors commented Jul 29, 2020

💔 Test failed - checks-actions

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

ehuss commented Jul 30, 2020

@bors retry
flaky test: rust-lang/cargo#8564

@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 Jul 30, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

⌛ Testing commit 89d7906 with merge 6e50a22...

@bors
Copy link
Contributor

bors commented Jul 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 6e50a22 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2020
@bors bors merged commit 6e50a22 into rust-lang:master Jul 30, 2020
@Mark-Simulacrum
Copy link
Member

This was a performance regression, likely due to rust-lang/cargo#8560.

Looking at the results, it seems the two crates affected, cargo and script-servo, had a roughly 0.5s and 0.8s bump in macro expansion.

This is presumably an expected (if somewhat unfortunate) consequence of the above PR -- it was all but guaranteed to be a regression on perf.rust-lang.org, as we only benchmark "final binary/library" compilation rather than the whole build. rust-lang/cargo#8560 can only help full, post-cargo clean, builds.

@ehuss
Copy link
Contributor Author

ehuss commented Aug 4, 2020

cc @alexcrichton

Yea, it is difficult to get the defaults to accommodate all use cases. I would expect the negative effects to mostly happen for someone doing development with optimizations enabled. I'm curious how common that is. I suspect it is rare, but I've had a project where it was unusable without optimizations, so maybe not so rare.

FWIW, I did some benchmarking on a variety of real-world projects and a variety of parallel jobs, and the change was faster in every case for a from-scratch --release build.

If you want to restore the old behavior, you can set the env var CARGO_PROFILE_RELEASE_BUILD_OVERRIDE_OPT_LEVEL=3 (assuming the project doesn't override the default opt-level).

@Mark-Simulacrum
Copy link
Member

I don't think I've ever had a Rust project that was more than a week long and didn't always need release mode, but I agree that it probably varies quite a bit.

I think that this is probably still the right call, though.

One thing we could try is splitting the build dependencies of proc macros and build scripts apart - the latter matter much less, and always run once. But proc macros used for in-workspace crates are essentially guaranteed to be run hundreds of times, so optimizing them to shave of a few milliseconds can be helpful.

I think from scratch builds will indeed always be faster for this, but we also don't have a great handle on whether that matters today (i.e., if you're on CI you probably only care about them, but locally you usually care about tiny incremental rebuilds).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

5 participants