Avoid redundant next-solver fulfillment scans

[amirHdev]

Refs #159933

On the reproducer from that

N	base next	patched next
2,000	0.981s	0.379s
4,000	3.762s	1.334s
8,000	14.581s	5.130s

[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