Track cycle function dependencies as part of the cyclic query#1018
Track cycle function dependencies as part of the cyclic query#1018MichaReiser merged 6 commits intosalsa-rs:masterfrom
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #1018 will not alter performanceComparing Summary
|
MichaReiser
force-pushed
the
cycle-fn-dependencies
branch
3 times, most recently
from
1339754 to
0354267
Compare
MichaReiser
requested review from
carljm and
ibraheemdev
and removed request for
carljm
| "WillCheckCancellation", | ||
| "WillIterateCycle { database_key: query(Id(0)), iteration_count: IterationCount(5) }", | ||
| "WillCheckCancellation", | ||
| "DidValidateMemoizedValue { database_key: entry(Id(0)) }", |
There was a problem hiding this comment.
Before the fix, this incorrectly re-executed entry but not the cycle (which is what depends on the value).
With the fix in execute_maybe_iterate, it did re-execute the cycle, but it also re-executed entry, which is incorrect because query returns the same value (it can be backdated). Now, the behavior is what we want. entry does not get re-executed but the cycle is
|
@carljm or @ibraheemdev could either of you take a look at this PR. We need this change to correctly handle diverging queries. |
MichaReiser
force-pushed
the
cycle-fn-dependencies
branch
from
470ee4c to
45158fc
Compare
MichaReiser
changed the title
| } | ||
|
|
||
| let mut completed_query = active_query.pop(); | ||
| *completed_query.revisions.verified_final.get_mut() = false; |
There was a problem hiding this comment.
Not sure I understand in what case this would be true and we need to reset it to false.
There was a problem hiding this comment.
QueryRevisions::new defaults to true if cycle heads are empty. The cycle heads are always empty when calling pop because of the preceding query_guard.take_cycle_heads
2 participants
This fixes a bug in our fixpoint handling: the dependencies introduced by the cycle recovery function were recorded as dependencies of the outer function rather than the query with cycle handling.
This PR also requires that a cycle recovery function not introduce any new cycles, since it's unclear how to handle that.
I have the impression that we should probably to the same for
cycle_initialbut this is a bit trickier because we'd have to merge theinput/outputsinexecute_maybe_iterate. I also think it matters less because the dependencies are added to some query within the cycle and we always need to execute the cycle as a whole if any input it depends on changed. That's why I think it's probably fine if the dependencies are tracked by another cycle participant.Test plan
I added a regression test, yay :)
Except... it revealed a bug with deep verify memo where we failed to remove the head when we early-returned. Luckily, this was easy to fix.