Avoid redundant next-solver fulfillment scans

[amirHdev]

View all comments

Refs #159933

The slowdown here comes from repeatedly walking the pending fulfillment queue even when nothing has changed that could make the stalled goals progress which this adds a generation to the inference context which is bumped whenever we constrain type, const, int and float inference variables or do type sub unification
fulfillment remembers the generation after reaching a fixpoint and can skip walking the queue again if the generation is unchanged and the pending goals are safe to track this way

The generation is part of the snapshot state and rolling back a snapshot restores it with the corresponding inference state which now we avoid putting every generation bump into the undo log

On the reproducer from issue against current status:

N	main	patched	speedup
2,000	0.867s	0.307s	2.82x
4,000	3.269s	1.038s	3.15x
8,000	13.212s	4.169s	3.17x

[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 ? 🤔

[lcnr]

you don't need to store this in the undo log 🤔 you can instead store the revision in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/snapshot/undo_log/struct.Snapshot.html and update the revision in the infcx when rolling it back.

View changes since the review

[lcnr]

that feels off, why not also track const vars? or well, probably easiest to increment the counter when doing any of

• sub unify
• constrain const / type infer var

View changes since the review

[amirHdev]

I think TyInt and TyFloat should bump it too alongside Ty/Const. they're part of TyOrConstInferVar and resolving any of them can make a stalled goal runnable again
keeping them out would leave the context-wide fast path with a second correctness condition while including them gives us one clear invalidation invariant
That seems like the cleaner model to me
What do you think?

[lcnr]

jup

[lcnr]

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

maybe? 🤷 seems worth to try to make it as "nice" as possible and see what works best. Or maybe experiment with both in case one of them is faster

[rustbot]

changes to the core type system

cc @lcnr

[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]

@lcnr I ended up using one coarse generation for type constraints and unification. now it stored in the snapshot now and restored on rollback and there’s no undo-log entry for every bump
It’s harder for the tracking to get out of sync now that we moved the non-type mutation paths behind helpers

this way was a bit faster locally and feels much simpler
wondering what you think of this format now