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

Try to normalize associated types before processing obligations #90887

Merged
merged 3 commits into from
Mar 8, 2022

Conversation

jackh726
Copy link
Member

Closes #90729

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2021
@jackh726
Copy link
Member 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 Nov 13, 2021
@bors
Copy link
Contributor

bors commented Nov 13, 2021

⌛ Trying commit 27a4ade1fdb09446b39e8f0cd7fd7544d1de7e26 with merge 3f8ac3c8c9bdbe3ad503f8356af71c9f3a5242df...

@bors
Copy link
Contributor

bors commented Nov 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued 3f8ac3c8c9bdbe3ad503f8356af71c9f3a5242df with parent b416e38, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3f8ac3c8c9bdbe3ad503f8356af71c9f3a5242df): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 2.8% on full builds of wg-grammar)

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 14, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2021
@nikomatsakis
Copy link
Contributor

@jackh726 I don't recall us talking about this PR, have I just forgotten? Did you ever look at the perf results?

@jackh726
Copy link
Member Author

@nikomatsakis I mentioned this at the end of our meeting on Monday.

As far as perf, I haven't done anything more than just looking through the benchmarks. It looks like wg-grammar was hit "hard" ~3%, whereas everything else is 1% or less. In theory, there might be an optimization that could be done (maybe first check if there are projection types in obligation.predicate?). But this does fix a number of things that I think, on balance, mean the perf hit is okay.

@rust-log-analyzer

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

@bors rollup=never

@jackh726
Copy link
Member Author

jackh726 commented Feb 7, 2022

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

bors commented Feb 7, 2022

⌛ Trying commit c920e6605cf55518ef9a7100395849e4914a8c75 with merge 2b9a45228e828686978ed677f84e1df9c4146666...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2022
@bors bors merged commit 67b3e81 into rust-lang:master Mar 8, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 8, 2022
@jackh726 jackh726 deleted the issue-90729 branch March 8, 2022 03:17
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (67b3e81): comparison url.

Summary: This benchmark run did not return any relevant results. 20 results were found to be statistically significant but too small to be relevant.

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

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Mar 8, 2022
aliemjay added a commit to aliemjay/rust that referenced this pull request Apr 23, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 17, 2022
Add a couple tests for rust-lang#90887 fixes

closes rust-lang#56556
closes rust-lang#90875

These are confirmed fixes by rust-lang#90887, so
r? `@jackh726`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 17, 2022
Add a couple tests for rust-lang#90887 fixes

closes rust-lang#56556
closes rust-lang#90875

These are confirmed fixes by rust-lang#90887, so
r? ``@jackh726``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 17, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#96329 (Add a couple tests for rust-lang#90887 fixes)
 - rust-lang#97009 (Allow `unused_macro_rules` in path tests)
 - rust-lang#97075 (Add regression test for rust-lang#81804)
 - rust-lang#97079 (Change `Successors` to `impl Iterator<Item = BasicBlock>`)
 - rust-lang#97080 (remove the `RelateResultCompare` trait)
 - rust-lang#97093 (Migrate `maybe_recover_from_bad_type_plus` diagnostic)
 - rust-lang#97102 (Update function pointer call error message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
xFrednet pushed a commit to xFrednet/rust that referenced this pull request May 21, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#96329 (Add a couple tests for rust-lang#90887 fixes)
 - rust-lang#97009 (Allow `unused_macro_rules` in path tests)
 - rust-lang#97075 (Add regression test for rust-lang#81804)
 - rust-lang#97079 (Change `Successors` to `impl Iterator<Item = BasicBlock>`)
 - rust-lang#97080 (remove the `RelateResultCompare` trait)
 - rust-lang#97093 (Migrate `maybe_recover_from_bad_type_plus` diagnostic)
 - rust-lang#97102 (Update function pointer call error message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2024
…<try>

never normalize without eager inference replacement

unsure why we previously didn't do so, wasn't able to find an explanation in
rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary,

r? types cc `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2024
…<try>

some trait system cleanups

Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary.

The second commit simplifies `pred_known_to_hold_modulo_regions`, afaict the optional `fulfill` isn't necessary anymore.

r? types cc `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
…jackh726,compiler-errors

some trait system cleanups

Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary.

The second commit simplifies `pred_known_to_hold_modulo_regions`, afaict the optional `fulfill` isn't necessary anymore.

r? types cc `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
…jackh726,compiler-errors

some trait system cleanups

Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary.

The second commit simplifies `pred_known_to_hold_modulo_regions`, afaict the optional `fulfill` isn't necessary anymore.

r? types cc `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2024
…compiler-errors

some trait system cleanups

Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary.

The second commit simplifies `pred_known_to_hold_modulo_regions`, afaict the optional `fulfill` isn't necessary anymore.

r? types cc `@jackh726`
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. perf-regression-triaged The performance regression has been triaged. 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.

Problem with GAT and HRTB
10 participants