rust_analyzer: don't build a tree of RustAnalyzerInfos #3028
Conversation
Before this patch, `RustAnalyzerInfo.deps` contains the RustAnalyzerInfo
of the target crate's deps, and so on recursively.
So for each target we build a tree capturing all transitive deps.
---
In fact it's worse when aliases are present, as they're stored there too.
We construct `RustAnalyzerInfo(aliases{//foo => x}, deps = [x])`
with a single shared `x` object:
RustAnalyzerInfo-----deps-->//foo[RustAnalyzerInfo]
\_aliases-/
But when we return it from the aspect, blaze deep-copies it, resulting
in two copies of //foo's data:
//bar[RustAnalyzerInfo]----deps-->//foo[RustAnalyzerInfo]
\_aliases-->//foo[RustAnalyzerInfo]
With multiple layers of dependencies with aliases, this blows up
exponentially, creating huge data structures that lead to very slow
generation times.
---
All of this is unneccesary: each crate only needs to know its own ID and
details, and the IDs of the dependencies it needs to refer to.
The per-crate JSON fragment describes one node in a graph, and
concatenating them completes the graph.
So we add an `id` to RustAnalyzerInfo, and replace all the recursive
RustAnalyzerInfos with the respective crate `id`s.
---
Timings for `ra_ap_rust-analyzer` (which heavily aliases its deps):
```
blaze build //third_party/bazel_rules/rules_rust/tools/rust_analyzer:gen_rust_project; \
time blaze run //third_party/bazel_rules/rules_rust/tools/rust_analyzer:gen_rust_project -- //third_party/rust/ra_ap_rust_analyzer/...
===Before===
Executed in 211.06 secs fish external
usr time 0.24 secs 0.00 micros 0.24 secs
sys time 1.24 secs 604.00 micros 1.24 sec
===After===
Executed in 3.24 secs fish external
usr time 0.15 secs 389.00 micros 0.15 secs
sys time 1.18 secs 125.00 micros 1.18 secs
```
The existing support for this has limitations: - only works for sources that are specially tagged (rust_generated_srcs). I've removed this mechanism as it's no longer needed. - only provides sources of the directly selected library, but rust-analyzer needs to parse its dependencies too. - is missing the target.json file, which rust-analyzer requires to parse the library (it invokes rustc --print cfg --target=...) The dependency depth to generate sources for is a tradeoff. More layers of deps will give more accurate analysis but be slower to generate. My guess is that in the end we'll want full transitive sources, but direct deps should be a non-controversial starting point. (Without these sources, rust-analyzer can't understand the direct API usage at all). We can change this later and probably need to reconcile with bazelbuild#3028.
|
I misunderstood the cause of the performance problems here. The data aren't actually duplicated, it just appears that way if you (a) print it, or (b) hash it. I was misled by (a), and the rules use these infos as hash keys for aliases, triggering (b). I do think eliminating the tree of RustAnalyzerInfos is a good idea if we don't need the power: it's harder to reason about the available data, carries risks of these performance cliffs, prevents print-debugging. |
|
How does this impact |
|
The Info will still be built for each target the aspect propagates to, and the json specs will still be gathered in the depset in the same way. The only thing that changes is the data structure used is not recursive. I don't know specifically how transitions affect the aspect's behavior in that example, but I'm fairly sure it's the same before/after the patch. |
…build#3028)" This reverts commit 443b089.
|
This change seems to have broken the ability for rust-analyzer to index into prost rules. None of the proto targets are being indexed. With this change: rust-project-main.json @sam-mccall @krasimirgg would you be able to take a look? |
|
@UebelAndre sorry, I wasn't able to get to this until now - I've been out sick. I'll take a look: at first glance all the crates are there, it looks like some deps from Do you know why Prost doesn't return a regular |
No worries, glad you're feeling better!
Prost returns |
This patch adds an
idto RustAnalyzerInfo, and replace all the recursive RustAnalyzerInfos with the respective crateids.This fixes RustAnalyzerInfos becoming exponentially expensive to build in the presence of aliases.
Before this patch,
RustAnalyzerInfo.depscontains the RustAnalyzerInfo of the target crate's deps, and so on recursively.This forms a graph of Infos where there is one Info per target, but multiple paths from one target to another.
e.g. these could all point to
bar:foo.deps[0] == foo.deps[1].deps[0] == foo.aliases[0].If we walk
fooas a tree, we will seebarmultiple times. Two operations that do this areprint(info)and{info: 42}which hashes the object. As the graph grows, the number of paths grows combinatorically.RustAnalyzerInfo.aliasesis defined as a dict fromRustAnalyzerInfo => string, so building this triggers the slowdown.It would be possible to fix this by e.g. replacing this dict with a list of pairs.
However the work
rust_analyzer_aspectdoes is fundamentally local and does not need a recursive data structure. Eliminating it means we can freely use these values as keys, print them, etc.Timings for
ra_ap_rust-analyzer(which heavily aliases its deps):