fix quadratic naming of duplicate sidebar links - #162976
Conversation
|
Implementation looks good to me. Let's confirm the perf gain. Also I'd like @lolbinarycat to have a look as she worked on this recently (iirc). @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fix quadratic naming of duplicate sidebar links
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,21 @@ | |||
| // Regression test for <https://github.com/rust-lang/rust/issues/158174>. | |||
There was a problem hiding this comment.
I feel like this should be a rustc-perf benchmark, not a regular test, since this is a performance improvement, not a bugfix. @GuillaumeGomez what do you think?
There was a problem hiding this comment.
Why not both? =D
But yes, very good idea!
| struct UsedLinks { | ||
| links: FxHashSet<String>, | ||
| /// Next number to try for each anchor. | ||
| next_suffix: FxHashMap<String, usize>, | ||
| } | ||
|
|
||
| fn get_next_url(used_links: &mut UsedLinks, url: String) -> String { | ||
| if used_links.links.insert(url.clone()) { | ||
| return url; | ||
| } | ||
| let mut add = 1; | ||
| while !used_links.insert(format!("{url}-{add}")) { | ||
| add += 1; | ||
| let add = used_links.next_suffix.entry(url.clone()).or_insert(1); | ||
| loop { | ||
| let candidate = format!("{url}-{add}"); | ||
| *add += 1; | ||
| if used_links.links.insert(candidate.clone()) { | ||
| return candidate; | ||
| } | ||
| } |
There was a problem hiding this comment.
Unless I'm missing an edge case somehow, this should be doable with 1 hashmap and no loop.
| struct UsedLinks { | |
| links: FxHashSet<String>, | |
| /// Next number to try for each anchor. | |
| next_suffix: FxHashMap<String, usize>, | |
| } | |
| fn get_next_url(used_links: &mut UsedLinks, url: String) -> String { | |
| if used_links.links.insert(url.clone()) { | |
| return url; | |
| } | |
| let mut add = 1; | |
| while !used_links.insert(format!("{url}-{add}")) { | |
| add += 1; | |
| let add = used_links.next_suffix.entry(url.clone()).or_insert(1); | |
| loop { | |
| let candidate = format!("{url}-{add}"); | |
| *add += 1; | |
| if used_links.links.insert(candidate.clone()) { | |
| return candidate; | |
| } | |
| } | |
| type UsedLinks = FxHashMap<String, usize>; | |
| fn get_next_url(used_links: &mut UsedLinks, url: String) -> String { | |
| let count = used_links.entry(url.clone()).or_insert(0); | |
| let res = if count == 0 { url } else { format!("{url}-{count}") }; | |
| *count += 1; | |
| res | |
| } |
There was a problem hiding this comment.
Yes, a counter is enough and better indeed . Will make the change with a bit of correctness here ( *count == 0) as in the suggested snippet it compares a &mut usize with a number . Which won't compile . Thank you for the review!
|
Finished benchmarking commit (cc18f18): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary 3.8%, secondary -3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 498.333s -> 499.277s (0.19%) |
|
Hum... Do we even have a test to stress test the sidebar? Should we add the new sidebar perf test? |
it doesn't seem like it
yes, that would be good, maybe even add a larger test using the python script in the original issue. |
|
If the perf. regression can be shown with a simple crate, I think it would be fine to add it as a stress test benchmark to |
|
Thanks! I'll open a follow-up rustc-perf PR with a stress benchmark based on the script from #158174 . |
|
Please add the new perf test first, like that we can have a more precise view of the impact of this PR (in addition to the numbers you already provided). |
|
After the next PR is merged by bors, we can do a perf. run here, rustc-perf should now contain the benchmark. |
|
Since it contains the perf, can't we do it now? |
|
Not really, because there would be no parent |
|
We just need a new merge in-between then? So we can wait for next merge on rust. Or I'm missing something obvious. ^^' |
|
Yes, that's in fact what I meant by "After the next PR is merged by bors" 😆 |
|
Ok so we were saying the same thing. Perfect! 🤣 |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fix quadratic naming of duplicate sidebar links
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (16d9f56): 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 rustc-perf 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 -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.8%, secondary -33.9%)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: 501.272s -> 489.678s (-2.31%) |
|
Wow. Now that's an impressive improvement. Congrats! @bors r=lolbinarycat,GuillaumeGomez rollup=iffy |
…r=lolbinarycat,GuillaumeGomez fix quadratic naming of duplicate sidebar links This PR fixes quadratic naming of duplicate sidebar links as discussed in rust-lang#158174. Fixes rust-lang#158174 `get_next_url` gives duplicate sidebar links with unique names but it started counting from 1 every time which makes the page quadratic. So to make it more optimal I changed that to remember the next unique number for each name making it linear per page. Also added the test at `tests/rustdoc-html/deref/sidebar-links-deref-chain.rs` ### Results | N | Instructions (before) | Instructions (after) | Time (before) | Time (after) | |-----:|----------------------:|---------------------:|--------------:|-------------:| | 200 | 5.71 B | 3.69 B | 1.20 s | 1.01 s | | 400 | 22.40 B | 9.52 B | 3.79 s | 1.98 s | | 800 | 126.83 B | 30.82 B | 19.68 s | 6.04 s | | 1600 | — | — | 118.59 s | 37.76 s | r? @GuillaumeGomez
Rollup of 8 pull requests Successful merges: - #162976 (fix quadratic naming of duplicate sidebar links) - #163222 (Enable EII tests for cg_gcc) - #162942 (Remove `StashKey::AssociatedTypeSuggestion`) - #163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - #163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - #163185 (properly decrement available_depth on cycles and provisional cache hits) - #163214 (revert r14 register names for arm) - #163226 (miri subtree update)
Rollup of 8 pull requests Successful merges: - #162976 (fix quadratic naming of duplicate sidebar links) - #163222 (Enable EII tests for cg_gcc) - #162942 (Remove `StashKey::AssociatedTypeSuggestion`) - #163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - #163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - #163185 (properly decrement available_depth on cycles and provisional cache hits) - #163214 (revert r14 register names for arm) - #163226 (miri subtree update)
…uwer Rollup of 14 pull requests Successful merges: - #162976 (fix quadratic naming of duplicate sidebar links) - #161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - #163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - #163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - #163211 (`rustc_builtin_macros` cleanup, part 6) - #161386 (Don't merge distinct impl candidates) - #162942 (Remove `StashKey::AssociatedTypeSuggestion`) - #163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - #163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - #163185 (properly decrement available_depth on cycles and provisional cache hits) - #163214 (revert r14 register names for arm) - #163226 (miri subtree update) - #163228 (Add regression test for trait predicate with escaping bounds) - #163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)
Rollup merge of #162976 - xonx4l:rustdoc-sidebar-quadratic, r=lolbinarycat,GuillaumeGomez fix quadratic naming of duplicate sidebar links This PR fixes quadratic naming of duplicate sidebar links as discussed in #158174. Fixes #158174 `get_next_url` gives duplicate sidebar links with unique names but it started counting from 1 every time which makes the page quadratic. So to make it more optimal I changed that to remember the next unique number for each name making it linear per page. Also added the test at `tests/rustdoc-html/deref/sidebar-links-deref-chain.rs` ### Results | N | Instructions (before) | Instructions (after) | Time (before) | Time (after) | |-----:|----------------------:|---------------------:|--------------:|-------------:| | 200 | 5.71 B | 3.69 B | 1.20 s | 1.01 s | | 400 | 22.40 B | 9.52 B | 3.79 s | 1.98 s | | 800 | 126.83 B | 30.82 B | 19.68 s | 6.04 s | | 1600 | — | — | 118.59 s | 37.76 s | r? @GuillaumeGomez
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (303bef3): 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)This perf run didn't have relevant results for this metric. CyclesResults (secondary -34.5%)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: missing data |
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#162976 (fix quadratic naming of duplicate sidebar links) - rust-lang/rust#161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - rust-lang/rust#163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - rust-lang/rust#163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - rust-lang/rust#163211 (`rustc_builtin_macros` cleanup, part 6) - rust-lang/rust#161386 (Don't merge distinct impl candidates) - rust-lang/rust#162942 (Remove `StashKey::AssociatedTypeSuggestion`) - rust-lang/rust#163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - rust-lang/rust#163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - rust-lang/rust#163185 (properly decrement available_depth on cycles and provisional cache hits) - rust-lang/rust#163214 (revert r14 register names for arm) - rust-lang/rust#163226 (miri subtree update) - rust-lang/rust#163228 (Add regression test for trait predicate with escaping bounds) - rust-lang/rust#163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)
View all comments
This PR fixes quadratic naming of duplicate sidebar links as discussed in #158174.
Fixes #158174
get_next_urlgives duplicate sidebar links with unique names but it started counting from 1 every time which makes the page quadratic.So to make it more optimal I changed that to remember the next unique number for each name making it linear per page.
Also added the test at
tests/rustdoc-html/deref/sidebar-links-deref-chain.rsResults
r? @GuillaumeGomez