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

Revert "remove num_cpus dependency" in rustc and update cargo #97911

Merged
merged 3 commits into from
Jun 9, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jun 9, 2022

Fixes #97549. This PR reverts #94524 and does a Cargo update to pull in rust-lang/cargo#10737.

Rust 1.61.0 has a regression in which it misidentifies the number of available CPUs in some environments, leading to enormously increased memory usage and failing builds. In between Rust 1.60 and 1.61 both rustc and cargo replaced some uses of num_cpus with available_parallelism, which eliminated support for cgroupv1, still apparently in common use. This PR switches both rustc and cargo back to using num_cpus in order to support environments where the available parallelism is controlled by cgroupv1. Both can use available_parallism again once it handles cgroupv1 (if ever).

I have confirmed that the rustc part of this PR fixes the memory usage regression in my non-Cargo environment, and others have confirmed in #97549 that the Cargo regression was at fault for the memory usage regression in their environments.

dtolnay added 2 commits June 9, 2022 03:07
1 commit in 85e457e158db216a2938d51bc3b617a5a7fe6015..4d92f07f34ba7fb7d7f207564942508f46c225d3
2022-06-07 21:57:52 +0000 to 2022-06-09 01:18:36 +0000
- Revert 10427: switch from num_cpus
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 9, 2022
@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 Jun 9, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 9, 2022

📌 Commit fbc86e0 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 Jun 9, 2022
@the8472
Copy link
Member

the8472 commented Jun 9, 2022

Both can use available_parallism again once it handles cgroupv1 (if ever).

I already claimed the issue and wrote that I'll be working on that.

@bors
Copy link
Contributor

bors commented Jun 9, 2022

⌛ Testing commit fbc86e0 with merge 420c970...

@compiler-errors
Copy link
Member

@dtolnay @Mark-Simulacrum, should this be beta nominated so users don't have to wait for 1.63 for this to reach stable?

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 9, 2022
@bors
Copy link
Contributor

bors commented Jun 9, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 420c970 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 9, 2022
@bors bors merged commit 420c970 into rust-lang:master Jun 9, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 9, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (420c970): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
1.0% 1.0% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 1.0% 1.0% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
4.1% 4.1% 1
Regressions 😿
(secondary)
2.7% 2.8% 4
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 4.1% 4.1% 1

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
2.3% 2.4% 2
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.3% 2.4% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 16, 2022
@dtolnay dtolnay deleted the numcpu branch June 16, 2022 15:21
ehuss pushed a commit to ehuss/rust that referenced this pull request Jun 24, 2022
Revert "remove num_cpus dependency" in rustc and update cargo

Fixes rust-lang#97549. This PR reverts rust-lang#94524 and does a Cargo update to pull in rust-lang/cargo#10737.

Rust 1.61.0 has a regression in which it misidentifies the number of available CPUs in some environments, leading to enormously increased memory usage and failing builds. In between Rust 1.60 and 1.61 both rustc and cargo replaced some uses of `num_cpus` with `available_parallelism`, which eliminated support for cgroupv1, still apparently in common use. This PR switches both rustc and cargo back to using `num_cpus` in order to support environments where the available parallelism is controlled by cgroupv1. Both can use `available_parallism` again once it handles cgroupv1 (if ever).

I have confirmed that the rustc part of this PR fixes the memory usage regression in my non-Cargo environment, and others have confirmed in rust-lang#97549 that the Cargo regression was at fault for the memory usage regression in their environments.
@ehuss ehuss mentioned this pull request Jun 24, 2022
@ehuss ehuss removed this from the 1.63.0 milestone Jun 24, 2022
@ehuss ehuss added this to the 1.62.0 milestone Jun 24, 2022
@ehuss ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2022
[beta] Beta backports

* Remove the unused-#[doc(hidden)] logic from the unused_attributes lint rust-lang#98336
* debuginfo: Fix NatVis for Rc and Arc with unsized pointees. rust-lang#98137
* Revert "remove num_cpus dependency" in rustc and update cargo rust-lang#97911
* Update LLVM submodule rust-lang#97690
* Revert rust-lang#96682. rust-lang#97636
* don't do Sized and other return type checks on RPIT's real type rust-lang#97431
* Temporarily disable submodule archive downloads. rust-lang#98423
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2022
…=jyn514

Reland changes replacing num_cpus with available_parallelism

Since rust-lang#97925 added cgroupv1 support the problem in rust-lang#97549 which lead to the previous revert should be addressed now.

Cargo has reapplied the replacement too rust-lang/cargo#10969

Reverts 1ae4b25 (part of rust-lang#97911)
Relands rust-lang#94524
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2022
…=jyn514

Reland changes replacing num_cpus with available_parallelism

Since rust-lang#97925 added cgroupv1 support the problem in rust-lang#97549 which lead to the previous revert should be addressed now.

Cargo has reapplied the replacement too rust-lang/cargo#10969

Reverts 1ae4b25 (part of rust-lang#97911)
Relands rust-lang#94524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Higher memory usage on Rust 1.61.0 (compared to 1.60.0) leading to SIGKILL
10 participants