fixed-point iteration support

[carljm]

This PR removes the existing unwind-based cycle fallback support (a plus for WASM compatibility), and replaces it with support for fixpoint iteration of cycles.

To opt in to fixpoint iteration, provide two additional arguments to salsa::tracked on the definition of a tracked function: cycle_initial and cycle_fn. The former is a function which should provide a provisional starting value for fixpoint iteration on this query, and the latter is a function which has the opportunity, after each iteration that failed to converge, to decide whether to continue iterating or fallback to some fixed value. See the added tests and docs for details.

Usability points that should be covered in the documentation:

• With the old cycle fallback, it was sufficient to avoid panic for at least one query in a cycle to define a cycle fallback. With fixpoint iteration, to avoid cycle panics you must define cycle_fn and cycle_initial on every query that might end up as the "head" of a cycle (that is, queried for its value while it is already executing.)
• It is entirely possible to define cycle_fn and cycle_initial so as to cause iteration to diverge and never terminate; it's up to the user to avoid this. Techniques to avoid this include a) ensuring that cycles will converge, by defining the initial value and the queries themselves monotonically (for example, in a type-inference scenario, the initial value is the bottom, or empty, type, and types will only widen, never narrow, as the cycle iterates -- thus the cycle must eventually converge to the top type, if nowhere else), and/or b) with a larger hammer, by ensuring that cycle_fn respects the iteration count it is given, and always halts iteration with a fallback value if the count reaches some "too large" value.
• It's also entirely possible to define cycle_fn and cycle_initial such that memoized results can vary depending only on the order in which queries occur. Avoid this by minimizing the number of tracked functions that support fixpoint iteration and ensuring initial values and fallback values are consistent among tracked functions that may occur in a cycle together.
• You can call Salsa queries from within your cycle_fn and cycle_initial queries, but if the query you call re-enters the same cycle, it could lead to unexpected behavior. Take care what queries you call inside cycle recovery functions.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	f421b94
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/67cf5e620ac0b50008ca5b5b

[codspeed-hq]

CodSpeed Performance Report

Merging #603 will degrade performances by 6.58%

_{Comparing carljm:fixpoint (f421b94) with master (9ebc8a3)}

Summary

❌ 6 regressions
✅ 5 untouched benchmarks
🆕 1 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	accumulator	3.2 ms	3.4 ms	-6.58%
❌	amortized[Input]	3.4 µs	3.6 µs	-5.98%
❌	amortized[InternedInput]	3.4 µs	3.5 µs	-4.43%
❌	amortized[SupertypeInput]	4.2 µs	4.3 µs	-3.76%
❌	new[InternedInput]	5.1 µs	5.3 µs	-4.07%
🆕	converge_diverge	N/A	169.7 µs	N/A
❌	many_tracked_structs	36.4 µs	38 µs	-3.96%

[nikomatsakis]

This is very cool! (Admittedly, I say this pre-review.)

[MichaReiser]

This looks great. I left a few comments where I struggled understanding the implementation or had smaller suggestions.

[carljm]

In writing more comprehensive tests for this, I realized that it needs some changes to correctly handle multi-revision scenarios; taking it to Draft mode until I get that fixed.

[carljm]

Ok, multiple-revision cases are now fixed, and we now populate the initial provisional value only lazily, in case a cycle is actually encountered, which should reduce the number of memos created by quite a lot.

Also added a bunch of tests, including multiple-revision cases and one test involving durability. Still need to add cross-thread cycle tests.

[MichaReiser]

The lazy creation of the initial value is a neat improvement. Nice for taking the time to work on it !

[MichaReiser]

The benchmarks show a 4-5% regression. It seems that we're now resizing some hash maps more often. Are we reporting more tracked reads than before? Could you take a look what's causing it?

[carljm]

Initial experiments using this in the red-knot type checker are promising: astral-sh/ruff#14029

Not yet using it for loopy control flow in that PR, but there are cycles in the core type definitions of Python builtins and standard library, which we previously had a hacky fallback in place for using Salsa's previous cycle fallback support. Moving over to fixpoint iteration just worked, and fixed the type of a builtin impacted by the cycle.

On the downside, it is a performance regression. Need to do more work there.

[Veykril]

This change (bool being replaced by VerifyResult) also seems like it will make non cycle queries pay a hefty perf price. That type has a destructor and is 40 bytes where as the previous return value could fit into a register, this can't.

[carljm]

I'll focus first on getting consistently right behavior (including across threads) and then revisit this and other possible ways to claw back perf. Off the top of my head I'm not sure how to improve this, will need to think about it.

[Veykril]

