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

rustc_metadata: track the simplified Self type for every trait impl. #75008

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 1, 2020

For the traits_impls_of query, we index the impls by fast_reject::SimplifiedType (a "shallow type"), which allows some simple cases like impl Trait<..> for Foo<..> to be efficiently iterated over, by e.g. for_each_relevant_impl.

This PR encodes the fast_reject::SimplifiedType cross-crate to avoid needing to deserialize the Self type of every impl in order to simplify it - the simplification itself should be cheap, but the deserialization is less so.

We could go further from here and make loading the list of impls lazy, for a given simplified Self type, but that would have more complicated implications for performance, and this PR doesn't do anything in that regard.

r? @nikomatsakis cc @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020
@eddyb
Copy link
Member Author

eddyb commented Aug 1, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Aug 1, 2020

⌛ Trying commit 6bf3a4b with merge 128201b46cdfa059217962ae2f159583e0ae5f93...

@Mark-Simulacrum
Copy link
Member

cc @jyn514 as well, you were asking about something similar recently

@bors
Copy link
Contributor

bors commented Aug 1, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 128201b46cdfa059217962ae2f159583e0ae5f93 (128201b46cdfa059217962ae2f159583e0ae5f93)

@rust-timer
Copy link
Collaborator

Queued 128201b46cdfa059217962ae2f159583e0ae5f93 with parent dfe1e3b, future comparison URL.

@jyn514
Copy link
Member

jyn514 commented Aug 1, 2020

cc @jyn514 as well, you were asking about something similar recently

This isn't quite what I was looking for ... this is making existing queries more efficient wrt metadata on disk, while #74489 is adding a new query so that looking up the traits for a type is less expensive. This looks good in and of itself though :)

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (128201b46cdfa059217962ae2f159583e0ae5f93): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@eddyb
Copy link
Member Author

eddyb commented Aug 1, 2020

I'm very suspicious of the decrease in item_attrs getting decoded. I wonder if we could avoid that and maybe get some of the same wins? Although the biggest delta seems to be in metadata_decode_entry and I don't really know what that measures.

@@ -187,32 +187,38 @@ pub(super) fn all_local_trait_impls<'tcx>(
pub(super) fn trait_impls_of_provider(tcx: TyCtxt<'_>, trait_id: DefId) -> TraitImpls {
let mut impls = TraitImpls::default();

{
let mut add_impl = |impl_def_id: DefId| {
let impl_self_ty = tcx.type_of(impl_def_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

so is the performance improvement here just that we don't have to compute type_of and then simplify the type, but instead we do it "up front" when encoding?

Copy link
Member Author

@eddyb eddyb Aug 3, 2020

Choose a reason for hiding this comment

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

Right, I think all the cost is in decoding the Ty from type_of (simplification should be relatively cheap).

And based on the detailed query profiling info, I'm guessing it's ty::Adt containing a &'tcx ty::AdtDef instead of just a DefId, and somehow item_attrs getting called for every struct/enum/union implementing the trait (in order to create the ty::AdtDef?).

I'm suspecting this is similar to what @nnethercote was talking about a few months ago, where the deserialization of doc comment attributes was a significant cost (I don't recall what happened with that line of investigation though).

Copy link
Contributor

Choose a reason for hiding this comment

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

#65750 introduced a specialized representation for doc comments, that might be what you are referring to.

@nikomatsakis
Copy link
Contributor

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 5, 2020

📌 Commit 6bf3a4b has been approved by nikomatsakis

@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 Aug 5, 2020
@bors
Copy link
Contributor

bors commented Aug 5, 2020

⌛ Testing commit 6bf3a4b with merge b1bc5c61f41b461d5976940b18d09dd316a57027...

@bors
Copy link
Contributor

bors commented Aug 6, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 6, 2020
@JohnTitor
Copy link
Member

@bors retry

@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 Aug 6, 2020
@bors
Copy link
Contributor

bors commented Aug 6, 2020

⌛ Testing commit 6bf3a4b with merge 3cfc7fe...

@bors
Copy link
Contributor

bors commented Aug 6, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 3cfc7fe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2020
@bors bors merged commit 3cfc7fe into rust-lang:master Aug 6, 2020
@eddyb eddyb deleted the rmeta-indexed-trait-impls branch August 6, 2020 05:28
@mati865
Copy link
Contributor

mati865 commented Aug 6, 2020

@eddyb
Copy link
Member Author

eddyb commented Aug 6, 2020

That's somewhat expected, it's hard to tell what the actual wins are from relative measurements, because the delta is absolute across the board (not exactly a startup cost, but very similar for any one given trait).

I mostly went on the basis of @Mark-Simulacrum finding out that a lot of the cost was in trait_impls_of -> type_of. So this was potentially more than half of the regression (in absolute terms), but it's far from making extra dependencies free.

Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 17, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 18, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 18, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 18, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.