From 7872d3b78cffe093a5375df10371a9de7e65c6c3 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 14 Oct 2025 14:08:10 +0200 Subject: [PATCH 1/5] fix: don't rerun async effects unnecessarily Since #16866, when an async effect runs multiple times, we rebase older batches and rerun those effects. This can have unintended consequences: In a case where an async effect only depends on a single source, and that single source was updated in a later batch, we know that we don't need to / should not rerun the older batch. This PR makes it so: We collect all the sources of older batches that are not part of the current batch that just committed, and then only mark those async effects as dirty which depend on one of those other sources. Fixes the bug I noticed while working on #16935 --- .changeset/wild-mirrors-take.md | 5 ++ .../src/internal/client/reactivity/batch.js | 61 ++++++++++++++----- .../internal/client/reactivity/deriveds.js | 7 +++ .../samples/async-resolve-stale/_config.js | 3 +- 4 files changed, 58 insertions(+), 18 deletions(-) create mode 100644 .changeset/wild-mirrors-take.md diff --git a/.changeset/wild-mirrors-take.md b/.changeset/wild-mirrors-take.md new file mode 100644 index 000000000000..faf28e7695c5 --- /dev/null +++ b/.changeset/wild-mirrors-take.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: don't rerun async effects unnecessarily diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 102d0670b664..5bae2b92a043 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -1,4 +1,4 @@ -/** @import { Derived, Effect, Source, Value } from '#client' */ +/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */ import { BLOCK_EFFECT, BRANCH_EFFECT, @@ -335,31 +335,43 @@ export class Batch { continue; } + /** @type {Source[]} */ + const sources = []; + for (const [source, value] of this.current) { if (batch.current.has(source)) { - if (is_earlier) { + if (is_earlier && value !== batch.current.get(source)) { // bring the value up to date batch.current.set(source, value); } else { - // later batch has more recent value, + // same value or later batch has more recent value, // no need to re-run these effects continue; } } - mark_effects(source); + sources.push(source); } - if (queued_root_effects.length > 0) { - current_batch = batch; - const revert = Batch.apply(batch); - - for (const root of queued_root_effects) { - batch.#traverse_effect_tree(root); + // Only reschedule an async effect if it depends on something else than + // the sources that were updated in this batch and that something else changed. + const others = [...batch.current.keys()].filter((s) => !this.current.has(s)); + if (others.length > 0) { + for (const source of sources) { + mark_effects(source, others); } - queued_root_effects = []; - revert(); + if (queued_root_effects.length > 0) { + current_batch = batch; + const revert = Batch.apply(batch); + + for (const root of queued_root_effects) { + batch.#traverse_effect_tree(root); + } + + queued_root_effects = []; + revert(); + } } } @@ -640,17 +652,34 @@ function flush_queued_effects(effects) { /** * This is similar to `mark_reactions`, but it only marks async/block effects - * so that these can re-run after another batch has been committed + * depending on one of the sources, so that these effects can re-run after + * another batch has been committed * @param {Value} value + * @param {Source[]} sources */ -function mark_effects(value) { +function mark_effects(value, sources) { if (value.reactions !== null) { + /** @param {Reaction} reaction */ + const depends_on_sources = (reaction) => { + if (reaction.deps !== null) { + for (const dep of reaction.deps) { + if (sources.includes(dep)) { + return true; + } + if ((dep.f & DERIVED) !== 0 && depends_on_sources(/** @type {Derived} */ (dep))) { + return true; + } + } + } + return false; + }; + for (const reaction of value.reactions) { const flags = reaction.f; if ((flags & DERIVED) !== 0) { - mark_effects(/** @type {Derived} */ (reaction)); - } else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0) { + mark_effects(/** @type {Derived} */ (reaction), sources); + } else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on_sources(reaction)) { set_signal_status(reaction, DIRTY); schedule_effect(/** @type {Effect} */ (reaction)); } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 076a91923680..eca9376b1c4d 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -171,6 +171,13 @@ export function async_derived(fn, location) { internal_set(signal, value); + // All prior async derived runs are now stale + for (const [b, d] of deferreds) { + deferreds.delete(b); + if (b === batch) break; + d.reject(STALE_REACTION); + } + if (DEV && location !== undefined) { recent_async_deriveds.add(signal); diff --git a/packages/svelte/tests/runtime-runes/samples/async-resolve-stale/_config.js b/packages/svelte/tests/runtime-runes/samples/async-resolve-stale/_config.js index bccf12562ad3..50bb414afc8b 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-resolve-stale/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-resolve-stale/_config.js @@ -20,7 +20,6 @@ export default test({ input.value = '12'; input.dispatchEvent(new Event('input', { bubbles: true })); await macrotask(6); - // TODO this is wrong (separate bug), this should be 3 | 12 - assert.htmlEqual(target.innerHTML, ' 5 | 12'); + assert.htmlEqual(target.innerHTML, ' 3 | 12'); } }); From 12b8ee6a85c21c91cf4ebac5ab9aefee36a504dd Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 14 Oct 2025 10:22:05 -0400 Subject: [PATCH 2/5] yoink out `depends_on_sources` --- .../src/internal/client/reactivity/batch.js | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 5bae2b92a043..2b084cbc7daf 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -659,27 +659,12 @@ function flush_queued_effects(effects) { */ function mark_effects(value, sources) { if (value.reactions !== null) { - /** @param {Reaction} reaction */ - const depends_on_sources = (reaction) => { - if (reaction.deps !== null) { - for (const dep of reaction.deps) { - if (sources.includes(dep)) { - return true; - } - if ((dep.f & DERIVED) !== 0 && depends_on_sources(/** @type {Derived} */ (dep))) { - return true; - } - } - } - return false; - }; - for (const reaction of value.reactions) { const flags = reaction.f; if ((flags & DERIVED) !== 0) { mark_effects(/** @type {Derived} */ (reaction), sources); - } else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on_sources(reaction)) { + } else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on(reaction, sources)) { set_signal_status(reaction, DIRTY); schedule_effect(/** @type {Effect} */ (reaction)); } @@ -687,6 +672,26 @@ function mark_effects(value, sources) { } } +/** + * @param {Reaction} reaction + * @param {Source[]} sources + */ +function depends_on(reaction, sources) { + if (reaction.deps !== null) { + for (const dep of reaction.deps) { + if (sources.includes(dep)) { + return true; + } + + if ((dep.f & DERIVED) !== 0 && depends_on(/** @type {Derived} */ (dep), sources)) { + return true; + } + } + } + + return false; +} + /** * @param {Effect} signal * @returns {void} From cffac17ab398ea67527f72ded316e9d8daf73d01 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 14 Oct 2025 17:23:22 +0200 Subject: [PATCH 3/5] Apply suggestion from @Rich-Harris Co-authored-by: Rich Harris --- packages/svelte/src/internal/client/reactivity/batch.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 181df0c8c1aa..965479110456 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -360,6 +360,10 @@ export class Batch { sources.push(source); } + if (sources.length === 0) { + continue; + } + // Only reschedule an async effect if it depends on something else than // the sources that were updated in this batch and that something else changed. const others = [...batch.current.keys()].filter((s) => !this.current.has(s)); From 625d8e06b0027a0c9cca0ff40107e2853b05ef9a Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 14 Oct 2025 17:26:32 +0200 Subject: [PATCH 4/5] Apply suggestion from @Rich-Harris Co-authored-by: Rich Harris --- packages/svelte/src/internal/client/reactivity/batch.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 965479110456..437c7adc61b7 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -637,8 +637,8 @@ function flush_queued_effects(effects) { /** * This is similar to `mark_reactions`, but it only marks async/block effects - * depending on one of the sources, so that these effects can re-run after - * another batch has been committed + * depending on `value` and at least one of the other `sources`, so that + * these effects can re-run after another batch has been committed * @param {Value} value * @param {Source[]} sources */ From 697445b903022aabb89220467a5473d6e4187bc5 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 14 Oct 2025 17:58:44 +0200 Subject: [PATCH 5/5] Update packages/svelte/src/internal/client/reactivity/batch.js Co-authored-by: Rich Harris --- packages/svelte/src/internal/client/reactivity/batch.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 437c7adc61b7..00932f099e2a 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -364,8 +364,7 @@ export class Batch { continue; } - // Only reschedule an async effect if it depends on something else than - // the sources that were updated in this batch and that something else changed. + // Re-run async/block effects that depend on distinct values changed in both batches const others = [...batch.current.keys()].filter((s) => !this.current.has(s)); if (others.length > 0) { for (const source of sources) {