Skip to content

Commit 5b2a97b

Browse files
MichaReiserMichaReiser
authored
refactor: Extract the cycle branches from fetch and maybe_changed_after (#955)
* Extract the cycle branches from `fetch` and `maybe_changed_after` * Add `inline(never)`
1 parent d66fe33 commit 5b2a97b

File tree

2 files changed

+148
-118
lines changed

2 files changed

+148
-118
lines changed

‎src/function/fetch.rs‎

Lines changed: 83 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use crate::function::sync::ClaimResult;
44
use crate::function::{Configuration, IngredientImpl, VerifyResult};
55
use crate::zalsa::{MemoIngredientIndex, Zalsa};
66
use crate::zalsa_local::{QueryRevisions, ZalsaLocal};
7-
use crate::Id;
7+
use crate::{DatabaseKeyIndex, Id};
88

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

@@ -146,75 +147,15 @@ where
146147
return None;
147148
}
148149
ClaimResult::Cycle { .. } => {
149-
// check if there's a provisional value for this query
150-
// Note we don't `validate_may_be_provisional` the memo here as we want to reuse an
151-
// existing provisional memo if it exists
152-
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
153-
if let Some(memo) = memo_guard {
154-
if memo.value.is_some()
155-
&& memo.revisions.cycle_heads().contains(&database_key_index)
156-
{
157-
let can_shallow_update =
158-
self.shallow_verify_memo(zalsa, database_key_index, memo);
159-
if can_shallow_update.yes() {
160-
self.update_shallow(
161-
zalsa,
162-
database_key_index,
163-
memo,
164-
can_shallow_update,
165-
);
166-
// SAFETY: memo is present in memo_map.
167-
return unsafe { Some(self.extend_memo_lifetime(memo)) };
168-
}
169-
}
170-
}
171-
// no provisional value; create/insert/return initial provisional value
172-
return match C::CYCLE_STRATEGY {
173-
// SAFETY: We do not access the query stack reentrantly.
174-
CycleRecoveryStrategy::Panic => unsafe {
175-
zalsa_local.with_query_stack_unchecked(|stack| {
176-
panic!(
177-
"dependency graph cycle when querying {database_key_index:#?}, \
178-
set cycle_fn/cycle_initial to fixpoint iterate.\n\
179-
Query stack:\n{stack:#?}",
180-
);
181-
})
182-
},
183-
CycleRecoveryStrategy::Fixpoint => {
184-
crate::tracing::debug!(
185-
"hit cycle at {database_key_index:#?}, \
186-
inserting and returning fixpoint initial value"
187-
);
188-
let revisions = QueryRevisions::fixpoint_initial(database_key_index);
189-
let initial_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
190-
Some(self.insert_memo(
191-
zalsa,
192-
id,
193-
Memo::new(Some(initial_value), zalsa.current_revision(), revisions),
194-
memo_ingredient_index,
195-
))
196-
}
197-
CycleRecoveryStrategy::FallbackImmediate => {
198-
crate::tracing::debug!(
199-
"hit a `FallbackImmediate` cycle at {database_key_index:#?}"
200-
);
201-
let active_query =
202-
zalsa_local.push_query(database_key_index, IterationCount::initial());
203-
let fallback_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
204-
let mut revisions = active_query.pop();
205-
revisions.set_cycle_heads(CycleHeads::initial(database_key_index));
206-
// We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.
207-
*revisions.verified_final.get_mut() = false;
208-
Some(self.insert_memo(
209-
zalsa,
210-
id,
211-
Memo::new(Some(fallback_value), zalsa.current_revision(), revisions),
212-
memo_ingredient_index,
213-
))
214-
}
215-
};
150+
return Some(self.fetch_cold_cycle(
151+
zalsa,
152+
zalsa_local,
153+
db,
154+
id,
155+
database_key_index,
156+
memo_ingredient_index,
157+
));
216158
}
217-
ClaimResult::Claimed(guard) => guard,
218159
};
219160

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

