fix: Runaway for unchanged queries participating in cycle #981
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #981 will improve performances by 7.22%Comparing Summary
Benchmarks breakdown
|
8a8d556 to
6d31c9d
Compare
|
I think I've an idea to fix this problem. We not only need to track the cycle heads, we also need to track for which queries we've already ran This could be a hash map from |
| // Salsa would end up recusively validating the hot query over and over again. | ||
| db.assert_logs(expect![[r#" | ||
| [ | ||
| "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", |
There was a problem hiding this comment.
Before, there were 3 or even 4 DidValidateInternedValue calls because each query_x calls maybe_changed_after(query_hot) which in turn called deep_verify_memo(query_hot)
As you can see, there are still multiple deep_verify_memo(query_hot) calls when re-executing head because salsa tries to narrow down which child query needs to re-run.
tests/cycle.rs
Outdated
| "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", | ||
| "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", |
There was a problem hiding this comment.
Uhm, why are we executing this query multiple times?
There was a problem hiding this comment.
This is now fixed
e052364 to
cb1bfd3
Compare
cb1bfd3 to
f39e194
Compare
This PR fixes a run-away (or even infinite loop) in
maybe_changed_afterfor queries participating in a cycle.For normal queries,
maybe_changed_afteris guaranteed to be only called exactly once (if they have an old value) because it always updates verified at:deep_verify_memoreturns unchangedUnfortunately, this isn't the case for queries participating in a cycle (that aren't a cycle head) because
deep_verify_memoreturnsunchangedwithout bumping theverified_atbecause the result depends on the outcome of the cycle head's verificationdeep_verify_memoreturnschangedbut we skip executing the query because the cycle head hasn't been fully verified at this point and this query might depend on an interned or tracked struct that only gets created later during the cycle execution.This PR fixes the run-away by caching the
maybe_changed_afterresults for queries participating in a cycle to avoid re-validating the same subtrees over and over again.However, I then noticed that we keep re-executing some queries because
validate_same_iterationis disabled formaybe_changed_after. This PR allowsvalidate_same_iterationbut I made it more strict to only return true if the memo and its cycle heads are from the same revision (verified at). The same is true forvalidate_provisional.this resolves astral-sh/ty#1024