fixed-point iteration support #603
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #603 will not alter performanceComparing Summary
Benchmarks breakdown
|
|
This is very cool! (Admittedly, I say this pre-review.) |
|
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. |
|
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. |
|
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? |
|
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. |
* master: ci: use `release-plz` for release internal: switch to `boxcar` from `append-only-vec` Deduplicate `Storage` and `StorageHandle` fields More test cases for `tracked_fn_return_ref` Emit better diagnostic for invalid tracked function return types `black_box` benchmark inputs and outputs Introduce `RefUnwindSafe` `StorageHandle` add guide of releasing add release workflow
|
Oops, looks like we're hitting some GitHub CI job limits. |
|
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 |
|
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. |
* master: Reduce method delegation duplication Automatically clear the cancellation flag when cancellation completes Allow trigger LRU eviction without increasing the current revision Simplify `Ingredient::reset_for_new_revision` setup Require mut Zalsa access for setting the lru limit Split off revision bumping from `zalsa_mut` access
* master: Remove some `ZalsaDatabase::zalsa` calls Remove outdated FIXME Replace `IngredientCache` lock with atomic primitive
|
The current code in this branch requires Rust 1.83, for
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 So we clearly don't have any CI set up to enforce our declared MSRV. I put up #712 to fix this. |
* master: update MSRV to 1.80 and test it in CI
|
Ok, I added the necessary boilerplate here to maintain compatibility with Rust 1.80 for now. |
|
Stumbled upon this issue #193, might be good to check whether that is still relevant after this PR rewriting most of the cycle handling |
* master: Fix book deployment take 2 Disable jemalloc decay in benches Fix book deployment Cancel duplicate test workflow runs Track revisions for tracked fields only implement `Update` trait for `hashbrown::HashMap` Use `db.zalsa` created_at documentation Fix bad-hash with in-place update Tracked struct recycling and coarse grained dependencies Move `unwind_if_revision_cancelled` from `ZalsaLocal` to `Zalsa` Don't clone strings in benchmarks Skip book and benchmark CI runs for `merge_group` Tidy up Cargo.toml dependencies Enforce `unsafe_op_in_unsafe_fn` Prevent fragmention of table due to frequent `ZalsaLocal` reconstruction
|
Another related issue #9 |
* master: fix typo more correct bounds on `Send` and `Sync` implementation `DeletedEntries` replace `arc-swap` with manual `AtomicPtr` Remove unnecessary `current_revision` call from `setup_interned_struct` Add test for durability changes `#[inline(never)]` queries in benchmarks Remove some dynamically dispatched `Database::event` calls Don't load `verified_at` twice in `shallow_verify_memo` Lazy fetching Merge pull request salsa-rs#3 from Veykril/veykril/push-tynvvyqoltqx Add small supertype input benchmark Replace a `DashMap` with `RwLock` as writing is rare for it internal: address review comments Skip memo ingredient index mapping for non enum tracked functions Trade off a bit of memory for more speed in `MemoIngredientIndices` fix enums bug Introduce Salsa enums
carljm
changed the title
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::trackedon the definition of a tracked function:cycle_initialandcycle_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 test incycle_fixpoint.rsfor details.Usability points that should be covered in the documentation:
cycle_fnandcycle_initialon every query that might end up as the "head" of a cycle (that is, queried for its value while it is already executing.)cycle_fnandcycle_initialso 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 thatcycle_fnrespects the iteration count it is given, and always halts iteration with a fallback value if the count reaches some "too large" value.cycle_fnandcycle_initialsuch 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.cycle_fnandcycle_initialqueries, 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.This is an RFC pull request to get initial reviewer feedback on the design and implementation. Remaining TODO items: