Cleanup and fix cycle handling #285 #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
283: Publish 0.17.0-pre.2 r=jonas-schievink a=jonas-schievink User-facing changes since 0.17.0-pre.1: - salsa-rs#267 - salsa-rs#276 - salsa-rs#280 Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
If I am not in an error, all current tests uses "static" cycles -- a cycle always present. Let's spice that up by adding conditional cycles: cycles that appear only for specific impls
This allows us to figure out whether a query can recover from a cycle (and how) without invoking the `recover` function.
Find the cycle recovery strategy for a given DatabaseKey.
They now use signals to guarantee we are testing the code paths we want to be testing.
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.
Before we could not observe the case where: * thread A is blocked on B * cycle detected in thread B * some participants are on thread A and have to be marked (In particular, I commented out some code that seemed necessary and didn't see any tests fail)
Instead of sending the result back, just have the waiting threads retry reading the cache.
Being generic over the keys made code harder to read.
Make `add_edge` infallible
This will allow me to add condvar logic to it
We are going to make it so that the runtime coordinates delivery of the WaitResults.
Thi sis an intermediate step towards having the runtime coordinate wakeups.
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.
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.
Do not wrap in Cancelled.
Created in draw.io, should be editable from there.
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).
`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!).
This is "wip" because it does not yet handle cross-thread recovery correctly. That is coming in a later commit.
I was finding the parallel test setup hard to read, so everything relating to one test is now in a single file, with shorter names.
The previous version had a lot of spurious wakeups, but it also resisted the refactoring I'm about to do. =)
Including the corner case where the active thread does not have recovery.
When a query Q invokes a cycle Q1...Q1 but Q is not a participant in that cycle, Q should not recover! Test that.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.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>
|
(going through the changes our fork is missing rn) This seems to not contain the perf regression we observed on other changes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.