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

Add rustdoc JS search benchmark in performance checks #118056

Open
GuillaumeGomez opened this issue Nov 19, 2023 · 10 comments
Open

Add rustdoc JS search benchmark in performance checks #118056

GuillaumeGomez opened this issue Nov 19, 2023 · 10 comments
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@GuillaumeGomez
Copy link
Member

@notriddle wrote a tool to benchmark performance for the rustdoc search. I think it's an important enough topic to watch if there are any changes (both positive and negative). Only question is how to integrate it in the current tool.

Example of usage of the tool in #118024.

cc @lqd @nnethercote (if you have any idea/suggestion?)

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 19, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 19, 2023
@lqd
Copy link
Member

lqd commented Nov 19, 2023

Most likely cc @Kobzol (and Mark but I won't ping him he has too much on his plate already)

@Kobzol
Copy link
Contributor

Kobzol commented Nov 19, 2023

If I understand it correctly, the benchmark measures how fast can the webpage generated by Rustdoc search within its index, via the search box at the top of the documentation page?

@GuillaumeGomez
Copy link
Member Author

To be more precise: running the search JS on a given query. Like we do in rustdoc-js and rustdoc-js-std so only a JS runner is needed (and not a web browser).

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 19, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Nov 21, 2023

I see. I'm not sure if specialized benchmarks like this are a good fit for rustc-perf at the moment. It would be great to include them there in the long run (perhaps along with cargo bench benchmarks from the stdlib, or some other custom benchmarks), but it's not really prepared for something like this at the moment, so it would need non-trivial infrastructure/DB changes. We're currently planning to expand the suite to multiple collector machines, which will require large architecture changes anyway, so perhaps after that is done, we can think about including more benchmarks that do not conform to the current compile/runtime benchmark scheme.

Out of curiosity, a few questions:

  1. How long does it take to run the benchmarks?
  2. What do you need to run the benchmarks? Is it enough to have a Rust toolchain downloaded from rustup (or rustup-toolchain-install-master)?
  3. What is the output of the benchmarks? Does it measure time, or also other things (memory, instruction counts)? Does it provide a single value or a distribution of measured values?

For now, I think that the first step could be to include this benchmark either in rust-lang/rust, or in rustc-perf, but only locally, so that it could be executed by rustc developers, but it wouldn't be running on CI.

@notriddle
Copy link
Contributor

I was planning to iterate on this out-of-tree before trying to integrate it upstream. The benchmark suite isn't very mature or statistically rigorous, I wouldn't want it to block other kinds of improvements (like new features or reductions in code or index size), and I'm still experimenting with different Node profilers.

How long does it take to run the benchmarks?

About 20 minutes total, most of it spent running cargo doc on Tor Arti (building both Arti itself and all its dependencies). 8 core x64 desktop.

What do you need to run the benchmarks? Is it enough to have a Rust toolchain downloaded from rustup (or rustup-toolchain-install-master)?

You need Node.JS and Rust.

It also uses a Node.JS tool called 0x to write out the flame graph, which can be installed from NPM.

What is the output of the benchmarks? Does it measure time, or also other things (memory, instruction counts)? Does it provide a single value or a distribution of measured values?

Right now, it's wall time, plus a flame graph.

@nnethercote
Copy link
Contributor

I have reservations about this, mostly because it's an entirely separate benchmark that has no overlap with the existing benchmark.

@GuillaumeGomez
Copy link
Member Author

It'd be nice to be able to trigger a specific benchmark for cases like this one. Like rustdoc got updated => running search JS perf to see if there is any change. Would be quite nice. :)

@notriddle
Copy link
Contributor

In short, it’s not ready to merge upstream.

@Kobzol
Copy link
Contributor

Kobzol commented Nov 22, 2023

It'd be nice to be able to trigger a specific benchmark for cases like this one. Like rustdoc got updated => running search JS perf to see if there is any change. Would be quite nice. :)

Even if it was easy to integrate into perf.RLO, it's a bit problematic to run benchmarks only on demand (we're dealing with a similar problem with clippy). Because if you only run on demand, you don't have benchmark data available for the parent commit (however, the commit probably was already benchmarked, so we have some data for it in the DB). Which means that we would need to add a way in perf.RLO to run benchmarks on the same commit multiple times, which is not possible at the moment. But probably we will need something like this when switching to multiple collector machines.

@GuillaumeGomez
Copy link
Member Author

Not just on demand but also specific to some paths. If you don't update rustdoc, there is not much point to run search benches.

@GuillaumeGomez GuillaumeGomez added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants