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.

Sign up for GitHub

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

Jump to bottom

perf(source): Don't du on every git source load #14252

Merged
merged 1 commit into from
Jul 15, 2024
Merged

perf(source): Don't du on every git source load #14252

merged 1 commit into from
Jul 15, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Jul 15, 2024

What does this PR try to resolve?

When profiling Zed (#14238), a major factor in their no-op run times is git patches and git dependencies. The slowest operation for each git source is running du. This is extraneous for a couple of reasons

  • GC isn't stable, slowing people down for a feature they aren't using
  • Size tracking was expected to be lazy, only reading sizes when the GC is configured for size, while this was eager
  • Git checkouts are immutable but we check on every load
  • This optimized for "while filesystem caches are warm" from a checkout operation when checkout operations are rare compared to all of the other commands run on a working directory.

This removes the du, relying on the lazy loading that happens in update_null_sizes.

For Zed, this removed about 40ms total from the runtime. While by itself, this is below the threshold of being noticed,

  • It adds up if any editor integrations are calling cargo metadata a lot
  • Over time, small gains like this will add up

How should we test and review this PR?

Additional information

cc @ehuss

@epage
perf(source): Don't du on every git source load
cd74470 
When profiling Zed (rust-lang#14238), a major factor in their no-op run times
is git patches and git dependencies.  The slowest operation for each git
source is running `du`.  This is extraneous for a couple of reasons
- GC isn't stable, slowing people down for a feature they aren't using
- Size tracking was expected to be lazy, only reading sizes when the GC
  is configured for size, while this was eager
- Git checkouts are immutable but we check on every load
- This optimized for "while filesystem caches are warm" from a checkout
  operation when checkout operations are rare compared to all of the other
  commands run on a working directory.

This removes the `du`, relying on the lazy loading that happens in
`update_null_sizes`.
@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2024
@ehuss
Copy link
Contributor

ehuss commented Jul 15, 2024

Seems good. I don't recall, but it looks like this was not working as intended. It should have only run du when the checkout was created, not every time it was loaded. I think it's fine to move forward with this. If we ever want to optimize for size, we can always consider a different approach.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2024

📌 Commit cd74470 has been approved by ehuss

It is now in the queue for this repository.

@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 15, 2024
@bors
Copy link
Contributor

bors commented Jul 15, 2024

⌛ Testing commit cd74470 with merge a2d45dc...

@bors
Copy link
Contributor

bors commented Jul 15, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing a2d45dc to master...

@bors bors merged commit a2d45dc into rust-lang:master Jul 15, 2024
22 checks passed
@epage epage deleted the du-less branch July 15, 2024 18:27
@weihanglo weihanglo mentioned this pull request Jul 17, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
@bors
Auto merge of rust-lang#127838 - weihanglo:update-cargo, r=weihanglo
cb12b52 
Update cargo

31 commits in 154fdac39ae9629954e19e9986fd2cf2cdd8d964..a2b58c3dad4d554ba01ed6c45c41ff85390560f2
2024-07-07 01:28:23 +0000 to 2024-07-16 00:52:02 +0000
- chore(ci): bump CI tools (rust-lang/cargo#14257)
- test: migrate fetch and list_availables to snapbox (rust-lang/cargo#14214)
- chore: downgrade to jobserver@0.1.28 (rust-lang/cargo#14254)
- perf(source): Don't `du` on every git source load (rust-lang/cargo#14252)
- fix(source): Don't warn about unreferenced duplicate packages (rust-lang/cargo#14239)
- feat(test): Add cargo_test to test-support prelude (rust-lang/cargo#14243)
- Add workflow to publish Cargo automatically (rust-lang/cargo#14202)
- test: migrate implicit_features to snapbox (rust-lang/cargo#14245)
- test: migrate build-std/main to snapbox (rust-lang/cargo#14241)
- test: migrate check_cfg to snapbox (rust-lang/cargo#14235)
- refactor(source): More RecursivePathSource clean up (rust-lang/cargo#14231)
- Add more profiling traces (rust-lang/cargo#14238)
- fix(overrides): Don't warn on duplicate packages from using '..' (rust-lang/cargo#14234)
- fix(test): Redact elapsed time in the minutes time frame (rust-lang/cargo#14233)
- test: Migrate lto tests to snapbox (rust-lang/cargo#14209)
- fix: Ensure dep/feature activates the dependency on 2024 (rust-lang/cargo#14221)
- chore(docs): update index of reference (rust-lang/cargo#14228)
- test: migrate test to snapbox (rust-lang/cargo#14226)
- chore: remove duplicate words (rust-lang/cargo#14229)
- docs(contrib): Document things I look for in RFCs (rust-lang/cargo#14222)
- docs(ref): Note MSRV for features in the docs (rust-lang/cargo#14224)
- test(progress): Resolve flakiness (rust-lang/cargo#14223)
- fix(test): Reduce over-prescription to the caller (rust-lang/cargo#14217)
- refactor: move get_source_id out of registry (rust-lang/cargo#14218)
- fix: rename to `rustdoc::broken_intra_doc_links` (rust-lang/cargo#14215)
- test: migrate member_errors, multitarget and new to snapbox (rust-lang/cargo#14210)
- test: migrate generate_lockfile and glob_targets to snapbox (rust-lang/cargo#14200)
- test: Ensure --list test works with cargo-bloat (rust-lang/cargo#14213)
- dont make new constant InternedString in hot path (rust-lang/cargo#14211)
- Fix compatible_with_older_cargo test. (rust-lang/cargo#14212)
- test: migrate metabuild, metadata and net_config to snapbox (rust-lang/cargo#14162)
@rustbot rustbot added this to the 1.81.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@weihanglo weihanglo

Labels
A-git Area: anything dealing with git S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
5 participants
@epage @rustbot @ehuss @bors @weihanglo