You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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/symbols query
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.
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:
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:
workspace/symbols
queryOriginally posted by @matklad in #14715 (comment)
The text was updated successfully, but these errors were encountered: