Use fulfillment in method probe, not evaluation #122317

compiler-errors · 2024-03-11T00:25:39Z

This PR reworks method probing to use fulfillment instead of a for-loop of evaluate_predicate calls, and moves normalization from method candidate assembly into the consider_probe, where it's applied to all candidates. This last part coincidentally fixes #121643 (comment).

Regarding why this large rewrite is done: In general, it's an anti-pattern to do for o in obligations { evaluate(o); } because it's not compatible with the way that the new solver emits alias-relate obligations which constrain variables that may show up in other predicates.

r? lcnr

rustbot · 2024-03-11T00:25:48Z

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

compiler/rustc_trait_selection/src/traits/engine.rs

compiler-errors · 2024-03-11T15:19:46Z

@bors try

compiler/rustc_hir_typeck/src/method/probe.rs

bors · 2024-03-11T15:20:56Z

⌛ Trying commit 3702ab1 with merge 1f75533...

…, r=<try> Use fulfillment in method probe, not evaluation This PR reworks method probing to use fulfillment instead of a `for`-loop of `evaluate_predicate` calls, and moves normalization from method candidate assembly into the `consider_probe`, where it's applied to *all* candidates. This last part coincidentally fixes rust-lang#121643 (comment). Regarding *why* this large rewrite is done: In general, it's an anti-pattern to do `for o in obligations { evaluate(o); }` because it's not compatible with the way that the new solver emits alias-relate obligations which constrain variables that may show up in other predicates. Putting this up for vibe-check mostly. Tests aren't yet blessed, and there are some nuances about whether it's worthwhile to restore regressed diagnostics. r? lcnr

bors · 2024-03-11T16:48:23Z

☀️ Try build successful - checks-actions
Build commit: 1f75533 (1f7553361c08f8eafdc645e09215667278ab86f4)

compiler-errors · 2024-03-11T16:57:50Z

@craterbot check

craterbot · 2024-03-11T16:57:57Z

👌 Experiment pr-122317 created and queued.
🤖 Automatically detected try build 1f75533
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Crater rollup This is a crater rollup of: - [ ] rust-lang#122317 - [ ] rust-lang#122501 - [ ] rust-lang#122077 What does that mean? Well, I'm gonna run crater with p=3 on this PR, and then re-run *just* the list of failed crates on each PR. That should deduplicate the bulk of crates which we expect are unaffected by these PRs. Don't touch this PR please. r? `@ghost`

compiler-errors · 2024-03-14T23:11:08Z

@bors try

bors · 2024-03-14T23:12:18Z

⌛ Trying commit 35d6003 with merge 3aea706...

…, r=<try> Use fulfillment in method probe, not evaluation This PR reworks method probing to use fulfillment instead of a `for`-loop of `evaluate_predicate` calls, and moves normalization from method candidate assembly into the `consider_probe`, where it's applied to *all* candidates. This last part coincidentally fixes rust-lang#121643 (comment). Regarding *why* this large rewrite is done: In general, it's an anti-pattern to do `for o in obligations { evaluate(o); }` because it's not compatible with the way that the new solver emits alias-relate obligations which constrain variables that may show up in other predicates. Putting this up for vibe-check mostly. Tests aren't yet blessed, and there are some nuances about whether it's worthwhile to restore regressed diagnostics. r? lcnr

bors · 2024-03-15T00:41:13Z

☀️ Try build successful - checks-actions
Build commit: 3aea706 (3aea7069a654d82dd39c4479794b5560f4977307)

bors · 2024-03-15T00:41:13Z

☀️ Try build successful - checks-actions
Build commit: 3aea706 (3aea7069a654d82dd39c4479794b5560f4977307)

craterbot · 2024-03-18T15:10:39Z

🚧 Experiment pr-122317 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-03-20T04:51:15Z

