From 99ec0a4870a52814a11ce735adb6652bca80ebc4 Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Wed, 23 Oct 2024 16:24:07 -0400 Subject: [PATCH 1/3] remove table-wide dependencies --- book/src/plumbing/jars_and_ingredients.md | 3 - src/accumulator.rs | 13 +-- src/active_query.rs | 12 +-- src/database.rs | 2 +- src/event.rs | 6 +- src/function.rs | 22 ++--- src/function/diff_outputs.rs | 10 +-- src/ingredient.rs | 20 ++--- src/input.rs | 19 ++-- src/input/input_field.rs | 14 +-- src/interned.rs | 61 ++++++------- src/key.rs | 102 +++++----------------- src/tracked_struct.rs | 21 ++--- src/tracked_struct/tracked_field.rs | 17 ++-- src/zalsa_local.rs | 21 +++-- 15 files changed, 117 insertions(+), 226 deletions(-) diff --git a/book/src/plumbing/jars_and_ingredients.md b/book/src/plumbing/jars_and_ingredients.md index 108935cd5..a9ad1897b 100644 --- a/book/src/plumbing/jars_and_ingredients.md +++ b/book/src/plumbing/jars_and_ingredients.md @@ -146,9 +146,6 @@ It combines an [`IngredientIndex`] with a `key_index`, which is a `salsa::Id`: {{#include ../../../src/key.rs:DatabaseKeyIndex}} ``` -A `DependencyIndex` is similar, but the `key_index` is optional. -This is used when we sometimes wish to refer to the ingredient as a whole, and not any specific value within the ingredient. - These kinds of indices are used to store connetions between ingredients. For example, each memoized value has to track its inputs. Those inputs are stored as dependency indices. diff --git a/src/accumulator.rs b/src/accumulator.rs index b9355419a..b43ad0c95 100644 --- a/src/accumulator.rs +++ b/src/accumulator.rs @@ -97,12 +97,7 @@ impl Ingredient for IngredientImpl { self.index } - fn maybe_changed_after( - &self, - _db: &dyn Database, - _input: Option, - _revision: Revision, - ) -> bool { + fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool { panic!("nothing should ever depend on an accumulator directly") } @@ -118,7 +113,7 @@ impl Ingredient for IngredientImpl { &self, _db: &dyn Database, _executor: DatabaseKeyIndex, - _output_key: Option, + _output_key: Id, ) { } @@ -126,7 +121,7 @@ impl Ingredient for IngredientImpl { &self, _db: &dyn Database, _executor: DatabaseKeyIndex, - _stale_output_key: Option, + _stale_output_key: Id, ) { } @@ -138,7 +133,7 @@ impl Ingredient for IngredientImpl { panic!("unexpected reset on accumulator") } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(A::DEBUG_NAME, index, fmt) } diff --git a/src/active_query.rs b/src/active_query.rs index aadcf1967..ef9778d5e 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -6,7 +6,7 @@ use crate::{ accumulator::accumulated_map::AccumulatedMap, durability::Durability, hash::FxIndexSet, - key::{DatabaseKeyIndex, DependencyIndex}, + key::DatabaseKeyIndex, tracked_struct::{Disambiguator, Identity}, zalsa_local::EMPTY_DEPENDENCIES, Cycle, Id, Revision, @@ -30,7 +30,7 @@ pub(crate) struct ActiveQuery { /// * tracked structs created /// * invocations of `specify` /// * accumulators pushed to - input_outputs: FxIndexSet<(EdgeKind, DependencyIndex)>, + input_outputs: FxIndexSet<(EdgeKind, DatabaseKeyIndex)>, /// True if there was an untracked read. untracked_read: bool, @@ -73,7 +73,7 @@ impl ActiveQuery { pub(super) fn add_read( &mut self, - input: DependencyIndex, + input: DatabaseKeyIndex, durability: Durability, revision: Revision, ) { @@ -95,12 +95,12 @@ impl ActiveQuery { } /// Adds a key to our list of outputs. - pub(super) fn add_output(&mut self, key: DependencyIndex) { + pub(super) fn add_output(&mut self, key: DatabaseKeyIndex) { self.input_outputs.insert((EdgeKind::Output, key)); } /// True if the given key was output by this query. - pub(super) fn is_output(&self, key: DependencyIndex) -> bool { + pub(super) fn is_output(&self, key: DatabaseKeyIndex) -> bool { self.input_outputs.contains(&(EdgeKind::Output, key)) } @@ -142,7 +142,7 @@ impl ActiveQuery { /// Used during cycle recovery, see [`Runtime::unblock_cycle_and_maybe_throw`]. pub(super) fn remove_cycle_participants(&mut self, cycle: &Cycle) { for p in cycle.participant_keys() { - let p: DependencyIndex = p.into(); + let p: DatabaseKeyIndex = p.into(); self.input_outputs.shift_remove(&(EdgeKind::Input, p)); } } diff --git a/src/database.rs b/src/database.rs index 5a32bd9b7..11d4ece53 100644 --- a/src/database.rs +++ b/src/database.rs @@ -45,7 +45,7 @@ pub trait Database: Send + AsDynDatabase + Any + ZalsaDatabase { /// which are the fine-grained components we use to track data. This is intended /// for debugging and the contents of the returned string are not semver-guaranteed. /// - /// Ingredient indices can be extracted from [`DependencyIndex`](`crate::DependencyIndex`) values. + /// Ingredient indices can be extracted from [`DatabaseIndex`](`crate::DatabaseIndex`) values. fn ingredient_debug_name(&self, ingredient_index: IngredientIndex) -> Cow<'_, str> { Cow::Borrowed( self.zalsa() diff --git a/src/event.rs b/src/event.rs index 8eb1ee605..b01f1f953 100644 --- a/src/event.rs +++ b/src/event.rs @@ -1,6 +1,6 @@ use std::thread::ThreadId; -use crate::{key::DatabaseKeyIndex, key::DependencyIndex}; +use crate::key::DatabaseKeyIndex; /// The `Event` struct identifies various notable things that can /// occur during salsa execution. Instances of this struct are given @@ -64,7 +64,7 @@ pub enum EventKind { execute_key: DatabaseKeyIndex, /// Key for the query that is no longer output - output_key: DependencyIndex, + output_key: DatabaseKeyIndex, }, /// Tracked structs or memoized data were discarded (freed). @@ -79,6 +79,6 @@ pub enum EventKind { executor_key: DatabaseKeyIndex, /// Accumulator that was accumulated into - accumulator: DependencyIndex, + accumulator: DatabaseKeyIndex, }, } diff --git a/src/function.rs b/src/function.rs index 07f13d496..fbdbaf54b 100644 --- a/src/function.rs +++ b/src/function.rs @@ -189,15 +189,9 @@ where self.index } - fn maybe_changed_after( - &self, - db: &dyn Database, - input: Option, - revision: Revision, - ) -> bool { - let key = input.unwrap(); + fn maybe_changed_after(&self, db: &dyn Database, input: Id, revision: Revision) -> bool { let db = db.as_view::(); - self.maybe_changed_after(db, key, revision) + self.maybe_changed_after(db, input, revision) } fn cycle_recovery_strategy(&self) -> CycleRecoveryStrategy { @@ -208,13 +202,7 @@ where self.origin(db.zalsa(), key) } - fn mark_validated_output( - &self, - db: &dyn Database, - executor: DatabaseKeyIndex, - output_key: Option, - ) { - let output_key = output_key.unwrap(); + fn mark_validated_output(&self, db: &dyn Database, executor: DatabaseKeyIndex, output_key: Id) { self.validate_specified_value(db, executor, output_key); } @@ -222,7 +210,7 @@ where &self, _db: &dyn Database, _executor: DatabaseKeyIndex, - _stale_output_key: Option, + _stale_output_key: Id, ) { // This function is invoked when a query Q specifies the value for `stale_output_key` in rev 1, // but not in rev 2. We don't do anything in this case, we just leave the (now stale) memo. @@ -237,7 +225,7 @@ where std::mem::take(&mut self.deleted_entries); } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(C::DEBUG_NAME, index, fmt) } diff --git a/src/function/diff_outputs.rs b/src/function/diff_outputs.rs index d89f0d52f..b4281b73a 100644 --- a/src/function/diff_outputs.rs +++ b/src/function/diff_outputs.rs @@ -1,7 +1,7 @@ use super::{memo::Memo, Configuration, IngredientImpl}; use crate::{ - hash::FxHashSet, key::DependencyIndex, zalsa_local::QueryRevisions, AsDynDatabase as _, - DatabaseKeyIndex, Event, EventKind, + hash::FxHashSet, zalsa_local::QueryRevisions, AsDynDatabase as _, DatabaseKeyIndex, Event, + EventKind, }; impl IngredientImpl @@ -33,9 +33,9 @@ where // Remove the outputs that are no longer present in the current revision // to prevent that the next revision is seeded with a id mapping that no longer exists. revisions.tracked_struct_ids.retain(|k, value| { - !old_outputs.contains(&DependencyIndex { + !old_outputs.contains(&DatabaseKeyIndex { ingredient_index: k.ingredient_index(), - key_index: Some(*value), + key_index: *value, }) }); } @@ -45,7 +45,7 @@ where } } - fn report_stale_output(db: &C::DbView, key: DatabaseKeyIndex, output: DependencyIndex) { + fn report_stale_output(db: &C::DbView, key: DatabaseKeyIndex, output: DatabaseKeyIndex) { let db = db.as_dyn_database(); db.salsa_event(&|| Event { diff --git a/src/ingredient.rs b/src/ingredient.rs index 383fdc6b7..2ab7d900d 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -36,7 +36,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { fn maybe_changed_after<'db>( &'db self, db: &'db dyn Database, - input: Option, + input: Id, revision: Revision, ) -> bool; @@ -60,7 +60,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { &'db self, db: &'db dyn Database, executor: DatabaseKeyIndex, - output_key: Option, + output_key: Id, ); /// Invoked when the value `stale_output` was output by `executor` in a previous @@ -71,7 +71,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { &self, db: &dyn Database, executor: DatabaseKeyIndex, - stale_output_key: Option, + stale_output_key: Id, ); /// Returns the [`IngredientIndex`] of this ingredient. @@ -98,7 +98,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { /// [`IngredientRequiresReset::RESET_ON_NEW_REVISION`] to true. fn reset_for_new_revision(&mut self); - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result; + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result; } impl dyn Ingredient { @@ -138,14 +138,6 @@ impl dyn Ingredient { } /// A helper function to show human readable fmt. -pub(crate) fn fmt_index( - debug_name: &str, - id: Option, - fmt: &mut fmt::Formatter<'_>, -) -> fmt::Result { - if let Some(i) = id { - write!(fmt, "{debug_name}({i:?})") - } else { - write!(fmt, "{debug_name}()") - } +pub(crate) fn fmt_index(debug_name: &str, id: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(fmt, "{debug_name}({id:?})") } diff --git a/src/input.rs b/src/input.rs index fdad27aca..01821c8d1 100644 --- a/src/input.rs +++ b/src/input.rs @@ -11,7 +11,7 @@ use crate::{ cycle::CycleRecoveryStrategy, id::{AsId, FromId}, ingredient::{fmt_index, Ingredient}, - key::{DatabaseKeyIndex, DependencyIndex}, + key::DatabaseKeyIndex, plumbing::{Jar, JarAux, Stamp}, table::{memo::MemoTable, sync::SyncTable, Slot, Table}, zalsa::{IngredientIndex, Zalsa}, @@ -182,9 +182,9 @@ impl IngredientImpl { let value = Self::data(zalsa, id); let stamp = &value.stamps[field_index]; zalsa_local.report_tracked_read( - DependencyIndex { + DatabaseKeyIndex { ingredient_index: field_ingredient_index, - key_index: Some(id), + key_index: id, }, stamp.durability, stamp.changed_at, @@ -207,12 +207,7 @@ impl Ingredient for IngredientImpl { self.ingredient_index } - fn maybe_changed_after( - &self, - _db: &dyn Database, - _input: Option, - _revision: Revision, - ) -> bool { + fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool { // Input ingredients are just a counter, they store no data, they are immortal. // Their *fields* are stored in function ingredients elsewhere. false @@ -230,7 +225,7 @@ impl Ingredient for IngredientImpl { &self, _db: &dyn Database, executor: DatabaseKeyIndex, - output_key: Option, + output_key: Id, ) { unreachable!( "mark_validated_output({:?}, {:?}): input cannot be the output of a tracked function", @@ -242,7 +237,7 @@ impl Ingredient for IngredientImpl { &self, _db: &dyn Database, executor: DatabaseKeyIndex, - stale_output_key: Option, + stale_output_key: Id, ) { unreachable!( "remove_stale_output({:?}, {:?}): input cannot be the output of a tracked function", @@ -258,7 +253,7 @@ impl Ingredient for IngredientImpl { panic!("unexpected call to `reset_for_new_revision`") } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(C::DEBUG_NAME, index, fmt) } diff --git a/src/input/input_field.rs b/src/input/input_field.rs index fd3082256..71a1a16ed 100644 --- a/src/input/input_field.rs +++ b/src/input/input_field.rs @@ -49,14 +49,8 @@ where CycleRecoveryStrategy::Panic } - fn maybe_changed_after( - &self, - db: &dyn Database, - input: Option, - revision: Revision, - ) -> bool { + fn maybe_changed_after(&self, db: &dyn Database, input: Id, revision: Revision) -> bool { let zalsa = db.zalsa(); - let input = input.unwrap(); let value = >::data(zalsa, input); value.stamps[self.field_index].changed_at > revision } @@ -69,7 +63,7 @@ where &self, _db: &dyn Database, _executor: DatabaseKeyIndex, - _output_key: Option, + _output_key: Id, ) { } @@ -77,7 +71,7 @@ where &self, _db: &dyn Database, _executor: DatabaseKeyIndex, - _stale_output_key: Option, + _stale_output_key: Id, ) { } @@ -89,7 +83,7 @@ where panic!("unexpected call: input fields don't register for resets"); } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(C::FIELD_DEBUG_NAMES[self.field_index], index, fmt) } diff --git a/src/interned.rs b/src/interned.rs index 0c6d32cde..e64f9ee50 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,7 +1,6 @@ use crate::durability::Durability; use crate::id::AsId; use crate::ingredient::fmt_index; -use crate::key::DependencyIndex; use crate::plumbing::{Jar, JarAux}; use crate::table::memo::MemoTable; use crate::table::sync::SyncTable; @@ -58,12 +57,6 @@ pub struct IngredientImpl { /// /// Deadlock requirement: We access `value_map` while holding lock on `key_map`, but not vice versa. key_map: FxDashMap, Id>, - - /// Stores the revision when this interned ingredient was last cleared. - /// You can clear an interned table at any point, deleting all its entries, - /// but that will make anything dependent on those entries dirty and in need - /// of being recomputed. - reset_at: Revision, } /// Struct storing the interned fields. @@ -102,7 +95,6 @@ where Self { ingredient_index, key_map: Default::default(), - reset_at: Revision::start(), } } @@ -129,11 +121,6 @@ where data: impl Lookup>, ) -> C::Struct<'db> { let zalsa_local = db.zalsa_local(); - zalsa_local.report_tracked_read( - DependencyIndex::for_table(self.ingredient_index), - Durability::MAX, - self.reset_at, - ); // Optimisation to only get read lock on the map if the data has already // been interned. @@ -153,7 +140,13 @@ where Lookup::eq(&data, a) }) { // SAFETY: Read lock on map is held during this block - return C::struct_from_id(unsafe { *bucket.as_ref().1.get() }); + let id = unsafe { *bucket.as_ref().1.get() }; + + // Record a dependency on this value. + let index = self.database_key_index(id); + zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + + return C::struct_from_id(id); } }; @@ -166,6 +159,11 @@ where dashmap::mapref::entry::Entry::Occupied(entry) => { let id = *entry.get(); drop(entry); + + // Record a dependency on this value. + let index = self.database_key_index(id); + zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + C::struct_from_id(id) } @@ -179,11 +177,24 @@ where syncs: Default::default(), }); entry.insert(next_id); + + // Record a dependency on this value. + let index = self.database_key_index(next_id); + zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + C::struct_from_id(next_id) } } } + /// Returns the database key index for an interned value with the given id. + pub fn database_key_index(&self, id: Id) -> DatabaseKeyIndex { + DatabaseKeyIndex { + ingredient_index: self.ingredient_index, + key_index: id, + } + } + /// Lookup the data for an interned value based on its id. /// Rarely used since end-users generally carry a struct with a pointer directly /// to the interned item. @@ -197,12 +208,6 @@ where pub fn fields<'db>(&'db self, db: &'db dyn Database, s: C::Struct<'db>) -> &'db C::Data<'db> { self.data(db, C::deref_struct(s)) } - - pub fn reset(&mut self, revision: Revision) { - assert!(revision > self.reset_at); - self.reset_at = revision; - self.key_map.clear(); - } } impl Ingredient for IngredientImpl @@ -213,13 +218,9 @@ where self.ingredient_index } - fn maybe_changed_after( - &self, - _db: &dyn Database, - _input: Option, - revision: Revision, - ) -> bool { - revision < self.reset_at + fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool { + // Interned data currently never changes. + false } fn cycle_recovery_strategy(&self) -> crate::cycle::CycleRecoveryStrategy { @@ -234,7 +235,7 @@ where &self, _db: &dyn Database, executor: DatabaseKeyIndex, - output_key: Option, + output_key: Id, ) { unreachable!( "mark_validated_output({:?}, {:?}): input cannot be the output of a tracked function", @@ -246,7 +247,7 @@ where &self, _db: &dyn Database, executor: DatabaseKeyIndex, - stale_output_key: Option, + stale_output_key: Id, ) { unreachable!( "remove_stale_output({:?}, {:?}): interned ids are not outputs", @@ -265,7 +266,7 @@ where panic!("unexpected call to `reset_for_new_revision`") } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(C::DEBUG_NAME, index, fmt) } diff --git a/src/key.rs b/src/key.rs index 92e63541d..415448b6c 100644 --- a/src/key.rs +++ b/src/key.rs @@ -3,37 +3,36 @@ use crate::{ zalsa::IngredientIndex, Database, Id, }; -/// An integer that uniquely identifies a particular query instance within the -/// database. Used to track dependencies between queries. Fully ordered and -/// equatable but those orderings are arbitrary, and meant to be used only for -/// inserting into maps and the like. +// ANCHOR: DatabaseKeyIndex +/// An "active" database key index represents a database key index +/// that is actively executing. In that case, the `key_index` cannot be +/// None. #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct DependencyIndex { +pub struct DatabaseKeyIndex { pub(crate) ingredient_index: IngredientIndex, - pub(crate) key_index: Option, + pub(crate) key_index: Id, } +// ANCHOR_END: DatabaseKeyIndex -impl DependencyIndex { - /// Create a database-key-index for an interning or entity table. - /// The `key_index` here is always zero, which deliberately corresponds to - /// no particular id or entry. This is because the data in such tables - /// remains valid until the table as a whole is reset. Using a single id avoids - /// creating tons of dependencies in the dependency listings. - pub(crate) fn for_table(ingredient_index: IngredientIndex) -> Self { - Self { - ingredient_index, - key_index: None, - } - } - +impl DatabaseKeyIndex { pub fn ingredient_index(self) -> IngredientIndex { self.ingredient_index } - pub fn key_index(self) -> Option { + pub fn key_index(self) -> Id { self.key_index } + pub(crate) fn cycle_recovery_strategy(self, db: &dyn Database) -> CycleRecoveryStrategy { + self.ingredient_index.cycle_recovery_strategy(db) + } + + pub(crate) fn accumulated(self, db: &dyn Database) -> Option<&AccumulatedMap> { + db.zalsa() + .lookup_ingredient(self.ingredient_index) + .accumulated(db, self.key_index) + } + pub(crate) fn remove_stale_output(&self, db: &dyn Database, executor: DatabaseKeyIndex) { db.zalsa() .lookup_ingredient(self.ingredient_index) @@ -61,76 +60,17 @@ impl DependencyIndex { } } -impl std::fmt::Debug for DependencyIndex { +impl std::fmt::Debug for DatabaseKeyIndex { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { crate::attach::with_attached_database(|db| { let ingredient = db.zalsa().lookup_ingredient(self.ingredient_index); ingredient.fmt_index(self.key_index, f) }) .unwrap_or_else(|| { - f.debug_tuple("DependencyIndex") + f.debug_tuple("DatabaseKeyIndex") .field(&self.ingredient_index) .field(&self.key_index) .finish() }) } } - -// ANCHOR: DatabaseKeyIndex -/// An "active" database key index represents a database key index -/// that is actively executing. In that case, the `key_index` cannot be -/// None. -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct DatabaseKeyIndex { - pub(crate) ingredient_index: IngredientIndex, - pub(crate) key_index: Id, -} -// ANCHOR_END: DatabaseKeyIndex - -impl DatabaseKeyIndex { - pub fn ingredient_index(self) -> IngredientIndex { - self.ingredient_index - } - - pub fn key_index(self) -> Id { - self.key_index - } - - pub(crate) fn cycle_recovery_strategy(self, db: &dyn Database) -> CycleRecoveryStrategy { - self.ingredient_index.cycle_recovery_strategy(db) - } - - pub(crate) fn accumulated(self, db: &dyn Database) -> Option<&AccumulatedMap> { - db.zalsa() - .lookup_ingredient(self.ingredient_index) - .accumulated(db, self.key_index) - } -} - -impl std::fmt::Debug for DatabaseKeyIndex { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let i: DependencyIndex = (*self).into(); - std::fmt::Debug::fmt(&i, f) - } -} - -impl From for DependencyIndex { - fn from(value: DatabaseKeyIndex) -> Self { - Self { - ingredient_index: value.ingredient_index, - key_index: Some(value.key_index), - } - } -} - -impl TryFrom for DatabaseKeyIndex { - type Error = (); - - fn try_from(value: DependencyIndex) -> Result { - let key_index = value.key_index.ok_or(())?; - Ok(Self { - ingredient_index: value.ingredient_index, - key_index, - }) - } -} diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index 540bb7652..b0276e0df 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -6,7 +6,7 @@ use tracked_field::FieldIngredientImpl; use crate::{ cycle::CycleRecoveryStrategy, ingredient::{fmt_index, Ingredient, Jar, JarAux}, - key::{DatabaseKeyIndex, DependencyIndex}, + key::DatabaseKeyIndex, plumbing::ZalsaLocal, runtime::StampedValue, salsa_struct::SalsaStructInDb, @@ -555,9 +555,9 @@ where let field_changed_at = data.revisions[field_index]; zalsa_local.report_tracked_read( - DependencyIndex { + DatabaseKeyIndex { ingredient_index: field_ingredient_index, - key_index: Some(id), + key_index: id, }, data.durability, field_changed_at, @@ -575,12 +575,7 @@ where self.ingredient_index } - fn maybe_changed_after( - &self, - _db: &dyn Database, - _input: Option, - _revision: Revision, - ) -> bool { + fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool { false } @@ -596,7 +591,7 @@ where &'db self, _db: &'db dyn Database, _executor: DatabaseKeyIndex, - _output_key: Option, + _output_key: Id, ) { // we used to update `update_at` field but now we do it lazilly when data is accessed // @@ -607,16 +602,16 @@ where &self, db: &dyn Database, _executor: DatabaseKeyIndex, - stale_output_key: Option, + stale_output_key: Id, ) { // This method is called when, in prior revisions, // `executor` creates a tracked struct `salsa_output_key`, // but it did not in the current revision. // In that case, we can delete `stale_output_key` and any data associated with it. - self.delete_entity(db.as_dyn_database(), stale_output_key.unwrap()); + self.delete_entity(db.as_dyn_database(), stale_output_key); } - fn fmt_index(&self, index: Option, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { fmt_index(C::DEBUG_NAME, index, fmt) } diff --git a/src/tracked_struct/tracked_field.rs b/src/tracked_struct/tracked_field.rs index ff1909397..a513fd2de 100644 --- a/src/tracked_struct/tracked_field.rs +++ b/src/tracked_struct/tracked_field.rs @@ -51,12 +51,11 @@ where fn maybe_changed_after<'db>( &'db self, db: &'db dyn Database, - input: Option, + input: Id, revision: crate::Revision, ) -> bool { let zalsa = db.zalsa(); - let id = input.unwrap(); - let data = >::data(zalsa.table(), id); + let data = >::data(zalsa.table(), input); let field_changed_at = data.revisions[self.field_index]; field_changed_at > revision } @@ -73,7 +72,7 @@ where &self, _db: &dyn Database, _executor: crate::DatabaseKeyIndex, - _output_key: Option, + _output_key: Id, ) { panic!("tracked field ingredients have no outputs") } @@ -82,7 +81,7 @@ where &self, _db: &dyn Database, _executor: crate::DatabaseKeyIndex, - _stale_output_key: Option, + _stale_output_key: Id, ) { panic!("tracked field ingredients have no outputs") } @@ -95,17 +94,13 @@ where panic!("tracked field ingredients do not require reset") } - fn fmt_index( - &self, - index: Option, - fmt: &mut std::fmt::Formatter<'_>, - ) -> std::fmt::Result { + fn fmt_index(&self, index: Id, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( fmt, "{}.{}({:?})", C::DEBUG_NAME, C::FIELD_DEBUG_NAMES[self.field_index], - index.unwrap() + index ) } diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index 9076ffa96..25c90f974 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -5,7 +5,6 @@ use crate::accumulator::accumulated_map::AccumulatedMap; use crate::active_query::ActiveQuery; use crate::durability::Durability; use crate::key::DatabaseKeyIndex; -use crate::key::DependencyIndex; use crate::runtime::StampedValue; use crate::table::PageIndex; use crate::table::Slot; @@ -145,7 +144,7 @@ impl ZalsaLocal { } /// Add an output to the current query's list of dependencies - pub(crate) fn add_output(&self, entity: DependencyIndex) { + pub(crate) fn add_output(&self, entity: DatabaseKeyIndex) { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { top_query.add_output(entity) @@ -154,7 +153,7 @@ impl ZalsaLocal { } /// Check whether `entity` is an output of the currently active query (if any) - pub(crate) fn is_output_of_active_query(&self, entity: DependencyIndex) -> bool { + pub(crate) fn is_output_of_active_query(&self, entity: DatabaseKeyIndex) -> bool { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { top_query.is_output(entity) @@ -167,7 +166,7 @@ impl ZalsaLocal { /// Register that currently active query reads the given input pub(crate) fn report_tracked_read( &self, - input: DependencyIndex, + input: DatabaseKeyIndex, durability: Durability, changed_at: Revision, ) { @@ -432,7 +431,7 @@ pub enum QueryOrigin { impl QueryOrigin { /// Indices for queries *read* by this query - pub(crate) fn inputs(&self) -> impl DoubleEndedIterator + '_ { + pub(crate) fn inputs(&self) -> impl DoubleEndedIterator + '_ { let opt_edges = match self { QueryOrigin::Derived(edges) | QueryOrigin::DerivedUntracked(edges) => Some(edges), QueryOrigin::Assigned(_) | QueryOrigin::BaseInput => None, @@ -441,7 +440,7 @@ impl QueryOrigin { } /// Indices for queries *written* by this query (if any) - pub(crate) fn outputs(&self) -> impl DoubleEndedIterator + '_ { + pub(crate) fn outputs(&self) -> impl DoubleEndedIterator + '_ { let opt_edges = match self { QueryOrigin::Derived(edges) | QueryOrigin::DerivedUntracked(edges) => Some(edges), QueryOrigin::Assigned(_) | QueryOrigin::BaseInput => None, @@ -457,7 +456,7 @@ pub enum EdgeKind { } lazy_static::lazy_static! { - pub(crate) static ref EMPTY_DEPENDENCIES: Arc<[(EdgeKind, DependencyIndex)]> = Arc::new([]); + pub(crate) static ref EMPTY_DEPENDENCIES: Arc<[(EdgeKind, DatabaseKeyIndex)]> = Arc::new([]); } /// The edges between a memoized value and other queries in the dependency graph. @@ -479,14 +478,14 @@ pub struct QueryEdges { /// Important: /// /// * The inputs must be in **execution order** for the red-green algorithm to work. - pub input_outputs: Arc<[(EdgeKind, DependencyIndex)]>, + pub input_outputs: Arc<[(EdgeKind, DatabaseKeyIndex)]>, } impl QueryEdges { /// Returns the (tracked) inputs that were executed in computing this memoized value. /// /// These will always be in execution order. - pub(crate) fn inputs(&self) -> impl DoubleEndedIterator + '_ { + pub(crate) fn inputs(&self) -> impl DoubleEndedIterator + '_ { self.input_outputs .iter() .filter(|(edge_kind, _)| *edge_kind == EdgeKind::Input) @@ -496,7 +495,7 @@ impl QueryEdges { /// Returns the (tracked) outputs that were executed in computing this memoized value. /// /// These will always be in execution order. - pub(crate) fn outputs(&self) -> impl DoubleEndedIterator + '_ { + pub(crate) fn outputs(&self) -> impl DoubleEndedIterator + '_ { self.input_outputs .iter() .filter(|(edge_kind, _)| *edge_kind == EdgeKind::Output) @@ -504,7 +503,7 @@ impl QueryEdges { } /// Creates a new `QueryEdges`; the values given for each field must meet struct invariants. - pub(crate) fn new(input_outputs: Arc<[(EdgeKind, DependencyIndex)]>) -> Self { + pub(crate) fn new(input_outputs: Arc<[(EdgeKind, DatabaseKeyIndex)]>) -> Self { Self { input_outputs } } } From db71a89137c84e4f9755850e0edd83dbc07926fa Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Wed, 23 Oct 2024 16:52:58 -0400 Subject: [PATCH 2/3] add plumbing to reuse interned slots --- src/active_query.rs | 2 +- src/function/fetch.rs | 2 +- src/function/specify.rs | 4 ++-- src/interned.rs | 52 ++++++++++++++++++++++++++++++++++------- src/revision.rs | 8 +++++++ src/tracked_struct.rs | 4 ++-- 6 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/active_query.rs b/src/active_query.rs index ef9778d5e..ebdff8a2c 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -142,7 +142,7 @@ impl ActiveQuery { /// Used during cycle recovery, see [`Runtime::unblock_cycle_and_maybe_throw`]. pub(super) fn remove_cycle_participants(&mut self, cycle: &Cycle) { for p in cycle.participant_keys() { - let p: DatabaseKeyIndex = p.into(); + let p: DatabaseKeyIndex = p; self.input_outputs.shift_remove(&(EdgeKind::Input, p)); } } diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 07a08d96c..b2161cbae 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -21,7 +21,7 @@ where self.evict_value_from_memo_for(zalsa, evicted); } - zalsa_local.report_tracked_read(self.database_key_index(id).into(), durability, changed_at); + zalsa_local.report_tracked_read(self.database_key_index(id), durability, changed_at); value } diff --git a/src/function/specify.rs b/src/function/specify.rs index 9eccad65b..46e70c353 100644 --- a/src/function/specify.rs +++ b/src/function/specify.rs @@ -39,7 +39,7 @@ where // // Now, if We invoke Q3 first, We get one result for Q2, but if We invoke Q4 first, We get a different value. That's no good. let database_key_index = >::database_key_index(db.as_dyn_database(), key); - let dependency_index = database_key_index.into(); + let dependency_index = database_key_index; if !zalsa_local.is_output_of_active_query(dependency_index) { panic!("can only use `specify` on salsa structs created during the current tracked fn"); } @@ -92,7 +92,7 @@ where // Record that the current query *specified* a value for this cell. let database_key_index = self.database_key_index(key); - zalsa_local.add_output(database_key_index.into()); + zalsa_local.add_output(database_key_index); } /// Invoked when the query `executor` has been validated as having green inputs diff --git a/src/interned.rs b/src/interned.rs index e64f9ee50..0477e97f4 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -2,6 +2,7 @@ use crate::durability::Durability; use crate::id::AsId; use crate::ingredient::fmt_index; use crate::plumbing::{Jar, JarAux}; +use crate::revision::AtomicRevision; use crate::table::memo::MemoTable; use crate::table::sync::SyncTable; use crate::table::Slot; @@ -67,6 +68,10 @@ where data: C::Data<'static>, memos: MemoTable, syncs: SyncTable, + // The revision the value was first interned in. + first_interned_at: Revision, + // The most recent interned revision. + last_interned_at: AtomicRevision, } impl Default for JarImpl { @@ -120,7 +125,8 @@ where db: &'db dyn crate::Database, data: impl Lookup>, ) -> C::Struct<'db> { - let zalsa_local = db.zalsa_local(); + let (zalsa, zalsa_local) = db.zalsas(); + let current_revision = zalsa.current_revision(); // Optimisation to only get read lock on the map if the data has already // been interned. @@ -142,9 +148,13 @@ where // SAFETY: Read lock on map is held during this block let id = unsafe { *bucket.as_ref().1.get() }; + // Sync the value's revision. + let value = zalsa.table().get::>(id); + value.last_interned_at.store(current_revision); + // Record a dependency on this value. let index = self.database_key_index(id); - zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + zalsa_local.report_tracked_read(index, Durability::MAX, value.first_interned_at); return C::struct_from_id(id); } @@ -160,9 +170,13 @@ where let id = *entry.get(); drop(entry); + // Sync the value's revision. + let value = zalsa.table().get::>(id); + value.last_interned_at.store(current_revision); + // Record a dependency on this value. let index = self.database_key_index(id); - zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + zalsa_local.report_tracked_read(index, Durability::MAX, value.first_interned_at); C::struct_from_id(id) } @@ -175,12 +189,14 @@ where data: internal_data, memos: Default::default(), syncs: Default::default(), + first_interned_at: current_revision, + last_interned_at: AtomicRevision::from(current_revision), }); entry.insert(next_id); // Record a dependency on this value. let index = self.database_key_index(next_id); - zalsa_local.report_tracked_read(index, Durability::MAX, Revision::start()); + zalsa_local.report_tracked_read(index, Durability::MAX, current_revision); C::struct_from_id(next_id) } @@ -199,8 +215,12 @@ where /// Rarely used since end-users generally carry a struct with a pointer directly /// to the interned item. pub fn data<'db>(&'db self, db: &'db dyn Database, id: Id) -> &'db C::Data<'db> { - let internal_data = db.zalsa().table().get::>(id); - unsafe { Self::from_internal_data(&internal_data.data) } + let value = db.zalsa().table().get::>(id); + assert!( + value.last_interned_at.load() >= db.zalsa().current_revision(), + "Data was not interned in the current revision." + ); + unsafe { Self::from_internal_data(&value.data) } } /// Lookup the fields from an interned struct. @@ -208,6 +228,12 @@ where pub fn fields<'db>(&'db self, db: &'db dyn Database, s: C::Struct<'db>) -> &'db C::Data<'db> { self.data(db, C::deref_struct(s)) } + + pub fn reset(&mut self, db: &mut dyn Database) { + // Trigger a new revision. + let _zalsa_mut = db.zalsa_mut(); + self.key_map.clear(); + } } impl Ingredient for IngredientImpl @@ -218,8 +244,18 @@ where self.ingredient_index } - fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool { - // Interned data currently never changes. + fn maybe_changed_after(&self, db: &dyn Database, input: Id, revision: Revision) -> bool { + let value = db.zalsa().table().get::>(input); + if value.first_interned_at > revision { + // The slot was reused. + return true; + } + + if value.first_interned_at < revision { + // The slot is valid in this revision but we have to sync the value's revision. + value.last_interned_at.store(db.zalsa().current_revision()); + } + false } diff --git a/src/revision.rs b/src/revision.rs index a74456102..0c62c6216 100644 --- a/src/revision.rs +++ b/src/revision.rs @@ -62,3 +62,11 @@ impl AtomicRevision { self.data.store(r.as_usize(), Ordering::SeqCst); } } + +impl From for AtomicRevision { + fn from(value: Revision) -> Self { + Self { + data: AtomicUsize::from(value.as_usize()), + } + } +} diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index b0276e0df..6e92a94e8 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -291,7 +291,7 @@ where match zalsa_local.tracked_struct_id(&identity) { Some(id) => { // The struct already exists in the intern map. - zalsa_local.add_output(self.database_key_index(id).into()); + zalsa_local.add_output(self.database_key_index(id)); self.update(zalsa, current_revision, id, ¤t_deps, fields); C::struct_from_id(id) } @@ -300,7 +300,7 @@ where // This is a new tracked struct, so create an entry in the struct map. let id = self.allocate(zalsa, zalsa_local, current_revision, ¤t_deps, fields); let key = self.database_key_index(id); - zalsa_local.add_output(key.into()); + zalsa_local.add_output(key); zalsa_local.store_tracked_struct_id(identity, id); C::struct_from_id(id) } From c6077c47184294ad8e4e69493836077cdb42d89e Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Mon, 4 Nov 2024 02:48:55 -0500 Subject: [PATCH 3/3] nits --- src/function/specify.rs | 3 +-- src/interned.rs | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/function/specify.rs b/src/function/specify.rs index 46e70c353..702fa2b0f 100644 --- a/src/function/specify.rs +++ b/src/function/specify.rs @@ -39,8 +39,7 @@ where // // Now, if We invoke Q3 first, We get one result for Q2, but if We invoke Q4 first, We get a different value. That's no good. let database_key_index = >::database_key_index(db.as_dyn_database(), key); - let dependency_index = database_key_index; - if !zalsa_local.is_output_of_active_query(dependency_index) { + if !zalsa_local.is_output_of_active_query(database_key_index) { panic!("can only use `specify` on salsa structs created during the current tracked fn"); } diff --git a/src/interned.rs b/src/interned.rs index 0477e97f4..766da3b7c 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -68,9 +68,9 @@ where data: C::Data<'static>, memos: MemoTable, syncs: SyncTable, - // The revision the value was first interned in. + /// The revision the value was first interned in. first_interned_at: Revision, - // The most recent interned revision. + /// The most recent interned revision. last_interned_at: AtomicRevision, }