Skip to content

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
@notriddle notriddle force-pushed the notriddle/search-speed branch from 5de7521 to a66972d Compare November 18, 2023 01:22
@GuillaumeGomez
Copy link
Member

That's great, thanks a lot!

@bors r+ rollup

@bors
Copy link
Collaborator

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.
@notriddle notriddle force-pushed the notriddle/search-speed branch from 4d5bfe8 to d82b374 Compare November 18, 2023 23:12
@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
Collaborator

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
Collaborator

bors commented Nov 19, 2023

⌛ Testing commit d82b374 with merge 27794f9...

@bors
Copy link
Collaborator

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

5 participants