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.

Sign up for GitHub

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

Jump to bottom

rustc_metadata: Encode list of all crate's traits into metadata #92244

Merged
merged 1 commit into from Dec 29, 2021
Merged

rustc_metadata: Encode list of all crate's traits into metadata #92244

merged 1 commit into from Dec 29, 2021

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 24, 2021 •
edited

While working on #88679 I noticed that rustdoc is casually doing something quite expensive, something that is used only for error reporting in rustc - collecting all traits from all crates in the dependency tree.

This PR trades some minor extra time spent by metadata encoder in rustc for major gains for rustdoc (and for rustc runs with errors, which execute the all_traits query for better diagnostics).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 24, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 24, 2021
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 24, 2021
@bors
Copy link
Contributor

bors commented Dec 24, 2021

⌛ Trying commit 41bc5e10f177278a024bc1b82a7c333229fe2546 with merge 9119f3203d18022267eb24170faf67eea61e949c...

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 24, 2021
@bors
Copy link
Contributor

bors commented Dec 24, 2021

☀️ Try build successful - checks-actions
Build commit: 9119f3203d18022267eb24170faf67eea61e949c (9119f3203d18022267eb24170faf67eea61e949c)

@rust-timer
Copy link
Collaborator

Queued 9119f3203d18022267eb24170faf67eea61e949c with parent e6f1f04, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9119f3203d18022267eb24170faf67eea61e949c): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

  • Very large improvement in instruction counts (up to -16.1% on full builds of webrender)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 24, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 24, 2021
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Dec 24, 2021 •
edited

The perf results are roughly as expected from callgrind runs, I need to cleanup the code.

@petrochenkov petrochenkov mentioned this pull request Dec 24, 2021
Merged
@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 24, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 24, 2021

This is awesome! I didn't realize it could be cached in the query system, I thought it had to be recalculated since it depends on which crates have been loaded.

@camelid
Copy link
Member

camelid commented Dec 25, 2021 •
edited

Which part of the changes affects rustdoc's behavior (internal behavior, not user-visible behavior)? I didn't see anything obvious in the diff.

@petrochenkov
Copy link
Contributor Author

@camelid

Which part of the changes affects rustdoc's behavior

Rustdoc uses the all_traits query which is much faster now.

@petrochenkov petrochenkov changed the title [WIP] rustc_metadata: Encode list of all crate's traits into metadata rustc_metadata: Encode list of all crate's traits into metadata Dec 28, 2021
@petrochenkov
Copy link
Contributor Author

Updated.

@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 28, 2021
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021
cjgillot
cjgillot reviewed Dec 28, 2021
View reviewed changes
compiler/rustc_metadata/src/rmeta/encoder.rs
tcx.hir().visit_all_item_likes(&mut visitor);

let mut all_traits = visitor.traits;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be implemented using tcx.trait_in_crate(LOCAL_CRATE)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some performance trade-off here.

From performance point of view all the stuff in fn encode_crate_root should be encoded by walking the crate once, collecting everything, and then encoding everything, instead of walking the crate many times by LOCAL_CRATE queries collecting their specific pieces of data.
But the code would certainly be cleaner and with less duplication with the queries.

I can make the refactoring you suggest later in a separate PR, and we'll land it if doesn't cause observable changes in benchmarks.

compiler/rustc_metadata/src/rmeta/encoder.rs
tcx.hir().visit_all_item_likes(&mut visitor);

let mut all_traits = visitor.traits;
let mut all_impls: Vec<_> = visitor.impls.into_iter().collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing: same question with tcx.all_trait_implementations(LOCAL_CRATE).

@cjgillot
Copy link
Contributor

r? @cjgillot
@bors r+

@bors
Copy link
Contributor

bors commented Dec 29, 2021

📌 Commit 90e3710 has been approved by cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 Dec 29, 2021
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2021
@bors
Copy link
Contributor

bors commented Dec 29, 2021

⌛ Testing commit 90e3710 with merge 78fd0f6...

@bors
Copy link
Contributor

bors commented Dec 29, 2021

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 78fd0f6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 29, 2021
@bors bors merged commit 78fd0f6 into rust-lang:master Dec 29, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 29, 2021
This was referenced Dec 29, 2021
Merged
Closed
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (78fd0f6): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

  • Very large improvement in instruction counts (up to -17.1% on full builds of webrender)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@petrochenkov petrochenkov mentioned this pull request Dec 31, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 1, 2022
@bors
Auto merge of rust-lang#92455 - petrochenkov:alltraits2, r=cjgillot
c145692 
rustc_metadata: Use a query for collecting all traits in encoder

Implement refactoring suggested in rust-lang#92244 (comment)
r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

@camelid camelid camelid left review comments

Assignees

@cjgillot cjgillot

Labels
I-compiletime Issue: Problems and improvements with respect to compile times. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@petrochenkov @rust-highfive @rust-timer @bors @jyn514 @camelid @cjgillot @jackh726 @rustbot