Avoid redundant next-solver fulfillment scans

[amirHdev]

View all comments

Refs #159933

the slowdown comes from repeatedly walking the pending fulfillment queue even when nothing has changed that could make the stalled goals progress and to avoid that the inference context tracks changes that may unblock stalled goals

fulfillment remembers the generation after reaching a fixpoint and can skip another queue scan when the generation is unchanged and all pending goals are safe to track this way
relevant changes to type, const, int and float inference variables are tracked including type sub-unification
repeated changes are coalesced until fulfillment starts another pass and we do not keep updating the generation unnecessarily

the generation is also part of snapshot state and rolling back a snapshot restores it together with the inference state without adding an undo-log entry for every change

On the reproducer from issue against current status:

N	main	patched	runtime reduction
2,000	0.867s	0.307s	64.6%
4,000	3.269s	1.038s	68.2%
8,000	13.212s	4.169s	68.4%

[amirHdev]

@nnethercote I avoided some of the redundant fulfillment checks where there was no change in the inference state
in the reproducer for N=8000 it goes down from 14.58s to 5.13s in the next solver while old solver is almost same.
would like to know your opinion about the direction

[nnethercote]

I'm not expert enough with the trait solver to evaluate this. (@lcnr or @jdonszelmann might have opinions.) It does look like a fairly complex change without much explanation. Did you write the code yourself?

[amirHdev]

I'm not expert enough with the trait solver to evaluate this. (@lcnr or @jdonszelmann might have opinions.) It does look like a fairly complex change without much explanation. Did you write the code yourself?

I used the stalled goal fast path from #158249 and the try_evaluate_obligations work in #160479
I looked at what was still happening in the #159933 reproducer after those changes and tried to avoid walking the whole pending queue when the relevant inference state hadn't changed
you're right that the PR needs more explanation I wanted to first verify that the overall direction is sound before adding the reasoning here

[lcnr]

this seems vaguely appropriate, the core idea "fast path if literally no infer var changed since last run" makes a lot of sense to me. The way it's written right now feels very brittle and I would like to encapsulate this somehow in a way that makes it harder to accidentally forget to check/update something.

I considered separately having a shared list of what every goal in the fulfillment context is stalled on, so we dont need to iterate over it if literally nothing in it changed even if unrelated infer vars changed.

Why do you not reset this counter when rolling back a snapshot.

There's a lot of open design space here, think this is definitely a good direction :>

[lcnr]

One option is to store the "state after last fulfillment loop" in the InferCtxt and mem::swap it out when entering a probe. I would be very surprised if we ever have 2 fulfillment contexts in the same probe.

That way you could also track "highest mentioned infer var indices" and "lowest constrained infer var" to ignore any constraints for infer vars which the fulfillment context is not stalled on. Idk if that matters :>

[amirHdev]

@lcnr Thanks for your review
the counters not being rolled back was intentional I'm using them as monotonic invalidation generations. it is not part of the inference state
If a tracked inference change happens inside a snapshot and that snapshot is rolled back the inference tables are restored but the generation remains bumped which prevents the next fulfillment call from taking the generation fast path and can cause an extra pass then it records the newer generation again

I agree about the brittleness though
the current version relies on the relevant mutation paths participating in this tracking

[lcnr]

the counters not being rolled back was intentional I'm using them as monotonic invalidation generations. it is not part of the inference state

but why? rolling them back should be completely correct, should it not?

[amirHdev]

but why? rolling them back should be completely correct, should it not?

I am still unsure about this part but I think you are right here
I kept them that way because a rolled back change would only cause a conservative invalidation and additional fulfillment pass but since the inference state itself is restored, restoring the corresponding generation as well should be correct and would keep the generation consistent with that state 🤷‍♂️
If I keep this approach I’ll make the revisions snapshot-aware too

[rustbot]

Some changes occurred to the core trait solver

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

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, types
• compiler, types expanded to 75 candidates
• Random selection from 17 candidates

[amirHdev]

@rustbot ready
r? lcnr

