Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix accumulated values for backdated queries #662

Merged
merged 2 commits into from
Jan 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use accumulated::AnyAccumulated;

use crate::{
cycle::CycleRecoveryStrategy,
ingredient::{fmt_index, Ingredient, Jar},
ingredient::{fmt_index, Ingredient, Jar, MaybeChangedAfter},
plumbing::JarAux,
zalsa::IngredientIndex,
zalsa_local::QueryOrigin,
Expand Down Expand Up @@ -100,7 +100,12 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
self.index
}

fn maybe_changed_after(&self, _db: &dyn Database, _input: Id, _revision: Revision) -> bool {
fn maybe_changed_after(
&self,
_db: &dyn Database,
_input: Id,
_revision: Revision,
) -> MaybeChangedAfter {
panic!("nothing should ever depend on an accumulator directly")
}

Expand Down
19 changes: 14 additions & 5 deletions src/accumulator/accumulated_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,28 @@ pub enum InputAccumulatedValues {
}

impl InputAccumulatedValues {
pub(crate) const fn is_any(self) -> bool {
pub const fn is_any(self) -> bool {
matches!(self, Self::Any)
}

pub(crate) const fn is_empty(self) -> bool {
pub const fn is_empty(self) -> bool {
matches!(self, Self::Empty)
}
}

impl ops::BitOr for InputAccumulatedValues {
type Output = Self;

fn bitor(self, rhs: Self) -> Self::Output {
match (self, rhs) {
(Self::Any, _) | (_, Self::Any) => Self::Any,
(Self::Empty, Self::Empty) => Self::Empty,
}
}
}

impl ops::BitOrAssign for InputAccumulatedValues {
fn bitor_assign(&mut self, rhs: Self) {
if rhs.is_any() {
*self = Self::Any;
}
*self = *self | rhs;
}
}
7 changes: 3 additions & 4 deletions src/active_query.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::ops::Not;

use crossbeam::atomic::AtomicCell;

use super::zalsa_local::{QueryEdges, QueryOrigin, QueryRevisions};
use crate::key::OutputDependencyIndex;
use crate::tracked_struct::{DisambiguatorMap, IdentityHash, IdentityMap};
Expand Down Expand Up @@ -129,10 +131,7 @@ impl ActiveQuery {
origin,
durability: self.durability,
tracked_struct_ids: self.tracked_struct_ids,
accumulated_inputs: match &accumulated {
Some(_) => InputAccumulatedValues::Any,
None => self.accumulated_inputs,
},
accumulated_inputs: AtomicCell::new(self.accumulated_inputs),
accumulated,
}
}
Expand Down
9 changes: 7 additions & 2 deletions src/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{any::Any, fmt, sync::Arc};
use crate::{
accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues},
cycle::CycleRecoveryStrategy,
ingredient::fmt_index,
ingredient::{fmt_index, MaybeChangedAfter},
key::DatabaseKeyIndex,
plumbing::JarAux,
salsa_struct::SalsaStructInDb,
Expand Down Expand Up @@ -189,7 +189,12 @@ where
self.index
}

fn maybe_changed_after(&self, db: &dyn Database, input: Id, revision: Revision) -> bool {
fn maybe_changed_after(
&self,
db: &dyn Database,
input: Id,
revision: Revision,
) -> MaybeChangedAfter {
let db = db.as_view::<C::DbView>();
self.maybe_changed_after(db, input, revision)
}
Expand Down
2 changes: 1 addition & 1 deletion src/function/accumulated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ where
let memo = self.refresh_memo(db, key);
(
memo.revisions.accumulated.as_deref(),
memo.revisions.accumulated_inputs,
memo.revisions.accumulated_inputs.load(),
)
}
}
10 changes: 8 additions & 2 deletions src/function/fetch.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use super::{memo::Memo, Configuration, IngredientImpl};
use crate::{runtime::StampedValue, zalsa::ZalsaDatabase, AsDynDatabase as _, Id};
use crate::{
accumulator::accumulated_map::InputAccumulatedValues, runtime::StampedValue,
zalsa::ZalsaDatabase, AsDynDatabase as _, Id,
};

