Canonicalizer cleanups #150748

nnethercote · 2026-01-07T06:25:54Z

Some cleanups in and around the canonicalizers, found while I was looking closely at this code.

rustbot · 2026-01-07T06:25:56Z

Some changes occurred to the core trait solver

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

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

lcnr · 2026-01-07T11:54:48Z

@bors try @rust-timer queue

r=me after perf, I assume that u measured it to not have an impact, but want to be sure

Canonicalizer cleanups

rust-bors · 2026-01-07T14:19:30Z

☀️ Try build successful (CI)
Build commit: 23ed3e9 (23ed3e95ddb50d9de56e5fba78af875e824ef8d7, parent: d9617c8d9a55773a96b61ba3a4acb107d65615c1)

rust-timer · 2026-01-07T15:00:33Z

Finished benchmarking commit (23ed3e9): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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
Regressions ❌ (secondary)	0.2%	[0.2%, 0.3%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.5%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -0.7%)

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

Cycles

Results (primary 2.7%, secondary -0.2%)

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

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.3%, 3.3%]	3
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	2.7%	[2.3%, 3.3%]	3

Binary size

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

Bootstrap: 474.542s -> 471.79s (-0.58%)
Artifact size: 390.84 MiB -> 390.85 MiB (0.00%)

It's an empty `Vec` at both call sites, and so is unnecessary.

Variables that are collections of `CanonicalVarKind` are sometimes called `var_kinds` and sometimes called `variables`. The former is much better, because `variables` is (a) non-descript, and (b) often used nearby for collections of `I::GenericArg`. I found the inconsistency made the canonicalization code harder to understand. This commit renames various `variables` things as `var_kinds`.

Currently it's a mutable reference, but it doesn't need to be, because what's passed in is always a mutable reference to an empty `Vec`. This requires returning variables in a few extra places, which is fine. It makes the handling of `variables` the same as the handling of `var_kinds` and `variable_lookup_table`.

It's weird. `clone` is better.

I did some measurements. The current choice of 16 is fine.

This explains why the predicate folding code looks different to the ty/const folding code, something I was wondering.

nnethercote · 2026-01-08T04:04:55Z

The slight regressions on the new-solver benchmarks are due to the "Make Canonicalizer::variables owned" commit. Let's try a possible fix:

@bors try @rust-timer queue

Canonicalizer cleanups

rust-bors · 2026-01-08T06:29:15Z

☀️ Try build successful (CI)
Build commit: 0a07a16 (0a07a161046bb0312b7c8ec7d9794b1fc0628e02, parent: 548e586795f6b6fe089d8329aa5edbf0f5202646)

rust-timer · 2026-01-08T07:10:16Z

Finished benchmarking commit (0a07a16): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.5%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.1%)

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

Cycles

Results (primary 2.4%, secondary -4.9%)

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

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

Binary size

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

Bootstrap: 474.664s -> 473.643s (-0.22%)
Artifact size: 390.87 MiB -> 390.87 MiB (-0.00%)

nnethercote · 2026-01-08T07:27:06Z

The final commit fixes the problem. I don't understand why, even though I spent a chunk of today investigating it. I can't work out why cloning a Vec would cause more allocations than extending an empty Vec.

lcnr · 2026-01-08T11:10:48Z

@bors r+ rollup=never

rust-bors · 2026-01-08T11:10:53Z

📌 Commit b222cf3 has been approved by lcnr

It is now in the queue for this repository.

rustbot assigned lcnr Jan 7, 2026

nnethercote force-pushed the canonicalizer-cleanups branch from 2c8e55e to a24aef0 Compare January 7, 2026 06:27

This comment has been minimized.

Sign in to view

nnethercote force-pushed the canonicalizer-cleanups branch from a24aef0 to d0278f9 Compare January 7, 2026 07:19

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jan 7, 2026

Auto merge of #150748 - nnethercote:canonicalizer-cleanups, r=<try>

23ed3e9

Canonicalizer cleanups

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 7, 2026

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 7, 2026

nnethercote added 9 commits January 8, 2026 13:37

Remove some unnecessary braces in patterns.

a4da8e8

Factor out duplicated code in Canonicalizer::finalize.

5ca112f

Remove variables arg from Canonicalizer::canonicalize_response.

0ad7701

It's an empty `Vec` at both call sites, and so is unnecessary.

Avoid using to_vec to clone Vecs.

e4bbfe8

It's weird. `clone` is better.

Update a comment.

eb16456

I did some measurements. The current choice of 16 is fine.

Add comments about predicate folding.

a07f717

This explains why the predicate folding code looks different to the ty/const folding code, something I was wondering.

Tweak variable cloning.

b222cf3

nnethercote force-pushed the canonicalizer-cleanups branch from d0278f9 to b222cf3 Compare January 8, 2026 04:04

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jan 8, 2026

Auto merge of #150748 - nnethercote:canonicalizer-cleanups, r=<try>

0a07a16

Canonicalizer cleanups

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 8, 2026

This comment has been minimized.

Sign in to view

rustbot removed perf-regression Performance regression. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 8, 2026

rust-bors bot 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 Jan 8, 2026

Uh oh!

Canonicalizer cleanups #150748

Are you sure you want to change the base?

Canonicalizer cleanups #150748

Uh oh!

Conversation

nnethercote commented Jan 7, 2026

Uh oh!

rustbot commented Jan 7, 2026

Uh oh!

This comment has been minimized.

lcnr commented Jan 7, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Jan 7, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jan 7, 2026

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

nnethercote commented Jan 8, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Jan 8, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jan 8, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

nnethercote commented Jan 8, 2026

Uh oh!

lcnr commented Jan 8, 2026

Uh oh!

rust-bors bot commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants