Skip to content

refactor: Extract the cycle branches from fetch and maybe_changed_after #955

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
merged 2 commits into from
Aug 6, 2025
Merged

refactor: Extract the cycle branches from fetch and maybe_changed_after #955

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
e1b8810
Extract the cycle branches from `fetch` and `maybe_changed_after`
MichaReiser Aug 2, 2025
8e0ee63
Add `inline(never)`
MichaReiser Aug 6, 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
152 changes: 83 additions & 69 deletions src/function/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::function::sync::ClaimResult;
use crate::function::{Configuration, IngredientImpl, VerifyResult};
use crate::zalsa::{MemoIngredientIndex, Zalsa};
use crate::zalsa_local::{QueryRevisions, ZalsaLocal};
use crate::Id;
use crate::{DatabaseKeyIndex, Id};

impl<C> IngredientImpl<C>
where
Expand Down Expand Up @@ -130,6 +130,7 @@ where
let database_key_index = self.database_key_index(id);
// Try to claim this query: if someone else has claimed it already, go back and start again.
let claim_guard = match self.sync_table.try_claim(zalsa, id) {
ClaimResult::Claimed(guard) => guard,
ClaimResult::Running(blocked_on) => {
blocked_on.block_on(zalsa);

Expand All @@ -146,75 +147,15 @@ where
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 memo.value.is_some()
&& memo.revisions.cycle_heads().contains(&database_key_index)
{
let can_shallow_update =
self.shallow_verify_memo(zalsa, database_key_index, memo);
if can_shallow_update.yes() {
self.update_shallow(
zalsa,
database_key_index,
memo,
can_shallow_update,
);
// SAFETY: memo is present in memo_map.
return unsafe { Some(self.extend_memo_lifetime(memo)) };
}
}
}
// no provisional value; create/insert/return initial provisional value
return match C::CYCLE_STRATEGY {
// SAFETY: We do not access the query stack reentrantly.
CycleRecoveryStrategy::Panic => unsafe {
zalsa_local.with_query_stack_unchecked(|stack| {
panic!(
"dependency graph cycle when querying {database_key_index:#?}, \
set cycle_fn/cycle_initial to fixpoint iterate.\n\
Query stack:\n{stack:#?}",
);
})
},
CycleRecoveryStrategy::Fixpoint => {
crate::tracing::debug!(
"hit cycle at {database_key_index:#?}, \
inserting and returning fixpoint initial value"
);
let revisions = QueryRevisions::fixpoint_initial(database_key_index);
let initial_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
Some(self.insert_memo(
zalsa,
id,
Memo::new(Some(initial_value), zalsa.current_revision(), revisions),
memo_ingredient_index,
))
}
CycleRecoveryStrategy::FallbackImmediate => {
crate::tracing::debug!(
"hit a `FallbackImmediate` cycle at {database_key_index:#?}"
);
let active_query =
zalsa_local.push_query(database_key_index, IterationCount::initial());
let fallback_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
let mut revisions = active_query.pop();
revisions.set_cycle_heads(CycleHeads::initial(database_key_index));
// We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.
*revisions.verified_final.get_mut() = false;
Some(self.insert_memo(
zalsa,
id,
Memo::new(Some(fallback_value), zalsa.current_revision(), revisions),
memo_ingredient_index,
))
}
};
return Some(self.fetch_cold_cycle(
zalsa,
zalsa_local,
db,
id,
database_key_index,
memo_ingredient_index,
));
}
ClaimResult::Claimed(guard) => guard,
};

// Now that we've claimed the item, check again to see if there's a "hot" value.
Expand Down Expand Up @@ -272,4 +213,77 @@ where

Some(memo)
}

#[cold]
#[inline(never)]
fn fetch_cold_cycle<'db>(
&'db self,
zalsa: &'db Zalsa,
zalsa_local: &'db ZalsaLocal,
db: &'db C::DbView,
id: Id,
database_key_index: DatabaseKeyIndex,
memo_ingredient_index: MemoIngredientIndex,
) -> &'db Memo<'db, C> {
// 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 memo.value.is_some() && memo.revisions.cycle_heads().contains(&database_key_index) {
let can_shallow_update = self.shallow_verify_memo(zalsa, database_key_index, memo);
if can_shallow_update.yes() {
self.update_shallow(zalsa, database_key_index, memo, can_shallow_update);
// SAFETY: memo is present in memo_map.
return unsafe { self.extend_memo_lifetime(memo) };
}
}
}