Yes of course, I am mainly noting down what I saw, no need to tackle perf nits/reviews until things are working properly. (also happy to help out with perf work on this PR once its at that stage)

[carljm]

With the option boxing we've now significantly reduced the size of VerifyResult, at least.

[Veykril]

On a related perf note, would it make sense to move the dataflow test (or a maybe slightly more involved version) into a benchmark?

[nikomatsakis]

@carljm -- to confirm, not yet ready for my review, right? (or did I forget something)

[carljm]

@nikomatsakis I commented last week at https://salsa.zulipchat.com/#narrow/channel/333573-Contributing-to-Salsa/topic/fixed.20point.20cycles that this was ready for review.

Two things have come up since:

1. A test failure here in CI (on the cycle_t1_a_t2_b test) that I have never seen locally and so far have not been able to reproduce; it suggests some kind of race condition that I need to track down and fix.
2. The PR that landed on Friday to fix accumulators (Fix accumulated values for backdated queries #662) conflicts badly with this PR, and I haven't completed that rebase yet (thus all the conflicts shown here.)

I think a review from you of what's here could still be useful (you might have an idea where the race condition is that is causing the test failure?) but it's also fine if you want to wait until I sort the above items.

[carljm]

I may try and see if https://github.com/awslabs/shuttle can track down the race condition. It's a bit invasive to use it, since it requires that all our uses from std::thread and std::sync throughout all of Salsa go through a shim module that can cfg redirect them to shuttle in tests.

[nikomatsakis]

Ok. I will try to find time to review just in case. Im busy this week with Things but will do my best.

[carljm]

Ok, problem (2) is addressed; I've merged in latest main branch and resolved all conflicts. Now back to problem (1).

[nikomatsakis]

This looks pretty good. I think we gotta get it landed and start testing it.

[carljm]

Oops, looks like we're hitting some GitHub CI job limits.

[davidbarsky]

No objections to the design or approach of this, but I was wondering if we could get rust-lang/rust-analyzer#18964 landed first. For the corresponding PR to land in rust-analyzer, we'll need an Salsa 1.0 alpha-1 release out with—hopefully—the ability dump the contents of tracked functions (as described in here). Those two together should should take roughly a a week or so of work, at which point salsa 1.0 alpha-2 could contain this change?

[carljm]

I'm not in a big rush to merge this on my side, though the merge conflicts do get exhausting to deal with. I'll add some more tests and some documentation, and work on using it in red-knot in the mean time. We may learn some new things from that.

[davidbarsky]

I'm not in a big rush to merge this on my side, though the merge conflicts do get exhausting to deal with. I'll add some more tests and some documentation, and work on using it in red-knot in the mean time. We may learn some new things from that.

Yeah, I'm familiar with the rebase pains. Hopefully you won't have too many conflicts with the remaining, in-flight changes, but in the event you do, I want to set a tolerably short upper bound for my "don't land!" request.

[carljm]

The current code in this branch requires Rust 1.83, for Default implementations of hashset iterators. Are we OK with this, or do we need to stay compatible with older Rust versions? If so, how old?

Cargo.toml still says rust-version = 1.76.

Main branch doesn't compile with 1.76, it only compiles with 1.80 or newer.

The tests only actually pass with 1.84 or newer, because we have snapshotted compiler output in the compile_fail tests.

So we clearly don't have any CI set up to enforce our declared MSRV.

I put up #712 to fix this.

[carljm]

Ok, I added the necessary boilerplate here to maintain compatibility with Rust 1.80 for now.

[Veykril]

Stumbled upon this issue #193, might be good to check whether that is still relevant after this PR rewriting most of the cycle handling

[Veykril]

Another related issue #9

[carljm]

I added some more tests (for tracked-struct and accumulator output from queries in cycles, panics in cycle handling in a cross-thread cycle), and fixed a bug in diff_outputs for outputs in cycles.

I also updated the book sections on cycle handling. I'm afraid I didn't reproduce the detail of the previous "plumbing" section on the old cycle handling, but I did add a doc comment about the new cycle handling in cycle.rs.

I now consider this branch "done" and ready for merge, pending any further review comments (cc @nikomatsakis, @MichaReiser, @Veykril). I'll wait to merge it until @davidbarsky and @Veykril give the green light from the rust-analyzer side.

[davidbarsky]

Feel free to land this! We—rust-analyzer—got the changes we needed in https://github.com/salsa-rs/salsa/releases/tag/salsa-v0.19.0 and are happily giving up the "land stuff" lock :)

[carljm]

Squashed to a single commit in preparation for landing; the commit history here has a lot of junk and false starts, as well as many merges from main branch.