Fix accumulated values for backdated queries #662
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #662 will not alter performanceComparing Summary
|
src/function/maybe_changed_after.rs
Outdated
| @@ -182,13 +202,19 @@ where | |||
| // valid, then some later input I1 might never have executed at all, so verifying | |||
| // it is still up to date is meaningless. | |||
| let last_verified_at = old_memo.verified_at.load(); | |||
| let mut inputs = old_memo.revisions.accumulated_inputs.load(); | |||
There was a problem hiding this comment.
This had to change from defaulting to Empty to the memo's value due to #636 as that changed the flag to track self and dependencies, instead of just dependencies. Notably this has a bigger implication though, it means a memo can never lose its Any accumulator status via verification, so if we the memo had a dependency that accumulated (which now no longer does) while not having accumulated itself we will still keep the flag as the memo having accumulated. (If am not making a mistake) I am not sure how bad this is, I feel like it shouldn't matter too much? It makes the optimization a bit less useful though I think this is a rather uncommon case to hit? @MichaReiser
There was a problem hiding this comment.
It makes the optimization less useful in an LSP case where it is very common to have many files that had some diagnostics at some point. For example, if you make edits to 20 files, each of those will have diagnostics (syntax) associated with it while making those edits, but they'll ultimately converge. I rarely close my editor. That means that these "stale" accumulated states keep adding up.
Why was it again that #636 had to change the tracking to track both self and its dependencies?
There was a problem hiding this comment.
It makes the optimization less useful in an LSP case where it is very common to have many files that had some diagnostics at some point. For example, if you make edits to 20 files, each of those will have diagnostics (syntax) associated with it while making those edits, but they'll ultimately converge. I rarely close my editor. That means that these "stale" accumulated states keep adding up.
I would expect in the general case that if accumulations changed, that the general output of a given tracked function also changed though which will then reset the flag anyways. Your test here shows the case where that does not happen, but for that the accumulation producing run and the non accumulation producing run would need to produce the same memoized value to trigger this specific case here which does not sound like something that will happen too often imo.
Why was it again that #636 had to change the tracking to track both self and its dependencies?
Not sure, will have to dig into that whether that was accidentally or required.
There was a problem hiding this comment.
Good point. Although this can happen frequently with nested queries where the outer queries still produce the same result and it's only the inner queries that changed. E.g. some function local types changed, but the function return type remains unchanged.
It would be nice if it can be avoided but is also not critical. Adding a comment on why it doesn't use ::default might be useful (with a possible TOOD)
There was a problem hiding this comment.
Restored the tracking behavior, it was accidental.
Veykril
force-pushed
the
veykril/push-wluqnwrsrsow
branch
from
51e64d7 to
496e076
Compare
Rebased version of #641