-
Notifications
You must be signed in to change notification settings - Fork 202
Fix cache invalidation when cycle head becomes non-head #1014
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
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #1014 will degrade performances by 6.01%Comparing Summary
Benchmarks breakdown
|
| Provisional { | ||
| iteration: IterationCount, | ||
| verified_at: Revision, | ||
| cycle_heads: &'db CycleHeads, |
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 decided to merge Ingredient::cycle_heads and Ingredient::provisional_status into provisional_status so that validate_same_iteration doesn't require an additional method call
This PR fixes a
assertionleft == rightfailed: Can't merge cycle heads, but panicked withClassLiteral < 'db >::is_typed_dict_(Id(3007)): execute: too many cycle iterationsinstead.` panic in ty.This panic is normally due to:
validate_same_iterationincorrectly returnsfalse)I can rule out 1. because I can reproduce the issue when running ty single-threaded. So it's two.
The smallest example I've been able to reduce it to is https://github.com/astral-sh/ruff/pull/21059/files#diff-601b44c1045125b03854a01be732f5bbda7b9b70673cbdc983bf9acc952c0a0d which still produced ~300MB of log files. Now, I didn't trace through the log file entirely (I also asked claude and it was very confused and always wanted to remove the assertion because that's obviously the correct fix to make the panic go away...), but my understanding of what's happening (and the fix confirms) is.:
Cthat hasBas its cycle headThe crucial point is that
Bswitches from being a cycle head to being a regular cycle participant. The issue with that is thatAdoesn't updateB'siteration_countwhen the iteration completes because it only does that for cycle heads (and collecting all queries participating in a query would be sort of expensive?).When we now pull
Cin a later iteration,validate_same_iterationiterates over all its cycle heads (B), and check if the iteration count still matches. Which is the case becauseAdidn't updateB's iteration count.The solution in this PR is that
validate_same_iterationshould also check whether the most recent memo for a query's cycle head is still a cycle head. If it isn't, then the query is obviously outdated and needs to be re-run.Testing
I tried very hard to write a test, I also employed Claude (with different models) and, while we all produced a lot of test code, none of us managed to write a test that triggers this condition successfully.
That's why I opted to write a regression test in ty instead astral-sh/ruff#21059