Skip to content

Commit c76b9e5

Browse files
VeykrilVeykril
authored
perf: Reduce unnecessary conditional work in deep_verify_memo (#759)
* Reduce unnecessary condituional work in `deep_verify_memo` * Restructure provisional validation a bit
1 parent 407e9e0 commit c76b9e5

File tree

3 files changed

+122
-101
lines changed

3 files changed

+122
-101
lines changed

‎src/function/fetch.rs

+13-7
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,11 @@ where
6262
// thread completing fixpoint iteration of the cycle, and then we can re-query for
6363
// our no-longer-provisional memo.
6464
if !(memo.may_be_provisional()
65-
&& memo.provisional_retry(db.as_dyn_database(), self.database_key_index(id)))
65+
&& memo.provisional_retry(
66+
db.as_dyn_database(),
67+
zalsa,
68+
self.database_key_index(id),
69+
))
6670
{
6771
return memo;
6872
}
@@ -80,8 +84,10 @@ where
8084
) -> Option<&'db Memo<C::Output<'db>>> {
8185
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
8286
if let Some(memo) = memo_guard {
87+
let database_key_index = self.database_key_index(id);
8388
if memo.value.is_some()
84-
&& self.shallow_verify_memo(db, zalsa, self.database_key_index(id), memo, false)
89+
&& self.validate_may_be_provisional(db, zalsa, database_key_index, memo)
90+
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
8591
{
8692
// Unsafety invariant: memo is present in memo_map and we have verified that it is
8793
// still valid for the current revision.
@@ -110,16 +116,16 @@ where
110116
ClaimResult::Retry => return None,
111117
ClaimResult::Cycle => {
112118
// check if there's a provisional value for this query
119+
// Note we don't `validate_may_be_provisional` the memo here as we want to reuse an
120+
// existing provisional memo if it exists
113121
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
114-
if let Some(memo) = &memo_guard {
122+
if let Some(memo) = memo_guard {
115123
if memo.value.is_some()
116124
&& memo.revisions.cycle_heads.contains(&database_key_index)
117-
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo, true)
125+
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
118126
{
119127
// Unsafety invariant: memo is present in memo_map.
120-
unsafe {
121-
return Some(self.extend_memo_lifetime(memo));
122-
}
128+
return unsafe { Some(self.extend_memo_lifetime(memo)) };
123129
}
124130
}
125131
// no provisional value; create/insert/return initial provisional value

‎src/function/maybe_changed_after.rs

+107-93
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,9 @@ where
6262
// Check if we have a verified version: this is the hot path.
6363
let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
6464
if let Some(memo) = memo_guard {
65-
if self.shallow_verify_memo(db, zalsa, database_key_index, memo, false) {
65+
if self.validate_may_be_provisional(db, zalsa, database_key_index, memo)
66+
&& self.shallow_verify_memo(db, zalsa, database_key_index, memo)
67+
{
6668
return if memo.revisions.changed_at > revision {
6769
VerifyResult::Changed
6870
} else {
@@ -173,33 +175,18 @@ where
173175
/// eagerly finalize all provisional memos in cycle iteration, we have to lazily check here
174176
/// (via `validate_provisional`) whether a may-be-provisional memo should actually be verified
175177
/// final, because its cycle heads are all now final.
176-
///
177-
/// If `allow_provisional` is `true`, don't check provisionality and return whatever memo we
178-
/// find that can be verified in this revision, whether provisional or not. This only occurs at
179-
/// one call-site, in `fetch_cold` when we actually encounter a cycle, and want to check if
180-
/// there is an existing provisional memo we can reuse.
181178
#[inline]
182179
pub(super) fn shallow_verify_memo(
183180
&self,
184181
db: &C::DbView,
185182
zalsa: &Zalsa,
186183
database_key_index: DatabaseKeyIndex,
187184
memo: &Memo<C::Output<'_>>,
188-
allow_provisional: bool,
189185
) -> bool {
190186
tracing::debug!(
191187
"{database_key_index:?}: shallow_verify_memo(memo = {memo:#?})",
192188
memo = memo.tracing_debug()
193189
);
194-
if !allow_provisional && memo.may_be_provisional() {
195-
tracing::debug!(
196-
"{database_key_index:?}: validate_provisional(memo = {memo:#?})",
197-
memo = memo.tracing_debug()
198-
);
199-
if !self.validate_provisional(db, zalsa, memo) {
200-
return false;
201-
}
202-
}
203190
let verified_at = memo.verified_at.load();
204191
let revision_now = zalsa.current_revision();
205192

@@ -231,14 +218,36 @@ where
231218
false
232219
}
233220

221+
/// Validates this memo if it is a provisional memo. Returns true for non provisional memos or
222+
/// if the provisional memo has been successfully marked as verified final, that is, its
223+
/// cycle heads have all been finalized.
224+
#[inline]
225+
pub(super) fn validate_may_be_provisional(
226+
&self,
227+
db: &C::DbView,
228+
zalsa: &Zalsa,
229+
database_key_index: DatabaseKeyIndex,
230+
memo: &Memo<C::Output<'_>>,
231+
) -> bool {
232+
// Wouldn't it be nice if rust had an implication operator ...
233+
// may_be_provisional -> validate_provisional
234+
!memo.may_be_provisional() || self.validate_provisional(db, zalsa, database_key_index, memo)
235+
}
236+
234237
/// Check if this memo's cycle heads have all been finalized. If so, mark it verified final and
235238
/// return true, if not return false.
239+
#[inline]
236240
fn validate_provisional(
237241
&self,
238242
db: &C::DbView,
239243
zalsa: &Zalsa,
244+
database_key_index: DatabaseKeyIndex,
240245
memo: &Memo<C::Output<'_>>,
241246
) -> bool {
247+
tracing::debug!(
248+
"{database_key_index:?}: validate_provisional(memo = {memo:#?})",
249+
memo = memo.tracing_debug()
250+
);
242251
if (&memo.revisions.cycle_heads).into_iter().any(|cycle_head| {
243252
zalsa
244253
.lookup_ingredient(cycle_head.ingredient_index)
@@ -273,38 +282,40 @@ where
273282
old_memo = old_memo.tracing_debug()
274283
);
275284

276-
if self.shallow_verify_memo(db, zalsa, database_key_index, old_memo, false) {
277-
return VerifyResult::Unchanged(InputAccumulatedValues::Empty, Default::default());
278-
}
279-
if old_memo.may_be_provisional() {
280-
return VerifyResult::Changed;
285+
if self.validate_may_be_provisional(db, zalsa, database_key_index, old_memo)
286+
&& self.shallow_verify_memo(db, zalsa, database_key_index, old_memo)
287+
{
288+
return VerifyResult::unchanged();
281289
}
282290

283-
let mut cycle_heads = vec![];
284-
loop {
285-
let inputs = match &old_memo.revisions.origin {
286-
QueryOrigin::Assigned(_) => {
287-
// If the value was assigneed by another query,
288-
// and that query were up-to-date,
289-
// then we would have updated the `verified_at` field already.
290-
// So the fact that we are here means that it was not specified
291-
// during this revision or is otherwise stale.
292-
//
293-
// Example of how this can happen:
294-
//
295-
// Conditionally specified queries
296-
// where the value is specified
297-
// in rev 1 but not in rev 2.
298-
return VerifyResult::Changed;
299-
}
300-
QueryOrigin::FixpointInitial => {
301-
return VerifyResult::unchanged();
302-
}
303-
QueryOrigin::DerivedUntracked(_) => {
304-
// Untracked inputs? Have to assume that it changed.
291+
match &old_memo.revisions.origin {
292+
QueryOrigin::Assigned(_) => {
293+
// If the value was assigneed by another query,
294+
// and that query were up-to-date,
295+
// then we would have updated the `verified_at` field already.
296+
// So the fact that we are here means that it was not specified
297+
// during this revision or is otherwise stale.
298+
//
299+
// Example of how this can happen:
300+
//
301+
// Conditionally specified queries
302+
// where the value is specified
303+
// in rev 1 but not in rev 2.
304+
VerifyResult::Changed
305+
}
306+
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => VerifyResult::Changed,
307+
QueryOrigin::FixpointInitial => VerifyResult::unchanged(),
308+
QueryOrigin::DerivedUntracked(_) => {
309+
// Untracked inputs? Have to assume that it changed.
310+
VerifyResult::Changed
311+
}
312+
QueryOrigin::Derived(edges) => {
313+
if old_memo.may_be_provisional() {
305314
return VerifyResult::Changed;
306315
}
307-
QueryOrigin::Derived(edges) => {
316+
317+
let mut cycle_heads = vec![];
318+
'cycle: loop {
308319
// Fully tracked inputs? Iterate over the inputs and check them, one by one.
309320
//
310321
// NB: It's important here that we are iterating the inputs in the order that
@@ -317,10 +328,9 @@ where
317328
for &edge in edges.input_outputs.iter() {
318329
match edge {
319330
QueryEdge::Input(dependency_index) => {
320-
match dependency_index
321-
.maybe_changed_after(db.as_dyn_database(), last_verified_at)
331+
match dependency_index.maybe_changed_after(dyn_db, last_verified_at)
322332
{
323-
VerifyResult::Changed => return VerifyResult::Changed,
333+
VerifyResult::Changed => break 'cycle VerifyResult::Changed,
324334
VerifyResult::Unchanged(input_accumulated, cycles) => {
325335
cycles.insert_into(&mut cycle_heads);
326336
inputs |= input_accumulated;
@@ -352,58 +362,62 @@ where
352362
}
353363
}
354364
}
355-
inputs
356-
}
357-
};
358365

359-
// Possible scenarios here:
360-
//
361-
// 1. Cycle heads is empty. We traversed our full dependency graph and neither hit any
362-
// cycles, nor found any changed dependencies. We can mark our memo verified and
363-
// return Unchanged with empty cycle heads.
364-
//
365-
// 2. Cycle heads is non-empty, and does not contain our own key index. We are part of
366-
// a cycle, and since we don't know if some other cycle participant that hasn't been
367-
// traversed yet (that is, some other dependency of the cycle head, which is only a
368-
// dependency of ours via the cycle) might still have changed, we can't yet mark our
369-
// memo verified. We can return a provisional Unchanged, with cycle heads.
370-
//
371-
// 3. Cycle heads is non-empty, and contains only our own key index. We are the head of
372-
// a cycle, and we've now traversed the entire cycle and found no changes, but no
373-
// other cycle participants were verified (they would have all hit case 2 above). We
374-
// can now safely mark our own memo as verified. Then we have to traverse the entire
375-
// cycle again. This time, since our own memo is verified, there will be no cycle
376-
// encountered, and the rest of the cycle will be able to verify itself.
377-
//
378-
// 4. Cycle heads is non-empty, and contains our own key index as well as other key
379-
// indices. We are the head of a cycle nested within another cycle. We can't mark
380-
// our own memo verified (for the same reason as in case 2: the full outer cycle
381-
// hasn't been validated unchanged yet). We return Unchanged, with ourself removed
382-
// from cycle heads. We will handle our own memo (and the rest of our cycle) on a
383-
// future iteration; first the outer cycle head needs to verify itself.
366+
// Possible scenarios here:
367+
//
368+
// 1. Cycle heads is empty. We traversed our full dependency graph and neither hit any
369+
// cycles, nor found any changed dependencies. We can mark our memo verified and
370+
// return Unchanged with empty cycle heads.
371+
//
372+
// 2. Cycle heads is non-empty, and does not contain our own key index. We are part of
373+
// a cycle, and since we don't know if some other cycle participant that hasn't been
374+
// traversed yet (that is, some other dependency of the cycle head, which is only a
375+
// dependency of ours via the cycle) might still have changed, we can't yet mark our
376+
// memo verified. We can return a provisional Unchanged, with cycle heads.
377+
//
378+
// 3. Cycle heads is non-empty, and contains only our own key index. We are the head of
379+
// a cycle, and we've now traversed the entire cycle and found no changes, but no
380+
// other cycle participants were verified (they would have all hit case 2 above). We
381+
// can now safely mark our own memo as verified. Then we have to traverse the entire
382+
// cycle again. This time, since our own memo is verified, there will be no cycle
383+
// encountered, and the rest of the cycle will be able to verify itself.
384+
//
385+
// 4. Cycle heads is non-empty, and contains our own key index as well as other key
386+
// indices. We are the head of a cycle nested within another cycle. We can't mark
387+
// our own memo verified (for the same reason as in case 2: the full outer cycle
388+
// hasn't been validated unchanged yet). We return Unchanged, with ourself removed
389+
// from cycle heads. We will handle our own memo (and the rest of our cycle) on a
390+
// future iteration; first the outer cycle head needs to verify itself.
384391

385-
let in_heads = cycle_heads
386-
.iter()
387-
.position(|&head| head == database_key_index)
388-
.inspect(|&head| _ = cycle_heads.swap_remove(head))
389-
.is_some();
392+
let in_heads = cycle_heads
393+
.iter()
394+
.position(|&head| head == database_key_index)
395+
.inspect(|&head| _ = cycle_heads.swap_remove(head))
396+
.is_some();
390397

391-
if cycle_heads.is_empty() {
392-
old_memo.mark_as_verified(db, zalsa.current_revision(), database_key_index, inputs);
398+
if cycle_heads.is_empty() {
399+
old_memo.mark_as_verified(
400+
db,
401+
zalsa.current_revision(),
402+
database_key_index,
403+
inputs,
404+
);
393405

394-
if in_heads {
395-
// Iterate our dependency graph again, starting from the top. We clear the
396-
// cycle heads here because we are starting a fresh traversal. (It might be
397-
// logically clearer to create a new HashSet each time, but clearing the
398-
// existing one is more efficient.)
399-
cycle_heads.clear();
400-
continue;
406+
if in_heads {
407+
// Iterate our dependency graph again, starting from the top. We clear the
408+
// cycle heads here because we are starting a fresh traversal. (It might be
409+
// logically clearer to create a new HashSet each time, but clearing the
410+
// existing one is more efficient.)
411+
cycle_heads.clear();
412+
continue 'cycle;
413+
}
414+
}
415+
break 'cycle VerifyResult::Unchanged(
416+
InputAccumulatedValues::Empty,
417+
CycleHeads::from(cycle_heads),
418+
);
401419
}
402420
}
403-
return VerifyResult::Unchanged(
404-
InputAccumulatedValues::Empty,
405-
CycleHeads::from(cycle_heads),
406-
);
407421
}
408422
}
409423
}

‎src/function/memo.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ impl<V> Memo<V> {
174174
pub(super) fn provisional_retry(
175175
&self,
176176
db: &dyn crate::Database,
177+
zalsa: &Zalsa,
177178
database_key_index: DatabaseKeyIndex,
178179
) -> bool {
179180
let mut retry = false;
@@ -182,7 +183,7 @@ impl<V> Memo<V> {
182183
.into_iter()
183184
.filter(|&head| head != database_key_index)
184185
.any(|head| {
185-
let ingredient = db.zalsa().lookup_ingredient(head.ingredient_index);
186+
let ingredient = zalsa.lookup_ingredient(head.ingredient_index);
186187
if !ingredient.is_provisional_cycle_head(db, head.key_index) {
187188
// This cycle is already finalized, so we don't need to wait on it;
188189
// keep looping through cycle heads.

0 commit comments

Comments
 (0)