-Znext-solver Less normalizes-to janks

[ShoyuVanilla]

View all comments

cc rust-lang/trait-system-refactor-initiative#223 (comment)

r? lcnr

[rustbot]

Some changes occurred to the core trait solver

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

changes to inspect_obligations.rs

cc @lcnr

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

[ShoyuVanilla]

@bors try @rust-timer queue

[ShoyuVanilla]

I haven't done the nested goals' recursion depth part yet 😅 I'm gonna work on it if the overall design here pan out to be feasible

[rust-bors]

☀️ Try build successful (CI)
Build commit: d4c528b (d4c528b37c59bf04c41ddba6375584d284d1a9ba, parent: 88ba7fbe0a6eda36e0adbfd0482856f3784031b9)

[rust-timer]

Finished benchmarking commit (d4c528b): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.3%, 0.8%]	7
Regressions ❌ (secondary)	22.5%	[0.6%, 46.0%]	12
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.1%]	20
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	29
All ❌✅ (primary)	-0.1%	[-0.7%, 0.8%]	27

Max RSS (memory usage)

Results (primary 1.5%, secondary 8.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.5%	[0.9%, 2.6%]	5
Regressions ❌ (secondary)	11.4%	[2.0%, 25.8%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-14.8%	[-14.8%, -14.8%]	1
All ❌✅ (primary)	1.5%	[0.9%, 2.6%]	5

Cycles

Results (secondary 26.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	26.9%	[17.5%, 41.5%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 510.626s -> 509.733s (-0.17%)
Artifact size: 398.07 MiB -> 398.06 MiB (-0.00%)

[ShoyuVanilla]

Oh, the perf regressions are insane 😅 I'll look into them

[ShoyuVanilla]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 52311d2 (52311d29eb5b4b58f67e481749703829fab93a19, parent: 35143615544ede08a47947901cd4a6b7c5ecd450)

[rust-timer]

Finished benchmarking commit (52311d2): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.2%, 1.2%]	8
Regressions ❌ (secondary)	26.5%	[0.1%, 45.5%]	10
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	23
Improvements ✅ (secondary)	-0.4%	[-0.8%, -0.2%]	29
All ❌✅ (primary)	0.0%	[-0.3%, 1.2%]	31

Max RSS (memory usage)

Results (primary 1.7%, secondary 8.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.7%	[1.0%, 2.8%]	6
Regressions ❌ (secondary)	11.3%	[2.1%, 25.8%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-14.7%	[-14.7%, -14.7%]	1
All ❌✅ (primary)	1.7%	[1.0%, 2.8%]	6

Cycles

Results (primary 2.3%, secondary 22.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	25.0%	[2.4%, 43.8%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 513.812s -> 509.975s (-0.75%)
Artifact size: 398.43 MiB -> 400.31 MiB (0.47%)

[ShoyuVanilla]

If my observation is correct, this would fix the perf regression 🤔
@bors try @rust-timer queue

[ShoyuVanilla]

Gosh, why these sus failures happens to my PRs every time 😂
@bors retry

[ShoyuVanilla]

@bors r-

[rust-bors]

This pull request was unapproved.

Auto build was cancelled due to unapproval. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/27601498493

View changes since this unapproval

[ShoyuVanilla]

I'm pretty sure the CI would be green because the diff is trivial and I tested locally 😅
@bors r=lcnr

[rust-bors]

📌 Commit a23b443 has been approved by lcnr

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 25m 16s
Pushing c712ea9 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 89a9993 (parent) -> c712ea9 (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard c712ea94628bbc7b9cd3030d4a79324c13bbc842 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. x86_64-gnu-llvm-22-2: 1h 4m -> 1h 46m (+65.7%)
2. x86_64-gnu-stable: 2h 31m -> 1h 32m (-38.8%)
3. dist-sparcv9-solaris: 1h 15m -> 1h 36m (+28.0%)
4. dist-arm-linux-gnueabi: 1h 13m -> 1h 32m (+26.4%)
5. dist-x86_64-netbsd: 1h 10m -> 1h 28m (+24.3%)
6. dist-aarch64-apple: 1h 41m -> 2h 5m (+23.8%)
7. dist-x86_64-msvc: 1h 44m -> 2h 9m (+23.6%)
8. dist-loongarch64-linux: 1h 33m -> 1h 54m (+23.6%)
9. x86_64-gnu-distcheck: 1h 58m -> 2h 26m (+23.5%)
10. dist-apple-various: 1h 53m -> 2h 18m (+22.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (c712ea9): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.8%	[0.8%, 0.9%]	3
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.1%]	16
Improvements ✅ (secondary)	-1.2%	[-3.6%, -0.1%]	20
All ❌✅ (primary)	-0.3%	[-0.6%, -0.1%]	16

Max RSS (memory usage)

Results (primary 4.9%, secondary -2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.9%	[2.9%, 6.9%]	2
Regressions ❌ (secondary)	2.8%	[0.7%, 6.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.9%	[-17.9%, -1.7%]	4
All ❌✅ (primary)	4.9%	[2.9%, 6.9%]	2

Cycles

Results (primary 0.3%, secondary 1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	3.7%	[2.1%, 5.3%]	9
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-2.9%	[-3.4%, -2.5%]	5
All ❌✅ (primary)	0.3%	[-2.5%, 3.1%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 522.035s -> 519.996s (-0.39%)
Artifact size: 401.81 MiB -> 401.25 MiB (-0.14%)

[ShoyuVanilla]

On perf regression: this is mostly improvement and the regression only happens with wg-grammar with the second benchmark, with the next-solver.

With this PR, we need relatively lesser depth in nested goals when normalizing things. This behavior slightly improves perf in most cases, but in wg-grammar, our type inference already has been spending a lot of time on gathering obligations_for_self_ty to deduce the types of closures:

rust/compiler/rustc_hir_typeck/src/closure.rs

Line 320 in 1c0f175

self.obligations_for_self_ty(vid)

rust/compiler/rustc_hir_typeck/src/fn_ctxt/inspect_obligations.rs

Lines 197 to 235 in 1c0f175

	InspectConfig { max_depth: 5 }
	}
	fn visit_goal(&mut self, inspect_goal: &InspectGoal<'_, 'tcx>) {
	// No need to walk into goal subtrees that certainly hold, since they
	// wouldn't then be stalled on an infer var.
	if inspect_goal.result() == Ok(Certainty::Yes) {
	return;
	}
	// We don't care about any pending goals which don't actually
	// use the self type.
	if !inspect_goal
	.orig_values()
	.iter()
	.filter_map(|arg| arg.as_type())
	.any(|ty| self.fcx.type_matches_expected_vid(ty, self.self_ty))
	{
	debug!(goal = ?inspect_goal.goal(), "goal does not mention self type");
	return;
	}
	let tcx = self.fcx.tcx;
	let goal = inspect_goal.goal();
	if self.fcx.predicate_has_self_ty(goal.predicate, self.self_ty) {
	self.obligations_for_self_ty.push(traits::Obligation::new(
	tcx,
	self.root_cause.clone(),
	goal.param_env,
	goal.predicate,
	));
	}
	// If there's a unique way to prove a given goal, recurse into
	// that candidate. This means that for `impl<F: FnOnce(u32)> Trait<F> for () {}`
	// and a `(): Trait<?0>` goal we recurse into the impl and look at
	// the nested `?0: FnOnce(u32)` goal.
	if let Some(candidate) = inspect_goal.unique_applicable_candidate() {
	candidate.visit_nested_no_probe(self)

Since we are using a ProofTreeVisitor with a depth limit for this and this PR handles similar amount of horizontal goals scale with the lesser depth, the visitor has to visit more goals than before. I think this is the natural outcome and inevitable side effect. And at the beginning, the regression was somewhat like > 30% but thanks to #156187, I think the numbers have become bearable.

For more details, see #t-types/trait-system-refactor > Are there any minimization of `wg-grammar`? @ 💬

@rustbot label: +perf-regression-triaged