Github rustdoc: reduce GC work during search by notriddle · Pull Request #83077...

Copy link

Contributor

rust-highfive commented 21 days ago

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

rust-highfive commented 21 days ago

r? @GuillaumeGomez

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

This comment has been hidden.

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...

notriddle commented 20 days ago

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.

This comment has been hidden.

jyn514 commented 20 days ago

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

This comment has been hidden.

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 commented 20 days ago

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);

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 commented 19 days ago

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 comment has been hidden.

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.

This comment has been hidden.

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

notriddle commented 19 days ago

@GuillaumeGomez

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

bors commented 18 days ago

Commit dcba95f has been approved by GuillaumeGomez