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

Normalize obligation and expected trait_refs in confirm_poly_trait_refs #94108

Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 18, 2022

Consolidate normalization the obligation and expected trait refs in confirm_poly_trait_refs. Also, always normalize these trait refs -- we were already normalizing the obligation trait ref when confirming closure and generator candidates, but this does it for fn pointer confirmation as well.

This presumably does more work in the case that the obligation's trait ref is already normalized, but we can see from the perf runs in #94070, it actually (paradoxically, perhaps) improves performance when paired with logic that normalizes projections in fulfillment loop.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 18, 2022
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

Forgot to remove a deleted test stderr..

@jackh726
Copy link
Member

@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 Feb 18, 2022
@bors
Copy link
Contributor

bors commented Feb 18, 2022

⌛ Trying commit 3b1717eea1995cb31c0bca26cc734354930a93b3 with merge 8bd972219e2ab01d74589bbc494630993b93c5a2...

@bors
Copy link
Contributor

bors commented Feb 18, 2022

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

@rust-timer
Copy link
Collaborator

Queued 8bd972219e2ab01d74589bbc494630993b93c5a2 with parent 30b3f35, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8bd972219e2ab01d74589bbc494630993b93c5a2): comparison url.

Summary: This benchmark run shows 1 relevant improvement 🎉 to instruction counts.

  • Largest improvement in instruction counts: -3.3% on incr-patched: add static arr item builds of coercions debug

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 Feb 18, 2022
@compiler-errors
Copy link
Member Author

Interesting to see that this alone doesn't really improve perf other than one case, but when paired with the other fulfillment normalization PRs, it causes many cases to no longer regress like 1%.

@jackh726
Copy link
Member

I think we can land this separately, but I need to review later.

@compiler-errors
Copy link
Member Author

Cool, I will make it not a draft and give it a real title and description, then.

@compiler-errors compiler-errors changed the title Just confirmation normalization Normalize obligation and expected trait_refs in confirm_poly_trait_refs Feb 18, 2022
@compiler-errors compiler-errors marked this pull request as ready for review February 18, 2022 17:39
@compiler-errors
Copy link
Member Author

compiler-errors commented Feb 18, 2022

r? @jackh726 (just for tracking purposes, no rush on the review)

@@ -2,18 +2,18 @@ error[E0631]: type mismatch in function arguments
--> $DIR/issue-88382.rs:27:40
|
LL | do_something(SomeImplementation(), test);
| ------------ ^^^^ expected signature of `for<'a> fn(&mut <SomeImplementation as Iterable>::Iterator<'a>) -> _`
| ------------ ^^^^ expected signature of `for<'r> fn(&'r mut std::iter::Empty<usize>) -> _`
Copy link
Member

Choose a reason for hiding this comment

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

This test highlights that something wonky is going on. It changes were we aren't able to normalize.

Copy link
Member Author

@compiler-errors compiler-errors Feb 18, 2022

Choose a reason for hiding this comment

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

Yeah, this is very interesting. Possibly due to changes in the ordering of resolving the inference variable in &mut <_ as Iterable>::Iterator<'a> and normalizing it. Now that we're eagerly normalizing the trait ref, we probably are substituting that value before we have a chance to unify the argument type with something else, possibly another projection type with an inference variable.

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 3b1717eea1995cb31c0bca26cc734354930a93b3 has been approved by jackh726

@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 Feb 18, 2022
@bors
Copy link
Contributor

bors commented Feb 19, 2022

☔ The latest upstream changes (presumably #94134) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2022
@jackh726
Copy link
Member

@bors delegate+

@bors
Copy link
Contributor

bors commented Feb 19, 2022

✌️ @compiler-errors can now approve this pull request

@compiler-errors
Copy link
Member Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Feb 19, 2022

📌 Commit b59c958 has been approved by jackh726

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2022
@bors
Copy link
Contributor

bors commented Feb 21, 2022

⌛ Testing commit b59c958 with merge a924ef7...

@bors
Copy link
Contributor

bors commented Feb 21, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing a924ef7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 21, 2022
@bors bors merged commit a924ef7 into rust-lang:master Feb 21, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 21, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a924ef7): comparison url.

Summary: This benchmark run did not return any relevant results.

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

@rustbot label: -perf-regression

@CAD97
Copy link
Contributor

CAD97 commented Mar 7, 2022

Just FYI: this likely changed inference to (correctly) accept more cases: https://www.reddit.com/r/rust/comments/t8ifzy/type_inference_fails_on_stable_but_succeeds_on/?utm_medium=android_app&utm_source=share

@compiler-errors
Copy link
Member Author

@CAD97 this seems consistent with the changes in this this PR (and the issues I was originally focusing on when I first wrote this code in #93361). Thanks very much for noting this!

@compiler-errors compiler-errors deleted the just-confirmation-normalization branch April 7, 2022 04:33
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants