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

include_rustc_srcs creates many configured targets #833

Closed
csmulhern opened this issue Jul 14, 2021 · 3 comments · Fixed by #844
Closed

include_rustc_srcs creates many configured targets #833

csmulhern opened this issue Jul 14, 2021 · 3 comments · Fixed by #844

Comments

@csmulhern
Copy link
Contributor

csmulhern commented Jul 14, 2021

Setting include_rustc_srcs=True creates a configured target for every rust source file. This greatly increases the number of targets in the external dependency.

With include_rustc_srcs=True:

> bazel query "deps(@rust_darwin_aarch64//...)" | wc
113861  113894 10974007

Without:

> bazel query "deps(@rust_darwin_aarch64//...)" | wc
254     254   15305

This has a significant effect on analysis time in my small workspace. Here is a profile of a clean build for a hello world rust binary.

With include_rustc_srcs=True:

=== PHASE SUMMARY INFORMATION ===

Total launch phase time         0.042 s    0.84%
Total init phase time           0.007 s    0.15%
Total target pattern evaluation phase time    0.059 s    1.19%
Total interleaved loading-and-analysis phase time    4.052 s   80.99%
Total preparation phase time    0.002 s    0.05%
Total execution phase time      0.839 s   16.77%
Total finish phase time         0.000 s    0.02%
------------------------------------------------
Total run time                  5.003 s  100.00%

Without:

=== PHASE SUMMARY INFORMATION ===

Total launch phase time         0.042 s    1.55%
Total init phase time           0.005 s    0.22%
Total target pattern evaluation phase time    0.052 s    1.95%
Total interleaved loading-and-analysis phase time    1.838 s   67.94%
Total preparation phase time    0.002 s    0.08%
Total execution phase time      0.764 s   28.24%
Total finish phase time         0.000 s    0.02%
------------------------------------------------
Total run time                  2.706 s  100.00%

It appears the filegroup created by include_rustc_srcs=True is only used to support the rust_analyzer rule. Is it possible to point the rust_analyzer rule at the source root without creating all these bazel targets?

@UebelAndre
Copy link
Collaborator

Hi! This is something we're interested in solving bug haven't gotten around to yet (#704). I may have a solution but the idea is built off of additional changes I'm thinking about (#815). If there's a more direct approach though, I'd love to discuss/see it 😄

@hlopko
Copy link
Member

hlopko commented Jul 19, 2021

So I read this as:

  • You want to use rust_analyzer, therefore you enabled include_rustc_srcs
  • That adds a lot of targets, so Bazel's cold start is slow

So I took a look, and yes, rust src has a lot of files! :) It goes down to 12K if you remove tests, and leave only rs and toml files. I guess pruning the sources in the repository rule makes sense (ideally we could download already pruned rust-src, so we don't pay the price of extracting + deleting, but I don't know if there is such archive available).

Alternatively, as you suggested, we could tell Bazel/rust-analyzer to look somewhere for the rust-srcs checkout. This has a small disadvantage because the version of the checked out sources may get out of sync with the version of the toolchain used by Bazel. To do that, we would first remove the check here and add a check + flag here

What do you think? If you have the capacity, would you be interested in pursuing this?

@csmulhern
Copy link
Contributor Author

Sure, I've submitted #844 which uses the pruned rust-src distribution (the component used by rustup for this purpose).

What I was proposing for "looking elsewhere" was not to not have rules_rust download the necessary files (that makes sense to me), but rather just that the filegroup itself might not be necessary. As far as I can tell, its only purpose is to create a label that _rust_analyzer_impl can use to write the path in rust-project.json.

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

Successfully merging a pull request may close this issue.

3 participants