impl<C> IngredientImpl<C>
where
Expand All @@ -24,7 +27,10 @@ where
self.database_key_index(id).into(),
durability,
changed_at,
memo.revisions.accumulated_inputs,
match &memo.revisions.accumulated {
Some(_) => InputAccumulatedValues::Any,
None => memo.revisions.accumulated_inputs.load(),
},
);

value
Expand Down
57 changes: 44 additions & 13 deletions src/function/maybe_changed_after.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::{
accumulator::accumulated_map::InputAccumulatedValues,
ingredient::MaybeChangedAfter,
key::DatabaseKeyIndex,
zalsa::{Zalsa, ZalsaDatabase},
zalsa_local::{ActiveQueryGuard, QueryEdge, QueryOrigin},
Expand All @@ -16,7 +18,7 @@ where
db: &'db C::DbView,
id: Id,
revision: Revision,
) -> bool {
) -> MaybeChangedAfter {
let (zalsa, zalsa_local) = db.zalsas();
zalsa_local.unwind_if_revision_cancelled(db.as_dyn_database());

Expand All @@ -29,7 +31,11 @@ where
let memo_guard = self.get_memo_from_table_for(zalsa, id);
if let Some(memo) = &memo_guard {
if self.shallow_verify_memo(db, zalsa, database_key_index, memo) {
return memo.revisions.changed_at > revision;
return if memo.revisions.changed_at > revision {
MaybeChangedAfter::Yes
} else {
MaybeChangedAfter::No(memo.revisions.accumulated_inputs.load())
};
}
drop(memo_guard); // release the arc-swap guard before cold path
if let Some(mcs) = self.maybe_changed_after_cold(db, id, revision) {
Expand All @@ -39,7 +45,7 @@ where
}
} else {
// No memo? Assume has changed.
return true;
return MaybeChangedAfter::Yes;
}
}
}
Expand All @@ -49,7 +55,7 @@ where
db: &'db C::DbView,
key_index: Id,
revision: Revision,
) -> Option<bool> {
) -> Option<MaybeChangedAfter> {
let (zalsa, zalsa_local) = db.zalsas();
let database_key_index = self.database_key_index(key_index);

Expand All @@ -63,7 +69,7 @@ where

// Load the current memo, if any.
let Some(old_memo) = self.get_memo_from_table_for(zalsa, key_index) else {
return Some(true);
return Some(MaybeChangedAfter::Yes);
};

tracing::debug!(
Expand All @@ -74,7 +80,11 @@ where

// Check if the inputs are still valid and we can just compare `changed_at`.
if self.deep_verify_memo(db, &old_memo, &active_query) {
return Some(old_memo.revisions.changed_at > revision);
return Some(if old_memo.revisions.changed_at > revision {
MaybeChangedAfter::Yes
} else {
MaybeChangedAfter::No(old_memo.revisions.accumulated_inputs.load())
});
}

// If inputs have changed, but we have an old value, we can re-execute.
Expand All @@ -84,11 +94,19 @@ where
if old_memo.value.is_some() {
let memo = self.execute(db, active_query, Some(old_memo));
let changed_at = memo.revisions.changed_at;
return Some(changed_at > revision);

return Some(if changed_at > revision {
MaybeChangedAfter::Yes
} else {
MaybeChangedAfter::No(match &memo.revisions.accumulated {
Some(_) => InputAccumulatedValues::Any,
None => memo.revisions.accumulated_inputs.load(),
})
});
}

// Otherwise, nothing for it: have to consider the value to have changed.
Some(true)
Some(MaybeChangedAfter::Yes)
}

/// True if the memo's value and `changed_at` time is still valid in this revision.
Expand Down Expand Up @@ -117,7 +135,12 @@ where
if memo.check_durability(zalsa) {
// No input of the suitable durability has changed since last verified.
let db = db.as_dyn_database();
memo.mark_as_verified(db, revision_now, database_key_index);
memo.mark_as_verified(
db,
revision_now,
database_key_index,
memo.revisions.accumulated_inputs.load(),
);
memo.mark_outputs_as_verified(db, database_key_index);
return true;
}
Expand Down Expand Up @@ -151,7 +174,7 @@ where
return true;
}

