Stop bailing out from compilation just because there were incoherent traits #120558

oli-obk · 2024-02-01T17:16:10Z

fixes #120343

but also has a lot of "type annotations needed" fallout. Some are fixed in the second commit.

rustbot · 2024-02-01T17:16:20Z

(rustbot has picked a reviewer for you, use r? to override)

tests/ui/specialization/min_specialization/bad-const-wf-doesnt-specialize.rs

nnethercote · 2024-02-01T21:10:23Z

That's a lot of error output changes for such small compiler changes. I'm uncomfortable reviewing this because I know very little about trait stuff in general.

r? @lcnr

(or anyone else who might want to steal it, such as @compiler-errors or @estebank)

tests/ui/marker_trait_attr/unsound-overlap.stderr

tests/ui/coherence/coherence-impls-copy.stderr

tests/ui/coherence/coherence-impls-sized.stderr

oli-obk · 2024-02-02T08:37:27Z

r? @estebank

tests/ui/coherence/occurs-check/associated-type2.rs

estebank · 2024-02-02T19:32:01Z

tests/ui/coherence/occurs-check/opaques.next.stderr

+error[E0282]: type annotations needed
+  --> $DIR/opaques.rs:13:20
+   |
+LL |     pub fn cast<T>(x: Container<Alias<T>, T>) -> Container<T, T> {
+   |                    ^ cannot infer type for struct `Container<Alias<T>, T>`


Unfortunate but ok.

yea this one had me scratching my head. It's an unfortunate combination of opaque types and type inference, but has a good chance of getting properly resolved with cleanups that will be enabled by #113169

estebank · 2024-02-02T19:32:54Z

@bors r+

bors · 2024-02-02T19:32:56Z

📌 Commit 965da3a has been approved by estebank

It is now in the queue for this repository.

bors · 2024-02-02T19:32:56Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

oli-obk · 2024-02-02T22:39:47Z

@bors r-

rust-timer · 2024-02-08T08:19:25Z

Finished benchmarking commit (870a01a): comparison URL.

Overall result: ❌ regressions - 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.4%	[0.2%, 1.0%]	14
Regressions ❌ (secondary)	1.2%	[0.1%, 3.5%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.0%]	14

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)	8.6%	[2.5%, 14.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.1%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.8%	[-1.1%, 14.8%]	4

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.6%	[3.6%, 3.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 663.578s -> 662.662s (-0.14%)
Artifact size: 308.27 MiB -> 308.26 MiB (-0.00%)

…l, r=<try> Use `ensure` when the result of the query is not needed beyond its `Result`ness while I would like to just remove the `tcx` methods for ensure-only queries, that is hard to do without another query annotation or by turning the `define_callbacks` macro into a proc macro to get more control should fix perf regression of rust-lang#120558

nnethercote · 2024-02-08T09:45:12Z

@oli-obk: a bunch of possible icount regressions, is that expected?

oli-obk · 2024-02-08T10:05:27Z

I already opened a PR: #120771

it is not expected, but from the detailed diffs (only incremental cache changes, no actual execution changes), I presume it's that one missing ensure call

oli-obk · 2024-02-08T17:41:22Z

Weirdly, while that PR is a perf improvement, it's completely orthogonal to this PR (because I also fixed another missing ensure).

I'll dig into what happened here, This regression must be fixable, we're not actually doing extra work in successful builds, so it must just be some query/caching thing

oli-obk · 2024-02-09T09:10:15Z

The queries I'm ensureing here, are not decoded from the cache at all (they aren't cached). All that is happening is that they get checked for being green. Since only incr_comp_encode_dep_graph regresses, the issue is in fact writing the additional edge to the dep graph. I do not see a way to resolve this regression.

I mean, since we only use the query to check for errors at the individual item level, we could just not ensure the query if

we know it must have already been invoked, and
has_errors is false

but that feels quite hacky.

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

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

oli-obk · 2024-02-09T13:52:42Z

Perf regression fully resolved after #120835 (comment)

@rustbot label: +perf-regression-triaged

Avoid accessing the HIR in the happy path of `coherent_trait` This may resolve part of the performance issue of rust-lang#120558

…l, r=davidtwco Use `ensure` when the result of the query is not needed beyond its `Result`ness while I would like to just remove the `tcx` methods for ensure-only queries, that is hard to do without another query annotation or by turning the `define_callbacks` macro into a proc macro to get more control should fix perf regression of rust-lang#120558

…dtwco Use `ensure` when the result of the query is not needed beyond its `Result`ness while I would like to just remove the `tcx` methods for ensure-only queries, that is hard to do without another query annotation or by turning the `define_callbacks` macro into a proc macro to get more control should fix perf regression of rust-lang/rust#120558

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

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/rust#120558

Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls fixes rust-lang#123292 Also removes a bunch of extra diagnostics that were introduced in rust-lang#121154 and rust-lang#120558

Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls fixes #123292 Also removes a bunch of extra diagnostics that were introduced in rust-lang/rust#121154 and rust-lang/rust#120558

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/rust#120558

rustbot assigned nnethercote Feb 1, 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. labels Feb 1, 2024

oli-obk commented Feb 1, 2024

View reviewed changes

tests/ui/specialization/min_specialization/bad-const-wf-doesnt-specialize.rs Outdated Show resolved Hide resolved

rustbot assigned lcnr and unassigned nnethercote Feb 1, 2024

Noratrieb reviewed Feb 1, 2024

View reviewed changes

tests/ui/marker_trait_attr/unsound-overlap.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

estebank reviewed Feb 1, 2024

View reviewed changes

tests/ui/coherence/coherence-impls-copy.stderr Outdated Show resolved Hide resolved

estebank reviewed Feb 1, 2024

View reviewed changes

tests/ui/coherence/coherence-impls-sized.stderr Outdated Show resolved Hide resolved

oli-obk force-pushed the missing_impl_item_ice branch from 08e7773 to a9ca5b1 Compare February 2, 2024 08:36

rustbot assigned estebank and unassigned lcnr Feb 2, 2024

lcnr reviewed Feb 2, 2024

View reviewed changes

tests/ui/coherence/occurs-check/associated-type2.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the missing_impl_item_ice branch from a9ca5b1 to 965da3a Compare February 2, 2024 09:00

estebank reviewed Feb 2, 2024

View reviewed changes

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

This comment has been minimized.

Sign in to view

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

oli-obk force-pushed the missing_impl_item_ice branch from 47c97b1 to b044242 Compare February 4, 2024 13:07

This was referenced Feb 8, 2024

Added traits implemented by FnPtr to fn docs with example function #112106

Open

Run const_prop_lint in check builds, too #120687

Closed

Change leak check and suspicious auto trait lint warning messages #120716

Merged

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

oli-obk deleted the missing_impl_item_ice branch February 8, 2024 09:16

oli-obk mentioned this pull request Feb 8, 2024

Use ensure when the result of the query is not needed beyond its Resultness #120771

Merged

oli-obk mentioned this pull request Feb 9, 2024

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

Merged

oli-obk mentioned this pull request Feb 9, 2024

Avoid accessing the HIR in the happy path of coherent_trait #120835

Merged

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024

Auto merge of rust-lang#120835 - oli-obk:no_hir_coherence, r=<try>

af14861

Avoid accessing the HIR in the happy path of `coherent_trait` This may resolve part of the performance issue of rust-lang#120558

oli-obk mentioned this pull request Apr 9, 2024

Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls #123674

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop bailing out from compilation just because there were incoherent traits #120558

Stop bailing out from compilation just because there were incoherent traits #120558

oli-obk commented Feb 1, 2024 •

edited

Loading

rustbot commented Feb 1, 2024

nnethercote commented Feb 1, 2024

This comment has been minimized.

oli-obk commented Feb 2, 2024

estebank Feb 2, 2024

oli-obk Feb 2, 2024

estebank commented Feb 2, 2024

bors commented Feb 2, 2024

bors commented Feb 2, 2024

This comment has been minimized.

This comment has been minimized.

oli-obk commented Feb 2, 2024

rust-timer commented Feb 8, 2024

nnethercote commented Feb 8, 2024

oli-obk commented Feb 8, 2024

oli-obk commented Feb 8, 2024

oli-obk commented Feb 9, 2024

oli-obk commented Feb 9, 2024

Stop bailing out from compilation just because there were incoherent traits #120558

Stop bailing out from compilation just because there were incoherent traits #120558

Conversation

oli-obk commented Feb 1, 2024 • edited Loading

rustbot commented Feb 1, 2024

nnethercote commented Feb 1, 2024

This comment has been minimized.

oli-obk commented Feb 2, 2024

estebank Feb 2, 2024

Choose a reason for hiding this comment

oli-obk Feb 2, 2024

Choose a reason for hiding this comment

estebank commented Feb 2, 2024

bors commented Feb 2, 2024

bors commented Feb 2, 2024

This comment has been minimized.

This comment has been minimized.

oli-obk commented Feb 2, 2024

rust-timer commented Feb 8, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented Feb 8, 2024

oli-obk commented Feb 8, 2024

oli-obk commented Feb 8, 2024

oli-obk commented Feb 9, 2024

oli-obk commented Feb 9, 2024

oli-obk commented Feb 1, 2024 •

edited

Loading