Skip to content
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.

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

rustdoc-search: optimize unifyFunctionTypes #118024

Merged
merged 4 commits into from
Nov 19, 2023

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Nov 17, 2023

Final profile output:
https://notriddle.com/rustdoc-html-demo-5/profile-4/index.html

This PR contains three commits that improve performance of this hot inner loop: reduces the number of allocations, a fast path for the 1-element basic query case, and reconstructing the multi-element query case to use recursion instead of an explicit backtracking array. It also adds new test cases that I found while working on this.

r? @GuillaumeGomez

@rustbot rustbot added 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. labels Nov 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@notriddle notriddle force-pushed the notriddle/search-speed branch 2 times, most recently from b436a38 to 5de7521 Compare November 18, 2023 00:58
This is a major source of expense on generic queries,
and this commit reduces them.

Profile output:
https://notriddle.com/rustdoc-html-demo-5/profile-2/index.html
Short queries, in addition to being common, are also the base
case for a lot of more complicated queries. We can avoid
most of the backtracking data structures, and use simple
recursive matching instead, by special casing them.

Profile output:
https://notriddle.com/rustdoc-html-demo-5/profile-3/index.html
@GuillaumeGomez
Copy link
Member

That's great, thanks a lot!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 18, 2023

📌 Commit a66972d has been approved by GuillaumeGomez

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 Nov 18, 2023
@notriddle
Copy link
Contributor Author

@bors r-

One more commit that I came up with while waiting for the review. Sorry for the noise!

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 18, 2023
@notriddle
Copy link
Contributor Author

@GuillaumeGomez

Again, sorry about the noise. Here's the last commit, which switches from using a backtracking array to using functional recursion to implement the full algorithm:

  • The "fast path" now becomes the "base case" that the recursive function builds upon. This results in a significant speed up for the query T, U, though it still doesn't perform very well.

  • It reduces the amount of code by a lot. This full stack of PR's is now, on net, two lines of code added to search.js.

    $ git diff --stat 4d7f952a02d0bca67c98a6b74895b7e3fbe38341 src/librustdoc 
     src/librustdoc/html/static/js/search.js | 302 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------
     1 file changed, 152 insertions(+), 150 deletions(-)
  • Here's the new profiler results: https://notriddle.com/rustdoc-html-demo-5/profile-4/index.html

This is significantly faster, because

- It allows the one-element fast path to kick in on multi-
  element queries.
- It constructs intermediate data structures more lazily
  than the old system did.

It's measurably faster than the old algo even without the fast path, but
that fast path still helps significantly.
@GuillaumeGomez
Copy link
Member

New changes look good to me. Big question though: would it be possible to include your benchmark in the rust performance tool? I think it's becoming quite an important topic to have good performance when performing a search.

In any case, thanks for this PR!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 19, 2023

📌 Commit d82b374 has been approved by GuillaumeGomez

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2023
@bors
Copy link
Contributor

bors commented Nov 19, 2023

⌛ Testing commit d82b374 with merge 27794f9...

@bors
Copy link
Contributor

bors commented Nov 19, 2023

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 27794f9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2023
@bors bors merged commit 27794f9 into rust-lang:master Nov 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (27794f9): 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

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

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

Bootstrap: 676.326s -> 676.664s (0.05%)
Artifact size: 313.84 MiB -> 313.81 MiB (-0.01%)

@notriddle notriddle deleted the notriddle/search-speed branch November 19, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants