rust-lang
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.
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
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...
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?
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
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.
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.
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.
I'll add the test timeout in a follow-up. Thanks for this fix!
@bors r+ rollup
