-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: update untracked writes for BLOCK_EFFECT
s too
#16110
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9b3677d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looking at the benchmark the overall time spent with GC has more than doubled, so as you say this is likely not the right fix (also because what would even be the difference between maybe dirty and dirty now?) |
Yeah to be fair I was even surprised no test failed but I guess it's just removing an optimization. I guess the right fix would be to find a way to set back this kind of Mmm...I guess technically the derived should be executed again since the value might change 🤔 |
Ok i tried a bit more, it's getting late: so I think the actual problem is that when we mark the reactions of the derived of derived the first time it's reactions are not yet populated because we are within the function call in Still need to find a proper solution. |
Ok this is a different fix: it's basically extending the logic for untracked writes to |
@dummdidumm looking at the benchmarks on main i see I wonder if this could be the fix. |
MAYBE_DIRTY
reactions tooBLOCK_EFFECT
s too
Tbf they seems pretty similar running
|
I'm not even sure the benchmark makes use of blocks. However, this looks like the right fix to me. |
Hey I was also working on #16090 and i just found out running in production mode behaves correctly without any changes from the main branch. |
Yeah however the other bug this closes is still present even in production builds |
I'm closing my other PR ^^. But will spend some time investigating why prod mode fixes that thing, might be revealing something odd |
Closes #16090
Closes #16104
This fixes both of this and all the test are actually green. However I'm not convinced this is the right fix because it's a bit greedy: the problem is the following.
MAYBE_DIRTY
MAYBE_DIRTY
MAYBE_DIRTY
his reactions are not marked/scheduledNow including
MAYBE_DIRTY
in the flags to mark/schedule fixes it but i think it will schedule more than necessary and i don't know if this could have performance issues. The best action would be to mark the derived as clean if we can somehow detect that it was marked after being read but it's a bit of a weird situation (becuase if you, for example, read it again in an effect the bug it's not present)Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint