Skip to content

perf: Reduce unnecessary conditional work in deep_verify_memo #759

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.

Sign up for GitHub

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

Merged
Veykril merged 2 commits into from
Mar 16, 2025
Merged

perf: Reduce unnecessary conditional work in deep_verify_memo #759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6259475
Reduce unnecessary condituional work in `deep_verify_memo`
Veykril Mar 15, 2025
f7df1d3
Restructure provisional validation a bit
Veykril Mar 15, 2025
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
20 changes: 13 additions & 7 deletions src/function/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ where
// thread completing fixpoint iteration of the cycle, and then we can re-query for
// our no-longer-provisional memo.
if !(memo.may_be_provisional()
&& memo.provisional_retry(db.as_dyn_database(), self.database_key_index(id)))
&& memo.provisional_retry(
db.as_dyn_database(),
zalsa,
self.database_key_index(id),
))
{
return memo;
}
Expand All @@ -80,8 +84,10 @@ where
) -> Option<&'db Memo<C::Output<'db>>> {
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
if let Some(memo) = memo_guard {
let database_key_index = self.database_key_index(id);
if memo.value.is_some()
&& self.shallow_verify_memo(db, zalsa, self.database_key_index(id), memo, false)
&& self.validate_may_be_provisional(db, zalsa, database_key_index, memo)
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
{
// Unsafety invariant: memo is present in memo_map and we have verified that it is
// still valid for the current revision.
Expand Down Expand Up @@ -110,16 +116,16 @@ where
ClaimResult::Retry => return None,
ClaimResult::Cycle => {
// check if there's a provisional value for this query
// Note we don't `validate_may_be_provisional` the memo here as we want to reuse an
// existing provisional memo if it exists
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
if let Some(memo) = &memo_guard {
if let Some(memo) = memo_guard {
if memo.value.is_some()
&& memo.revisions.cycle_heads.contains(&database_key_index)
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo, true)
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
{
// Unsafety invariant: memo is present in memo_map.
unsafe {
return Some(self.extend_memo_lifetime(memo));
}
return unsafe { Some(self.extend_memo_lifetime(memo)) };
}
}
// no provisional value; create/insert/return initial provisional value
Expand Down
200 changes: 107 additions & 93 deletions src/function/maybe_changed_after.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ where
// Check if we have a verified version: this is the hot path.
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
if let Some(memo) = memo_guard {
if self.shallow_verify_memo(db, zalsa, database_key_index, memo, false) {
if self.validate_may_be_provisional(db, zalsa, database_key_index, memo)
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
{
return if memo.revisions.changed_at > revision {
VerifyResult::Changed
} else {
Expand Down Expand Up @@ -173,33 +175,18 @@ where
/// eagerly finalize all provisional memos in cycle iteration, we have to lazily check here
/// (via `validate_provisional`) whether a may-be-provisional memo should actually be verified
/// final, because its cycle heads are all now final.
///
/// If `allow_provisional` is `true`, don't check provisionality and return whatever memo we
/// find that can be verified in this revision, whether provisional or not. This only occurs at
/// one call-site, in `fetch_cold` when we actually encounter a cycle, and want to check if
/// there is an existing provisional memo we can reuse.
#[inline]
pub(super) fn shallow_verify_memo(
&self,
db: &C::DbView,
zalsa: &Zalsa,
database_key_index: DatabaseKeyIndex,
memo: &Memo<C::Output<'_>>,
allow_provisional: bool,
) -> bool {
tracing::debug!(
"{database_key_index:?}: shallow_verify_memo(memo = {memo:#?})",
memo = memo.tracing_debug()
);
if !allow_provisional && memo.may_be_provisional() {
tracing::debug!(
"{database_key_index:?}: validate_provisional(memo = {memo:#?})",
memo = memo.tracing_debug()
);
if !self.validate_provisional(db, zalsa, memo) {
return false;
}
}
let verified_at = memo.verified_at.load();
let revision_now = zalsa.current_revision();

Expand Down Expand Up @@ -231,14 +218,36 @@ where
false
}

/// Validates this memo if it is a provisional memo. Returns true for non provisional memos or
/// if the provisional memo has been successfully marked as verified final, that is, its
/// cycle heads have all been finalized.
#[inline]
pub(super) fn validate_may_be_provisional(
&self,
db: &C::DbView,
zalsa: &Zalsa,
database_key_index: DatabaseKeyIndex,
memo: &Memo<C::Output<'_>>,
) -> bool {
// Wouldn't it be nice if rust had an implication operator ...
// may_be_provisional -> validate_provisional
!memo.may_be_provisional() || self.validate_provisional(db, zalsa, database_key_index, memo)
Copy link
Contributor

@carljm carljm Mar 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be just as good to not add this additional wrapper, and instead just check memo.may_be_provisional inside validate_provisional? This is the only call site of validate_provisional.

Copy link
Member Author

@Veykril Veykril Mar 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to regress perf, presumably due to inlining changes

}

/// Check if this memo's cycle heads have all been finalized. If so, mark it verified final and
/// return true, if not return false.
#[inline]
fn validate_provisional(
&self,
db: &C::DbView,
zalsa: &Zalsa,
database_key_index: DatabaseKeyIndex,
memo: &Memo<C::Output<'_>>,
) -> bool {
tracing::debug!(
"{database_key_index:?}: validate_provisional(memo = {memo:#?})",
memo = memo.tracing_debug()
);
if (&memo.revisions.cycle_heads).into_iter().any(|cycle_head| {
zalsa
.lookup_ingredient(cycle_head.ingredient_index)
Expand Down Expand Up @@ -273,38 +282,40 @@ where
old_memo = old_memo.tracing_debug()
);

if self.shallow_verify_memo(db, zalsa, database_key_index, old_memo, false) {
return VerifyResult::Unchanged(InputAccumulatedValues::Empty, Default::default());
}
if old_memo.may_be_provisional() {
return VerifyResult::Changed;
if self.validate_may_be_provisional(db, zalsa, database_key_index, old_memo)
&& self.shallow_verify_memo(db, zalsa, database_key_index, old_memo)
{
return VerifyResult::unchanged();
}

let mut cycle_heads = vec![];
loop {
let inputs = match &old_memo.revisions.origin {
QueryOrigin::Assigned(_) => {
// If the value was assigneed by another query,
// and that query were up-to-date,
// then we would have updated the `verified_at` field already.
// So the fact that we are here means that it was not specified
// during this revision or is otherwise stale.
//
// Example of how this can happen:
//
// Conditionally specified queries
// where the value is specified
// in rev 1 but not in rev 2.
return VerifyResult::Changed;
}
QueryOrigin::FixpointInitial => {
return VerifyResult::unchanged();
}
QueryOrigin::DerivedUntracked(_) => {
// Untracked inputs? Have to assume that it changed.
match &old_memo.revisions.origin {
QueryOrigin::Assigned(_) => {
// If the value was assigneed by another query,
// and that query were up-to-date,
// then we would have updated the `verified_at` field already.
// So the fact that we are here means that it was not specified
// during this revision or is otherwise stale.
//
// Example of how this can happen:
//
// Conditionally specified queries
// where the value is specified
// in rev 1 but not in rev 2.
VerifyResult::Changed
}
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => VerifyResult::Changed,
QueryOrigin::FixpointInitial => VerifyResult::unchanged(),
QueryOrigin::DerivedUntracked(_) => {
// Untracked inputs? Have to assume that it changed.
VerifyResult::Changed
}
QueryOrigin::Derived(edges) => {
if old_memo.may_be_provisional() {
return VerifyResult::Changed;
}
QueryOrigin::Derived(edges) => {

let mut cycle_heads = vec![];
'cycle: loop {
// Fully tracked inputs? Iterate over the inputs and check them, one by one.
//
// NB: It's important here that we are iterating the inputs in the order that
Expand All @@ -317,10 +328,9 @@ where
for &edge in edges.input_outputs.iter() {
match edge {
QueryEdge::Input(dependency_index) => {
match dependency_index
.maybe_changed_after(db.as_dyn_database(), last_verified_at)
match dependency_index.maybe_changed_after(dyn_db, last_verified_at)
{
VerifyResult::Changed => return VerifyResult::Changed,
VerifyResult::Changed => break 'cycle VerifyResult::Changed,
VerifyResult::Unchanged(input_accumulated, cycles) => {
cycles.insert_into(&mut cycle_heads);
inputs |= input_accumulated;
Expand Down Expand Up @@ -352,58 +362,62 @@ where
}
}
}
inputs
}
};

// Possible scenarios here:
//
// 1. Cycle heads is empty. We traversed our full dependency graph and neither hit any
// cycles, nor found any changed dependencies. We can mark our memo verified and
// return Unchanged with empty cycle heads.
//
// 2. Cycle heads is non-empty, and does not contain our own key index. We are part of
// a cycle, and since we don't know if some other cycle participant that hasn't been
// traversed yet (that is, some other dependency of the cycle head, which is only a
// dependency of ours via the cycle) might still have changed, we can't yet mark our
// memo verified. We can return a provisional Unchanged, with cycle heads.
//
// 3. Cycle heads is non-empty, and contains only our own key index. We are the head of
// a cycle, and we've now traversed the entire cycle and found no changes, but no
// other cycle participants were verified (they would have all hit case 2 above). We
// can now safely mark our own memo as verified. Then we have to traverse the entire
// cycle again. This time, since our own memo is verified, there will be no cycle
// encountered, and the rest of the cycle will be able to verify itself.
//
// 4. Cycle heads is non-empty, and contains our own key index as well as other key
// indices. We are the head of a cycle nested within another cycle. We can't mark
// our own memo verified (for the same reason as in case 2: the full outer cycle
// hasn't been validated unchanged yet). We return Unchanged, with ourself removed
// from cycle heads. We will handle our own memo (and the rest of our cycle) on a
// future iteration; first the outer cycle head needs to verify itself.
// Possible scenarios here:
//
// 1. Cycle heads is empty. We traversed our full dependency graph and neither hit any
// cycles, nor found any changed dependencies. We can mark our memo verified and
// return Unchanged with empty cycle heads.
//
// 2. Cycle heads is non-empty, and does not contain our own key index. We are part of
// a cycle, and since we don't know if some other cycle participant that hasn't been
// traversed yet (that is, some other dependency of the cycle head, which is only a
// dependency of ours via the cycle) might still have changed, we can't yet mark our
// memo verified. We can return a provisional Unchanged, with cycle heads.
//
// 3. Cycle heads is non-empty, and contains only our own key index. We are the head of
// a cycle, and we've now traversed the entire cycle and found no changes, but no
// other cycle participants were verified (they would have all hit case 2 above). We
// can now safely mark our own memo as verified. Then we have to traverse the entire
// cycle again. This time, since our own memo is verified, there will be no cycle
// encountered, and the rest of the cycle will be able to verify itself.
//
// 4. Cycle heads is non-empty, and contains our own key index as well as other key
// indices. We are the head of a cycle nested within another cycle. We can't mark
// our own memo verified (for the same reason as in case 2: the full outer cycle
// hasn't been validated unchanged yet). We return Unchanged, with ourself removed
// from cycle heads. We will handle our own memo (and the rest of our cycle) on a
// future iteration; first the outer cycle head needs to verify itself.

let in_heads = cycle_heads
.iter()
.position(|&head| head == database_key_index)
.inspect(|&head| _ = cycle_heads.swap_remove(head))
.is_some();
let in_heads = cycle_heads
.iter()
.position(|&head| head == database_key_index)
.inspect(|&head| _ = cycle_heads.swap_remove(head))
.is_some();

if cycle_heads.is_empty() {
old_memo.mark_as_verified(db, zalsa.current_revision(), database_key_index, inputs);
if cycle_heads.is_empty() {
old_memo.mark_as_verified(
db,
zalsa.current_revision(),
database_key_index,
inputs,
);

if in_heads {
// Iterate our dependency graph again, starting from the top. We clear the
// cycle heads here because we are starting a fresh traversal. (It might be
// logically clearer to create a new HashSet each time, but clearing the
// existing one is more efficient.)
cycle_heads.clear();
continue;
if in_heads {
// Iterate our dependency graph again, starting from the top. We clear the
// cycle heads here because we are starting a fresh traversal. (It might be
// logically clearer to create a new HashSet each time, but clearing the
// existing one is more efficient.)
cycle_heads.clear();
continue 'cycle;
}
}
break 'cycle VerifyResult::Unchanged(
InputAccumulatedValues::Empty,
CycleHeads::from(cycle_heads),
);
}
}
return VerifyResult::Unchanged(
InputAccumulatedValues::Empty,
CycleHeads::from(cycle_heads),
);
}
}
}
3 changes: 2 additions & 1 deletion src/function/memo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ impl<V> Memo<V> {
pub(super) fn provisional_retry(
&self,
db: &dyn crate::Database,
zalsa: &Zalsa,
database_key_index: DatabaseKeyIndex,
) -> bool {
let mut retry = false;
Expand All @@ -182,7 +183,7 @@ impl<V> Memo<V> {
.into_iter()
.filter(|&head| head != database_key_index)
.any(|head| {
let ingredient = db.zalsa().lookup_ingredient(head.ingredient_index);
let ingredient = zalsa.lookup_ingredient(head.ingredient_index);
if !ingredient.is_provisional_cycle_head(db, head.key_index) {
// This cycle is already finalized, so we don't need to wait on it;
// keep looping through cycle heads.
Expand Down