Add a query for resolving an impl item from the trait item by matthewjasper · Pull Request #90639 · rust-lang/rust

matthewjasper · 2021-11-05T23:13:43Z

This makes finding the item in an impl that implements a given trait item a query. This is for a few reasons:

To slightly improve performance
To avoid having to do name resolution during monomorphisation
To make it easier to implement potential future features that create anonymous associated items

rust-highfive · 2021-11-05T23:13:45Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2021-11-05T23:13:46Z

r? @oli-obk

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

matthewjasper · 2021-11-06T01:09:03Z

@bors try @rust-timer queue

rust-timer · 2021-11-06T01:09:05Z

Awaiting bors try build completion.

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

bors · 2021-11-06T01:09:12Z

⌛ Trying commit 96121514f93fd7bcf6507d9f04503656387680bf with merge 43062b0944a88c99e00100158c147d67e34081c0...

bors · 2021-11-06T02:24:21Z

☀️ Try build successful - checks-actions
Build commit: 43062b0944a88c99e00100158c147d67e34081c0 (43062b0944a88c99e00100158c147d67e34081c0)

rust-timer · 2021-11-06T02:24:23Z

Queued 43062b0944a88c99e00100158c147d67e34081c0 with parent 0d1754e, future comparison URL.

rust-timer · 2021-11-06T04:23:06Z

Finished benchmarking commit (43062b0944a88c99e00100158c147d67e34081c0): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -2.1% on full builds of deeply-nested)
Large regression in instruction counts (up to 2.6% on incr-patched: compile one builds of regex)

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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

cjgillot

This seems like there is an unfortunate recomputation of typeck. Do you have an idea where it comes from?
The three first commits look good. Can they be merged independently?
I have the impression this PR introduces queries for two separate things:

first a query to translate impl item <-> trait item;
second a query for associated_item_lookup.

Should their effect be assessed independently?

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_typeck/src/check/mod.rs

matthewjasper · 2021-11-06T12:54:48Z

The recomputation probably comes from the Span of the Ident in NamedAssocItem being included in the stable hash. That said I agree that this PR should be split up to make it easier to experiment on each change individually.

…jgillot Assoc item cleanup This removes some fields from ObligationCauseCode Split out of rust-lang#90639

cjgillot · 2022-01-07T23:24:18Z

Just in case:
@bors try @rust-timer queue

rust-timer · 2022-01-07T23:24:19Z

Awaiting bors try build completion.

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

bors · 2022-01-07T23:24:27Z

⌛ Trying commit 9d625bc with merge b4a612688e5135afb476f64bbe2508f028fe9268...

bors · 2022-01-08T00:40:18Z

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

rust-timer · 2022-01-08T00:40:21Z

Queued b4a612688e5135afb476f64bbe2508f028fe9268 with parent e012a19, future comparison URL.

rust-timer · 2022-01-08T02:11:31Z

Finished benchmarking commit (b4a612688e5135afb476f64bbe2508f028fe9268): comparison url.

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

Small improvement in instruction counts (up to -0.9% on full builds of deeply-nested-closures)

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

camelid · 2022-01-08T02:38:51Z

Perf looks good.

cjgillot · 2022-01-08T09:25:05Z

@bors r+

bors · 2022-01-08T09:25:07Z

📌 Commit 9d625bc has been approved by cjgillot

bors · 2022-01-08T15:20:40Z

⌛ Testing commit 9d625bc with merge 488acf8...

bors · 2022-01-08T18:32:25Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 488acf8 to master...

rust-timer · 2022-01-08T20:03:43Z

Finished benchmarking commit (488acf8): 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

camelid · 2022-01-08T20:21:22Z

Hmm, not sure why the summary included the doc improvements before but not now. The doc improvements are still there though.

WaffleLapkin · 2022-01-09T17:27:49Z

compiler/rustc_typeck/src/check/check.rs

            let is_implemented = ancestors
-                .leaf_def(tcx, trait_item.ident, trait_item.kind)
-                .map(|node_item| !node_item.defining_node.is_from_trait())
-                .unwrap_or(false);
+                .leaf_def(tcx, trait_item_id)
+                .map_or(false, |node_item| node_item.item.defaultness.has_value());


I think this changed values of is_implemented. Previously it was true only if it's implemented in this implementation (and was false when there were a default impl in the trait). Is this intended? (this broke #92164 and not I'm figuring out how to fix it)

(the first screenshot from e012a19 commit, just before these changes, the second is after these changes; I've added a debug! in this loop)

And if this was intended, how can I specifically check that this item is implemented manually in a given impl (ie it doesn't use default implementation from the trait)?

…atthewjasper Link impl items to corresponding trait items in late resolver. Hygienically linking trait impl items to declarations in the trait can be done directly by the late resolver. In fact, it is already done to diagnose unknown items. This PR uses this resolution work and stores the `DefId` of the trait item in the HIR. This avoids having to do this resolution manually later. r? `@matthewjasper` Related to rust-lang#90639. The added `trait_item_id` field can be moved to `ImplItemRef` to be used directly by your PR.

rust-highfive assigned oli-obk Nov 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2021

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the leaf-def-cache branch from a7a8b8e to 9612151 Compare November 6, 2021 00:15

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 6, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 6, 2021

cjgillot self-assigned this Nov 6, 2021

cjgillot reviewed Nov 6, 2021

View reviewed changes

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/mod.rs Outdated Show resolved Hide resolved

matthewjasper mentioned this pull request Nov 6, 2021

Assoc item cleanup #90648

Merged

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2021

Auto merge of rust-lang#90648 - matthewjasper:assoc-item-cleanup, r=c…

3d29b68

…jgillot Assoc item cleanup This removes some fields from ObligationCauseCode Split out of rust-lang#90639

matthewjasper mentioned this pull request Nov 12, 2021

Assoc item cleanup Part 2 #90827

Merged

matthewjasper 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 Nov 12, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 7, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 8, 2022

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 Jan 8, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 8, 2022

bors merged commit 488acf8 into rust-lang:master Jan 8, 2022

rustbot added this to the 1.60.0 milestone Jan 8, 2022

WaffleLapkin reviewed Jan 9, 2022

View reviewed changes

Uh oh!

Conversation

matthewjasper commented Nov 5, 2021 • edited by camelid Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 5, 2021

Uh oh!

rust-highfive commented Nov 5, 2021

Uh oh!

This comment has been minimized.

matthewjasper commented Nov 6, 2021

Uh oh!

rust-timer commented Nov 6, 2021

Uh oh!

bors commented Nov 6, 2021

Uh oh!

bors commented Nov 6, 2021

Uh oh!

rust-timer commented Nov 6, 2021

Uh oh!

rust-timer commented Nov 6, 2021

Uh oh!

cjgillot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matthewjasper commented Nov 6, 2021

Uh oh!

cjgillot commented Jan 7, 2022

Uh oh!

rust-timer commented Jan 7, 2022

Uh oh!

bors commented Jan 7, 2022

Uh oh!

bors commented Jan 8, 2022

Uh oh!

rust-timer commented Jan 8, 2022

Uh oh!

rust-timer commented Jan 8, 2022

Uh oh!

camelid commented Jan 8, 2022

Uh oh!

cjgillot commented Jan 8, 2022

Uh oh!

bors commented Jan 8, 2022

Uh oh!

bors commented Jan 8, 2022

Uh oh!

bors commented Jan 8, 2022

Uh oh!

rust-timer commented Jan 8, 2022

Uh oh!

camelid commented Jan 8, 2022

Uh oh!

WaffleLapkin Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

matthewjasper commented Nov 5, 2021 •

edited by camelid

Loading