Implement `deferred_projection_equality` for erica solver #107507

BoxyUwU · 2023-01-31T09:25:38Z

Somewhat of a revival of #96912. When relating projections now emit an AliasEq obligation instead of attempting to determine equality of projections that may not be as normalized as possible (i.e. because of lazy norm, or just containing inference variables that prevent us from resolving an impl). Only do this when the new solver is enabled

rustbot · 2023-01-31T09:25:46Z

r? @jackh726

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

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs

compiler/rustc_trait_selection/src/traits/object_safety.rs

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

compiler/rustc_infer/src/infer/combine.rs

compiler/rustc_trait_selection/src/solve/mod.rs

rustbot · 2023-02-01T05:38:26Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the core trait solver

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

lcnr

only looked at solver impl, but alright looks pretty nice.

compiler/rustc_hir_analysis/src/astconv/mod.rs

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

compiler/rustc_hir_analysis/src/outlives/explicit.rs

compiler/rustc_trait_selection/src/solve/mod.rs

bors · 2023-02-05T11:11:42Z

☔ The latest upstream changes (presumably #107434) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

compiler/rustc_infer/src/infer/nll_relate/mod.rs

compiler/rustc_infer/src/infer/projection.rs

lcnr · 2023-02-08T19:12:48Z

compiler/rustc_lint/src/builtin.rs

                    // Ignore bounds that a user can't type
                    WellFormed(..) |
                    ObjectSafe(..) |
                    ClosureKind(..) |
                    Subtype(..) |
                    Coerce(..) |
+                    // FIXME(generic_const_exprs): `ConstEvaluatable` can be written


same for WellFormed, isn't it 😁

compiler/rustc_middle/src/ty/flags.rs

compiler/rustc_trait_selection/src/solve/mod.rs

compiler/rustc_trait_selection/src/traits/object_safety.rs

compiler/rustc_trait_selection/src/traits/wf.rs

lcnr · 2023-02-08T19:29:40Z

src/librustdoc/clean/mod.rs

@@ -310,10 +310,12 @@ pub(crate) fn clean_predicate<'tcx>(
        ty::PredicateKind::Clause(ty::Clause::Projection(pred)) => {
            Some(clean_projection_predicate(bound_predicate.rebind(pred), cx))
        }
+        // FIXME(generic_const_exprs): should this do something?


nope, unreachable anyways (hopefully)

bors · 2023-02-10T06:21:01Z

☔ The latest upstream changes (presumably #101680) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr

very cool :3

r=me after final nits (and perf i guess)

compiler/rustc_trait_selection/src/solve/mod.rs

tests/ui/traits/new-solver/alias_eq_dont_use_normalizes_to_if_substs_eq.rs

tests/ui/traits/new-solver/normalizes_to_ignores_unnormalizable_candidate.rs

compiler/rustc_middle/src/ty/context.rs

lcnr · 2023-02-10T14:17:31Z

@bors try @rust-timer queue

bors · 2023-02-10T14:17:40Z

⌛ Trying commit deaea6a7570b74033e739a622862c3cb720289fc with merge 9ea0bea458fc11f79a038ebaab04bd654183277f...

bors · 2023-02-10T20:18:16Z

⌛ Trying commit 4c98429 with merge ae79965995e38045893ccc99907b3efabd9475f2...

bors · 2023-02-10T22:30:16Z

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

rust-timer · 2023-02-11T00:04:52Z

Finished benchmarking commit (ae79965995e38045893ccc99907b3efabd9475f2): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Cycles

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

BoxyUwU · 2023-02-11T00:10:01Z

@bors r=lcnr

bors · 2023-02-11T00:10:04Z

📌 Commit 4c98429 has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-02-11T05:46:27Z

⌛ Testing commit 4c98429 with merge 1623ab0...

bors · 2023-02-11T09:19:01Z

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

rust-timer · 2023-02-11T10:38:05Z

Finished benchmarking commit (1623ab0): 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.8%	[0.7%, 1.0%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.7%, 1.0%]	6

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.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.2%	[-3.7%, -2.6%]	7
Improvements ✅ (secondary)	-2.6%	[-3.8%, -0.8%]	38
All ❌✅ (primary)	-2.5%	[-3.7%, 2.5%]	8

Cycles

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

BoxyUwU · 2023-02-11T23:11:09Z

noise

rylev · 2023-02-14T16:12:15Z

Doing perf triage - @BoxyUwU are you sure this is noise? Looking at the graphs, it looks like this change was sustained. The graph shoes changes from the previous run and the spike is this change. Noise is usually indicated by a corresponding correction in the other direction which this doesn't have. The change is large enough that it might be worth looking into.

Given that the regression is in Diesel which exercises the type checker really heavily it's not so surprising that we might find a regression there.

BoxyUwU · 2023-02-14T16:24:10Z

we did a perf run before landing this and it was fine and nothing had changed since.

rylev · 2023-02-14T17:07:02Z

@BoxyUwU unfortunately there can still be a perf regression. It's possible that changes introduced in master since you first ran the perf regression interact with these change differently causing these changes to introduce perf regressions. In other words, it's completely possible for a perf run to show no regressions and then regressions to appear after the change is merged into master. That's not noise unfortunately.

A change of 1% against a popular crate isn't huge but it's also not small. Then again the significance factors are fairly low. It's only .5 times higher than the significance threshold. I'll leave it up to you and @lcnr to decide (please do leave your decision with the label "perf-regression-triaged" set).

BoxyUwU · 2023-02-14T17:12:49Z

This PR doesn't really do Much stable facing, it looks a lot bigger than it actually is. The only changes that could really effect anything would be

a few branches on what solver is enabled
adding a new PredicateKind (which did not increase the size) maybe meaning that some matches got a bit different codegen

I don't think we can really do anything about second thing, the first thing otoh will go away when the new solver is stabilized and the old one is removed. Lazy norm afaik is pretty much required long term for rustc, there are soundness bugs caused by us not having it, and its generally a bit of a nightmare attempting to maintain and implement stuff in the type system without. Probably just have to eat this regression?

nnethercote · 2023-02-14T22:24:27Z

I bet the new PredicateKind is the cause. diesel stresses the predicate folding and interning code much more than other benchmarks. I can believe that an extra variant could cause this regression. (But I'm not advocating one way or another whether this regression is acceptable.)

jackh726 · 2023-02-15T00:47:02Z

I'm just going to weigh in a little bit here:

First, it's not completely clear that this regression is or isn't noise; I've seen both regressions and improvements to diesel and the like of this magnitude that are nearly certainly noise. If this is noise, just because we haven't seen the correction "yet" doesn't mean we won't at some point in the future.

Second, to expand on that a bit, while it is true that one perf run to the next can mean that the base has changed enough to cause a regression, the time between these in this case was less than 12 hours. In the time between perf builds, there were two commits, one clippy update and one mir opt (#85158); neither touched trait code, but it could very well be that the mir opt could cause a difference with this PR. However, I'm not really even confident in saying that.

Third, to be frank, the regression here (noise or not) is certainly acceptable. The work in this PR is critical for future trait solver endeavors. Because the regression is basically in the "noise" range, I don't think it's something we should even think about if there is something to "unregress" perf.

rylev · 2023-02-15T10:46:12Z

Probably just have to eat this regression?

That's certainly a valid path forward. Given the additional context here, it seems like it's likely the right one too.

The work in this PR is critical for future trait solver endeavors.

This is just the context I was looking for. Since the perf triagers are not involved in the day to day of many of the compiler changes we see, we rely on the authors and reviewers to give us this additional context to know whether the regression is worth poking into more.

…, r=lcnr Implement `deferred_projection_equality` for erica solver Somewhat of a revival of rust-lang#96912. When relating projections now emit an `AliasEq` obligation instead of attempting to determine equality of projections that may not be as normalized as possible (i.e. because of lazy norm, or just containing inference variables that prevent us from resolving an impl). Only do this when the new solver is enabled

rustbot assigned jackh726 Jan 31, 2023

compiler-errors reviewed Jan 31, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jan 31, 2023

View reviewed changes

BoxyUwU force-pushed the deferred_projection_equality branch from bbed750 to d999f00 Compare February 1, 2023 05:38

BoxyUwU assigned lcnr Feb 1, 2023

BoxyUwU added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Feb 1, 2023

lcnr reviewed Feb 1, 2023

View reviewed changes

BoxyUwU force-pushed the deferred_projection_equality branch 4 times, most recently from bd77e37 to 6393303 Compare February 2, 2023 06:20

BoxyUwU force-pushed the deferred_projection_equality branch from 6393303 to a38e363 Compare February 5, 2023 16:05

lcnr reviewed Feb 8, 2023

View reviewed changes

add AliasEq to PredicateKind

23ab246

BoxyUwU force-pushed the deferred_projection_equality branch from a38e363 to deaea6a Compare February 10, 2023 13:51

lcnr reviewed Feb 10, 2023

View reviewed changes

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 10, 2023

emit AliasEq when relating type and const aliases

1f89e2a

This comment has been minimized.

Sign in to view

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

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 11, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 11, 2023

bors merged commit 1623ab0 into rust-lang:master Feb 11, 2023

rustbot added this to the 1.69.0 milestone Feb 11, 2023

rustbot added the perf-regression Performance regression. label Feb 11, 2023

BoxyUwU added the perf-regression-triaged The performance regression has been triaged. label Feb 11, 2023

lcnr mentioned this pull request Feb 14, 2023

Tracking Issue for -Znext-solver #107374

Open

11 tasks

compiler-errors mentioned this pull request Jul 27, 2023

async fn trait's method's future does not implement Send trait bound #114142

Closed

matthiaskrgr mentioned this pull request Nov 14, 2023

ICE: resolve: unreachable!() #117920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `deferred_projection_equality` for erica solver #107507

Implement `deferred_projection_equality` for erica solver #107507

BoxyUwU commented Jan 31, 2023 •

edited

Loading

rustbot commented Jan 31, 2023

rustbot commented Feb 1, 2023

lcnr left a comment

bors commented Feb 5, 2023

lcnr Feb 8, 2023

BoxyUwU Feb 10, 2023

lcnr Feb 8, 2023

bors commented Feb 10, 2023

lcnr left a comment

lcnr commented Feb 10, 2023

This comment has been minimized.

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

This comment has been minimized.

rust-timer commented Feb 11, 2023

BoxyUwU commented Feb 11, 2023

bors commented Feb 11, 2023

bors commented Feb 11, 2023

bors commented Feb 11, 2023

rust-timer commented Feb 11, 2023

BoxyUwU commented Feb 11, 2023

rylev commented Feb 14, 2023

BoxyUwU commented Feb 14, 2023

rylev commented Feb 14, 2023

BoxyUwU commented Feb 14, 2023

nnethercote commented Feb 14, 2023

jackh726 commented Feb 15, 2023

rylev commented Feb 15, 2023

Implement deferred_projection_equality for erica solver #107507

Implement deferred_projection_equality for erica solver #107507

Conversation

BoxyUwU commented Jan 31, 2023 • edited Loading

rustbot commented Jan 31, 2023

rustbot commented Feb 1, 2023

lcnr left a comment

Choose a reason for hiding this comment

bors commented Feb 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 10, 2023

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Feb 10, 2023

This comment has been minimized.

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

This comment has been minimized.

rust-timer commented Feb 11, 2023

Overall result: no relevant changes - no action needed

BoxyUwU commented Feb 11, 2023

bors commented Feb 11, 2023

bors commented Feb 11, 2023

bors commented Feb 11, 2023

rust-timer commented Feb 11, 2023

Overall result: ❌ regressions - ACTION NEEDED

BoxyUwU commented Feb 11, 2023

rylev commented Feb 14, 2023

BoxyUwU commented Feb 14, 2023

rylev commented Feb 14, 2023

BoxyUwU commented Feb 14, 2023

nnethercote commented Feb 14, 2023

jackh726 commented Feb 15, 2023

rylev commented Feb 15, 2023

Implement `deferred_projection_equality` for erica solver #107507

Implement `deferred_projection_equality` for erica solver #107507

BoxyUwU commented Jan 31, 2023 •

edited

Loading