-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup and fix cycle handling #285
Merged
Merged
Changes from all commits
Commits
Show all changes
86 commits
Select commit
Hold shift + click to select a range
ae8348c
Add failing test for cycle revalidation
flodiebold 0f3bc72
Add tests for "dynamic" cycles
matklad b72b251
add should-panic annotations to cycle tests that fail
nikomatsakis 40139ab
enable debug logging in the cycles tests
nikomatsakis 7b4ee6f
improve panic error message
nikomatsakis d082270
introduce CYCLE_STRATEGY constant for queries
nikomatsakis 853006f
isolate `find_cycle_participants` into its own fn
nikomatsakis e490886
introduce `cycle_recovery_strategy` function
nikomatsakis fc826b0
add cycle_recovery_strategy function on database
nikomatsakis bcffa4a
find cycle recovery strategy for a given cycle
nikomatsakis b4a0453
document what we are testing, rename variables
nikomatsakis 7b9c383
improve parallel cycle tests
nikomatsakis 42a653c
use computed recovery strategy
nikomatsakis 187bd54
move CycleError to plumbing
nikomatsakis 79f8acc
improve parallel cycle tests
nikomatsakis 0298163
move CycleError to the plumbing module
nikomatsakis eb1e06d
fix typo that's always bugging me
nikomatsakis 66b26f0
introduce `Retry` probe result
nikomatsakis 2d7a84b
silence lint
nikomatsakis da188fe
extract DependencyGraph into its own module
nikomatsakis e870d02
make the dep-graph not generic
nikomatsakis ec38398
extract the `depends_on` helper function
nikomatsakis ba61657
rename labels to query_dependents
nikomatsakis 213e16f
remove explicit Default impl
nikomatsakis f5a15e5
rework the dep-graph API to take the lock
nikomatsakis 21cb4ef
move WaitResult to runtime
nikomatsakis 3c094d2
midpoint: make runtime take wait-result
nikomatsakis e83bae7
have runtime coordinate blocking
nikomatsakis 35ddd36
move the stack instead of cloning
nikomatsakis 5bd2cbc
remove unnecessary type argument K
nikomatsakis de7ac89
put cycle participants into an arc
nikomatsakis dc5ea93
WaitResult can just be panicked|completed now
nikomatsakis 0630fb1
track caller to give better line numbers on test errors
nikomatsakis e7a8abc
distinguish stale vs absent
nikomatsakis 91ed93a
distinguish no-value from not computed at all
nikomatsakis d0c7493
break out a helper for executing queries
nikomatsakis 5adb3c9
add debug methods that carry db
nikomatsakis 293e103
make "maybe changed since" share code with read
nikomatsakis 54277d4
remove silly duration parameter
nikomatsakis 0af2a9f
add helper for pushing queries
nikomatsakis b0eaa59
make execute_query take active_query argument
nikomatsakis a53b1c5
put queries on the stack when validating
nikomatsakis cb00077
silence clippy warnings
nikomatsakis d8a792c
use `RefCell::take`
nikomatsakis 5fe4824
take take take, that's all you do
nikomatsakis f659e1d
docs: improve diagram, split gen. code and runtime
nikomatsakis 4a1dffe
cleanup: move cycle recovery into `try_block_on`
nikomatsakis 61599cc
report unexpected cycles using Cancelled
nikomatsakis ea6eedd
test cycles with mixed recovery
nikomatsakis 3caf965
limit visibility of CycleError
nikomatsakis 75ee3ed
introduce Cycle type and use in recovery, errors
nikomatsakis 891a866
refactor `Memo` to just be parameterized over `V`
nikomatsakis 1027342
introduce QueryRevisions and use everywhere
nikomatsakis 3f95c0b
panic when recovering from a cycle
nikomatsakis c096f71
fix clippy warnings
nikomatsakis 83b2f40
apply cargo fmt
nikomatsakis 006848a
matklad nikomatsakis
nikomatsakis 187de0f
rename fn: query may be in same thread
nikomatsakis 1011274
repeat test configuration
nikomatsakis 7dbacbc
rotate the participants in the cycle once
nikomatsakis 62e2fab
on fallback, get deps from all cycle participants
nikomatsakis eb307e4
rename and incorporate new test
nikomatsakis ff7f5b6
first RFC draft
nikomatsakis 33d47cc
throw Cycle value directly
nikomatsakis 072184f
FetchMaybeChanged.drawio
nikomatsakis 2f5e1d1
add diagram of derived query reads
nikomatsakis 7d66224
start expanding the book (wip!)
nikomatsakis accde0a
doc "maybe changed since" and terminology
nikomatsakis cc2f887
Cleanup edges, panics for diagram
nikomatsakis b9d748a
move content from rfc into book,d ocs
nikomatsakis 7c02d19
add cycle recovery
nikomatsakis 961599a
unwind from "block on" for panic/cancellation
nikomatsakis c14a3d4
remove `memo` from `PanicGuard`
nikomatsakis 45434cf
rework cycle to permit partial recovery (wip)
nikomatsakis bfa74bc
spread parallel tests into their own files
nikomatsakis 92b5c02
introduce a condvar per thread, instead of one
nikomatsakis 93ee78e
factor out a helper to unblock a given runtime
nikomatsakis cb658b9
enable partial recovery across threads
nikomatsakis b8f628d
improve RFC guide description
nikomatsakis 5ebd221
don't recover when not a participant
nikomatsakis db2daf0
describe implementation in RFC
nikomatsakis 7081fe4
move the RFC contents to salsa book
nikomatsakis ff71d77
avoid overloading the term "participant"
nikomatsakis 563334d
fix comments
nikomatsakis cd265bf
Update src/runtime/dependency_graph.rs
nikomatsakis fc020de
s/maybe_changed_since/maybe_changed_after/
nikomatsakis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# Cycle handling | ||
|
||
By default, when Salsa detects a cycle in the computation graph, Salsa will panic with a [`salsa::Cycle`] as the panic value. The [`salsa::Cycle`] structure that describes the cycle, which can be useful for diagnosing what went wrong. |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Recovering via fallback | ||
|
||
Panicking when a cycle occurs is ok for situations where you believe a cycle is impossible. But sometimes cycles can result from illegal user input and cannot be statically prevented. In these cases, you might prefer to gracefully recover from a cycle rather than panicking the entire query. Salsa supports that with the idea of *cycle recovery*. | ||
|
||
To use cycle recovery, you annotate potential participants in the cycle with a `#[salsa::recover(my_recover_fn)]` attribute. When a cycle occurs, if any participant P has recovery information, then no panic occurs. Instead, the execution of P is aborted and P will execute the recovery function to generate its result. Participants in the cycle that do not have recovery information continue executing as normal, using this recovery result. | ||
|
||
The recovery function has a similar signature to a query function. It is given a reference to your database along with a `salsa::Cycle` describing the cycle that occurred; it returns the result of the query. Example: | ||
|
||
```rust | ||
fn my_recover_fn( | ||
db: &dyn MyDatabase, | ||
cycle: &salsa::Cycle, | ||
) -> MyResultValue | ||
``` | ||
|
||
The `db` and `cycle` argument can be used to prepare a useful error message for your users. | ||
|
||
**Important:** Although the recovery function is given a `db` handle, you should be careful to avoid creating a cycle from within recovery or invoking queries that may be participating in the current cycle. Attempting to do so can result in inconsistent results. | ||
|
||
## Figuring out why recovery did not work | ||
|
||
If a cycle occurs and *some* of the participant queries have `#[salsa::recover]` annotations and others do not, then the query will be treated as irrecoverable and will simply panic. You can use the `Cycle::unexpected_participants` method to figure out why recovery did not succeed and add the appropriate `#[salsa::recover]` annotations. |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# Meta: about the book itself | ||
|
||
## Linking policy | ||
|
||
We try to avoid links that easily become fragile. | ||
|
||
**Do:** | ||
|
||
* Link to `docs.rs` types to document the public API, but modify the link to use `latest` as the version. | ||
* Link to modules in the source code. | ||
* Create ["named anchors"] and embed source code directly. | ||
|
||
["named anchors"]: https://rust-lang.github.io/mdBook/format/mdbook.html?highlight=ANCHOR#including-portions-of-a-file | ||
|
||
**Don't:** | ||
|
||
* Link to direct lines on github, even within a specific commit, unless you are trying to reference a historical piece of code ("how things were at the time"). |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
# Cycles | ||
|
||
## Cross-thread blocking | ||
|
||
The interface for blocking across threads now works as follows: | ||
|
||
* When one thread `T1` wishes to block on a query `Q` being executed by another thread `T2`, it invokes `Runtime::try_block_on`. This will check for cycles. Assuming no cycle is detected, it will block `T1` until `T2` has completed with `Q`. At that point, `T1` reawakens. However, we don't know the result of executing `Q`, so `T1` now has to "retry". Typically, this will result in successfully reading the cached value. | ||
* While `T1` is blocking, the runtime moves its query stack (a `Vec`) into the shared dependency graph data structure. When `T1` reawakens, it recovers ownership of its query stack before returning from `try_block_on`. | ||
|
||
## Cycle detection | ||
|
||
When a thread `T1` attempts to execute a query `Q`, it will try to load the value for `Q` from the memoization tables. If it finds an `InProgress` marker, that indicates that `Q` is currently being computed. This indicates a potential cycle. `T1` will then try to block on the query `Q`: | ||
|
||
* If `Q` is also being computed by `T1`, then there is a cycle. | ||
* Otherwise, if `Q` is being computed by some other thread `T2`, we have to check whether `T2` is (transitively) blocked on `T1`. If so, there is a cycle. | ||
|
||
These two cases are handled internally by the `Runtime::try_block_on` function. Detecting the intra-thread cycle case is easy; to detect cross-thread cycles, the runtime maintains a dependency DAG between threads (identified by `RuntimeId`). Before adding an edge `T1 -> T2` (i.e., `T1` is blocked waiting for `T2`) into the DAG, it checks whether a path exists from `T2` to `T1`. If so, we have a cycle and the edge cannot be added (then the DAG would not longer be acyclic). | ||
|
||
When a cycle is detected, the current thread `T1` has full access to the query stacks that are participating in the cycle. Consider: naturally, `T1` has access to its own stack. There is also a path `T2 -> ... -> Tn -> T1` of blocked threads. Each of the blocked threads `T2 ..= Tn` will have moved their query stacks into the dependency graph, so those query stacks are available for inspection. | ||
|
||
Using the available stacks, we can create a list of cycle participants `Q0 ... Qn` and store that into a `Cycle` struct. If none of the participants `Q0 ... Qn` have cycle recovery enabled, we panic with the `Cycle` struct, which will trigger all the queries on this thread to panic. | ||
|
||
## Cycle recovery via fallback | ||
|
||
If any of the cycle participants `Q0 ... Qn` has cycle recovery set, we recover from the cycle. To help explain how this works, we will use this example cycle which contains three threads. Beginning with the current query, the cycle participants are `QA3`, `QB2`, `QB3`, `QC2`, `QC3`, and `QA2`. | ||
|
||
``` | ||
The cyclic | ||
edge we have | ||
failed to add. | ||
: | ||
A : B C | ||
: | ||
QA1 v QB1 QC1 | ||
┌► QA2 ┌──► QB2 ┌─► QC2 | ||
│ QA3 ───┘ QB3 ──┘ QC3 ───┐ | ||
│ │ | ||
└───────────────────────────────┘ | ||
``` | ||
|
||
Recovery works in phases: | ||
|
||
* **Analyze:** As we enumerate the query participants, we collect their collective inputs (all queries invoked so far by any cycle participant) and the max changed-at and min duration. We then remove the cycle participants themselves from this list of inputs, leaving only the queries external to the cycle. | ||
* **Mark**: For each query Q that is annotated with `#[salsa::recover]`, we mark it and all of its successors on the same thread by setting its `cycle` flag to the `c: Cycle` we constructed earlier; we also reset its inputs to the collective inputs gathering during analysis. If those queries resume execution later, those marks will trigger them to immediately unwind and use cycle recovery, and the inputs will be used as the inputs to the recovery value. | ||
* Note that we mark *all* the successors of Q on the same thread, whether or not they have recovery set. We'll discuss later how this is important in the case where the active thread (A, here) doesn't have any recovery set. | ||
* **Unblock**: Each blocked thread T that has a recovering query is forcibly reawoken; the outgoing edge from that thread to its successor in the cycle is removed. Its condvar is signalled with a `WaitResult::Cycle(c)`. When the thread reawakens, it will see that and start unwinding with the cycle `c`. | ||
* **Handle the current thread:** Finally, we have to choose how to have the current thread proceed. If the current thread includes any cycle with recovery information, then we can begin unwinding. Otherwise, the current thread simply continues as if there had been no cycle, and so the cyclic edge is added to the graph and the current thread blocks. This is possible because some other thread had recovery information and therefore has been awoken. | ||
|
||
Let's walk through the process with a few examples. | ||
|
||
### Example 1: Recovery on the detecting thread | ||
|
||
Consider the case where only the query QA2 has recovery set. It and QA3 will be marked with their `cycle` flag set to `c: Cycle`. Threads B and C will not be unblocked, as they do not have any cycle recovery nodes. The current thread (Thread A) will initiate unwinding with the cycle `c` as the value. Unwinding will pass through QA3 and be caught by QA2. QA2 will substitute the recovery value and return normally. QA1 and QC3 will then complete normally and so forth, on up until all queries have completed. | ||
|
||
### Example 2: Recovery in two queries on the detecting thread | ||
|
||
Consider the case where both query QA2 and QA3 have recovery set. It proceeds the same Example 1 until the the current initiates unwinding, as described in Example 1. When QA3 receives the cycle, it stores its recovery value and completes normally. QA2 then adds QA3 as an input dependency: at that point, QA2 observes that it too has the cycle mark set, and so it initiates unwinding. The rest of QA2 therefore never executes. This unwinding is caught by QA2's entry point and it stores the recovery value and returns normally. QA1 and QC3 then continue normally, as they have not had their `cycle` flag set. | ||
|
||
### Example 3: Recovery on another thread | ||
|
||
Now consider the case where only the query QB2 has recovery set. It and QB3 will be marked with the cycle `c: Cycle` and thread B will be unblocked; the edge `QB3 -> QC2` will be removed from the dependency graph. Thread A will then add an edge `QA3 -> QB2` and block on thread B. At that point, thread A releases the lock on the dependency graph, and so thread B is re-awoken. It observes the `WaitResult::Cycle` and initiates unwinding. Unwinding proceeds through QB3 and into QB2, which recovers. QB1 is then able to execute normally, as is QA3, and execution proceeds from there. | ||
|
||
### Example 4: Recovery on all queries | ||
|
||
Now consider the case where all the queries have recovery set. In that case, they are all marked with the cycle, and all the cross-thread edges are removed from the graph. Each thread will independently awaken and initiate unwinding. Each query will recover. |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# Derived queries flowchart | ||
|
||
Derived queries are by far the most complex. This flowchart documents the flow of the [maybe changed after] and [fetch] operations. This flowchart can be edited on [draw.io]: | ||
|
||
[draw.io]: https://draw.io | ||
[fetch]: ./fetch.md | ||
[maybe changed after]: ./maybe_changed_after.md | ||
|
||
<!-- The explicit div is there because, otherwise, the flowchart is unreadable when using "dark mode" --> | ||
<div style="background-color:white;"> | ||
|
||
![Flowchart](../derived-query-read.drawio.svg) | ||
|
||
</div> |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
# Fetch | ||
|
||
```rust,no_run,noplayground | ||
{{#include ../../../src/plumbing.rs:fetch}} | ||
``` | ||
|
||
The `fetch` operation computes the value of a query. It prefers to reuse memoized values when it can. | ||
|
||
## Input queries | ||
|
||
Input queries simply load the result from the table. | ||
|
||
## Interned queries | ||
|
||
Interned queries map the input into a hashmap to find an existing integer. If none is present, a new value is created. | ||
|
||
## Derived queries | ||
|
||
The logic for derived queries is more complex. We summarize the high-level ideas here, but you may find the [flowchart](./derived_flowchart.md) useful to dig deeper. The [terminology](./terminology.md) section may also be useful; in some cases, we link to that section on the first usage of a word. | ||
|
||
* If an existing [memo] is found, then we check if the memo was [verified] in the current [revision]. If so, we can directly return the memoized value. | ||
* Otherwise, if the memo contains a memoized value, we must check whether [dependencies] have been modified: | ||
* Let R be the revision in which the memo was last verified; we wish to know if any of the dependencies have changed since revision R. | ||
* First, we check the [durability]. For each memo, we track the minimum durability of the memo's dependencies. If the memo has durability D, and there have been no changes to an input with durability D since the last time the memo was verified, then we can consider the memo verified without any further work. | ||
* If the durability check is not sufficient, then we must check the dependencies individually. For this, we iterate over each dependency D and invoke the [maybe changed after](./maybe_changed_after.md) operation to check whether D has changed since the revision R. | ||
* If no dependency was modified: | ||
* We can mark the memo as verified and return its memoized value. | ||
* Assuming dependencies have been modified or the memo does not contain a memoized value: | ||
* Then we execute the user's query function. | ||
* Next, we compute the revision in which the memoized value last changed: | ||
* *Backdate:* If there was a previous memoized value, and the new value is equal to that old value, then we can *backdate* the memo, which means to use the 'changed at' revision from before. | ||
* Thanks to backdating, it is possible for a dependency of the query to have changed in some revision R1 but for the *output* of the query to have changed in some revision R2 where R2 predates R1. | ||
* Otherwise, we use the current revision. | ||
* Construct a memo for the new value and return it. | ||
|
||
[durability]: ./terminology/durability.md | ||
[backdate]: ./terminology/backdate.md | ||
[dependency]: ./terminology/dependency.md | ||
[dependencies]: ./terminology/dependency.md | ||
[memo]: ./terminology/memo.md | ||
[revision]: ./terminology/revision.md | ||
[verified]: ./terminology/verified.md |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# Generated code | ||
|
||
This page walks through the ["Hello, World!"] example and explains the code that | ||
it generates. Please take it with a grain of salt: while we make an effort to | ||
keep this documentation up to date, this sort of thing can fall out of date | ||
easily. See the page history below for major updates. | ||
|
||
["Hello, World!"]: https://github.com/salsa-rs/salsa/blob/master/examples/hello_world/main.rs | ||
|
||
If you'd like to see for yourself, you can set the environment variable | ||
`SALSA_DUMP` to 1 while the procedural macro runs, and it will dump the full | ||
output to stdout. I recommend piping the output through rustfmt. | ||
|
||
## Sources | ||
|
||
The main parts of the source that we are focused on are as follows. | ||
|
||
### Query group | ||
|
||
```rust,ignore | ||
{{#include ../../../examples/hello_world/main.rs:trait}} | ||
``` | ||
|
||
### Database | ||
|
||
```rust,ignore | ||
{{#include ../../../examples/hello_world/main.rs:database}} | ||
``` |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, in rust-analyzer we switched from
insta
to https://github.com/rust-analyzer/expect-test quite a while ago. expect-test is a bit more minimal. Rather than needing acargo-insta
command to upgrade the thest, the test runtime modifies code in-place.I kinda like it more, but no strong opinion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does look reasonably nice! I also don't have a super strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit: you should crate a
cargo expect
command or something instead of using an environment variable! :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, not having
cargo expect
is one of the design goals forexpect-test
. I don't like cargo subcommands because:cargo install foo
instead of things just workingSo that's why expect-test is strictly only a library, but that necessitates somewhat unusual env-var workflow.