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

A trait's local impls are trivially coherent if there are no impls. #120834

Merged
merged 1 commit into from
Feb 12, 2024
Merged

A trait's local impls are trivially coherent if there are no impls. #120834

merged 1 commit into from
Feb 12, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 9, 2024

This avoids creating a dependency edge on the hir or the specialization graph

This may resolve part of the performance issue of #120558

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

@bors try @rust-timer queue

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 9, 2024
@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 9, 2024
@bors
Copy link
Contributor

bors commented Feb 9, 2024

⌛ Trying commit 0727f9a with merge 7c63899...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
@bors
Auto merge of rust-lang#120834 - oli-obk:only_local_coherence, r=<try>
7c63899 
A trait's local impls are trivially coherent if there are no impls.

This avoids creating a dependency edge on the hir or the specialization graph

This may resolve part of the performance issue of rust-lang#120558
lcnr
lcnr reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

unfortunate if this is necessary for perf

r=me after perf + comment

compiler/rustc_hir_analysis/src/coherence/mod.rs
@@ -120,11 +120,11 @@ pub fn provide(providers: &mut Providers) {
}

fn coherent_trait(tcx: TyCtxt<'_>, def_id: DefId) -> Result<(), ErrorGuaranteed> {
let Some(impls) = tcx.all_local_trait_impls(()).get(&def_id) else { return Ok(()) };
Copy link
Contributor

Choose a reason for hiding this comment

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

please explain why we early return here and why this is sound

@oli-obk
A trait's local impls are trivially coherent if there are no impls.
e2349ea 
This avoids creating a dependency edge on the hir or the specialization graph
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

unfortunate if this is necessary for perf

I think the change is good by itself. Avoiding directly accessing the HIR except in the error paths is always an improvement for incremental.

@oli-obk oli-obk mentioned this pull request Feb 9, 2024
Merged
@lcnr
Copy link
Contributor

lcnr commented Feb 9, 2024

I think the change is good by itself. Avoiding directly accessing the HIR except in the error paths is always an improvement for incremental.

for perf/incremental yes, but it does add complexity :< i would not add this special-case to a formalization of rust :p

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

it does add complexity

the order of operations should not matter in a formalization.

We could probably just move the specialization graph query call out of the coherent_trait query and invoke it in parallel instead.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
@bors
Auto merge of rust-lang#120835 - oli-obk:no_hir_coherence, r=<try>
83055d7 
Avoid accessing the HIR in the happy path of `coherent_trait`

based on rust-lang#120834

This may resolve part of the performance issue of rust-lang#120558
@bors
Copy link
Contributor

bors commented Feb 9, 2024

☀️ Try build successful - checks-actions
Build commit: 7c63899 (7c638996380cc1b1285e6d37b124cce4e771e721)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7c63899): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.5% [-1.0%, -0.2%] 12
Improvements ✅
(secondary)		 -1.4% [-3.9%, -0.3%] 46
All ❌✅ (primary) -0.5% [-1.0%, -0.2%] 12

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -6.0% [-14.5%, -0.9%] 3
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -6.0% [-14.5%, -0.9%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.7% [-4.7%, -2.1%] 8
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 661.327s -> 662.269s (0.14%)
Artifact size: 308.28 MiB -> 308.25 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 9, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Feb 9, 2024

📌 Commit 0727f9a has been approved by lcnr

It is now in the queue for this repository.

@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 9, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

@bors r- I only pushed the new comment on the other PR and forgot it here

@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 9, 2024
@oli-obk oli-obk force-pushed the only_local_coherence branch from 0727f9a to e2349ea Compare February 9, 2024 17:02
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2024

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Feb 9, 2024

📌 Commit e2349ea has been approved by lcnr

It is now in the queue for this repository.

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

bors commented Feb 12, 2024

⌛ Testing commit e2349ea with merge de4d615...

@bors
Copy link
Contributor

bors commented Feb 12, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing de4d615 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2024
@bors bors merged commit de4d615 into rust-lang:master Feb 12, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 12, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (de4d615): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.4% [-1.0%, -0.2%] 10
Improvements ✅
(secondary)		 -1.4% [-3.5%, -0.3%] 24
All ❌✅ (primary) -0.4% [-1.0%, -0.2%] 10

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.2% [-3.3%, -3.1%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -3.2% [-3.3%, -3.1%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.1% [5.1%, 5.2%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.9% [-4.7%, -2.1%] 8
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 664.268s -> 663.665s (-0.09%)
Artifact size: 308.43 MiB -> 308.44 MiB (0.00%)

@oli-obk oli-obk deleted the only_local_coherence branch February 12, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

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.78.0
Development

Successfully merging this pull request may close these issues.
5 participants
@oli-obk @rustbot @rust-timer @bors @lcnr