[lcnr]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5d793fd (5d793fd50aa57aff3583d8fdf12550df672fe8bd)
Base parent: f7d782a (f7d782a3be46d6bb4b9792fe69a61db389ba1769)

[rust-timer]

Finished benchmarking commit (5d793fd): 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 rustc-perf
@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.2%	[0.1%, 0.4%]	25
Regressions ❌ (secondary)	0.7%	[0.1%, 3.7%]	48
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	2
All ❌✅ (primary)	0.2%	[0.1%, 0.4%]	25

Max RSS (memory usage)

Results (primary -1.5%, secondary -0.5%)

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)	2.2%	[2.1%, 2.3%]	2
Improvements ✅ (primary)	-1.5%	[-2.0%, -1.0%]	2
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
All ❌✅ (primary)	-1.5%	[-2.0%, -1.0%]	2

Cycles

Results (secondary 3.0%)

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)	3.0%	[2.4%, 3.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	2

Bootstrap: 456.256s -> 463.808s (1.66%)
Artifact size: 398.95 MiB -> 399.07 MiB (0.03%)

[rust-bors]

☀️ Try build successful (CI)
Build commit: e44f7f4 (e44f7f4f4ba84be73cd0bfd323ee9f156ad0f104)
Base parent: 420ed2a (420ed2a0c3d7225b1744266fd884d431b4d8cfe0)

[rust-timer]

Finished benchmarking commit (e44f7f4): 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 rustc-perf
@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.2%	[0.2%, 0.2%]	2
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	4
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	2

Max RSS (memory usage)

Results (primary -0.1%)

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

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.3%, -1.7%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-2.3%, 3.8%]	3

Cycles

Results (primary 2.1%, secondary 2.4%)

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

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	4.2%	[3.5%, 4.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

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

Bootstrap: 498.333s -> 497.609s (-0.15%)
Artifact size: 408.93 MiB -> 409.71 MiB (0.19%)

[mejrs]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3857bb3 (3857bb33ae8edf99d68bcd9183a031f14d5cc71b)
Base parent: 36b04f3 (36b04f3ad12a98b1a3cf9efe5313306ef7d5848d)

[rust-timer]

Finished benchmarking commit (3857bb3): 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 rustc-perf
@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.2%	[0.1%, 0.2%]	2
Regressions ❌ (secondary)	0.3%	[0.1%, 0.5%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	10
All ❌✅ (primary)	0.2%	[0.1%, 0.2%]	2

Max RSS (memory usage)

Results (primary -2.3%, secondary -5.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.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-5.1%	[-5.1%, -5.1%]	1
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

Results (primary 1.2%)

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

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

Binary size

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

Bootstrap: 497.193s -> 496.559s (-0.13%)
Artifact size: 408.84 MiB -> 408.87 MiB (0.01%)

[amirHdev]

for the #159933 style case the speedup stayed pretty consistent as the case got larger

I got these results locally
N=2000: ~0.87s → ~0.31s
N=4000: ~3.27s → ~1.04s
N=8000: ~13.2s → ~4.17s

on wg-grammar-new-solver there were about 172k bump attempts but only 27k actual updates and most of the bumps happened while the generation was already dirty

I reran many-assoc-items because one of my earlier local runs made it look ~3% worse 😬 that didn't reproduce with a longer interleaved run and ended up much closer to the rustc perf numbers here

I don't think there's a larger regression hiding here

[lcnr]

when is this the case

View changes since the review

[amirHdev]

this happens when at least one pending goal cannot be tracked through GoalStalledOn

we cannot know that the whole fulfillment context is safely stalled
in that case we avoid taking the generation fast path

[lcnr]

hmm, could we make this a even simpler by having a single all_goals_known_to_be_stalled: bool flag?

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[amirHdev]

hmm, could we make this a even simpler by having a single all_goals_known_to_be_stalled: bool flag?

I considered this but kept the generation-based invalidation approach

the generation tracks whether the recorded stalled state may be stale while the other fields track the conditions needed to safely use the fast path