match &old_memo.revisions.origin {
let inputs = match &old_memo.revisions.origin {
QueryOrigin::Assigned(_) => {
// If the value was assigneed by another query,
// and that query were up-to-date,
Expand Down Expand Up @@ -182,13 +205,19 @@ where
// valid, then some later input I1 might never have executed at all, so verifying
// it is still up to date is meaningless.
let last_verified_at = old_memo.verified_at.load();
let mut inputs = InputAccumulatedValues::Empty;
for &edge in edges.input_outputs.iter() {
match edge {
QueryEdge::Input(dependency_index) => {
if dependency_index
match dependency_index
.maybe_changed_after(db.as_dyn_database(), last_verified_at)
{
return false;
MaybeChangedAfter::Yes => {
return false;
}
MaybeChangedAfter::No(input_accumulated) => {
inputs |= input_accumulated;
}
}
}
QueryEdge::Output(dependency_index) => {
Expand All @@ -213,13 +242,15 @@ where
}
}
}
inputs
}
}
};

old_memo.mark_as_verified(
db.as_dyn_database(),
zalsa.current_revision(),
database_key_index,
inputs,
);
true
}
Expand Down
7 changes: 5 additions & 2 deletions src/function/memo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::sync::Arc;

use crossbeam::atomic::AtomicCell;

use crate::accumulator::accumulated_map::InputAccumulatedValues;
use crate::zalsa_local::QueryOrigin;
use crate::{
key::DatabaseKeyIndex, zalsa::Zalsa, zalsa_local::QueryRevisions, Event, EventKind, Id,
Expand Down Expand Up @@ -82,7 +83,7 @@ impl<C: Configuration> IngredientImpl<C> {
ref origin,
ref tracked_struct_ids,
ref accumulated,
accumulated_inputs,
ref accumulated_inputs,
} = &memo.revisions;
// Re-assemble the memo but with the value set to `None`
Arc::new(Memo::new(
Expand All @@ -94,7 +95,7 @@ impl<C: Configuration> IngredientImpl<C> {
origin: origin.clone(),
tracked_struct_ids: tracked_struct_ids.clone(),
accumulated: accumulated.clone(),
accumulated_inputs,
accumulated_inputs: AtomicCell::new(accumulated_inputs.load()),
},
))
}
Expand Down Expand Up @@ -150,6 +151,7 @@ impl<V> Memo<V> {
db: &dyn crate::Database,
revision_now: Revision,
database_key_index: DatabaseKeyIndex,
accumulated: InputAccumulatedValues,
) {
db.salsa_event(&|| {
Event::new(EventKind::DidValidateMemoizedValue {
Expand All @@ -158,6 +160,7 @@ impl<V> Memo<V> {
});

self.verified_at.store(revision_now);
self.revisions.accumulated_inputs.store(accumulated);
}

pub(super) fn mark_outputs_as_verified(
Expand Down
2 changes: 2 additions & 0 deletions src/function/specify.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crossbeam::atomic::AtomicCell;

use crate::{
accumulator::accumulated_map::InputAccumulatedValues,
tracked_struct::TrackedStructInDb,
zalsa::ZalsaDatabase,
zalsa_local::{QueryOrigin, QueryRevisions},
Expand Down Expand Up @@ -128,6 +129,7 @@ where
db.as_dyn_database(),
zalsa.current_revision(),
database_key_index,
InputAccumulatedValues::Empty,
);
}
}
22 changes: 21 additions & 1 deletion src/ingredient.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync {
db: &'db dyn Database,
input: Id,
revision: Revision,
) -> bool;
) -> MaybeChangedAfter;

/// What were the inputs (if any) that were used to create the value at `key_index`.
fn origin(&self, db: &dyn Database, key_index: Id) -> Option<QueryOrigin>;
Expand Down Expand Up @@ -175,3 +175,23 @@ pub(crate) fn fmt_index(
write!(fmt, "{debug_name}()")
}
}

#[derive(Copy, Clone, Debug)]
pub enum MaybeChangedAfter {
/// The query result hasn't changed.
///
/// The inner value tracks whether the memo or any of its dependencies have an accumulated value.
No(InputAccumulatedValues),

/// The query's result has changed since the last revision or the query isn't cached yet.
Yes,
}

impl From<bool> for MaybeChangedAfter {
fn from(value: bool) -> Self {
match value {
true => MaybeChangedAfter::Yes,
false => MaybeChangedAfter::No(InputAccumulatedValues::Empty),
}
}
}
Loading
Loading