Conversation
|
Updated benchmarks — now much more of a toss-up (though
|
extracted from #18035 — this should have been part of #18047 but I missed it off, oops. Will self-merge so it doesn't get in the way of #18035, and because the `performance-investigation` skill is predicated on this code existing --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| return () => { | ||
| flushSync(); | ||
| assert.deepEqual(log, [20]); | ||
| assert.deepEqual(log, [20, 20]); |
There was a problem hiding this comment.
This is the one observable-in-the-test-suite behaviour change (though there are things that work now that didn't work before; I should add tests). At first I thought it indicated a bug, but the more I think about it the more I think the previous behaviour is buggy — the effect should run again.
This makes the benchmarks slightly more honest — it adds the async flag, and a benchmark that tests the (common) scenario where most effects are clean. Relevant to #18035, will self-merge so we can do a fresh comparison
|
Okay, tests are all passing, but I update the benchmarks so that they didn't gloss over certain changes in this branch and they show some significant regressions: DetailsSo the work isn't done yet. Still hopeful we can land this though, it feels pretty nice to me code-wise |
This overhauls how batches work. The idea is that instead of reactions having both a write version and a
DIRTY(orMAYBE_DIRTY) flag, dirtiness is determined solely by comparing the write versions of a reaction's dependencies with the check version of the reaction itself.When processing a batch, we 'overlay' the latest values with
batch_values,batch_cvsandbatch_wvs(which could probably be better-named). This means that we can track whether or not a derived/effect is dirty within the batch, rather than potentially re-running it unnecessarily as a batch is reprocessed following a promise resolution.If I'm honest, this hasn't really worked out the way I hoped. It makes the logic quite a bit simpler in some cases, but there's actually more code now (though it could probably be massaged down). But worse, it has a very detrimental effect on performance:
results of `pnpm bench:compare`
It's possible that this could be alleviated with a few small tweaks (e.g. writing the overlay directly to the signals, and reverting at the end of batch processing) but as of yet I don't know.
It does help a lot with #17908, which isn't nothing. (In fact that was the original motivation for this work — the fact that we over-execute each block effects is a bad bug, and I wasn't able to fix it any other way; maybe I just need to take another run at it.)
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 testand lint the project withpnpm lint