salsa-rs / salsa

A generic framework for on-demand, incrementalized computation. Inspired by adapton, glimmer, and rustc's query system.
https://salsa-rs.netlify.app/
Apache License 2.0
2.14k stars 152 forks source link

Coarse-grained tracked structs #598

Open nikomatsakis opened 1 month ago

nikomatsakis commented 1 month ago

As described in this hackmd, we want to change how tracked struct dependencies work. The basic idea is to switch the default: instead of labeling some fields as #[id] and having the rest be independently tracked, we will have fields be #[id] by default unless they are annotated with #[salsa::tracked]. This is a fairly straightforward change that can be done in the proc-macro.

Example

Before:

#[salsa::tracked]
struct Function {
    #[id]
    name: String,
    body: String,
}

After:

#[salsa::tracked]
struct Function {
    name: String,

    #[salsa::tracked]
    body: String,
}
MichaReiser commented 1 month ago

CC: @ibraheemdev

nikomatsakis commented 1 month ago

Mentoring instructions

Ok, I realized this is a bit more complicated than I thought. There are two parts required

  1. Swap the default.
  2. Change how we track reads of #[id] fields -- we don't currently distinguish them, but we should.

swap the default

The decision about which fields are "id" fields is driven by this input to the macro-rules macro:

https://github.com/salsa-rs/salsa/blob/master/components/salsa-macro-rules/src/setup_tracked_struct.rs#L32-L33

which is provided here

https://github.com/salsa-rs/salsa/blob/c6c51a0ea0bb11ce5a449c4b9256c66c5e484fee/components/salsa-macros/src/tracked_struct.rs#L112

it should be fairly straightforward to modify the latter file to flip the defaults. (Note that the next section will also likely change this interface slightly.)

I'd probably also rename these from "id" fields to "untracked" fields.

change how we track reads of #[id] fields

This is the trickier bit. Currently we create a field ingredient for every field, regardless of whether it's part of the #[id] hash:

https://github.com/salsa-rs/salsa/blob/c6c51a0ea0bb11ce5a449c4b9256c66c5e484fee/src/tracked_struct.rs#L101-L115

the field accessor for every field is the same:

https://github.com/salsa-rs/salsa/blob/c6c51a0ea0bb11ce5a449c4b9256c66c5e484fee/components/salsa-macro-rules/src/setup_tracked_struct.rs#L198-L212

in particular it invokes the field method from the tracked struct on line 205. That function is defined here:

https://github.com/salsa-rs/salsa/blob/c6c51a0ea0bb11ce5a449c4b9256c66c5e484fee/src/tracked_struct.rs#L516-L524

It does a fair bit of work and in particular adds a read.

All of that work is unnecessary for id-fields. Just as with an interned struct, the idea is that if the id fields have changed, then you will have a new tracked struct instance from salsa's perspective, so they can be regarded as immutable. As a result, we just have to read from them. Compare to the fields method from interned structs.

I think what I would do is to modify the setup_tracked_struct macro so that when we generate the field getters, we generate the getters for id fields differently (they invoke a untracked_fields method on the ingredient or something like that, and we rename the existing getter to tracked_fields).

This will require changing the macro inputs. There are a few few ways to go about this. Perhaps the easiest is to add some kind of field_getter_method argument that is either untracked_fields or tracked_fields depending and then modify this line

https://github.com/salsa-rs/salsa/blob/c6c51a0ea0bb11ce5a449c4b9256c66c5e484fee/components/salsa-macro-rules/src/setup_tracked_struct.rs#L205

to read let fields = $Configuration::ingredient(db).$field_getter_method(db, self, $field_index);.

ibraheemdev commented 1 month ago

Thanks for the writeup! I can work on this after https://github.com/salsa-rs/salsa/issues/597 (if no one else has gotten to it first).

OLUWAMUYIWA commented 3 weeks ago

Picking this up if you haven't @ibraheemdev

ibraheemdev commented 2 weeks ago

@OLUWAMUYIWA Feel free!