Make `DefiningAnchor::Bind` only store the opaque types that may be constrained, instead of the current infcx root item. by oli-obk · Pull Request #121796 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation54 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This makes Bind almost always be empty, so we can start forwarding it to queries, allowing us to remove Bubble entirely (not done in this PR)

The only behaviour change is in diagnostics.

r? @lcnr @compiler-errors

This comment was marked as outdated.

rustbot added S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Mar 1, 2024

This comment was marked as resolved.

@rustbot ready for review

the first commit is from #121991 which is already approved and part of a rollup

@bors try @rust-timer queue

(would be fine with landing this even if it's perf negative, i'm just curious)

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

Make DefiningAnchor::Bind only store the opaque types that may be constrained, instead of the current infcx root item.

This makes Bind almost always be empty, so we can start forwarding it to queries, allowing us to remove Bubble entirely (not done in this PR)

The only behaviour change is in diagnostics.

r? @lcnr @compiler-errors

☀️ Try build successful - checks-actions
Build commit: ee25694 (ee25694924ae739071a8cbd23ab1252b5eb7994a)

This comment has been minimized.

Finished benchmarking commit (ee25694): comparison URL.

Overall result: ❌ regressions - 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.

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-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.6%	[0.2%, 1.8%]	123
Regressions ❌ (secondary)	0.6%	[0.2%, 1.2%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 1.8%]	123

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)	3.8%	[0.6%, 8.5%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[-1.1%, 8.5%]	6

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)	1.2%	[1.1%, 1.3%]	2
Regressions ❌ (secondary)	2.4%	[2.3%, 2.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	1.2%	[1.1%, 1.3%]	2

Binary size

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

Bootstrap: 644.654s -> 647.067s (0.37%)
Artifact size: 175.03 MiB -> 175.07 MiB (0.03%)

The regression in full builds is entirely due to all the invocations of opaque_types_defined_by

Incremental builds regress due to depgraph encoding.

I have ideas for the incremental cases, not sure what to do about the extra work in full builds

…onstrained, instead of the current infcx root item.

This makes Bind almost always be empty, so we can start forwarding it to queries, allowing us to remove Bubble entirely

…ningAnchor::Bind` with an empty list

📌 Commit b0328c2 has been approved by lcnr

It is now in the queue for this repository.

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

Mar 11, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 11, 2024

Pass list of defineable opaque types into canonical queries

based on rust-lang#121796

This eliminates DefiningAnchor::Bubble for good and brings the old solver closer to the new one wrt cycles and nested obligations. At that point the difference between DefiningAnchor::Bind([]) and DefiningAnchor::Error was academic. We only used the difference for some sanity checks, which actually had to be worked around in places, so I just removed DefiningAnchor entirely and just stored the list of opaques that may be defined.

fixes rust-lang#108498

run crater

1 similar comment

Finished benchmarking commit (4ccbb7d): 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.6%	[0.2%, 1.8%]	112
Regressions ❌ (secondary)	0.7%	[0.2%, 1.8%]	27
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 1.8%]	112

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)	2.1%	[0.7%, 3.2%]	3
Regressions ❌ (secondary)	4.7%	[2.0%, 13.2%]	10
Improvements ✅ (primary)	-1.6%	[-2.2%, -1.1%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-2.2%, 3.2%]	7

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

Binary size

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

Bootstrap: 669.92s -> 671.027s (0.17%)
Artifact size: 309.97 MiB -> 310.05 MiB (0.03%)

oli-obk deleted the eager_opaque_checks3 branch

March 12, 2024 05:45

This was referenced

Mar 13, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 21, 2024

Strip placeholders from hidden types before remapping generic parameter

When remapping generic parameters in the hidden type to the generic parameters of the definition of the opaque, we assume that placeholders cannot exist. Instead of just patching that site, I decided to handle it earlier, directly in infer_opaque_types, where we are already doing all the careful lifetime handling.

fixes rust-lang#122694

the reason that ICE now occurred was that we stopped treating operation as being in the defining scope, so the TAIT became part of the hidden type of the async fn's opaque type instead of just bailing out as ambiguos

I think

use std::future::Future;

mod foo {
    type FutNothing<'a> = impl 'a + Future<Output = ()>;
    //~^ ERROR: unconstrained opaque type
}

async fn operation(_: &mut ()) -> () {
    //~^ ERROR: concrete type differs from previous
    call(operation).await
    //~^ ERROR: concrete type differs from previous
}

async fn call<F>(_f: F)
where
    for<'any> F: FnMut(&'any mut ()) -> foo::FutNothing<'any>,
{
    //~^ ERROR: expected generic lifetime parameter, found `'any`
}

would have already had the same ICE before rust-lang#121796

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 21, 2024

Strip placeholders from hidden types before remapping generic parameter

fixes rust-lang#122694

I think

use std::future::Future;

mod foo {
    type FutNothing<'a> = impl 'a + Future<Output = ()>;
    //~^ ERROR: unconstrained opaque type
}

async fn operation(_: &mut ()) -> () {
    //~^ ERROR: concrete type differs from previous
    call(operation).await
    //~^ ERROR: concrete type differs from previous
}

async fn call<F>(_f: F)
where
    for<'any> F: FnMut(&'any mut ()) -> foo::FutNothing<'any>,
{
    //~^ ERROR: expected generic lifetime parameter, found `'any`
}

would have already had the same ICE before rust-lang#121796

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 21, 2024

Rollup merge of rust-lang#122733 - oli-obk:error_prop, r=compiler-errors

Strip placeholders from hidden types before remapping generic parameter

fixes rust-lang#122694

I think

use std::future::Future;

mod foo {
    type FutNothing<'a> = impl 'a + Future<Output = ()>;
    //~^ ERROR: unconstrained opaque type
}

async fn operation(_: &mut ()) -> () {
    //~^ ERROR: concrete type differs from previous
    call(operation).await
    //~^ ERROR: concrete type differs from previous
}

async fn call<F>(_f: F)
where
    for<'any> F: FnMut(&'any mut ()) -> foo::FutNothing<'any>,
{
    //~^ ERROR: expected generic lifetime parameter, found `'any`
}

would have already had the same ICE before rust-lang#121796

Make DefiningAnchor::Bind only store the opaque types that may be constrained, instead of the current infcx root item. by oli-obk · Pull Request #121796 · rust-lang/rust (original) (raw)

Conversation

This comment was marked as outdated.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

Overall result: ❌ regressions - ACTION NEEDED

Overall result: ❌ regressions - ACTION NEEDED

Make `DefiningAnchor::Bind` only store the opaque types that may be constrained, instead of the current infcx root item. by oli-obk · Pull Request #121796 · rust-lang/rust (original) (raw)