Coarse-grained tracked structs

[ibraheemdev]

This PR makes tracked structs coarse-grained by default. To declare a field independently tracked, you now use the #[tracked] attribute. This is the opposite of the previous behavior, where #[id] effectively declared a field untracked. The #[id] attribute now has no effect. Importantly, reads of untracked fields do not require a read dependency to be declared.

There is some duplicated code in the macros as we need to generate separate accessors for tracked vs. untracked fields and I would like these to be static (as opposed to branching at runtime).

I believe the guide documentation also needs to be updated.

Resolves #598.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	a63bd53
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/678afefe4220bb000804326f

[codspeed-hq]

CodSpeed Performance Report

Merging #657 will not alter performance

_{Comparing ibraheemdev:coarse-deps (a63bd53) with master (e4d65a6)}

Summary

✅ 9 untouched benchmarks

[ibraheemdev]

I tried not adding this #[tracked] attribute because it seems unnecessary, but I actually got a test failure that I don't really understand:

---- type_check::fix_bad_variable_in_function stdout ----
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: parse_statements(Id(0)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: type_check_program(Id(1400)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: type_check_function(Id(1800)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: type_check_function(Id(1801)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: find_function(Id(1c00)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: find_function(Id(1c01)) } }
Event: Event { thread_id: ThreadId(11), kind: DidSetCancellationFlag }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillExecute { database_key: parse_statements(Id(0)) } }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
Event: Event { thread_id: ThreadId(11), kind: WillCheckCancellation }
thread 'type_check::fix_bad_variable_in_function' panicked at /home/ibraheem/dev/sync/salsa/src/table.rs:281:9:
assertion `left == right` failed: page has hidden type `"salsa::table::Page<salsa::tracked_struct::Value<calc::ir::Function>>"` but `"salsa::table::Page<salsa::tracked_struct::Value<calc::ir::Program>>"` was expected
  left: TypeId(0xbeb3ca6f8846f92e6ae4fc7569b4bf5f)
 right: TypeId(0xbbdce2561688eb1bfc9fa5f5006a8342)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[MichaReiser]

I added some debug statements to function::maybe_chagned_after to log the dependencies and compared the logs between a run that succeeds and a run that fails.

The most interesting difference is

Successful

[src/tracked_struct/tracked_field.rs:58:9] self = salsa::tracked_struct::tracked_field::FieldIngredientImpl<calc::ir::Program> {
    ingredient_index: IngredientIndex(
        13,
    ),
    field_index: 0,
}
Event: Event { thread_id: ThreadId(2), kind: WillCheckCancellation }
[src/function/maybe_changed_after.rs:154:9] &database_key_index = type_check_function(Id(1800))
[src/function/maybe_changed_after.rs:156:15] &old_memo.revisions.origin = Derived(
    QueryEdges {
        input_outputs: [
            Input(
                Function.args(Id(1000)),
            ),
            Input(
                Program(Id(1000)),
            ),
            Input(
                Span.start(Id(406)),
            ),
            Input(
                Span.end(Id(406)),
            ),
        ],
    },
)

With Panic

[src/tracked_struct/tracked_field.rs:58:9] self = salsa::tracked_struct::tracked_field::FieldIngredientImpl<calc::ir::Program> {
    ingredient_index: IngredientIndex(
        12,
    ),
    field_index: 0,
}
Event: Event { thread_id: ThreadId(2), kind: WillCheckCancellation }
[src/function/maybe_changed_after.rs:154:9] &database_key_index = type_check_function(Id(1800))
[src/function/maybe_changed_after.rs:156:15] &old_memo.revisions.origin = Derived(
    QueryEdges {
        input_outputs: [
            Input(
                Program(Id(1000)),
            ),
            Input(
                Program.statements(Id(1000)),
            ),
            Input(
                Span.start(Id(406)),
            ),
            Input(
                Span.end(Id(406)),
            ),
        ],
    },
)

The main difference is that the ingredient on which we run maybe_changed_after is different, AND so are the inputs. Looking at type_check_function then the inputs of the panic case make no sense. The query never calls Program.statements but it does call Function.args. I don't understand how it is happening but I suspect that we're incorrectly reusing a memo and turn a Function memo into a Program which would align with the panic

[MichaReiser]

Okay, I think I know what the issue is...

You changed the tracked struct's Configuration::create_ingredients to only create ingredients for tracked fields. I think this change is correct. However, you call Ingredient::Index::successor in tracked_field with the field_index which is the absolute offset including all fields -- including untracked fields. What this means is that we now add incorrect dependencies as soon as we have a tracked struct where a tracked field follows an untracked field.

I hope that helps. I let you take a stab at correctly mapping the field indices. It would be great to have a unit test demonstrating the behavior and possibly an assertion somewhere that would help us catch this sooner.

[ibraheemdev]

Ah, that is a tricky. I tried a couple things here:

• Creating the ingredients with the absolute indices directly. Unfortunately this causes a panic, as some of the ingredient code relies on indices being create sequentially.
• Passing the correct relative index to successor in tracked_field. Unfortunately this also did not seem to work correctly and didn't result in the correct fields being tracked, but I'm not exactly sure why. The logging code also relies on the indices being absolute in order to print field names, so this was a little awkward.

Eventually I think the easiest solution is just creating ingredients for all fields. I added a failing test that is now passing with this fix (and also fails with the second idea I tried).

[MichaReiser]

Hmm, that's unfortunate but seems reasonable. Would it be possible to avoid creating any field ingredients if the structure only has tracked fields? Doing this optimization seems worthwhile, considering that all-fields-untracked is the new default

[MichaReiser]

I assume that keeping a mapping table from field_index to relative_ingredient_index (a [Option<usize>] where the value is Some for all tracked fields) is annoying to write the macro code for? If not, maybe that's worth a try. I otherwise suggest we keep it as is for now (but optimize for the no-tracked fields case)

[MichaReiser]

This looks great to me. I think the idea of tracked and untracked fields is easier to understand than id (I'm fairly certain we used id incorrectly in Red Knot).

I think I have an understanding why the panic occurs and I added an inline comment with an explanation. I let you take a stab at fixing it.

[MichaReiser]

I added some debug statements to function::maybe_chagned_after to log the dependencies and compared the logs between a run that succeeds and a run that fails.

The most interesting difference is

Successful

[src/tracked_struct/tracked_field.rs:58:9] self = salsa::tracked_struct::tracked_field::FieldIngredientImpl<calc::ir::Program> {
    ingredient_index: IngredientIndex(
        13,
    ),
    field_index: 0,
}
Event: Event { thread_id: ThreadId(2), kind: WillCheckCancellation }
[src/function/maybe_changed_after.rs:154:9] &database_key_index = type_check_function(Id(1800))
[src/function/maybe_changed_after.rs:156:15] &old_memo.revisions.origin = Derived(
    QueryEdges {
        input_outputs: [
            Input(
                Function.args(Id(1000)),
            ),
            Input(
                Program(Id(1000)),
            ),
            Input(
                Span.start(Id(406)),
            ),
            Input(
                Span.end(Id(406)),
            ),
        ],
    },
)

With Panic

[src/tracked_struct/tracked_field.rs:58:9] self = salsa::tracked_struct::tracked_field::FieldIngredientImpl<calc::ir::Program> {
    ingredient_index: IngredientIndex(
        12,
    ),
    field_index: 0,
}
Event: Event { thread_id: ThreadId(2), kind: WillCheckCancellation }
[src/function/maybe_changed_after.rs:154:9] &database_key_index = type_check_function(Id(1800))
[src/function/maybe_changed_after.rs:156:15] &old_memo.revisions.origin = Derived(
    QueryEdges {
        input_outputs: [
            Input(
                Program(Id(1000)),
            ),
            Input(
                Program.statements(Id(1000)),
            ),
            Input(
                Span.start(Id(406)),
            ),
            Input(
                Span.end(Id(406)),
            ),
        ],
    },
)

The main difference is that the ingredient on which we run maybe_changed_after is different, AND so are the inputs. Looking at type_check_function then the inputs of the panic case make no sense. The query never calls Program.statements but it does call Function.args. I don't understand how it is happening but I suspect that we're incorrectly reusing a memo and turn a Function memo into a Program which would align with the panic

[MichaReiser]

Okay, I think I know what the issue is...

You changed the tracked struct's Configuration::create_ingredients to only create ingredients for tracked fields. I think this change is correct. However, you call Ingredient::Index::successor in tracked_field with the field_index which is the absolute offset including all fields -- including untracked fields. What this means is that we now add incorrect dependencies as soon as we have a tracked struct where a tracked field follows an untracked field.

I hope that helps. I let you take a stab at correctly mapping the field indices. It would be great to have a unit test demonstrating the behavior and possibly an assertion somewhere that would help us catch this sooner.

[ibraheemdev]

Is the read_lock necessary here for an untracked field? It seems not because leak_fields does not call it.

[MichaReiser]

Not a 100% but the comment on Value::updated_at and the inline comment inside update does suggest to me that the lock is required before accessing any field (handing out references to field) and it not being present in leak_field seems like a bug.

[MichaReiser]

thanks. This is great. I think we should be able to optimize the implementation to avoid creating any field-ingredients if all fields are tracked.