salsa 3.0

[nikomatsakis]

This branch implements the Salsa 3.0 plan, at least in broad strokes. It's not quite ready to merge, needs more documentation, especially around the use of unsafe. Rather than write an extensive comment here, I'm going to push some documentation commits.

[netlify[bot]]

✅ Deploy Preview for salsa-rs ready!

Name	Link
🔨 Latest commit	1544ee97ce402e8ed93265215acfeac6079ea2f2
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/666d6848d3e890000882a5dc
😎 Deploy Preview	https://deploy-preview-490--salsa-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nikomatsakis]

I wondered what miri would make of all this. I had forgotten we had tests for it, though. All the more reason to write up how it works, so I can try to figure out if miri is giving me a legit error or not ...

[nikomatsakis]

Reproduced locally. The good news is that just running on salsa-2022-tests, lots of tests fail...

error: 19 targets failed:
    `-p salsa-2022-tests --test debug`
    `-p salsa-2022-tests --test deletion`
    `-p salsa-2022-tests --test deletion-cascade`
    `-p salsa-2022-tests --test expect_reuse_field_x_of_a_tracked_struct_changes_but_fn_depends_on_field_y`
    `-p salsa-2022-tests --test hello_world`
    `-p salsa-2022-tests --test is_send_sync`
    `-p salsa-2022-tests --test preverify-struct-with-leaked-data`
    `-p salsa-2022-tests --test specify-only-works-if-the-key-is-created-in-the-current-query`
    `-p salsa-2022-tests --test specify_tracked_fn_in_rev_1_but_not_2`
    `-p salsa-2022-tests --test tracked-struct-id-field-bad-eq`
    `-p salsa-2022-tests --test tracked-struct-id-field-bad-hash`
    `-p salsa-2022-tests --test tracked-struct-unchanged-in-new-rev`
    `-p salsa-2022-tests --test tracked-struct-value-field-bad-eq`
    `-p salsa-2022-tests --test tracked-struct-value-field-not-eq`
    `-p salsa-2022-tests --test tracked_fn_on_tracked`
    `-p salsa-2022-tests --test tracked_fn_on_tracked_specify`
    `-p salsa-2022-tests --test tracked_fn_read_own_entity`
    `-p salsa-2022-tests --test tracked_fn_read_own_specify`
    `-p salsa-2022-tests --test tracked_with_struct_db`

Good news because it indicates the error is not just some random edge case. I am pushing past the trivial stacked borrows use case, so I wouldn't be surprised if we are running into some issues in the model itself, but I don't know anything at this point. I'm going to start digging into the hello-world example I think as it seems .. well .. the most basic. :)

EDIT: Actually tracked_with_struct_db is even more basic I think.

[nikomatsakis]

I don't understand exactly what you are trying to do -- can you elaborate?

On Tue, May 28, 2024, at 1:45 AM, Micha Reiser wrote:

@.**** commented on this pull request.

In components/salsa-2022/src/id.rs https://github.com/salsa-rs/salsa/pull/490#discussion_r1616838938:

• /// Lookup from an Id to get an instance of the type.
• ///
• /// # Panics
• ///
• /// This fn may panic if the value with this id has not been
• /// produced in this revision already (e.g., for a tracked
• /// struct, the function will panic if the tracked struct
• /// has not yet been created in this revision). Salsa's
• /// dependency tracking typically ensures this does not
• /// occur, but it is possible for a user to violate this
• /// rule.
• fn lookup_id(id: Id, db: DB) -> Self; +}

• +/// Internal Salsa trait for types that are just a newtype'd [Id][]. +pub trait FromId: AsId + Copy + Eq + Hash + Debug {

This is interesting. We might be doing something wrong but one thing we've been thinking about is that it would be nice if we could e.g. intern an entire SymbolTable but still use individual Symbols as salsa ingredients (because we always rebuild the entire symbol table but we want symbol level invalidation). I'm not sure if that's something that the new FromId trait would enable (it would probably still require implementing a custom Ingredient)

— Reply to this email directly, view it on GitHub https://github.com/salsa-rs/salsa/pull/490#pullrequestreview-2082144912, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF4ZRXB7FETQHIXMRBEPDZEQ7ZTAVCNFSM6AAAAABIJEURNGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBSGE2DIOJRGI. You are receiving this because you authored the thread.Message ID: @.***>

[nikomatsakis]

I am debating about how to proceed here. It's taking me far longer to write these docs than I hoped. And they are complicated. I'm somewhat inclined to merge the PR so that people can start trying it out and then continue to write the safety docs in parallel.

But I'm also eager to not overlook the safety docs.

Thoughts from others?

[nikomatsakis]

One thing I don't know 100% is how much the memory safety of the scheme relies on salsa correctly tracking dependencies. I'd prefer if it didn't, because I don't trust users not to poke holes in that system, but I feel like as I talked out the reasoning I found myself wanting to rely on "and we know that the fn will have been re-executed". But then I think I did add some judicious panics in there partly for this reason (i.e., to double check users and panic if they messed things up), I just don't know yet if that's enough.

[nikomatsakis]

OK, I pushed an update to the safety documentation that I think covers all the key details in a readable way. It also identifies what I believe to be a flaw in the current setup -- I think it's possible for users to abuse salsa through leaked structs or nondeterminism and access freed memory. It's not easy to do, and I should make a test (will do later).

There are various ways to close this hole, and in particular I think one of the planned improvements I had in mind (adopting a sharded-slab like structure) would serve for it. But fundamentally we need some way to test if a pointer is still "in bounds".

I'm inclined to land the PR in its current state and work on those improvements as follow-up.

[MichaReiser]

I'm somewhat inclined to merge the PR so that people can start trying it out and then continue to write the safety docs in parallel.

I don't expect to have time to try out the new branch before the end of next week, but having something to play with certainly is nice (although that's also possible by pointing cargo to this PR's revision).

I'm inclined to land the PR in its current state and work on those improvements as follow-up.

I'm supportive of this. I would find separate PR's useful for more focused discussions around specific improvements.

[nikomatsakis]

OK, I'm going to land this change. We can discuss future developments on Zulip. One thing I plan to do is explore using sharded-slab in the implementation, which will make the safety concerns much simpler but (maybe, not clearly) cost a bit of performance. It'd be useful to have measurements at some point.

[nikomatsakis]

That said, I do feel certain that the lifetimes and requirement to use the Update trait etc is good. It is a bit more restrictive but gives us a lot of room for future improvement.

[ematipico]

I think you should merge and publish this version.

I want to start experimenting, so I can help filing possible issues and help with the developments, if they are accepted.

[nikomatsakis]

I'm going to land it. I'm not inclined to publish though I would be, I think, ok with something like 3.0-alpha or something