Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance of rust_analyzer #962

Closed
purkhusid opened this issue Oct 4, 2021 · 6 comments
Closed

Performance of rust_analyzer #962

purkhusid opened this issue Oct 4, 2021 · 6 comments

Comments

@purkhusid
Copy link

The performance for the rust_analyzer rule is not that great when you have a lot of targets included in the targets list.

The issue seems to compound if you have a deep hierarchy where there are a lot of duplicate dependencies and there are a lot of dependencies between targets.

Unfortunately I can't share a reproduction since the repository is proprietary but here is a profile of a bazel build //:rust_analyzer:

image

@purkhusid
Copy link
Author

The issue is very likely with this here for loop:

for info in depset(

@hlopko
Copy link
Member

hlopko commented Oct 11, 2021

Thanks for reporting! I thought about it a bit more, and I came to the conclusion that there is no easy way out here. Right now we do the depset flattening in the analysis phase of Bazel, and that seems to be dominating the profile. What we can do:

  1. move the flattening to the execution phase, this way we can let Bazel execute other actions needed by the rust analyzer action sooner (by potentially removing the depset flattening from the critical path). End to end time may be shorter. Bazel actually has to do depset flattening (or equivalent logic) when constructing the collection of all action inputs. And by flattening the depset in analysis we make the depset flattening in execution a bit faster. We'll need more data. However we know this approach will not scale forever.
  2. change the current eager approach to the lazy one. It seems your build is big enough that a single user rarely needs the whole index to do their job. Since we build the workspace-wide rust-project.json so rust-analyzer can consume it in one go, from the perspective of a user we do unnecessary work in both Bazel and rust-analyzer. If instead rust-analyzer could ask Bazel for the build information about the currently-opened-file and its dependencies, remember them, invalidate them when necessary, and also reuse the running Bazel instance, at the end the user experience may be better (and for certain repository size the experience will be better). I also mentioned this in Rust Analyzer: Generate rust-project.json without explicit target list #907 (review).

Your thoughts?

@purkhusid
Copy link
Author

If rust-analyzer can be made lazy then that would be the ideal approach IMO, this needs upstream changes in rust-analyzer though?

I also wonder if we get more flexibility if we move into the execution phase since we might be able to have better caching during the execution, e.g. a lookup table for each crate so that we don't process them more than once.

I'm not too familiar with the internals of rust-analyzer to be of much help there but I can definitely test out any changes to the rules on our repo and see if there are improvemtents.

@hlopko
Copy link
Member

hlopko commented Oct 12, 2021

Yup, we will need to talk to rust-analyzer folks and see if they would be willing to let us contribute the support for Bazel. I'm happy to ask them once we have landed #907. I don't promise I will have time to contribute the code to rust-analyzer though :)

You raise good points about benefits of the computation in the execution phase. Those will apply when #907 lands.

I'll make sure we link any discussions related to rust analyzer support either through issues in rules_rust repo, or in Discussions in the repo.

@hlopko
Copy link
Member

hlopko commented Nov 29, 2021

Could you check if #1010 made any difference in the performance?

@purkhusid
Copy link
Author

@hlopko This seems to have improved the performance significantly, it still takes some 10s of seconds for my repo but it's not too bad compared to how it was. Thanks for following up on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants