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

Part 8 of RFC2906 - Keep InheritableFields in a LazyCell inside `… #10568

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

Muscraft
Copy link
Member

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565

This PR focuses on optimizations surrounding InheritabeFields and searching for a WorkspaceRootConfig:

  • Keep InheritableFields in a LazyCell inside to_real_manifest so we only search for a workspace root once per to_real_manifest
  • Changed calls for getting a workspace root to use inherit_cell.try_borrow_with()

Testing Framework
Test Results:
nest=1, runs=7, members=75, step=25

  • 25 Members:

    • Optimized: 0.145s
    • Not Optimized: 0.181s
    • Normal: 0.141s
    • Percent change from Normal: 2.837%
  • 50 Members

    • Optimized: 0.152s
    • Not Optimized: 0.267s
    • Normal: 0.141s
    • Percent change from Normal: 7.801%

nest=2, runs=7, members=75, step=25

  • 25 Members

    • Optimized: 0.147s,
    • Not Optimized: 0.437s
    • Normal: 0.14s
    • Percent change from Normal: 5.0%
  • 50 Members

    • Optimized: 0.159s
    • Not Optimized: 1.023s
    • Normal: 0.141s
    • Percent change from Normal: 12.766%

Using a LazyCell for InheritableFields brought down runtimes to more acceptable levels. It is worth noting that this is a very harsh test case and not one that represents real-world scenarios. That being said there are still some optimizations that could be done if we need to. The biggest one is making sure we only parse a Cargo.toml once.

Remaining implementation work for the RFC

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2022
@Muscraft
Copy link
Member Author

r? @epage

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
…to_real_manifest` so we only search for a workspace root once per package
@epage
Copy link
Contributor

epage commented Apr 14, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 14, 2022

📌 Commit 125c274 has been approved by epage

@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 Apr 14, 2022
@bors
Copy link
Contributor

bors commented Apr 14, 2022

⌛ Testing commit 125c274 with merge b75281b...

@bors
Copy link
Contributor

bors commented Apr 14, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing b75281b to master...

@bors bors merged commit b75281b into rust-lang:master Apr 14, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2022
Update cargo

7 commits in dba5baf4345858c591517b24801902a062c399f8..edffc4ada3d77799e5a04eeafd9b2f843d29fc23
2022-04-13 21:58:27 +0000 to 2022-04-19 17:38:29 +0000
- Document cargo-add (rust-lang/cargo#10578)
- feat: Support '-F' as an alias for '--features' (rust-lang/cargo#10576)
- Completion support for `cargo-add` (rust-lang/cargo#10577)
- Add a link to the document in the timings report (rust-lang/cargo#10492)
- feat: Import cargo-add into cargo (rust-lang/cargo#10472)
- Part 8 of RFC2906 - Keep `InheritableFields` in a `LazyCell` inside `… (rust-lang/cargo#10568)
- Part 7 of RFC2906 - Add support for inheriting `exclude` and `include` (rust-lang/cargo#10565)
bors added a commit that referenced this pull request Apr 20, 2022
Prefer `key.workspace = true` to `key = { workspace = true }`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565
Part 8: #10568

This PR fell out of [this discussion](#10497 (comment)) regarding if `key.workspace = true` is better than `key = { workspace = true }`.

Changes:
- Made all fields into `field.workspace = true`
- Made dependencies into `dep.workspace = true` when a they only specify `{ workspace = true }`

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations as needed
@ehuss ehuss added this to the 1.62.0 milestone Apr 22, 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.

5 participants