Commit graph

891 commits

Author SHA1 Message Date
Maxwell Elliot Heiber
7fba34afd9 Document how to tune Salsa 2022-02-12 10:34:11 +00:00
bors[bot]
f4e09fee23
Merge #297
297: Fix netlify deployment r=nikomatsakis a=nikomatsakis

I think I just configured this in a pretty bogus way

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>
2022-02-07 20:50:03 +00:00
Niko Matsakis
796ca0fd27 move to book directory 2022-02-07 15:44:29 -05:00
Niko Matsakis
2ff73b9ae5 trace commands 2022-02-07 15:41:50 -05:00
Niko Matsakis
3bdc61f9d8 add ci/netlify.sh 2022-02-07 15:38:51 -05:00
bors[bot]
0f9971ad94
Merge #296
296: Slot no more: overhauled internal algorithm r=nikomatsakis a=nikomatsakis

This is the overhauled implementation that avoids slots, is more parallel friendly, and paves the way to fixed point and more expressive cycle handling.

We just spent 90 minutes going over it. [Some rough notes are available here,](https://hackmd.io/6x9f6mavTRS2imfG96tP5A) and a video will be posted soon.

You may find the [flowgraph useful](https://raw.githubusercontent.com/nikomatsakis/salsa/slot-no-more/book/src/derived-query-read.drawio.svg).



Co-authored-by: Niko Matsakis <niko@alum.mit.edu>
2022-02-07 20:30:07 +00:00
bors[bot]
d294be791e
Merge #294
294: Fix mdbook warning re "Potential incomplete link" r=nikomatsakis a=mheiber

In cycle.md, change `[salsa::Cycle]` to `salsa::Cycle` so mdbook doesn't get confused
and think there is a broken Markdown link ("warning: Potential incomplete link").

After fixing this problem, there are no more mdbook warnings when
running `mdbook build` in the "book" directory.



Co-authored-by: Maxwell Elliot Heiber <mheiber@fb.com>
2022-01-25 19:57:44 +00:00
bors[bot]
73aba192c1
Merge #295
295: Make storage fields of #nameGroupStorage private r=nikomatsakis a=mheiber

This change resolves a fixme that referenced #120.

This change breaks no tests, and, if I understand
correctly, does not affect user-facing API.

Here is the difference for the `HelloWorldGroupStorage__` struct
generated from macros in the `hello_world` example:

**Before:**

```rs
struct HelloWorldGroupStorage__ {
    pub input_string:std::sync::Arc<<InputStringQuery as salsa::Query> ::Storage> ,pub length:std::sync::Arc<<LengthQuery as salsa::Query> ::Storage> ,
}
```

**After:**

```rs
struct HelloWorldGroupStorage__ {
    input_string:std::sync::Arc<<InputStringQuery as salsa::Query> ::Storage> ,length:std::sync::Arc<<LengthQuery as salsa::Query> ::Storage> ,
}
```

Co-authored-by: Maxwell Elliot Heiber <mheiber@fb.com>
2022-01-25 19:54:56 +00:00
Maxwell Elliot Heiber
a1be30bc13 Make storage fields of #nameGroupStorage private
This change resolves a fixme that referenced #120.

This change breaks no tests, and, if I understand
correctly, does not affect user-facing API.

Here is the difference for the `HelloWorldGroupStorage__` struct
generated from macros in the `hello_world` example:

**Before:**

```rs
struct HelloWorldGroupStorage__ {
    pub input_string:std::sync::Arc<<InputStringQuery as salsa::Query> ::Storage> ,pub length:std::sync::Arc<<LengthQuery as salsa::Query> ::Storage> ,
}
```

**After:**

```rs
struct HelloWorldGroupStorage__ {
    input_string:std::sync::Arc<<InputStringQuery as salsa::Query> ::Storage> ,length:std::sync::Arc<<LengthQuery as salsa::Query> ::Storage> ,
}
```
2022-01-23 20:18:25 +00:00
Maxwell Elliot Heiber
d80d10d0c4 Fix mdbook warning re "Potential incomplete link"
In cycle.md, change `[salsa::Cycle]`
to `salsa::Cycle` so mdbook doesn't get confused
and think there is a broken Markdown link.

The warning was as follows:

```
warning: Potential incomplete link
  ┌─ cycles.md:3:131
  │
3 │ 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.
  │                                                                                                                                   ^^^^^^^^^^^^^^^^ Did you forget to define a URL for ``salsa::Cycle``?
  │
  = hint: declare the link's URL. For example: `[`salsa::Cycle`]: http://example.com/`
```

After fixing this problem, there are no more mdbook warnings.
2022-01-23 19:55:02 +00:00
Niko Matsakis
d4a6b24358 Update derived-query-maybe-changed-after.drawio.svg 2022-01-21 15:14:10 -05:00
Niko Matsakis
416884bdfa Update derived-query-maybe-changed-after.drawio.svg 2022-01-21 15:14:04 -05:00
Niko Matsakis
ff5c6137ac Update derived-query-maybe-changed-after.drawio.svg 2022-01-21 15:12:01 -05:00
Niko Matsakis
d69bddea9e Update derived-query-maybe-changed-after.drawio.svg 2022-01-21 15:06:47 -05:00
Niko Matsakis
be7a6ed06c Added derived-query-maybe-changed-after.drawio.svg 2022-01-21 14:58:26 -05:00
Niko Matsakis
b3242064a2 Update derived-query-read.drawio.svg to indicate fn boundaries 2022-01-21 14:55:46 -05:00
Niko Matsakis
37a188c9b7 remove dead struct 2022-01-21 14:16:14 -05:00
Niko Matsakis
ade6bcf2b1 check for cancellation more aggressively 2022-01-21 13:58:15 -05:00
Niko Matsakis
2e0301f043 "maybe changed AFTER" in diagram 2022-01-21 13:58:15 -05:00
Niko Matsakis
1f5d701075 tweak SVG to rename "maybe changed after" 2022-01-21 13:58:15 -05:00
Niko Matsakis
e5f59cb7c9 add RFC that describes the new scheme 2022-01-21 13:58:15 -05:00
Niko Matsakis
08be38ae62 Update derived-query-read.drawio.svg 2022-01-21 13:58:15 -05:00
Niko Matsakis
356392578b new parallel friendly algorithm 2022-01-21 13:58:13 -05:00
Niko Matsakis
685fccc9c5 move to dash-map
Because dash-map isn't indexable, we need to store a copy of the
key and have two separate maps. I expect to iterate on the best
data structures here.
2022-01-21 13:56:16 -05:00
Niko Matsakis
c0d9070a64 refactor _mut path to not take arc
Instead of grabbing the arc, just pass back an `&mut Runtime`.

The eventual goal is to get rid of the lock on the `set` pathway
altogether, but one step at a time.
2022-01-21 13:52:43 -05:00
Niko Matsakis
82d695b9a7 extract hashing definitions into a utility module 2022-01-21 13:49:50 -05:00
bors[bot]
e5cb77472b
Merge #290
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>
2022-01-21 18:38:10 +00:00
bors[bot]
2709ee4d10
Merge #288 #289
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>
2022-01-21 18:35:51 +00:00
bors[bot]
d830152ade
Merge #287
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>
2022-01-21 18:33:32 +00:00
bors[bot]
a2e5cd84f4
Merge #285
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>
2022-01-21 18:27:20 +00:00
Maxwell Elliot Heiber
eb2b36948d Update doc in macro about query.in_db for dyn db
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)`"
2021-12-30 11:36:05 +00:00
Maxwell Elliot Heiber
145202e376 Remove ': salsa::Database' bound from two examples
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)
2021-12-30 11:02:30 +00:00
Maxwell Elliot Heiber
befa3ddef5 Make with_incremented_revision take FnOnce
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.
2021-12-30 10:44:35 +00:00
DSPOM2
23c495f82d
Remove unused crossbeam_utils dependency
crossbeam_utils is currently not used anywhere withing this crate so I propose to remove the dependency to (slightly) reduce compile times
2021-12-26 22:32:12 +01:00
Niko Matsakis
fc020de9c4 s/maybe_changed_since/maybe_changed_after/ 2021-11-13 16:39:41 -05:00
Niko Matsakis
cd265bfd12
Update src/runtime/dependency_graph.rs
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
2021-11-13 16:28:59 -05:00
Niko Matsakis
563334da17 fix comments 2021-11-13 12:02:25 -05:00
Niko Matsakis
ff71d77a24 avoid overloading the term "participant" 2021-11-13 11:58:18 -05:00
Niko Matsakis
7081fe428c move the RFC contents to salsa book 2021-11-12 19:16:04 -05:00
Niko Matsakis
db2daf03e3 describe implementation in RFC 2021-11-12 09:02:40 -05:00
Niko Matsakis
5ebd2211a5 don't recover when not a participant
When a query Q invokes a cycle Q1...Q1 but Q is not a
participant in that cycle, Q should not recover! Test that.
2021-11-12 05:50:06 -05:00
Niko Matsakis
b8f628d664 improve RFC guide description 2021-11-11 09:07:45 -05:00
Niko Matsakis
cb658b9b89 enable partial recovery across threads
Including the corner case where the active thread does not have
recovery.
2021-11-11 09:07:45 -05:00
Niko Matsakis
93ee78e73f factor out a helper to unblock a given runtime 2021-11-11 06:54:52 -05:00
Niko Matsakis
92b5c0261b introduce a condvar per thread, instead of one
The previous version had a lot of spurious wakeups, but it
also resisted the refactoring I'm about to do. =)
2021-11-11 06:54:52 -05:00
Niko Matsakis
bfa74bc865 spread parallel tests into their own files
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.
2021-11-11 06:54:52 -05:00
Niko Matsakis
45434cfa93 rework cycle to permit partial recovery (wip)
This is "wip" because it does not yet handle cross-thread recovery
correctly. That is coming in a later commit.
2021-11-11 06:54:32 -05:00
Niko Matsakis
c14a3d47ea remove memo from PanicGuard
`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!).
2021-11-10 21:01:00 -05:00
Niko Matsakis
961599aa39 unwind from "block on" for panic/cancellation
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).
2021-11-08 08:07:45 -05:00
Niko Matsakis
7c02d1910b add cycle recovery 2021-11-06 07:59:20 -04:00