-
Notifications
You must be signed in to change notification settings - Fork 175
fix: change detection for fixpoint queries #836
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.
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
fix: change detection for fixpoint queries #836
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #836 will degrade performances by 10.44%Comparing Summary
Benchmarks breakdown
|
5a435a7
to
953dc34
Compare
It's not entirely clear what behavior we want for accumulated values. E.g. If no, then we'll need a way to distinguish between dependencies from the current and previous iterations. |
9036ecf
to
22a383b
Compare
"salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(403)) })", | ||
"salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })", | ||
"salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", | ||
"salsa_event(WillExecute { database_key: query_b(Id(0)) })", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test captures fix 2. Salsa tries to see if query_b
changed (cycle head). query_b
depends on query_a
and query_a
depends on query_b
and query_d
.
Salsa returns a provisional value for query_b
.
Before
maybe_changed_after
:
query_b
: Unchangedquery_a
:- Calls
maybe_changed_after
forquery_b
: ReturnsUnchanged
(but fixpoint initial) - Calls
maybe_changed_after
forquery_d
: Detects that one input changed. Tries backdating by executing the query (which isn't safe because it may depend on a struct interned byquery_b
which was never fully verified
Now
query_d
now returns Changed
without trying to backdate, same for query_a
because both see that they're part of a cycle. Salsa then executes query_b
, which may get backdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I guess since you reverted the earlier (not really right) change I'd made to avoid having output-validation fail in this case, this test will also now fail without the other fix in this PR (and not only due to the log assertions.)
src/function/maybe_changed_after.rs
Outdated
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => { | ||
VerifyResult::changed() | ||
} | ||
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => VerifyResult::Changed, | ||
QueryOrigin::FixpointInitial => VerifyResult::unchanged(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carljm I'm not 100% sure about the two fixpoint arms here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not either :) I don't have a memory of why this is this way -- maybe it was needed in some much-earlier version of fixpoint iteration?
Locally it seems that replacing both arms with this doesn't fail any tests, and it feels more clearly correct (but I'd want to also check it for perf regression on ty):
QueryOrigin:FixpointInitial => VerifyResult::changed(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder if we even need this extra variant or if we could just use Derived
everywhere because we also have revisions.maybe_changed_after
to identify provisional values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So good!!
This all makes sense to me, fantastic work.
"salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(403)) })", | ||
"salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })", | ||
"salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", | ||
"salsa_event(WillExecute { database_key: query_b(Id(0)) })", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I guess since you reverted the earlier (not really right) change I'd made to avoid having output-validation fail in this case, this test will also now fail without the other fix in this PR (and not only due to the log assertions.)
fn query_a_initial(_db: &dyn Database, _input: Input) -> u32 { | ||
0 | ||
} | ||
|
||
fn query_a_recover( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these shouldn't be named query_a_*
since they are used by both query_a
and query_d
? Looks a bit weird above.
src/function/maybe_changed_after.rs
Outdated
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => { | ||
VerifyResult::changed() | ||
} | ||
QueryOrigin::FixpointInitial if old_memo.may_be_provisional() => VerifyResult::Changed, | ||
QueryOrigin::FixpointInitial => VerifyResult::unchanged(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not either :) I don't have a memory of why this is this way -- maybe it was needed in some much-earlier version of fixpoint iteration?
Locally it seems that replacing both arms with this doesn't fail any tests, and it feels more clearly correct (but I'd want to also check it for perf regression on ty):
QueryOrigin:FixpointInitial => VerifyResult::changed(),
This PR fixes two bugs in
maybe_changed_after
for cyclic queriesFix number 1
Salsa doesn't invalidate a cycle of
query_a
->query_b
ifquery_b
only depends on the input in the first iteration.Fixing this requires that we carry over inputs to later iterations. This is a very similar fix to what I did for tracked structs.
This fixes #834.
Fix number 2
We saw many access to interned struct that wasn't interned in this revision panics. This was caused by
maybe_changed_after
re-running a query in the attempt to backdate it even when the query participates in a cycle (in which case salsa returned Fixpoint initial on which the query that Salsa tries to backdate now depends on). This is problematic the assumption before callingexecute
is that all its inputs are valid but this isn't the case when returning fixpoint initial, Salsa then intentionally defers validating all the input/outputs.The fix here is to only perform the backdating if not in a cycle. If incide a cycle, then the backdating must happen for the cycle head (which then calls
maybe_changed_after
of inner queries again)Performance
The performance results on ty (former Red Knot) are neutral. The performance should mainly regress if cycles have many different inputs/outputs between iterations because they're now carried over