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.

Sign up for GitHub

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

Jump to bottom

Move overlap_mode into trait level attribute #93348

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Move overlap_mode into trait level attribute #93348

merged 2 commits into from
Jan 31, 2022

Conversation

spastorino
Copy link
Member

r? @nikomatsakis

Should fix some performance regressions noted on #93175

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 26, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2022
@spastorino
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 Jan 26, 2022
@bors
Copy link
Contributor

bors commented Jan 26, 2022

⌛ Trying commit 291a4c335552ba97bdf641c006ce9e86f6f7fab2 with merge 107640c50fe413527be6069e6f181c54734f1b21...

nikomatsakis
nikomatsakis requested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me otherwise

compiler/rustc_feature/src/active.rs Outdated Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

@bors rollup=never

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lqd lqd mentioned this pull request Jan 26, 2022
Merged
nikomatsakis
nikomatsakis requested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I think I have a better approach. Go back to the old attribute system, but put the attribute ON THE TRAIT, not the impls. Then check it once, when we start the coherence check, and pass the boolean down to the impls.

@bors
Copy link
Contributor

bors commented Jan 26, 2022

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

@rust-timer
Copy link
Collaborator

Queued 107640c50fe413527be6069e6f181c54734f1b21 with parent a7f3757, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (107640c50fe413527be6069e6f181c54734f1b21): comparison url.

Summary: This benchmark run shows 128 relevant regressions 😿 to instruction counts.

  • Average relevant regression: 0.7%
  • Average relevant improvement: -0.8%
  • Largest regression in instruction counts: 1.9% on incr-patched: add vec item builds of deep-vector check

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 Jan 27, 2022
@lqd
Copy link
Member

lqd commented Jan 27, 2022 •
edited
Loading

As we thought yesterday, memoizing the overlap mode should somewhat alleviate these new losses (and turn some of them into small improvements), in addition to regaining the losses from the previous negative impl PR (as the stm32f4 benchmarks, which had been affected the most, look to be improved now). That'd be easy to try.

(Needless to mention, Niko's idea seems also worthwhile: coherence is hot, and the pairwise overlap checks already have superlinear scaling issues.)

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

bors commented Jan 28, 2022

⌛ Trying commit 6a78206c9a7d457b49dd481b2faa1bc548afd1a3 with merge ca75d679589565de2713c8cc6a2bc258d9604b40...

@spastorino
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

@bors
Copy link
Contributor

bors commented Jan 28, 2022

⌛ Trying commit 9ec1d6d1f44f8fc74aa754c145bfc12fdc50708b with merge b5dac1743636517402b355fd198ada684180f709...

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

bors commented Jan 30, 2022

⌛ Trying commit 240418a81de2bbded99885f79171ba3bf3d14a46 with merge 07719c4f11b6c559d462b1e46ba138537cb47505...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 30, 2022

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

@rust-timer
Copy link
Collaborator

Queued 07719c4f11b6c559d462b1e46ba138537cb47505 with parent 8c7f2bf, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (07719c4f11b6c559d462b1e46ba138537cb47505): comparison url.

Summary: This benchmark run did not return any relevant results. 4 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.

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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2022
nikomatsakis
nikomatsakis reviewed Jan 31, 2022
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me but one possible change would be to move overlap_mode out so it is not stored as a field; the idea is that it is only used during construction and doesn't matter afterwards, so it doesn't have to be a field

compiler/rustc_middle/src/traits/specialization_graph.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs Outdated Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

Here is a test we could use ---

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=16337df308de4a32e8117dfe8b9521c3

Without the feature gate, this gets an error.

With the feature gate, I think it should pass, because we have impl !DerefMut for &T

nikomatsakis
nikomatsakis approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit 0decf14 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 Jan 31, 2022
@bors
Copy link
Contributor

bors commented Jan 31, 2022

⌛ Testing commit 0decf14 with merge 498eeb7...

@bors
Copy link
Contributor

bors commented Jan 31, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 498eeb7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 31, 2022
@bors bors merged commit 498eeb7 into rust-lang:master Jan 31, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 31, 2022
This was referenced Jan 31, 2022
Merged
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (498eeb7): comparison url.

Summary: This benchmark run did not return any relevant results. 17 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

@matthiaskrgr matthiaskrgr mentioned this pull request Nov 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@spastorino @rust-timer @bors @nikomatsakis @rust-log-analyzer @lqd @rust-highfive @rustbot