Clean package perf improvements #13818

osiewicz · 2024-04-28T11:05:10Z

What does this PR try to resolve?

I've noticed that cargo clean -p execution time scales poorly with size of target directory; in my case (~250GB target directory on M1 Mac) running cargo clean -p takes circa 35 seconds. Notably, it's the file listing that takes that time, not deleting the package itself. That is, when running cargo clean -p SOME_PACKAGE twice in a row, both executions take roughly the same time.

I've tracked it down to the fact that we seem quite happy to use glob::glob function, which iterates over contents of target dir. It also was a bit sub-optimal when it came to doing that, for which I've already filled a PR in rust-lang/glob#144 - that PR alone takes down cleaning time down to ~14 seconds. While it is a good improvement for a relatively straightforward change, this PR tries to take it even further. With glob PR applied + changes from this PR, my test case goes down to ~6 seconds. I'm pretty sure that we could squeeze this further, but I'd rather do so in a follow-up PR.

Notably, this PR doesn't help with just super-large target directories. cargo clean -p serde on cargo repo (with ~7Gb target directory size) went down from ~380ms to ~100ms for me. Not too shabby.

How should we test and review this PR?

I've mostly tested it manually, running cargo clean against multiple different repos.

Additional information

TODO:

c770700 is not quite correct; we need to consider that it changes how progress reporting works; as is, we're gonna report all progress relatively quickly and stall at the end (when we're actually iterating over directories, globbing, removing files, that kind of jazz). I'll address that.

…_types

rustbot · 2024-04-28T11:05:14Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

epage · 2024-04-29T15:43:12Z

#5931 will require re-working the target directory to be organized around package names. Depending on the complexity this entails, I wonder if its better to wait on that. Granted, my bias is that I never use cargo clean -p and instead do cargo clean.

osiewicz · 2024-04-29T15:50:28Z

Ah, I see; thanks for pointing that issue out. That's definitely another good argument for not squeezing every last bit out of cargo clean -p if it's gonna be reworked in near-ish term.
The only pending item I want to address is that TODO for progress reporting; I expect this PR to be ready for review tomorrow-ish.

Starting with this commit we deduplicate calls to rm_rf_prefix_list by crate name and not by directory; this can lead to more calls to rm_rf_prefix_list (especially in presence of multiple -p arguments), but it is also more transparent in terms of progress reporting (we're just storing away whether a given directory + glob pair has already been removed)

weihanglo

Thanks for the contribution!

src/cargo/ops/cargo_clean.rs

Change Rc<str> to &str Co-authored-by: Weihang Lo <[email protected]>

weihanglo

Thanks!

weihanglo · 2024-05-02T13:58:22Z

@bors r+

bors · 2024-05-02T13:58:27Z

📌 Commit 9c1139e has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-05-02T13:59:34Z

⌛ Testing commit 9c1139e with merge 27d3e3d...

bors · 2024-05-02T14:35:00Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 27d3e3d to master...

Update cargo 18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f 2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000 - chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857) - Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571) - fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852) - refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851) - perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849) - Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850) - chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848) - Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845) - Clean package perf improvements (rust-lang/cargo#13818) - fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841) - fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839) - chore(ci): Ignore openssl deps (rust-lang/cargo#13840) - fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837) - fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791) - fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836) - chore(deps): update compatible (rust-lang/cargo#13834) - Error when unstable lints are specified but not enabled (rust-lang/cargo#13805) - fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833) r? ghost Note: this includes the fix that was beta backported in rust-lang#124647

Update cargo 18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f 2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000 - chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857) - Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571) - fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852) - refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851) - perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849) - Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850) - chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848) - Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845) - Clean package perf improvements (rust-lang/cargo#13818) - fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841) - fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839) - chore(ci): Ignore openssl deps (rust-lang/cargo#13840) - fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837) - fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791) - fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836) - chore(deps): update compatible (rust-lang/cargo#13834) - Error when unstable lints are specified but not enabled (rust-lang/cargo#13805) - fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833) r? ghost Note: this includes the fix that was beta backported in #124647

osiewicz added 2 commits April 27, 2024 21:11

Do not call rm_rf_glob in a loop for paths that do not depend on file…

d4c83ee

…_types

Add rm_rf_prefix_list

9c685ab

rustbot assigned ehuss Apr 28, 2024

rustbot added Command-clean S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2024

Clean non-filetype based entries just once

c770700

osiewicz force-pushed the clean-package-perf-improvements branch from e2a1df8 to c770700 Compare April 28, 2024 11:09

osiewicz marked this pull request as ready for review April 29, 2024 22:36

Clean up commentary for new code

634dca4

weihanglo reviewed May 2, 2024

View reviewed changes

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

osiewicz and others added 3 commits May 2, 2024 12:29

Update src/cargo/ops/cargo_clean.rs

a49a2b9

Change Rc<str> to &str Co-authored-by: Weihang Lo <[email protected]>

fixup! Update src/cargo/ops/cargo_clean.rs

574d086

feedback: rewrite doc comment for rm_rf_prefix_list

9c1139e

weihanglo approved these changes May 2, 2024

View reviewed changes

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 May 2, 2024

bors merged commit 27d3e3d into rust-lang:master May 2, 2024
21 checks passed

Muscraft mentioned this pull request May 3, 2024

Update cargo rust-lang/rust#124684

Merged

rustbot added this to the 1.80.0 milestone May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean package perf improvements #13818

Clean package perf improvements #13818

osiewicz commented Apr 28, 2024 •

edited

Loading

rustbot commented Apr 28, 2024

epage commented Apr 29, 2024

osiewicz commented Apr 29, 2024

weihanglo left a comment

weihanglo left a comment

weihanglo commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

Clean package perf improvements #13818

Clean package perf improvements #13818

Conversation

osiewicz commented Apr 28, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Apr 28, 2024

epage commented Apr 29, 2024

osiewicz commented Apr 29, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

osiewicz commented Apr 28, 2024 •

edited

Loading