Skip to content

Speed up remote Cargo.toml deserialization - #17296

Draft
zozo123 wants to merge 1 commit into
rust-lang:masterfrom
zozo123:agent/registry-manifest-cache
Draft

Speed up remote Cargo.toml deserialization#17296
zozo123 wants to merge 1 commit into
rust-lang:masterfrom
zozo123:agent/registry-manifest-cache

Conversation

@zozo123

@zozo123 zozo123 commented Aug 1, 2026

Copy link
Copy Markdown

What does this resolve?

Cargo retained an owned spanned TOML document for every manifest, including cached remote registry and git packages. Those remote packages do not need a retained document after deserialization.

The source policy is explicit and exhaustive:

Source kind Parse mode
Path, local registry, directory/vendor, embedded Deserialize and retain the spanned document
Git, remote registry, sparse registry Deserialize directly; do not retain the document

The direct path still uses serde_ignored and emit_toml_diagnostic, preserving unused-key tracking plus diagnostic messages and spans. Only document cloning and ownership are skipped.

Review feedback

  • Atomicity: one parse-only commit; the unrelated dep-info change is removed.
  • Locality: an explicit SourceKind match replaces the broad !is_path() classification.
  • Structure: the logic is inline in read_manifest; there is no helper module.
  • Methodology: the earlier combined Zed run is not presented as evidence for this isolated head.

Validation

  • cargo +1.97.0 fmt --all -- --check
  • parser unit tests for diagnostic and unused-key equivalence
  • cargo +1.97.0 test -p cargo --test testsuite registry::remote_registry_manifest_without_document

@rustbot rustbot added A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2026
@rustbot

rustbot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @epage (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
Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @epage, @weihanglo
  • @epage, @weihanglo expanded to epage, weihanglo
  • Random selection from epage, weihanglo

Comment thread src/compiler/fingerprint/mod.rs

@weihanglo weihanglo Aug 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: You seems to open a bunch of PRs addressing performance issues recently. It would be good to chime in with us on Zulip or open an issue dicussing what you're looking at and what your methodology is, so that we can be more aligned on the project development direction and expectations. We also have issues with Performance label that capture more algorithmic and architectural possibility to improve Cargo's performance.

View changes since the review

Comment thread src/workspace/parser/mod.rs Outdated
Comment thread src/workspace/parser/registry_cache.rs Outdated
Comment thread src/compiler/fingerprint/mod.rs Outdated
@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for the review — addressed in the force-pushed update:

  1. Atomic: dropped the empty dep-info early-return from this PR (will measure in isolation before proposing anything there).
  2. Local gate: full spanned path when source_id.is_path() || is_embedded; light toml::from_str otherwise (registry and git, not only is_registry()).
  3. No helper file: inlined in read_manifest.

Also noted on methodology / not opening a burst of mixed perf PRs — happy to sync on Zulip as you suggested.

@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

@rustbot review

@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

Converting this PR back to draft.

After re-review, this is not merge-ready:

  1. Source boundary is wrong as written. !is_path() is not “non-local”: it also includes local registries and directory/vendor sources, which can be user-controlled and may need full diagnostics.
  2. Behavior change, not just speed. Fast path uses toml::from_str + document = None, which can change unused-key warnings, parse error presentation, and span-backed diagnostics. The existing test only checks a valid registry dep builds twice.
  3. Cited A/B does not measure current head. The previous hosted run bundled empty dep-info + a different classification. Current-head performance is not isolated.

Next before leaving draft:

  • explicit SourceKind → parse-mode map (no wildcard);
  • stderr-equivalence tests (malformed registry/git, unknown keys, local registry, vendor);
  • exact current-base vs current-head A/B for parse only.

Sorry for the premature “ready” framing.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2026
@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

Taking back out of draft for review after rustfmt fix.

Scope remains: faster non-local manifest deserialize (toml::from_str when not path/embedded). Happy to tighten the SourceKind map or add stderr-equivalence tests as follow-ups if reviewers want them as blocking items.

@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

@rustbot review

@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

Back to draft until review concerns are actually closed

Thanks again @weihanglo @epage. Taking this out of ready so we don’t ask for merge review while these are open:

  1. Atomicity — dep-info already removed; keep this PR parse-only.
  2. Source boundary — need an explicit SourceKind map (not a vague “non-local” story). Path/embedded full diagnostics is right; remote registry/git are the intended fast path; local registry / directory / vendor must be decided deliberately, not via !is_path().
  3. Diagnostics — need stderr-equivalence tests (malformed + unknown keys) before claiming behavior-preserving.
  4. Evidence — current-head isolated A/B (not the older combined run).
  5. Process — will post a short methodology note on Zulip before further Cargo perf PRs, as requested.

I will only mark ready again when (2)+(3) are in the branch. No more ready/draft thrash.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2026
@zozo123

zozo123 commented Aug 1, 2026

Copy link
Copy Markdown
Author

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Aug 1, 2026
@rustbot

rustbot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants