Skip to content

rustdoc_json: reduce allocations #142335

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
Merged
merged 4 commits into from
Jun 15, 2025
Merged

rustdoc_json: reduce allocations #142335

merged 4 commits into from
Jun 15, 2025

Conversation

nnethercote
Copy link
Contributor

These commits reduce the number of allocations done for rustdoc_json, mostly by avoiding unnecessary clones.

Best reviewed one commit at a time.

r? @aDotInTheVoid

@nnethercote
Simplify JsonRenderer.
cc67175 
- It doesn't need to be cloneable.
- Some of the `Rc`s and `RefCell`s aren't doing anything.
- `after_krate` can consume `self`.
@nnethercote
Avoid cloning self.index in after_krate.
195c985 
It can be very big.

This reduces peak memory usage for some `--output-format=json` runs by
up to 8%.
@nnethercote
Avoid more clones in rustdoc JSON output.
79b3c08 
By making `JsonRenderer::item` take `&clean::Item` instead of a
`clean::Item`. This required also changing `FromClean` and `IntoJson`
methods to take references, which required a lot of follow-on sigil
wrangling that is mostly tedious.
@nnethercote
Don't clone new_item in after_krate.
278f4b2 
We can avoid it by using the `entry` API, which lets us do the
`assert_eq` comparison before `new_item` is consumed.
@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jun 11, 2025
@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 11, 2025 •
edited
Loading

rustc-perf doesn't measure rustdoc_json so a perf run won't tell us anything. But I modified rustc-perf locally to use --output-format=json --document-private-items --document-hidden-items (what cargo-semver-checks does) and did a local run on my Linux box.

wall-time reductions:

aa-walltime

max-rss (peak memory) reductions:

aa-max-rss

(You can ignore the red entries. There's enough noise in these metrics that even on clear improvements there will usually be one or two benchmarks that look like a regression, but they're not.)

Other metrics like instruction counts, cycles, cache-misses, page faults, and allocation counts were all improved as well.

@nnethercote nnethercote mentioned this pull request Jun 11, 2025
Open
@nnethercote
Copy link
Contributor Author

cc @obi1kenobi

@aDotInTheVoid
Copy link
Member

Amazing work!

Also, thanks so much for splitting into different commits, it make it a breeze to review.

@bors r+ rollup=never

(Is that needed here? This is perf sensitive 🤷‍♀️)

@bors
Copy link
Collaborator

bors commented Jun 11, 2025

📌 Commit 278f4b2 has been approved by aDotInTheVoid

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 Jun 11, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jun 11, 2025

It's nice if it's rollup=never, since we can then check at least manually what was the effect on the CI built artifacts, since it's now possible in rustc-perf.

@aDotInTheVoid aDotInTheVoid mentioned this pull request Jun 11, 2025
Open
14 tasks
@aDotInTheVoid aDotInTheVoid removed the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Jun 11, 2025
@bors
Copy link
Collaborator

bors commented Jun 14, 2025

⌛ Testing commit 278f4b2 with merge 38c41d0...

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

☀️ Test successful - checks-actions
Approved by: aDotInTheVoid
Pushing 38c41d0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2025
@bors bors merged commit 38c41d0 into rust-lang:master Jun 15, 2025
11 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 15, 2025
@github-actions GitHub Actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 49a8ba0 (parent) -> 38c41d0 (this PR)

Test differences

No test diffs found

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 38c41d0f926d77985fc17087c21eeb7896dfe61e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. x86_64-apple-2: 5128.8s -> 3163.2s (-38.3%)
  2. dist-x86_64-apple: 7570.8s -> 9748.4s (28.8%)
  3. x86_64-apple-1: 7297.8s -> 5802.8s (-20.5%)
  4. mingw-check-1: 2005.6s -> 1634.0s (-18.5%)
  5. x86_64-rust-for-linux: 2982.7s -> 2496.6s (-16.3%)
  6. dist-ohos-aarch64: 4157.9s -> 4662.1s (12.1%)
  7. i686-gnu-1: 7919.9s -> 7114.6s (-10.2%)
  8. x86_64-gnu-aux: 6368.6s -> 5750.3s (-9.7%)
  9. i686-gnu-nopt-1: 7874.7s -> 7141.2s (-9.3%)
  10. dist-apple-various: 6075.0s -> 5533.6s (-8.9%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

This was referenced Jun 15, 2025
Open
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (38c41d0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.2% [2.3%, 4.1%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.9% [-1.9%, -1.9%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 758.463s -> 757.13s (-0.18%)
Artifact size: 372.25 MiB -> 372.17 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aDotInTheVoid aDotInTheVoid

Labels
A-rustdoc-json Area: Rustdoc JSON backend 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @aDotInTheVoid @bors @Kobzol @rust-timer @rustbot