// no provisional value; create/insert/return initial provisional value
match C::CYCLE_STRATEGY {
// SAFETY: We do not access the query stack reentrantly.
CycleRecoveryStrategy::Panic => unsafe {
zalsa_local.with_query_stack_unchecked(|stack| {
panic!(
"dependency graph cycle when querying {database_key_index:#?}, \
set cycle_fn/cycle_initial to fixpoint iterate.\n\
Query stack:\n{stack:#?}",
);
})
},
CycleRecoveryStrategy::Fixpoint => {
crate::tracing::debug!(
"hit cycle at {database_key_index:#?}, \
inserting and returning fixpoint initial value"
);
let revisions = QueryRevisions::fixpoint_initial(database_key_index);
let initial_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
self.insert_memo(
zalsa,
id,
Memo::new(Some(initial_value), zalsa.current_revision(), revisions),
memo_ingredient_index,
)
}
CycleRecoveryStrategy::FallbackImmediate => {
crate::tracing::debug!(
"hit a `FallbackImmediate` cycle at {database_key_index:#?}"
);
let active_query =
zalsa_local.push_query(database_key_index, IterationCount::initial());
let fallback_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
let mut revisions = active_query.pop();
revisions.set_cycle_heads(CycleHeads::initial(database_key_index));
// We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.
*revisions.verified_final.get_mut() = false;
self.insert_memo(
zalsa,
id,
Memo::new(Some(fallback_value), zalsa.current_revision(), revisions),
memo_ingredient_index,
)
}
}
}
}
114 changes: 65 additions & 49 deletions src/function/maybe_changed_after.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,33 +113,18 @@ where
let database_key_index = self.database_key_index(key_index);

let _claim_guard = match self.sync_table.try_claim(zalsa, key_index) {
ClaimResult::Claimed(guard) => guard,
ClaimResult::Running(blocked_on) => {
blocked_on.block_on(zalsa);
return None;
}
ClaimResult::Cycle { .. } => match C::CYCLE_STRATEGY {
// SAFETY: We do not access the query stack reentrantly.
CycleRecoveryStrategy::Panic => unsafe {
db.zalsa_local().with_query_stack_unchecked(|stack| {
panic!(
"dependency graph cycle when validating {database_key_index:#?}, \
set cycle_fn/cycle_initial to fixpoint iterate.\n\
Query stack:\n{stack:#?}",
);
})
},
CycleRecoveryStrategy::FallbackImmediate => {
return Some(VerifyResult::unchanged());
}
CycleRecoveryStrategy::Fixpoint => {
crate::tracing::debug!(
"hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value",
);
cycle_heads.insert(database_key_index);
return Some(VerifyResult::unchanged());
}
},
ClaimResult::Claimed(guard) => guard,
ClaimResult::Cycle { .. } => {
return Some(self.maybe_changed_after_cold_cycle(
db,
database_key_index,
cycle_heads,
))
}
};
// Load the current memo, if any.
let Some(old_memo) = self.get_memo_from_table_for(zalsa, key_index, memo_ingredient_index)
Expand Down Expand Up @@ -205,6 +190,36 @@ where
Some(VerifyResult::Changed)
}

#[cold]
#[inline(never)]
fn maybe_changed_after_cold_cycle<'db>(
&'db self,
db: &'db C::DbView,
database_key_index: DatabaseKeyIndex,
cycle_heads: &mut CycleHeadKeys,
) -> VerifyResult {
match C::CYCLE_STRATEGY {
// SAFETY: We do not access the query stack reentrantly.
CycleRecoveryStrategy::Panic => unsafe {
db.zalsa_local().with_query_stack_unchecked(|stack| {
panic!(
"dependency graph cycle when validating {database_key_index:#?}, \
set cycle_fn/cycle_initial to fixpoint iterate.\n\
Query stack:\n{stack:#?}",
);
})
},
CycleRecoveryStrategy::FallbackImmediate => VerifyResult::unchanged(),
CycleRecoveryStrategy::Fixpoint => {
crate::tracing::debug!(
"hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value",
);
cycle_heads.insert(database_key_index);
VerifyResult::unchanged()
}
}
}

/// `Some` if the memo's value and `changed_at` time is still valid in this revision.
/// Does only a shallow O(1) check, doesn't walk the dependencies.
///
Expand Down Expand Up @@ -455,32 +470,6 @@ where
}

match old_memo.revisions.origin.as_ref() {
QueryOriginRef::Assigned(_) => {
// If the value was assigned 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
}
// Return `Unchanged` similar to the initial value that we insert
// when we hit the cycle. Any dependencies accessed when creating the fixpoint initial
// are tracked by the outer query. Nothing should have changed assuming that the
// fixpoint initial function is deterministic.
QueryOriginRef::FixpointInitial => {
cycle_heads.insert(database_key_index);
VerifyResult::unchanged()
}
QueryOriginRef::DerivedUntracked(_) => {
// Untracked inputs? Have to assume that it changed.
VerifyResult::Changed
}
QueryOriginRef::Derived(edges) => {
let is_provisional = old_memo.may_be_provisional();

Expand Down Expand Up @@ -584,6 +573,33 @@ where
accumulated: inputs,
}
}

QueryOriginRef::Assigned(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is just to put the more frequent match arm first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I find this also helps with readability because I can reason about the common case first

// If the value was assigned 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
}
// Return `Unchanged` similar to the initial value that we insert
// when we hit the cycle. Any dependencies accessed when creating the fixpoint initial
// are tracked by the outer query. Nothing should have changed assuming that the
// fixpoint initial function is deterministic.
QueryOriginRef::FixpointInitial => {
cycle_heads.insert(database_key_index);
VerifyResult::unchanged()
}
QueryOriginRef::DerivedUntracked(_) => {
// Untracked inputs? Have to assume that it changed.
VerifyResult::Changed
}
}
}
}
Expand Down