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.

Sign up for GitHub

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

Jump to bottom

rustdoc-search: depth limit T<U> -> U unboxing #122247

Merged
merged 2 commits into from
Mar 14, 2024
Merged

rustdoc-search: depth limit T<U> -> U unboxing #122247

merged 2 commits into from
Mar 14, 2024

Conversation

notriddle
Copy link
Contributor

Profiler output:
https://notriddle.com/rustdoc-html-demo-9/search-unbox-limit/ (the only significant change is that one of the rust tests went from 378416ms to 16ms).

This is a performance enhancement aimed at a problem I found while using type-driven search on the Rust compiler. It is caused by Interner, a trait with 41 associated types, many of which recurse back to Self again.

This caused search.js to struggle. It eventually terminates, after about 10 minutes of turning my PC into a space header, but it's doing 41! unifications and that's too slow.

@notriddle
rustdoc-search: depth limit T<U> -> U unboxing
bcc3f19 
Profiler output:
https://notriddle.com/rustdoc-html-demo-9/search-unbox-limit/

This is a performance enhancement aimed at a problem I found while
using type-driven search on the Rust compiler. It is caused by
[`Interner`], a trait with 41 associated types, many of which
recurse back to `Self` again.

This caused search.js to struggle. It eventually terminates,
after about 10 minutes of turning my PC into a space header, but it's
doing `41!` unifications and that's too slow.

[`Interner`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/trait.Interner.html
@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Mar 9, 2024
@notriddle notriddle marked this pull request as ready for review March 9, 2024 18:03
@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@GuillaumeGomez
Copy link
Member

This is a great improvement, thanks!

Could you add a rustdoc-js test for it and also add a time limit to ensure it doesn't take more than 10 seconds (to be large) to ensure this regression doesn't appear again? Maybe we should make this limit by default for all rustdoc JS search tests...

@fmease fmease 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024
@fmease fmease assigned GuillaumeGomez and unassigned fmease Mar 11, 2024
@notriddle
Copy link
Contributor Author

Could you add a rustdoc-js test for it

Sure, the second commit now adds that.

also add a time limit to ensure it doesn't take more than 10 seconds

I can't find any way to make compiletest do that, or precedent in other parts of the compiler to use it that way. Am I missing something?

@GuillaumeGomez
Copy link
Member

I can't find any way to make compiletest do that, or precedent in other parts of the compiler to use it that way. Am I missing something?

I don't think we have a way to do this currently. I had in mind to add a timer for each search JS test we run and once we reach the limit, to kill the test execution. If you want I can take a look in a follow-up PR? If so r=me

@notriddle
Copy link
Contributor Author

notriddle commented Mar 11, 2024 •
edited
Loading

I don't think using setTimeout in Node will work, because the search will block the event loop and prevent the timer from actually firing.

Doing it in a follow-up PR sounds like a good idea. This discussion should involve the compiletest maintainers.

@GuillaumeGomez
Copy link
Member

I had something else in mind in fact with async.

@notriddle
Copy link
Contributor Author

That still sounds like you're thinking of implementing the timeout in Node. Since search.js doesn't explicitly yield to the event loop (and, even if it did, we wouldn't want to rely on it for this kind of testing), the timer's callback won't be able to dispatch until the search is done.

@GuillaumeGomez
Copy link
Member

You're right, we might need to use threads or subprocesses for that. But yes, I think it'd be better to have it done with node directly.

@GuillaumeGomez
Copy link
Member

I'll add the test timeout in a follow-up. Thanks for this fix!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 14, 2024

📌 Commit fa5b9f0 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 Mar 14, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 14, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
@bors
Auto merge of rust-lang#122497 - matthiaskrgr:rollup-pg9ux4r, r=matth…
30f74ff 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#119029 (Avoid closing invalid handles)
 - rust-lang#122238 (Document some builtin impls in the next solver)
 - rust-lang#122247 (rustdoc-search: depth limit `T<U>` -> `U` unboxing)
 - rust-lang#122287 (add test ensuring simd codegen checks don't run when a static assertion failed)
 - rust-lang#122368 (chore: remove repetitive words)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122406 (Fix WF for `AsyncFnKindHelper` in new trait solver)
 - rust-lang#122477 (Change some attribute to only_local)
 - rust-lang#122482 (Ungate the `UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES` lint)
 - rust-lang#122490 (Update build instructions for OpenHarmony)

Failed merges:

 - rust-lang#122471 (preserve span when evaluating mir::ConstOperand)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a95e2f9 into rust-lang:master Mar 14, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
@rust-timer
Unrolled build for rust-lang#122247
c0dbe52 
Rollup merge of rust-lang#122247 - notriddle:notriddle/search-unbox-limit, r=GuillaumeGomez

rustdoc-search: depth limit `T<U>` -> `U` unboxing

Profiler output:
https://notriddle.com/rustdoc-html-demo-9/search-unbox-limit/ (the only significant change is that one of the `rust` tests went from 378416ms to 16ms).

This is a performance enhancement aimed at a problem I found while using type-driven search on the Rust compiler. It is caused by [`Interner`], a trait with 41 associated types, many of which recurse back to `Self` again.

This caused search.js to struggle. It eventually terminates, after about 10 minutes of turning my PC into a space header, but it's doing `41!` unifications and that's too slow.

[`Interner`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/trait.Interner.html
@notriddle notriddle deleted the notriddle/search-unbox-limit branch March 14, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
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
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
5 participants
@notriddle @rustbot @GuillaumeGomez @bors @fmease