internal: Refactor symbol index #14715
Conversation
rustbot
added
the
S-waiting-on-review
|
@bors r+ |
|
☀️ Test successful - checks-actions |
| @@ -398,7 +398,7 @@ impl Analysis { | |||
| self.with_db(|db| { | |||
| symbol_index::world_symbols(db, query) | |||
| .into_iter() // xx: should we make this a par iter? | |||
| .filter_map(|s| s.try_to_nav(db)) | |||
| .filter_map(|s| s.def.try_to_nav(db)) | |||
There was a problem hiding this comment.
I think this is a significant slowdown. If I try to fetch all symbols (including libraries, and without our limit) for rust analyser, I get the following status:
EDIT: the slowdown part is wrong, this is pre-existing, but I think this PR still elevates perf bug to architecture bug.
total number of results 98752
computing symbol index for all libs: 3.64s
filtering by (trivial, empty) query: 16.70ms
converting resuults to : 98.54ms
handle_worskpace_symbol: 50.16s
I think this try_to_nav call is the single place where the the rest of 50 seconds went. (and this is unrelated to the PR, before the behavior was the same).
This is an essentially computationally hard problem -- this is a global search, and it's hard to make it lazy.
That's why the original design didn't use defs, but rather had a special data structure which materialized all parts of a def which is needed for rendering. That is, the intention of the code was to have impl TryToNav for FileSymbol to be O(1) and not triggering semantic analysis. So that we can compute LibrarySymbols once and then just never update them.
To clarify, this PR doesn't break it, because the code is slow even before the change. And it doesn't practically matter, because:
- search in libraries is impossible to trigger anyway because LSP sucks
- we have a limit on the number of items
However, while we had FileSymbol, we could fixed the bug. Without library symbols, you can't really just fix that.
Architectually, I think we really need two indexes here:
- Name to Def, for semantic purposes like auto imports
- Name to "JsonBlob", specifically for
workspace/symbolsquery
There was a problem hiding this comment.
Architectually, I think we really need two indexes here:
Right, I was hoping that we didn't but I totally see why we do now. That's a shame.
fix: Fix perf regression from symbol index refactor Should fix the regressions introduced by #14715 by partially rolling back the PR
Closes #14677
instead of eagerly fetching the source data in symbol index we do it lazily now, this shouldn't make it much more expensive as we had to parse the source most of the time anyways even after fetching.