273214
Some(memo)
274215
}
216+
217+
#[cold]
218+
#[inline(never)]
219+
fn fetch_cold_cycle<'db>(
220+
&'db self,
221+
zalsa: &'db Zalsa,
222+
zalsa_local: &'db ZalsaLocal,
223+
db: &'db C::DbView,
224+
id: Id,
225+
database_key_index: DatabaseKeyIndex,
226+
memo_ingredient_index: MemoIngredientIndex,
227+
) -> &'db Memo<'db, C> {
228+
// check if there's a provisional value for this query
229+
// Note we don't `validate_may_be_provisional` the memo here as we want to reuse an
230+
// existing provisional memo if it exists
231+
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
232+
if let Some(memo) = memo_guard {
233+
if memo.value.is_some() && memo.revisions.cycle_heads().contains(&database_key_index) {
234+
let can_shallow_update = self.shallow_verify_memo(zalsa, database_key_index, memo);
235+
if can_shallow_update.yes() {
236+
self.update_shallow(zalsa, database_key_index, memo, can_shallow_update);
237+
// SAFETY: memo is present in memo_map.
238+
return unsafe { self.extend_memo_lifetime(memo) };
239+
}
240+
}
241+
}
242+
243+
// no provisional value; create/insert/return initial provisional value
244+
match C::CYCLE_STRATEGY {
245+
// SAFETY: We do not access the query stack reentrantly.
246+
CycleRecoveryStrategy::Panic => unsafe {
247+
zalsa_local.with_query_stack_unchecked(|stack| {
248+
panic!(
249+
"dependency graph cycle when querying {database_key_index:#?}, \
250+
set cycle_fn/cycle_initial to fixpoint iterate.\n\
251+
Query stack:\n{stack:#?}",
252+
);
253+
})
254+
},
255+
CycleRecoveryStrategy::Fixpoint => {
256+
crate::tracing::debug!(
257+
"hit cycle at {database_key_index:#?}, \
258+
inserting and returning fixpoint initial value"
259+
);
260+
let revisions = QueryRevisions::fixpoint_initial(database_key_index);
261+
let initial_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
262+
self.insert_memo(
263+
zalsa,
264+
id,
265+
Memo::new(Some(initial_value), zalsa.current_revision(), revisions),
266+
memo_ingredient_index,
267+
)
268+
}
269+
CycleRecoveryStrategy::FallbackImmediate => {
270+
crate::tracing::debug!(
271+
"hit a `FallbackImmediate` cycle at {database_key_index:#?}"
272+
);
273+
let active_query =
274+
zalsa_local.push_query(database_key_index, IterationCount::initial());
275+
let fallback_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
276+
let mut revisions = active_query.pop();
277+
revisions.set_cycle_heads(CycleHeads::initial(database_key_index));
278+
// We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.
279+
*revisions.verified_final.get_mut() = false;
280+
self.insert_memo(
281+
zalsa,
282+
id,
283+
Memo::new(Some(fallback_value), zalsa.current_revision(), revisions),
284+
memo_ingredient_index,
285+
)
286+
}
287+
}
288+
}
275289
}

‎src/function/maybe_changed_after.rs‎

Lines changed: 65 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -113,33 +113,18 @@ where
113113
let database_key_index = self.database_key_index(key_index);
114114

115115
let _claim_guard = match self.sync_table.try_claim(zalsa, key_index) {
116+
ClaimResult::Claimed(guard) => guard,
116117
ClaimResult::Running(blocked_on) => {
117118
blocked_on.block_on(zalsa);
118119
return None;
119120
}
120-
ClaimResult::Cycle { .. } => match C::CYCLE_STRATEGY {
121-
// SAFETY: We do not access the query stack reentrantly.
122-
CycleRecoveryStrategy::Panic => unsafe {
123-
db.zalsa_local().with_query_stack_unchecked(|stack| {
124-
panic!(
125-
"dependency graph cycle when validating {database_key_index:#?}, \
126-
set cycle_fn/cycle_initial to fixpoint iterate.\n\
127-
Query stack:\n{stack:#?}",
128-
);
129-
})
130-
},
131-
CycleRecoveryStrategy::FallbackImmediate => {
132-
return Some(VerifyResult::unchanged());
133-
}
134-
CycleRecoveryStrategy::Fixpoint => {
135-
crate::tracing::debug!(
136-
"hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value",
137-
);
138-
cycle_heads.insert(database_key_index);
139-
return Some(VerifyResult::unchanged());
140-
}
141-
},
142-
ClaimResult::Claimed(guard) => guard,
121+
ClaimResult::Cycle { .. } => {
122+
return Some(self.maybe_changed_after_cold_cycle(
123+
db,
124+
database_key_index,
125+
cycle_heads,
126+
))
127+
}
143128
};
144129
// Load the current memo, if any.
145130
let Some(old_memo) = self.get_memo_from_table_for(zalsa, key_index, memo_ingredient_index)
@@ -205,6 +190,36 @@ where
205190
Some(VerifyResult::Changed)
206191
}
207192

