Avoid redundant next-solver fulfillment scans

[amirHdev]

Refs #159933

The regression comes from repeatedly traversing the pending fulfillment queue even when the inference state the pending goals are stalled on has not changed. it introduces a fast path which records the relevant inference revisions and aggregate stalled state. a subsequent fulfillment invocation can skip traversing the pending queue when that state is unchanged and all pending goals can be tracked, otherwise we fall back to the regular fulfillment loop
the type-variable revisions are snapshot-aware. revision changes use the existing inference undo log and rolling back a probe restores the revisions together with the corresponding inference state

On the reproducer from the issue:

N	base next	patched next
2,000	0.981s	0.38s
4,000	3.762s	1.33s
8,000	14.581s	5.01s

[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%)

[amirHdev]

@lcnr do you think the InferCtxt state + mem::swap approach would be cleaner than keeping revisions in TypeVariableStorage ? 🤔