fix: incremental batches#18035
Conversation
tiny self-mergeable change extracted from #18035 — just turns `each_array` from a `Derived<any[]>` to a `Derived<V[]>`
some more stuff extracted from #18035
|
This also fixes #18355, though we should add a test for it |
dummdidumm
left a comment
There was a problem hiding this comment.
Gotta say this is surprisingly readable/understandeable, nice! (maybe also because I spent more time with this reactivity system than I'd like to admit).
Has a few rough edges (eager effect) and the perf needs to get better but otherwise this is looking good.
| // we treat the earlier values as "already applied". This way we don't need to rerun async | ||
| // effects of the earlier batch in case they are merged. | ||
| // As a result you can think of batch_values as having the latest values of all intersecting | ||
| // batches up until this batch. |
There was a problem hiding this comment.
Why did you remove this comment?
| */ | ||
| #maybe_dirty_effects = new Set(); | ||
| /** @type {Map<Reaction, number>} */ | ||
| cvs = new Map(); |
There was a problem hiding this comment.
what does cvs stand for? This variable need a bit of jsdoc
| * Tuple format: [value, is_derived] (note: is_derived is false for deriveds, too, if they were overridden via assignment) | ||
| * They keys of this map are identical to `this.#previous` | ||
| * @type {Map<Value, [any, boolean]>} | ||
| * @type {Map<Value, ValueSnapshot<unknown>>} |
There was a problem hiding this comment.
comment is outdated, needs to be updated (though see comment further below that we're not using value.f & DERIVED for a reason)
| var current = [...batch.current.keys()].filter( | ||
| (source) => !(/** @type {[any, boolean]} */ (batch.current.get(source))[1]) | ||
| ); | ||
| var current = Array.from(batch.current.keys()).filter((value) => (value.f & DERIVED) === 0); |
There was a problem hiding this comment.
I fear this may be buggy - a derived can be overridden and then acts as state temporarily. This is the reason why it was a [value, is_derived] tuple and not just checking the flag. I see that you removed it everywhere else as well - what was the rationale?
| deps: null | Value[]; | ||
| /** An AbortController that aborts when the signal is destroyed */ | ||
| ac: null | AbortController; | ||
| /** Check version */ |
There was a problem hiding this comment.
needs a bit more elaboration what the check version is for
| return; | ||
| } | ||
| if (deps !== null) { | ||
| cv = -Infinity; |
There was a problem hiding this comment.
elsewhere you do -1, shouldn't that be enough here, too?
|
|
||
| if (derived.v === UNINITIALIZED) { | ||
| // assigning before first read — execute to track dependencies | ||
| execute_derived(derived); |
There was a problem hiding this comment.
this seems different to before - if a derived should be updated, but someone overrides it before it has a chance to, then the dependencies will be wrong/outdated. So I think we need to keep checking if it's dirty.
| active_reaction !== null && | ||
| (active_reaction.f & REACTION_RAN) !== 0 | ||
| ) { | ||
| reaction.f |= WAS_MARKED; |
There was a problem hiding this comment.
i see you had to apply some more work to WAS_MARKED - I really think we should just ditch it in favor of the Set heuristic
| var unmount = component_root(() => { | ||
| var anchor_node = anchor ?? target.appendChild(create_text()); | ||
|
|
||
| push({}); |
There was a problem hiding this comment.
drive-by or why is it necessary?
| // ensure that if any of those untracked writes result in re-invalidation | ||
| // of the current effect, then that happens accordingly | ||
| if ( | ||
| !async_mode_flag && |
There was a problem hiding this comment.
wait, does that mean we chose to not do this anymore in async mode? I didn't even realize we made that change back then.
Edit: Ok I'm pretty sure we did not do that change, this is changing behavior. It's just by accident in another file that this works. Pushing a fix.
…initial run not marking without not bailing makes two tests fail, revealing that the "don't self-invalidate in async mode" change would actually be a subtle breaking change
|
Opened #18362 with an idea how to tackle eager effects + async while getting rid of the eager flag. |
Replaces the ad-hoc `STATE_EAGER_EFFECT` flag which makes eager effects over-execute with a mechanism that is two-fold: 1. eager versions are sorted by batch. If a batch belongs to a fork, that way it's made sure it doesn't "escape" that fork 2. batches now have an `eager` field, which is set for forks with eager work. It's flushed on commit just before the main commit. This way the eager work happens separately and eagerly just before the main fork commits, which itself may not be ready yet. Additionally in `sources.js` we make sure `active_batch` is set properly when flushing eager effects. --------- Co-authored-by: Rich Harris <rich.harris@vercel.com> Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com> Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com> Co-authored-by: Rich-Harris <hello@rich-harris.dev>
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