rustdoc: optimize impl sorting#157179
Conversation
Currently impls printed in alternate mode use plain text for the first part and HTML for the where clause. This commit fixes the where clause so it's also plain text.
These sorts are hot, so it's significantly faster to use concise plain text rather than verbose HTML.
We don't need to because the char ordering works out naturally.
rustbot
added
S-waiting-on-author
|
LLM disclosure: I wrote the three commits by hand after viewing profiling data. I used an LLM to review the commits before submitting. The LLM identified that |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
rustdoc: optimize impl sorting
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e444e2a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -3.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.2%, secondary -4.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 513.315s -> 511.874s (-0.28%) |
rustbot
removed
the
S-waiting-on-perf
|
Nice wins! Have you tried replacing Btw I noticed at https://doc.rust-lang.org/std/cell/struct.Cell.html#trait-implementations, then in the bar on the left, |
Yes... this approach gets about 90-95% of the improvement of replacing
Good question, I'm not sure. I just checked with my local builds. The orders are unchanged by this PR, so I think I've successfully preserved the existing behaviour. Which seems reasonable for an optimization PR. Any bugs should probably be fixed in a separate PR. |
rustbot
added
S-waiting-on-review
|
Looks great, thanks! @bors r+ rollup |
rust-bors
Bot
added
S-waiting-on-bors
|
Woups, perf sensitive. @bors rollup=never |
This comment has been minimized.
This comment has been minimized.
I meant combining this PR with replacing compare_names with just cmp. |
rust-bors
Bot
added
merged-by-bors
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 9e293ae (parent) -> 1ce45a0 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1ce45a011a3fe99ab08643d8eb7229bfc3b40bda --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (1ce45a0): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.2%, secondary -5.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 508.947s -> 513.507s (0.90%) |
I have a fix for this in #157233. |
…pls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…pls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…pls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…pls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…pls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
Rollup merge of #157540 - yotamofek:pr/rustdoc/render_all_impls-cleanup, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to #157233 and #157179 (cc @nnethercote - thanks!) This kinda undoes f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…up, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang/rust#157233 and rust-lang/rust#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang/rust@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
…up, r=GuillaumeGomez Cleanup and optimize `render_impls` - take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting Somewhat of a follow-up to rust-lang/rust#157233 and rust-lang/rust#157179 (cc @nnethercote - thanks!) This kinda undoes rust-lang/rust@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it. r? @GuillaumeGomez LLM disclosure: I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)
5 participants
Currently rustdoc sorts impls in a couple of places using long HTML string representations of the impls. Using plain text representations instead speeds things up. Details in individual commits.