290: Update doc in macro about query.in_db for dyn db r=nikomatsakis a=mheiber
Update the macro for `query_group` so the comment
on `fn in_db` no longer says that it is more common
to use the trait method on `db`.
Afaict, the trait methods referred to were removed
when dyn database were introduced in RFC0006:
./book/src/rfcs/RFC0006-Dynamic-Databases.md, as
described in the section
"Instead of `db.query(Q)`, you write `Q.in_db(&db)`"
Co-authored-by: Maxwell Elliot Heiber <mheiber@fb.com>
288: Make with_incremented_revision take FnOnce r=nikomatsakis a=mheiber
Removes a bug vector, since this function would
panic if the closure is used more than once.
iuc, the code opted out of the compiler's checks
before via a clever `.take()`.
Another change is to take the closure by value
rather than by reference. The monomorphization
seems harmless, since `with_incremented_revision`
is only called from two places.
289: Remove ': salsa::Database' bound from two examples r=nikomatsakis a=mheiber
Two examples had a superfluous bound
': salsa::Database' that wasn't present
in the `compiler` example and doesn't seem to be needed.
The `query_group` macro adds this bound
automatically.
This change can lead to a trailing `+` in
the bounds list. I verified this is OK by
running the examples and verifying that the production
is allowed
[per the Rust Reference](https://doc.rust-lang.org/reference/trait-bounds.html)
Co-authored-by: Maxwell Elliot Heiber <mheiber@fb.com>
287: Remove unused crossbeam_utils dependency r=nikomatsakis a=DSPOM2
crossbeam_utils is currently not used anywhere withing this crate so I propose to remove the dependency to (slightly) reduce compile times
Co-authored-by: DSPOM2 <61850714+DSPOM2@users.noreply.github.com>
285: Cleanup and fix cycle handling r=nikomatsakis a=nikomatsakis
This PR is...kind of long. It reshapes the core logic of salsa to fix various bugs in cycle handling and generally simplify how we handle cross-thread coordination.
Best read commit by commit: every commit passes all tests, afaik.
The core bug I was taking aim at was the fact that, when you invoke `maybe_changed_since`, you can sometimes wind up detecting a cycle without having pushed some of the relevant queries onto the stack. This is now fixed.
From a user's POV, ~~nothing changes from this PR~~, there are only minimal changes to the public interface. The biggest one is that recover functions now get a `&salsa::Cycle` which has methods for accessing the participants; the other is that queries that are participating in cycle fallback will use unwinding to avoid executing past the point where the cycle is discovered. Otherwise, things work the same as before:
* If you encounter a cycle and all participant queries are marked with `#[salsa::recover]`, then they will take on the recovery value. (At the moment, they continue executing after the cycle is observed, but their final result is ignored; I plan to change this in a follow-up PR, or maybe some future commit to this PR.)
* If you encounter a cycle and some or all participants are NOT marked with `#[salsa::recover]`, then the code panics. This is treated like any other panic, cancelling all other work.
Along the way, I made... a few... other changes:
* Cross-thread handling is simplified. When we block on another thread, it no longer sends us a final result. Instead, it just gets re-awoken and then it retries the original request. This is helpful when you encounter cycles in `maybe_changed_since` vs `read`, but it's also more compatible with some of the directions I have in mind.
* Cycle detection is simplified and more centrally coordinated. Previously, when a cycle was detected, we would mark all the participants on the current thread, but then we would mark other threads 'lazilly'. Now, threads move ownership of their stack into the shared dep graph when they block, so that we can mark all the stack frames at once. This also means less cloning on blocking, so it should be mildly more efficient.
* The code is DRY-er, since `maybe_changed_since` has been re-implemented in terms of the same core building blocks as `read` (`probe` and friends). I originally tried to unify them, but I realized that they behave somewhat differently from one another and both of them make sense. (In particular, we want to be able to free values with the LRU cache while still checking if they are up to date.)
Ah, I realize now that I had planned to write a bunch of docs in the salsa book before I landed this. Well, I'm going to open the PR anyway, as I've let this branch go far too long.
r? `@matklad`
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: Niko Matsakis <niko@alum.mit.edu>
Update the macro for `query_group` so the comment
on `fn in_db` no longer says that it is more common
to use the trait method on `db`.
Afaict, the trait methods referred to were removed
when dyn database were introduced in RFC0006:
./book/src/rfcs/RFC0006-Dynamic-Databases.md, as
described in the section
"Instead of `db.query(Q)`, you write `Q.in_db(&db)`"
Two examples had a superfluous bound
': salsa::Database' that wasn't present
in the `compiler` example.
The `query_group` macro adds this bound
automatically.
This change can lead to a trailing `+` in
the bounds list. I verified this is OK by
running the examples and verifying that the production
is allowed
[per the Rust Reference](https://doc.rust-lang.org/reference/trait-bounds.html)
The function is morally an FnOnce, since if
called multiple times the function will panic.
iuc, the code opted out of the compiler's checks
before via a clever `.take()`.
Another change is to take the closure by value
rather than by reference. The monomorphization
seems harmless, since `with_incremented_revision`
is only called from two places.
`PanicGuard` used to own the memo so that, in the case of panic,
we could reinstall the old value -- but there's no reason for us to
do that. It's just as good to clear the slot in that case and recompute
it later. Also, it makes the code nicer to remove it, since
it allows us to have more precision about where we know the memo is
not null.
My motivation though is to work towards "partial cycle recovery".
We need a clean and easy way to cancel the ongoing execution and reset
the slot to "not computed" (turns out we used that in
`maybe_changed_since` too!).
We still record the same dependencies (or else the tests fail,
so +1 for test coverage).
This has the immediate advantage that we don't invoke the fallback
function twice for the repeated node in the cycle.
Also, fix a bug where revalidating cycles could lead to a
CycleParticipant error that is not caught (added a test for it).
We used to store a changed-at/durability that reflected only
the current frame in a cycle -- but really we are dependent
across the entire cycle, so we now store the max changed-at and
min durability from the entire thing.
It turns out this is necessary, as this test reveals!
If we don't panic, you might encounter further cycles
that aren't supposed to have executed. (Prior to these changes,
this test was panicking from the second cycle.)
`QueryRevisions` is a shared struct that contains
"everything a query read thus far" -- it is roughly
the old `MemoRevisions` but without `verified_at`.
This change is useful because when a cycle occurs it
allows us to isolate out "inputs thus far" more easily.
The `Cancelled` struct now reflects multiple potential reasons
for a query execution to be cancelled, including unexpected cycles.
It also gives information about the participants that lacked
recovery information.
