perf: Speed up search for short associated functions, especially very common identifiers such as new
#17927
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
changed the title
newnew
ChayimFriedman2
force-pushed
the
speedup-new-usages
branch
from
111f950
to
a71faaf
Compare
This is unfortunately likely to change soon 😅 rust-lang/rfcs#3591 So we'd likely have to adjust this for trait methods then. Speed for accuracy is an interesting tradeoff to consider here (I wonder if we can juggle that depending on what we are looking for / for what purpose we are searching etc) (haven't reviewed this yet) |
|
Testing with |
This PR doesn't consider trait methods (they have a bunch of other problems), so no, this won't change. This will change if Rust ever adds the ability to |
This is unrelated to this PR - this was before too (this PR returns exactly the same usages for |
It was, but not searching the standard library might be a big speed-up in itself, arguably without trading off accuracy.
Sure in that case, but if I'm searching in the loaded workspace, I don't really want to see results from the libraries. |
That is actually an interesting heuristic to consider. I do like searching for usages within my dependencies / std, but having that behavior change depending on whether you kicked of the search in a workspace member or not might be a decent trade off (though that raises the question of discoverabilty). (that is assuming we can do that, don't have the LSP request in my head for this right now) |
If you only skip std? Not by much. If you skip all deps? Probably yes, but this PR will still be needed - |
Just to be clear, are you searching for |
Something else. Specifically, . But it doesn't really matter: I imagine all |
|
It feels like there's something fishy here. |
|
I didn't benchmark from the IDE, because it will not be stable - anything you have done before may impact the speed. Instead, I created a benchmark to run from the command line, that loads the workspace then immediately searches. This is an upper bound on search time - it can be lower, but not higher (I can share the benchmark code). |
|
Just checked: |
|
For the record, I checked what happens on real IDE. |
| name: &str, | ||
| files: impl Iterator<Item = (Arc<str>, EditionedFileId, TextRange)>, | ||
| mut container_predicate: impl FnMut( | ||
| &SyntaxNode, |
There was a problem hiding this comment.
Couldn't we keep this as &ast::Path given the only calls to it are with paths? Then we don't need to use the untyped descendants api and instead can traverse the typed path api for the relevant bits
There was a problem hiding this comment.
We can keep it as ast::Path, but this will give us nothing. We will still need to use the raw tokens API, because if we search for plain path only, we will miss cases like type Itself<T> = T; Itself<Self>. We need to check if the identifier appears anywhere in the path.
There was a problem hiding this comment.
Ah okay, I misunderstood the idea behind this
ChayimFriedman2
force-pushed
the
speedup-new-usages
branch
from
a71faaf
to
4ac4556
Compare
|
☔ The latest upstream changes (presumably #17898) made this pull request unmergeable. Please resolve the merge conflicts. |
ChayimFriedman2
force-pushed
the
speedup-new-usages
branch
from
4ac4556
to
8a72d68
Compare
… to - i.e. if we are inside a macro call This avoids the need to analyze the file when we are not inside a macro call. This is especially important for the optimization in the next commit(s), as there the common case will be to descent into macros but then not analyze.
…n identifiers such as `new` The search is used by IDE features such as rename and find all references. The search is slow because we need to verify each candidate, and that requires analyzing it; the key to speeding it up is to avoid the analysis where possible. I did that with a bunch of tricks that exploits knowledge about the language and its possibilities. The first key insight is that associated methods may only be referenced in the form `ContainerName::func_name` (parentheses are not necessary!) (Rust doesn't include a way to `use Container::func_name`, and even if it will in the future most usages are likely to stay in that form. Searching for `::` will help only a bit, but searching for `Container` can help considerably, since it is very rare that there will be two identical instances of both a container and a method of it. However, things are not as simple as they sound. In Rust a container can be aliased in multiple ways, and even aliased from different files/modules. If we will try to resolve the alias, we will lose any gain from the textual search (although very common method names such as `new` will still benefit, most will suffer because there are more instances of a container name than its associated item). This is where the key trick enters the picture. The key insight is that there is still a textual property: a container namer cannot be aliased, unless its name is mentioned in the alias declaration, or a name of alias of it is mentioned in the alias declaration. This becomes a fixpoint algorithm: we expand our list of aliases as we collect more and more (possible) aliases, until we eventually reach a fixpoint. A fixpoint is not guaranteed (and we do have guards for the rare cases where it does not happen), but it is almost so: most types have very few aliases, if at all. We do use some semantic information while analyzing aliases. It's a balance: too much semantic analysis, and the search will become slow. But too few of it, and we will bring many incorrect aliases to our list, and risk it expands and expands and never reach a fixpoint. At the end, based on benchmarks, it seems worth to do a lot to avoid adding an alias (but not too much), while it is worth to do a lot to avoid the need to semantically analyze func_name matches (but again, not too much). After we collected our list of aliases, we filter matches based on this list. Only if a match can be real, we do semantic analysis for it. The results are promising: searching for all references on `new()` in `base-db` in the rust-analyzer repository, which previously took around 60 seconds, now takes as least as two seconds and a half (roughly), while searching for `Vec::new()`, almost an upper bound to how much a symbol can be used, that used to take 7-9 minutes(!) now completes in 100-120 seconds, and with less than half of non-verified results (aka. false positives). This is the less strictly correct (but faster) of this patch; it can miss some (rare) cases (there is a test for that - `goto_ref_on_short_associated_function_complicated_type_magic_can_confuse_our_logic()`). There is another branch that have no false negatives but is slower to search (`Vec::new()` never reaches a fixpoint in aliases collection there). I believe it is possible to create a strategy that will have the best of both worlds, but it will involve significant complexity and I didn't bother, especially considering that in the vast majority of the searches the other branch will be more than enough. But all in all, I decided to bring this branch (of course if the maintainers will agree), since our search is already not 100% accurate (it misses macros), and I believe there is value in the additional perf.
ChayimFriedman2
force-pushed
the
speedup-new-usages
branch
from
8a72d68
to
6a910f6
Compare
|
Thanks! |
|
☀️ Test successful - checks-actions |
|
Fantastic work @ChayimFriedman2, thanks! Consider writing a blog post about it, it would be an interesting read for some! |
…Veykril fix: Don't enable the search fast path for short associated functions when a search scope is set In most places where we set a search scope it is a single file, and so the fast path will actually harm performance, since it has to search for aliases in the whole project. The only exception that qualifies for the fast path is SSR (there is an exception that don't qualify for the fast path as it search for `use` items). It sets the search scope to avoid dependencies. We could make it use the fast path, but I didn't bother. I forgot this while working on #17927.
@Veykril said in #17908 (comment) that people complain searches for
new()are slow (they are right), so here I am to help!The search is used by IDE features such as rename and find all references.
The search is slow because we need to verify each candidate, and that requires analyzing it; the key to speeding it up is to avoid the analysis where possible.
I did that with a bunch of tricks that exploits knowledge about the language and its possibilities. The first key insight is that associated methods may only be referenced in the form
ContainerName::func_name(parentheses are not necessary!) (Rust doesn't include a way touse Container::func_name, and even if it will in the future most usages are likely to stay in that form.Searching for
::will help only a bit, but searching forContainercan help considerably, since it is very rare that there will be two identical instances of both a container and a method of it.However, things are not as simple as they sound. In Rust a container can be aliased in multiple ways, and even aliased from different files/modules. If we will try to resolve the alias, we will lose any gain from the textual search (although very common method names such as
newwill still benefit, most will suffer because there are more instances of a container name than its associated item).This is where the key trick enters the picture. The key insight is that there is still a textual property: a container namer cannot be aliased, unless its name is mentioned in the alias declaration, or a name of alias of it is mentioned in the alias declaration.
This becomes a fixpoint algorithm: we expand our list of aliases as we collect more and more (possible) aliases, until we eventually reach a fixpoint. A fixpoint is not guaranteed (and we do have guards for the rare cases where it does not happen), but it is almost so: most types have very few aliases, if at all.
We do use some semantic information while analyzing aliases. It's a balance: too much semantic analysis, and the search will become slow. But too few of it, and we will bring many incorrect aliases to our list, and risk it expands and expands and never reach a fixpoint. At the end, based on benchmarks, it seems worth to do a lot to avoid adding an alias (but not too much), while it is worth to do a lot to avoid the need to semantically analyze func_name matches (but again, not too much).
After we collected our list of aliases, we filter matches based on this list. Only if a match can be real, we do semantic analysis for it.
The results are promising: searching for all references on
new()inbase-dbin the rust-analyzer repository, which previously took around 60 seconds, now takes as least as two seconds and a half (roughly), while searching forVec::new(), almost an upper bound to how much a symbol can be used, that used to take 7-9 minutes(!) now completes in 100-120 seconds, and with less than half of non-verified results (aka. false positives).This is the less strictly correct (but faster) branch of this patch; it can miss some (rare) cases (there is a test for that -
goto_ref_on_short_associated_function_complicated_type_magic_can_confuse_our_logic()). There is another branch that have no false negatives but is slower to search (Vec::new()never reaches a fixpoint in aliases collection there). I believe it is possible to create a strategy that will have the best of both worlds, but it will involve significant complexity and I didn't bother, especially considering that in the vast majority of the searches the other branch will be more than enough. But all in all, I decided to bring this branch (of course if the maintainers will agree), since our search is already not 100% accurate (it misses macros), and I believe there is value in the additional perf.You can find the strict branch at https://github.com/ChayimFriedman2/rust-analyzer/tree/speedup-new-usages-strict.
Should fix #7404, I guess (will check now).