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>
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.
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.
Because it bugs me to clone the vector.
Maybe silly, I admit, since cycle recovery
is not the hot path.
But by that same token, we now spend only 1 word
for a null pointer instead of 4 words for a (usually empty) vector.
Currently, when one thread blocks on another, we clone the
stack from that task. This results in a lot of clones, but it also
means that we can't mark all the frames involved in a cycle atomically.
Instead, when we propagate information between threads, we also
propagate the participants of the cycle and so forth.
This branch *moves* the stack into the runtime while a thread
is blocked, and then moves it back out when the thread resumes.
This permits the runtime to mark all the cycle participants at once.
It also avoids cloning.
Instead of creating a future for each edge
in the graph, we now have all dependent queries
block in the runtime and wake up whenever results
are published to see if their results are ready.
We could certainly allocate a CondVar for each dependent
query if we found that spurious wakeups were a problem.
I consider this highly unlikely in practice.
Rather than checking return value of from `Q::cycle_fallback`, we
now consult the computed recovery strategy to decide whether to
panic or to recover. We can thus assume that we will successfully
recover and don't need to check for `None` results anymore.