🎉 Experiment pr-122317 is completed!
📊 34 regressed and 1 fixed (425628 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

bors · 2024-04-21T23:43:09Z

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

lcnr · 2024-04-23T13:41:42Z

@bors r+ rollup=never

bors · 2024-04-23T13:41:44Z

📌 Commit 8995c2c has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-04-23T14:07:08Z

⌛ Testing commit 8995c2c with merge cd90d5c...

bors · 2024-04-23T16:11:00Z

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

rust-timer · 2024-04-23T17:26:31Z

Finished benchmarking commit (cd90d5c): 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.5%	[0.2%, 1.3%]	38
Regressions ❌ (secondary)	4.2%	[0.5%, 23.6%]	39
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.2%, 1.3%]	38

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

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.5%	[1.3%, 1.6%]	2
Regressions ❌ (secondary)	6.7%	[2.1%, 25.2%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[1.3%, 1.6%]	2

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

Bootstrap: 673.699s -> 674.88s (0.18%)
Artifact size: 316.17 MiB -> 316.16 MiB (-0.00%)

compiler-errors · 2024-04-23T18:40:37Z

checking perf in #124303

Mark-Simulacrum · 2024-04-29T12:59:50Z

Marking this as triaged -- looks like attempts were made to improve performance in #124303 but did not pan out. The regressions are mostly pretty small with the exception of a stress test for exactly(?) this code.

…=<try> Relate receiver invariantly in method probe for `Mode::Path` Reverts rust-lang#126128 Fixes the part of rust-lang#122317 which always used subtyping instead of eq Fixes rust-lang#126227 (TODO: test) WIP: description r? lcnr

rustbot assigned lcnr Mar 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. labels Mar 11, 2024

compiler-errors commented Mar 11, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/engine.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

lcnr reviewed Mar 11, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/method/probe.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_typeck/src/method/probe.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_typeck/src/method/probe.rs Show resolved Hide resolved

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024

compiler-errors mentioned this pull request Mar 13, 2024

multiple applicable items in scope errors when using the new trait solver with diesel #122433

Closed

lcnr mentioned this pull request Mar 14, 2024

Recurse into args in WF even if they have escaping bound vars #122501

Closed

compiler-errors mentioned this pull request Mar 14, 2024

Crater rollup #122502

Closed

4 tasks

compiler-errors force-pushed the fulfill-method-probe branch from 3702ab1 to 35d6003 Compare March 14, 2024 18:52

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 20, 2024

compiler-errors added 6 commits April 21, 2024 20:10

Some early clean-ups in method probe

c13af7d

Use fulfillment, not evaluate, during method probe

ff4653a

Normalize xform_ret_ty after constrained

d9fec13

Check that predicate may hold BEFORE registering it in ocx

9d4f1d8

Restore error messaging

93c6c04

Use sup instead of eq when unifying self type

8995c2c

compiler-errors force-pushed the fulfill-method-probe branch from 685ba72 to 8995c2c Compare April 22, 2024 00:10

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 Apr 23, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2024

bors merged commit cd90d5c into rust-lang:master Apr 23, 2024
13 checks passed

rustbot added this to the 1.79.0 milestone Apr 23, 2024

This was referenced Apr 23, 2024

Perform autoref/autoderef on .await #111773

Closed

change method resolution to constrain hidden types instead of rejecting method candidates #123962

Merged

Add IntoIterator for Box<[T]> + edition 2024-specific lints #124097

Merged

rustbot added the perf-regression Performance regression. label Apr 23, 2024

matthiaskrgr mentioned this pull request Apr 24, 2024

ICE: entered unreachable code: encountered 'Item(Item.. due to Some(ObligationCause .. #124350

Open

apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Apr 29, 2024

oli-obk mentioned this pull request Jun 10, 2024

Consistently use subtyping in method resolution #126128

Merged

compiler-errors mentioned this pull request Aug 14, 2024

Relate receiver invariantly in method probe for Mode::Path #129073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fulfillment in method probe, not evaluation #122317

Use fulfillment in method probe, not evaluation #122317

compiler-errors commented Mar 11, 2024 •

edited

Loading

rustbot commented Mar 11, 2024

This comment has been minimized.

compiler-errors commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

compiler-errors commented Mar 11, 2024

craterbot commented Mar 11, 2024

compiler-errors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 15, 2024

bors commented Mar 15, 2024

craterbot commented Mar 18, 2024

craterbot commented Mar 20, 2024

bors commented Apr 21, 2024

lcnr commented Apr 23, 2024

bors commented Apr 23, 2024

bors commented Apr 23, 2024

bors commented Apr 23, 2024

rust-timer commented Apr 23, 2024

compiler-errors commented Apr 23, 2024

Mark-Simulacrum commented Apr 29, 2024

Use fulfillment in method probe, not evaluation #122317

Use fulfillment in method probe, not evaluation #122317

Conversation

compiler-errors commented Mar 11, 2024 • edited Loading

rustbot commented Mar 11, 2024

This comment has been minimized.

compiler-errors commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

compiler-errors commented Mar 11, 2024

craterbot commented Mar 11, 2024

compiler-errors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 15, 2024

bors commented Mar 15, 2024

craterbot commented Mar 18, 2024

craterbot commented Mar 20, 2024

bors commented Apr 21, 2024

lcnr commented Apr 23, 2024

bors commented Apr 23, 2024

bors commented Apr 23, 2024

bors commented Apr 23, 2024

rust-timer commented Apr 23, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

compiler-errors commented Apr 23, 2024

Mark-Simulacrum commented Apr 29, 2024

compiler-errors commented Mar 11, 2024 •

edited

Loading