-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
rustc_metadata: Switch all decoder methods from vectors to iterators #92245
Conversation
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 2a074cf61105ad9a668719faa3a240e7d1f78c7e with merge a1b48ab4b3ea7e5795cb5b843b52edd696feaaf3... |
☀️ Try build successful - checks-actions |
Queued a1b48ab4b3ea7e5795cb5b843b52edd696feaaf3 with parent 59337cd, future comparison URL. |
Finished benchmarking commit (a1b48ab4b3ea7e5795cb5b843b52edd696feaaf3): comparison url. Summary: This change led to small relevant mixed results 🤷 in compiler performance.
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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
No idea what could cause the regressions here, maybe some unlucky new inlining choices, I'll check. |
CrateMetadataRef
by reference CrateMetadataRef
by reference (step 2)
Blocked on #92277. |
This comment has been minimized.
This comment has been minimized.
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1) It's already a (fat) reference. Double referencing it creates lifetime issues for its methods that want to return iterators. --- Extracted from rust-lang#92245 for a perf run. The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1) It's already a (fat) reference. Double referencing it creates lifetime issues for its methods that want to return iterators. --- Extracted from rust-lang#92245 for a perf run. The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1) It's already a (fat) reference. Double referencing it creates lifetime issues for its methods that want to return iterators. --- Extracted from rust-lang#92245 for a perf run. The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1) It's already a (fat) reference. Double referencing it creates lifetime issues for its methods that want to return iterators. --- Extracted from rust-lang#92245 for a perf run. The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
CrateMetadataRef
by reference (step 2)
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 14d56df404eb124a3599698edc6a53be9f30dca0 with merge 565e74827743aed951f73a4d339f5e9379afef9e... |
☀️ Try build successful - checks-actions |
Queued 565e74827743aed951f73a4d339f5e9379afef9e with parent 86f7f78, future comparison URL. |
Finished benchmarking commit (565e74827743aed951f73a4d339f5e9379afef9e): comparison url. Summary: This change led to small relevant improvements 🎉 in compiler performance.
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 |
self.root.native_libraries.decode((self, sess)).collect() | ||
} | ||
fn get_native_libraries(self, sess: &'a Session) -> impl Iterator<Item = NativeLib> + 'a { | ||
self.root.native_libraries.decode((self, sess)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it isn't entirely obvious to me that native_libraries
will always decode to an empty list for is_proc_macro_crate() == true
, though I'll admit that I didn't spend a long time spelunking the relevant code trying to figure out if there's any code elsewhere that ensures this property holds.
Is there any way it would make sense to add an assertion or something along the lines here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For proc macro crates encoder now encodes only a specific set of metadata fields related to proc macros, everything else is empty.
It wasn't always like that, so decoder still has many is_proc_macro
conditions.
We should generally cleanup it and remove them. But I guess they can be replaced with asserts instead, that would be more reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are not checking the is_proc_macro
property in majority of cases.
This is how it looks if the asserts are added - petrochenkov@b026841
For Lazy<[T]>
it looks like we cannot even check whether it's empty without decoding, that's why some checks are commented out.
So I think we should just drop the is_proc_macro
checks without adding asserts, and assume that encoder encodes everything correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Lazy<[T]> it looks like we cannot even check whether it's empty without decoding, that's why some checks are commented out.
I think you can check the .meta
field on a Lazy<[T]>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
IMO, it's still simpler to assume that we are just decoding whatever the encoder put there, and make the decoder "fast and dumb", I've updated the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with not adding any of the asserts too, if we are sufficiently confident lack of them will not really cause any issues. Just wanted to make sure we at least considered it.
Also remove unnecessary `is_proc_macro_crate` checks from decoder
@bors r+ rollup=never |
📌 Commit 4549b13 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (48e89b0): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
To avoid allocations in some cases.
Also remove unnecessary
is_proc_macro_crate
checks from decoder, currently the general strategy is to shift all the work to the encoder and assume that all the encoded data is correct and can be decoded unconditionally in the decoder.