Add a third cycle mode, equivalent to old Salsa cycle behavior

[ChayimFriedman2]

That is, directly set a value for all queries that have fallbacks, and ignore all other queries in the cycle.

Unlike old Salsa, we still need all cycle heads to be marked, and we still execute the queries to completion, but we throw their result.

This is an alternative to @Veykril's #798 PR, that doesn't suffer from non-determinism.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	f61f973
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/68068b049cc5a70008ae20b9

[codspeed-hq]

CodSpeed Performance Report

Merging #801 will degrade performances by 5.79%

_{Comparing ChayimFriedman2:cycle-no-fixpoint (f61f973) with master (3b84ac4)}

Summary

❌ 1 (👁 1) regressions
✅ 11 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
👁	amortized[SupertypeInput]	3.8 µs	4 µs	-5.79%

[ChayimFriedman2]

I just realized there is still some non-determinism: if we enter a cycle and cache the result, then we call a query from the cycle that calls the cached result (that is, A -> B -> A then B -> A), it may be different than if we started if the other query (B -> A -> B). But this was also present in old Salsa and we didn't have problems from it, so 🤷

[ChayimFriedman2]

The failing test was a demonstration of this non-determinism... I deleted it as it is fundamentally not compatible with this model.

[carljm]

I don't think this is equivalent to the old Salsa cycle handling, and I don't think the old Salsa cycle handling was subject to the same non-determinism that this still is.

The old cycle handling used catch-unwind to unwind and collect all queries participating in the cycle, without executing any of them, and then set all of them (not just the cycle head) to the same single fallback result.

I think if we are OK with non-determinism when cycle_result is used, we should prefer the simpler approach in @Veykril's PR.

[carljm]

I don't see how it is possible to recreate equivalent behavior to the old Salsa cycle handling without reintroducing the use of catch-unwind. Catch-unwind is incompatible with WASM (and we need WASM in red-knot), but I think it should be fine for us as long as it's only opt-in with the use of cycle_result, which we just wouldn't use. It will reintroduce a lot of complex code that I was able to remove in #603.

I think catch-unwind is necessary because otherwise you can't cleanly propagate a form of "cancellation" of all the active queries in the cycle back up through the stack to the cycle head. (At least not without requiring cooperation / a new calling convention for all user-authored query code, which I don't think is an option.)

[Veykril]

Yes as Carl pointed out this is still not equivalent unfortunately, and we can't really bring back the old style fully without bringing back the cycle handling logic that was remoived with the fixpoint PR.

[ChayimFriedman2]

I believe we can do the same, but what did old Salsa do when there was no fallback for query participating in the cycle?

[ChayimFriedman2]

To explain: we can't cancel cycle participants, this is true. But we can run them to completion then ignore the result. For rust-analyzer at least this should be enough.

[ChayimFriedman2]

@carljm @Veykril I modified the PR to do exactly what old Salsa did - use fallback for every query in the cycle that has them, and ignore the rest (the difference from old Salsa is that we require the cycle head to have fallback, and that we still execute the queries to completion (but throw the result)), and I believe it is fully deterministic now. I also brought back the test.

[carljm]

This approach looks reasonable to me.

[ChayimFriedman2]

@carljm Addressed comments.

[ChayimFriedman2]

The failing benchmark is very noisy currently for some reason.

[carljm]

Looks good! I'm comfortable with this. Wouldn't mind a review from @Veykril or @MichaReiser as well, but fine with merge and post-land review as well.

[Veykril]

Acknowledge the perf regression. The supertype bench has been the noisiest of the bunch.

[MichaReiser]

Simply dropping the query result after running it to completion can result in ever growing memory usage. The fallback query could have created tracked structs that now no longer have an owning query, and thus, never get removed.