193+
#[cold]
194+
#[inline(never)]
195+
fn maybe_changed_after_cold_cycle<'db>(
196+
&'db self,
197+
db: &'db C::DbView,
198+
database_key_index: DatabaseKeyIndex,
199+
cycle_heads: &mut CycleHeadKeys,
200+
) -> VerifyResult {
201+
match C::CYCLE_STRATEGY {
202+
// SAFETY: We do not access the query stack reentrantly.
203+
CycleRecoveryStrategy::Panic => unsafe {
204+
db.zalsa_local().with_query_stack_unchecked(|stack| {
205+
panic!(
206+
"dependency graph cycle when validating {database_key_index:#?}, \
207+
set cycle_fn/cycle_initial to fixpoint iterate.\n\
208+
Query stack:\n{stack:#?}",
209+
);
210+
})
211+
},
212+
CycleRecoveryStrategy::FallbackImmediate => VerifyResult::unchanged(),
213+
CycleRecoveryStrategy::Fixpoint => {
214+
crate::tracing::debug!(
215+
"hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value",
216+
);
217+
cycle_heads.insert(database_key_index);
218+
VerifyResult::unchanged()
219+
}
220+
}
221+
}
222+
208223
/// `Some` if the memo's value and `changed_at` time is still valid in this revision.
209224
/// Does only a shallow O(1) check, doesn't walk the dependencies.
210225
///
@@ -455,32 +470,6 @@ where
455470
}
456471

457472
match old_memo.revisions.origin.as_ref() {
458-
QueryOriginRef::Assigned(_) => {
459-
// If the value was assigned by another query,
460-
// and that query were up-to-date,
461-
// then we would have updated the `verified_at` field already.
462-
// So the fact that we are here means that it was not specified
463-
// during this revision or is otherwise stale.
464-
//
465-
// Example of how this can happen:
466-
//
467-
// Conditionally specified queries
468-
// where the value is specified
469-
// in rev 1 but not in rev 2.
470-
VerifyResult::Changed
471-
}
472-
// Return `Unchanged` similar to the initial value that we insert
473-
// when we hit the cycle. Any dependencies accessed when creating the fixpoint initial
474-
// are tracked by the outer query. Nothing should have changed assuming that the
475-
// fixpoint initial function is deterministic.
476-
QueryOriginRef::FixpointInitial => {
477-
cycle_heads.insert(database_key_index);
478-
VerifyResult::unchanged()
479-
}
480-
QueryOriginRef::DerivedUntracked(_) => {
481-
// Untracked inputs? Have to assume that it changed.
482-
VerifyResult::Changed
483-
}
484473
QueryOriginRef::Derived(edges) => {
485474
let is_provisional = old_memo.may_be_provisional();
486475

@@ -584,6 +573,33 @@ where
584573
accumulated: inputs,
585574
}
586575
}
576+
577+
QueryOriginRef::Assigned(_) => {
578+
// If the value was assigned by another query,
579+
// and that query were up-to-date,
580+
// then we would have updated the `verified_at` field already.
581+
// So the fact that we are here means that it was not specified
582+
// during this revision or is otherwise stale.
583+
//
584+
// Example of how this can happen:
585+
//
586+
// Conditionally specified queries
587+
// where the value is specified
588+
// in rev 1 but not in rev 2.
589+
VerifyResult::Changed
590+
}
591+
// Return `Unchanged` similar to the initial value that we insert
592+
// when we hit the cycle. Any dependencies accessed when creating the fixpoint initial
593+
// are tracked by the outer query. Nothing should have changed assuming that the
594+
// fixpoint initial function is deterministic.
595+
QueryOriginRef::FixpointInitial => {
596+
cycle_heads.insert(database_key_index);
597+
VerifyResult::unchanged()
598+
}
599+
QueryOriginRef::DerivedUntracked(_) => {
600+
// Untracked inputs? Have to assume that it changed.
601+
VerifyResult::Changed
602+
}
587603
}
588604
}
589605
}

0 commit comments

Comments
 (0)