rustdoc: reduce GC work during search #83077

notriddle · 2021-03-13T06:59:23Z

No description provided.

There is no reason for this function to return an object, since it is always used for getting at the name anyhow. It's used in the inner loop for some popular functions, so we want to avoid allocating in it.

rust-highfive · 2021-03-13T06:59:26Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

rust-highfive · 2021-03-13T06:59:27Z

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

Every time splice() is called, another temporary object is created. This version, which uses plain objects as a sort of Hash Bag, should only produce one temporary object each time it's called.

Basically, it doesn't make sense to generate those things every time you search. That generates a bunch of stuff for the GC to clean up, when, if the user wanted to do another search, it would just need to re-do it again.

src/librustdoc/html/static/main.js

GuillaumeGomez · 2021-03-13T13:04:43Z

So overall, you reduce the GC but you instead use more memory (a lot more from what I can see). Not sure how good of a tradeoff this is...

Co-authored-by: Guillaume Gomez <guillaume1.gomez@gmail.com>

notriddle · 2021-03-13T16:40:29Z

So overall, you reduce the GC but you instead use more memory (a lot more from what I can see). Not sure how good of a tradeoff this is...

If it's controversial, I can skip the nameWithoutUnderscores and id changes while keeping the rest of it. None of the other tweaks should increase the memory usage.

jyn514 · 2021-03-13T17:37:23Z

Do you have benchmarks for how much this helps or hurts performance?

GuillaumeGomez · 2021-03-13T18:45:48Z

Having benchmarks (on the std docs because it's big enough to see any impact) would definitely be appreciated here. What we want to check is the performance improvement and the memory usage.

notriddle · 2021-03-13T22:46:30Z

Running this thing in the Web Console does a pretty good job of consistently recording how long a search takes, without having a bunch of other noise dominating the measurements. The new version spend about 634 ms in the GC, and 3,238 ms in the old version.

getSearchInput().focus(); focus = new FocusEvent("focus", {cancelable: true, bubbles: true}); getSearchInput().dispatchEvent(focus); getSearchInput().value = "Vec<AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,AA,BA,CA,DA,EA,FA,GA,HA,IA,JA,KA,LA,>"; focus = new InputEvent("change", {cancelable: true, bubbles: true}); getSearchInput().dispatchEvent(focus); setTimeout(() => {console.profile();document.getElementsByClassName("search-form")[0].onsubmit(focus); setTimeout(console.profileEnd);}, 100);

GuillaumeGomez · 2021-03-14T10:52:29Z

That seems very surprising to see such a difference. An important point though: the search-index is loaded when the search input gets the focus, not when you start a search.

Also: can you get the memory diff somehow? That'd be really interesting too.

notriddle · 2021-03-14T17:03:05Z

According to the Firefox memory profiler, these changes raised the memory use from 26MiB to 31MiB.

And, reading through the code again, I found two more tweaks that significantly help with memory consumption:

row.id doesn't have any reason to be a string. It's only used for indexing anyhow, so it might as well be a number
nameWithoutUndescores doesn't necessarily need to be a fresh allocation, since most names don't have underscores in them

With those two tweaks applied, we're down to 28MiB. Still higher than the original 26, because strings have been raised from 2 -> 3, and objects have been raised from 14 to 15.

This should have negligible effect on time, but it cuts about 1MiB off of resident memory usage.

There's no reason for it to be a string, since it's only used for de-duplicating the results arrays anyhow.

GuillaumeGomez · 2021-03-14T17:35:27Z

I think the tradeoff is now very acceptable. Thanks a lot for the improvements! I think the CI will fail because you renamed getObjectFromId to getObjectNameFromId. You'll need to update src/tools/rustdoc-js (just grep the old function name) to fix it.

GuillaumeGomez · 2021-03-14T17:55:01Z

Apparently you don't need to update the tool. However, this failure means that the search behavior has been modified.

This basically fixes a search bug introduced by earlier changes.

notriddle · 2021-03-14T21:45:54Z

@GuillaumeGomez

Yep. Turns out I forgot to account for case folding.

src/librustdoc/html/static/main.js

GuillaumeGomez · 2021-03-15T20:35:47Z

@bors: r+

bors · 2021-03-15T20:35:48Z

📌 Commit dcba95f has been approved by GuillaumeGomez

…, r=GuillaumeGomez rustdoc: reduce GC work during search

Rollup of 10 pull requests Successful merges: - rust-lang#81822 (Added `try_exists()` method to `std::path::Path`) - rust-lang#83072 (Update `Vec` docs) - rust-lang#83077 (rustdoc: reduce GC work during search) - rust-lang#83091 (Constify `copy` related functions) - rust-lang#83156 (Fall-back to sans-serif if Arial is not available) - rust-lang#83157 (No background for code in portability snippets) - rust-lang#83160 (Deprecate RustcEncodable and RustcDecodable.) - rust-lang#83162 (Specify *.woff2 files as binary) - rust-lang#83172 (More informative diagnotic from `x.py test` attempt atop beta checkout) - rust-lang#83196 (Use delay_span_bug instead of panic in layout_scalar_valid_range) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Get rid of the garbage produced by getObjectFromId

5fe3b87

There is no reason for this function to return an object, since it is always used for getting at the name anyhow. It's used in the inner loop for some popular functions, so we want to avoid allocating in it.

rust-highfive assigned GuillaumeGomez Mar 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021

notriddle force-pushed the gc-cleanup-rustdoc-search branch from 4627c7c to 4be594e Compare March 13, 2021 07:02

notriddle added 2 commits March 13, 2021 00:02

In checkGenerics and checkType, don't use Array.prototype.splice so much

d7971e5

Every time splice() is called, another temporary object is created. This version, which uses plain objects as a sort of Hash Bag, should only produce one temporary object each time it's called.

Eagerly generate the underscore-less name to search on

3f70bfa

Basically, it doesn't make sense to generate those things every time you search. That generates a bunch of stuff for the GC to clean up, when, if the user wanted to do another search, it would just need to re-do it again.

notriddle force-pushed the gc-cleanup-rustdoc-search branch from 4be594e to 3f70bfa Compare March 13, 2021 07:04

This comment has been minimized.

Sign in to view