Merge `impl_polarity` and `impl_trait_ref` queries by oli-obk · Pull Request #120919 · rust-lang/rust

oli-obk · 2024-02-11T07:19:42Z

Hopefully this is perf neutral. I want to finish #120835 and stop using the HIR in coherent_trait, which should then give us a perf improvement.

rustbot · 2024-02-11T07:19:50Z

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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 · 2024-02-11T07:19:52Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

oli-obk · 2024-02-11T07:20:54Z

@bors try @rust-timer queue

Merge `impl_polarity` and `impl_trait_ref` queries Hopefully this is perf neutral. I wan to finish rust-lang#120835 and stop using the HIR in `coherent_trait`, which should then give us a perf improvement.

bors · 2024-02-11T07:22:05Z

⌛ Trying commit 1ce3522 with merge a711662...

bors · 2024-02-11T08:51:25Z

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

oli-obk · 2024-02-11T10:00:26Z

@rust-timer build a711662

rust-timer · 2024-02-11T13:29:58Z

Finished benchmarking commit (a711662): 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.5%	[0.3%, 0.7%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.2%]	47
Improvements ✅ (secondary)	-0.5%	[-1.3%, -0.3%]	10
All ❌✅ (primary)	-0.3%	[-0.7%, 0.7%]	51

Max RSS (memory usage)

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

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	3.7%	[2.0%, 5.4%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Binary size

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.1%	[-0.3%, -0.0%]	93
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.0%]	15
All ❌✅ (primary)	-0.1%	[-0.3%, -0.0%]	93

Bootstrap: 665.965s -> 664.862s (-0.17%)
Artifact size: 308.33 MiB -> 308.41 MiB (0.03%)

oli-obk · 2024-02-11T13:45:37Z

The few regressions we have all seem to be in metadata decoding. There are more improvements than regressions tho, so this seems good?

I'll give the regressions another go tho, I may have forgotten to remove an impl_polarity call where we already call impl_trait_header

Kobzol · 2024-02-11T14:47:03Z

All regressions are on a single benchmark (bitmaps), and there is a lot of improvements, so I'd go with it.

oli-obk · 2024-02-11T14:56:12Z

Some more review of the regressions:

it is not due to metadata loading, that balances out with the metadata loading gains from removing the other two queries
it's in the callers of the query (mir_drops_elaborated_and_const_checked, evaluate_obligation, check_well_formed). Tho I'm not sure why the callers got more expensive. Maybe just inlining noise due to the reordering of statements I had to do?

compiler-errors · 2024-02-11T15:44:52Z

I'll review this on monday

r? compiler-errors

compiler-errors

Do we have tests for unsafe impl Inherent being denied? r=me after fixing up the nits though.

…t the impl

oli-obk · 2024-02-12T09:47:32Z

Do we have tests for unsafe impl Inherent being denied?

yes https://github.com/rust-lang/rust/blob/master/tests/ui/error-codes/E0197.rs

@bors r=compiler-errors

bors · 2024-02-12T09:47:34Z

📌 Commit 74c9dff has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-13T02:48:52Z

⌛ Testing commit 74c9dff with merge d26b417...

bors · 2024-02-13T05:03:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing d26b417 to master...

rust-timer · 2024-02-13T08:31:38Z

Finished benchmarking commit (d26b417): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.6%	[0.5%, 0.7%]	3
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.2%]	43
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.2%]	5
All ❌✅ (primary)	-0.3%	[-0.7%, 0.7%]	46

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)	-0.9%	[-1.3%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-1.3%, -0.5%]	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)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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.1%	[-0.3%, -0.0%]	82
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.1%]	15
All ❌✅ (primary)	-0.1%	[-0.3%, -0.0%]	82

Bootstrap: 663.087s -> 662.665s (-0.06%)
Artifact size: 308.38 MiB -> 308.47 MiB (0.03%)

nnethercote · 2024-02-13T21:19:54Z

A net win overall, especially for icounts and binary size.

@rustbot label: +perf-regression-triaged

rustbot assigned michaelwoerister Feb 11, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 11, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2024

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2024

rustbot assigned compiler-errors and unassigned michaelwoerister Feb 11, 2024

compiler-errors suggested changes Feb 11, 2024

View reviewed changes

Comment thread compiler/rustc_hir_analysis/src/check/wfcheck.rs

Comment thread compiler/rustc_hir_analysis/src/collect.rs Outdated

Comment thread compiler/rustc_hir_typeck/src/method/suggest.rs Outdated

rustbot 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 Feb 11, 2024

oli-obk added 6 commits February 12, 2024 09:42

Make impl_trait_ref into a query also returning more information abou…

916951e

…t the impl

Use a struct instead of a tuple

90a43f1

Unwrap an Option that can only be Some, as inherent impls can't overlap

a9e0e96

Eagerly dismiss binder

ab0e8b3

Stop calling impl_polarity when impl_trait_ref was also called

b43fbe6

Remove impl_polarity query

74c9dff

oli-obk force-pushed the impl_polarity branch from 1ce3522 to 74c9dff Compare February 12, 2024 09:47

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 12, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 13, 2024

bors merged commit d26b417 into rust-lang:master Feb 13, 2024

rustbot added this to the 1.78.0 milestone Feb 13, 2024

bors mentioned this pull request Feb 13, 2024

Move alignment checks to codegen #117473

Closed

rustbot added the perf-regression Performance regression. label Feb 13, 2024

oli-obk deleted the impl_polarity branch February 13, 2024 08:32

rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 13, 2024

Uh oh!

Conversation

oli-obk commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 11, 2024

Uh oh!

rustbot commented Feb 11, 2024

Uh oh!

oli-obk commented Feb 11, 2024

Uh oh!

This comment has been minimized.

bors commented Feb 11, 2024

Uh oh!

bors commented Feb 11, 2024

Uh oh!

oli-obk commented Feb 11, 2024

Uh oh!

rust-timer commented Feb 11, 2024

Overall result: ✅ improvements - no action needed

Uh oh!

oli-obk commented Feb 11, 2024

Uh oh!

Kobzol commented Feb 11, 2024

Uh oh!

oli-obk commented Feb 11, 2024

Uh oh!

compiler-errors commented Feb 11, 2024

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Feb 12, 2024

Uh oh!

bors commented Feb 12, 2024

Uh oh!

bors commented Feb 13, 2024

Uh oh!

bors commented Feb 13, 2024

Uh oh!

rust-timer commented Feb 13, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

nnethercote commented Feb 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

oli-obk commented Feb 11, 2024 •

edited

Loading