Merge remote-tracking branch 'origin/main' into svelte-custom-renderer

svelte-custom-renderer
paoloricciuti 3 days ago
commit a1d9bb6936

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: prevent effect tree of batches from interfering with each other

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: remove `WAS_MARKED` flag in favor of `Set`

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: keep `$state.eager` when used as a variable initializer

@ -13,7 +13,7 @@ import {
sbench_create_4to1, sbench_create_4to1,
sbench_create_signals sbench_create_signals
} from './sbench.js'; } from './sbench.js';
import { fileURLToPath } from 'node:url'; import { fileURLToPath, pathToFileURL } from 'node:url';
import { create_test } from './util.js'; import { create_test } from './util.js';
// This benchmark has been adapted from the js-reactivity-benchmark (https://github.com/milomg/js-reactivity-benchmark) // This benchmark has been adapted from the js-reactivity-benchmark (https://github.com/milomg/js-reactivity-benchmark)
@ -39,7 +39,8 @@ for (const file of fs.readdirSync(`${dirname}/tests`)) {
const name = file.replace('.bench.js', ''); const name = file.replace('.bench.js', '');
const module = await import(`${dirname}/tests/${file}`); const module_url = pathToFileURL(path.join(dirname, 'tests', file));
const module = await import(module_url.href);
const { owned, unowned } = create_test(name, module.default); const { owned, unowned } = create_test(name, module.default);
reactivity_benchmarks.push(owned, unowned); reactivity_benchmarks.push(owned, unowned);

@ -35,6 +35,7 @@ export function VariableDeclaration(node, context) {
rune === '$inspect' || rune === '$inspect' ||
rune === '$inspect.trace' || rune === '$inspect.trace' ||
rune === '$state.snapshot' || rune === '$state.snapshot' ||
rune === '$state.eager' ||
rune === '$host' rune === '$host'
) { ) {
declarations.push(/** @type {VariableDeclarator} */ (context.visit(declarator))); declarations.push(/** @type {VariableDeclarator} */ (context.visit(declarator)));

@ -51,15 +51,6 @@ export const EFFECT_PRESERVED = 1 << 19;
export const USER_EFFECT = 1 << 20; export const USER_EFFECT = 1 << 20;
export const EFFECT_OFFSCREEN = 1 << 25; export const EFFECT_OFFSCREEN = 1 << 25;
// Flags exclusive to deriveds
/**
* Tells that we marked this derived and its reactions as visited during the "mark as (maybe) dirty"-phase.
* Will be lifted during execution of the derived and during checking its dirty state (both are necessary
* because a derived might be checked but not executed). This is a pure performance optimization flag and
* should not be used for any other purpose!
*/
export const WAS_MARKED = 1 << 16;
// Flags used for async // Flags used for async
export const REACTION_IS_UPDATING = 1 << 21; export const REACTION_IS_UPDATING = 1 << 21;
export const ASYNC = 1 << 22; export const ASYNC = 1 << 22;

@ -15,7 +15,6 @@ import {
MAYBE_DIRTY, MAYBE_DIRTY,
RENDER_EFFECT, RENDER_EFFECT,
ROOT_EFFECT, ROOT_EFFECT,
WAS_MARKED,
MANAGED_EFFECT MANAGED_EFFECT
} from '#client/constants'; } from '#client/constants';
import { snapshot } from '../../shared/clone.js'; import { snapshot } from '../../shared/clone.js';
@ -204,7 +203,6 @@ export function log_reactions(signal) {
if ((flags & DIRTY) !== 0) names.push('DIRTY'); if ((flags & DIRTY) !== 0) names.push('DIRTY');
if ((flags & MAYBE_DIRTY) !== 0) names.push('MAYBE_DIRTY'); if ((flags & MAYBE_DIRTY) !== 0) names.push('MAYBE_DIRTY');
if ((flags & CONNECTED) !== 0) names.push('CONNECTED'); if ((flags & CONNECTED) !== 0) names.push('CONNECTED');
if ((flags & WAS_MARKED) !== 0) names.push('WAS_MARKED');
if ((flags & INERT) !== 0) names.push('INERT'); if ((flags & INERT) !== 0) names.push('INERT');
if ((flags & DESTROYED) !== 0) names.push('DESTROYED'); if ((flags & DESTROYED) !== 0) names.push('DESTROYED');

@ -22,7 +22,6 @@ import {
import { async_mode_flag } from '../../flags/index.js'; import { async_mode_flag } from '../../flags/index.js';
import { deferred, define_property, includes } from '../../shared/utils.js'; import { deferred, define_property, includes } from '../../shared/utils.js';
import { import {
active_effect,
active_reaction, active_reaction,
get, get,
increment_write_version, increment_write_version,
@ -38,7 +37,6 @@ import { eager_effect, teardown, unlink_effect } from './effects.js';
import { defer_effect } from './utils.js'; import { defer_effect } from './utils.js';
import { UNINITIALIZED } from '../../../constants.js'; import { UNINITIALIZED } from '../../../constants.js';
import { set_signal_status } from './status.js'; import { set_signal_status } from './status.js';
import { legacy_is_updating_store } from './store.js';
import { invariant } from '../../shared/dev.js'; import { invariant } from '../../shared/dev.js';
import { log_effect_tree } from '../dev/debug.js'; import { log_effect_tree } from '../dev/debug.js';
import { OBSOLETE } from './deriveds.js'; import { OBSOLETE } from './deriveds.js';
@ -160,10 +158,17 @@ export class Batch {
#deferred = null; #deferred = null;
/** /**
* The root effects that need to be flushed * Effects that were scheduled in this batch but not yet 'resolved' into the
* root effects that need to be flushed. Resolving the upwards traversal that
* marks the path to each effect on the shared effect tree (see #resolve) is
* deferred until the batch is processed, so that the markers are created and
* consumed within a single traversal. Scheduling into other batches (which can
* happen concurrently, e.g. while a batch is committed) can therefore never
* observe (and be confused by) this batch's markers.
* May contain duplicates deduplication happens during resolving
* @type {Effect[]} * @type {Effect[]}
*/ */
#roots = []; #scheduled = [];
/** /**
* Effects created while this batch was active. * Effects created while this batch was active.
@ -273,14 +278,57 @@ export class Batch {
this.#unskipped_branches.add(effect); this.#unskipped_branches.add(effect);
} }
#process() { /**
this.#started = true; * Convert the effects that were scheduled in this batch into the root effects
* that need to be traversed, marking the path to each effect (by clearing the
* `CLEAN` flag on ancestor branches) so that the traversal can find them.
* This happens right before traversal rather than at scheduling time, so that
* the markers left on the (shared) effect tree are created and consumed within
* a single traversal scheduling into other batches can never observe them
* @returns {Effect[]}
*/
#resolve() {
/** @type {Effect[]} */
var roots = [];
if (flush_count++ > 1000) { for (const effect of this.#scheduled) {
this.#unlink(); // skip effects that are destroyed, or that already ran (e.g. because
infinite_loop_guard(); // they were reached by the traversal that preceded a drain iteration,
// or because they were scheduled twice)
if ((effect.f & DESTROYED) !== 0 || (effect.f & (DIRTY | MAYBE_DIRTY)) === 0) continue;
var e = effect;
var covered = false;
while (e.parent !== null) {
e = e.parent;
var flags = e.f;
if ((flags & (ROOT_EFFECT | BRANCH_EFFECT)) !== 0) {
if ((flags & CLEAN) === 0) {
// the path to the root was already marked, meaning the
// root was already collected — nothing left to do
covered = true;
break;
}
e.f ^= CLEAN;
}
}
if (!covered) {
roots.push(e);
}
} }
this.#scheduled = [];
return roots;
}
#process() {
this.#started = true;
if (DEV) { if (DEV) {
// track all the values that were updated during this flush, // track all the values that were updated during this flush,
// so that they can be reset afterwards // so that they can be reset afterwards
@ -304,9 +352,6 @@ export class Batch {
this.schedule(e); this.schedule(e);
} }
const roots = this.#roots;
this.#roots = [];
this.apply(); this.apply();
/** @type {Effect[]} */ /** @type {Effect[]} */
@ -321,18 +366,28 @@ export class Batch {
*/ */
var updates = (legacy_updates = []); var updates = (legacy_updates = []);
for (const root of roots) { // Effects can be scheduled during traversal (e.g. because a parent each/await/etc
try { // block updated an internal source, or because an effect invalidated itself)
this.#traverse(root, effects, render_effects); // hence we loop until there are no more scheduled effects.
} catch (e) { while (this.#scheduled.length > 0) {
reset_all(root); if (flush_count++ > 1000) {
// If there's no async work left, this branch is now dead and needs this.#unlink();
// to be discarded to not become a zombie that is never cleaned up. infinite_loop_guard(); // TODO try to reset_all() here?
// See https://github.com/sveltejs/svelte/issues/18221#issuecomment-4497918414 }
// for a (non-minimal) reproduction that demonstrates a case where this is necessary
// to not get follow-up false-positives via "batch has scheduled roots" invariant errors. for (const root of this.#resolve()) {
if (!this.#is_deferred()) this.discard(); try {
throw e; this.#traverse(root, effects, render_effects);
} catch (e) {
reset_all(root);
// If there's no async work left, this branch is now dead and needs
// to be discarded to not become a zombie that is never cleaned up.
// See https://github.com/sveltejs/svelte/issues/18221#issuecomment-4497918414
// for a (non-minimal) reproduction that demonstrates a case where this is necessary
// to not get follow-up false-positives via "batch has scheduled roots" invariant errors.
if (!this.#is_deferred()) this.discard();
throw e;
}
} }
} }
@ -393,7 +448,7 @@ export class Batch {
var next_batch = /** @type {Batch | null} */ (/** @type {unknown} */ (current_batch)); var next_batch = /** @type {Batch | null} */ (/** @type {unknown} */ (current_batch));
if (this.#pending === 0 && (this.#roots.length === 0 || next_batch !== null)) { if (this.#pending === 0 && (this.#scheduled.length === 0 || next_batch !== null)) {
this.#unlink(); this.#unlink();
// Order matters here - we need to commit and THEN continue flushing new batches, not the other way around, // Order matters here - we need to commit and THEN continue flushing new batches, not the other way around,
@ -408,12 +463,15 @@ export class Batch {
} }
// Edge case: During traversal new branches might create effects that run immediately and set state, // Edge case: During traversal new branches might create effects that run immediately and set state,
// causing an effect and therefore a root to be scheduled again. We need to traverse the current batch // causing an effect to be scheduled again. We need to traverse the current batch
// once more in that case - most of the time this will just clean up dirty branches. // once more in that case - most of the time this will just clean up dirty branches.
if (this.#roots.length > 0) { if (this.#scheduled.length > 0) {
if (next_batch !== null) { if (next_batch !== null) {
const batch = next_batch; for (const e of this.#scheduled) {
batch.#roots.push(...this.#roots.filter((r) => !batch.#roots.includes(r))); next_batch.#scheduled.push(e);
}
this.#scheduled = [];
} else { } else {
next_batch = this; next_batch = this;
} }
@ -712,7 +770,7 @@ export class Batch {
// The microtask queue can contain the batch already scheduled to run right // The microtask queue can contain the batch already scheduled to run right
// after this one is finished, so throwing the invariant would be wrong here. // after this one is finished, so throwing the invariant would be wrong here.
if (DEV && !batch.#decrement_queued) { if (DEV && !batch.#decrement_queued) {
invariant(batch.#roots.length === 0, 'Batch has scheduled roots'); invariant(batch.#scheduled.length === 0, 'Batch has scheduled effects');
} }
// A batch was unskipped in a later batch -> tell prior batches to unskip it, too // A batch was unskipped in a later batch -> tell prior batches to unskip it, too
@ -768,14 +826,12 @@ export class Batch {
// Only apply and traverse when we know we triggered async work with marking the effects // Only apply and traverse when we know we triggered async work with marking the effects
// and know this won't run anyway right afterwards // and know this won't run anyway right afterwards
if (batch.#roots.length > 0 && !batch.#decrement_queued) { if (batch.#scheduled.length > 0 && !batch.#decrement_queued) {
batch.apply(); batch.apply();
for (var root of batch.#roots) { for (var root of batch.#resolve()) {
batch.#traverse(root, [], []); batch.#traverse(root, [], []);
} }
batch.#roots = [];
} }
batch.deactivate(); batch.deactivate();
@ -939,43 +995,7 @@ export class Batch {
return; return;
} }
var e = effect; this.#scheduled.push(effect);
while (e.parent !== null) {
e = e.parent;
var flags = e.f;
// if the effect is being scheduled because a parent (each/await/etc) block
// updated an internal source, or because a branch is being unskipped,
// bail out or we'll cause a second flush
if (collected_effects !== null && e === active_effect) {
if (async_mode_flag) return;
// in sync mode, render effects run during traversal. in an extreme edge case
// — namely that we're setting a value inside a derived read during traversal —
// they can be made dirty after they have already been visited, in which
// case we shouldn't bail out. we also shouldn't bail out if we're
// updating a store inside a `$:`, since this might invalidate
// effects that were already visited
if (
(active_reaction === null || (active_reaction.f & DERIVED) === 0) &&
!legacy_is_updating_store
) {
return;
}
}
if ((flags & (ROOT_EFFECT | BRANCH_EFFECT)) !== 0) {
if ((flags & CLEAN) === 0) {
// branch is already dirty, bail
return;
}
e.f ^= CLEAN;
}
}
this.#roots.push(e);
} }
#unlink() { #unlink() {

@ -9,7 +9,6 @@ import {
EFFECT_PRESERVED, EFFECT_PRESERVED,
STALE_REACTION, STALE_REACTION,
ASYNC, ASYNC,
WAS_MARKED,
DESTROYED, DESTROYED,
CLEAN, CLEAN,
REACTION_RAN, REACTION_RAN,
@ -365,7 +364,6 @@ export function execute_derived(derived) {
stack.push(derived); stack.push(derived);
derived.f &= ~WAS_MARKED;
destroy_derived_effects(derived); destroy_derived_effects(derived);
value = update_reaction(derived); value = update_reaction(derived);
} finally { } finally {
@ -375,7 +373,6 @@ export function execute_derived(derived) {
} }
} else { } else {
try { try {
derived.f &= ~WAS_MARKED;
destroy_derived_effects(derived); destroy_derived_effects(derived);
value = update_reaction(derived); value = update_reaction(derived);
} finally { } finally {

@ -25,10 +25,7 @@ import {
MAYBE_DIRTY, MAYBE_DIRTY,
BLOCK_EFFECT, BLOCK_EFFECT,
ROOT_EFFECT, ROOT_EFFECT,
ASYNC, ASYNC
WAS_MARKED,
CONNECTED,
REACTION_IS_UPDATING
} from '#client/constants'; } from '#client/constants';
import * as e from '../errors.js'; import * as e from '../errors.js';
import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js'; import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js';
@ -171,6 +168,17 @@ export function set(source, value, should_proxy = false) {
return internal_set(source, new_value, legacy_updates); return internal_set(source, new_value, legacy_updates);
} }
/**
* A set of signals we have already seen while traversing in mark_reactions.
* Not always set to balance the common case of sources only having a couple
* of (transitive) dependencies (where always creating a Set would be bad for perf)
* with the edge case of extremely deep or wide dependency arrays with cycles.
* @type {Set<any> | null}
*/
var seen = null;
/** Number of transitive dependencies, see {@link seen} for more info */
var count_deps = 0;
/** /**
* @template V * @template V
* @param {Source<V>} source * @param {Source<V>} source
@ -240,7 +248,10 @@ export function internal_set(source, value, updated_during_traversal = null) {
// For debugging, in case you want to know which reactions are being scheduled: // For debugging, in case you want to know which reactions are being scheduled:
// log_reactions(source); // log_reactions(source);
seen = null;
count_deps = 0;
mark_reactions(source, DIRTY, updated_during_traversal); mark_reactions(source, DIRTY, updated_during_traversal);
seen = null;
// It's possible that the current reaction might not have up-to-date dependencies // It's possible that the current reaction might not have up-to-date dependencies
// whilst it's actively running. So in the case of ensuring it registers the reaction // whilst it's actively running. So in the case of ensuring it registers the reaction
@ -347,6 +358,18 @@ function mark_reactions(signal, status, updated_during_traversal) {
var runes = is_runes(); var runes = is_runes();
var length = reactions.length; var length = reactions.length;
count_deps += length;
// Activate the `seen` Set if we think from the unusually high number of deps that
// there might be cycles in the graph, to avoid repeated lookups for reactions
// Example: https://github.com/sveltejs/svelte/issues/16658 has a graph with one source
// reaching ~10000 distinct deriveds/effects each, resulting in 65 million walks through repeated visits.
if (count_deps > 100000 && seen === null) seen = new Set();
if (seen !== null) {
if (seen.has(signal)) return;
seen.add(signal);
}
for (var i = 0; i < length; i++) { for (var i = 0; i < length; i++) {
var reaction = reactions[i]; var reaction = reactions[i];
var flags = reaction.f; var flags = reaction.f;
@ -370,18 +393,7 @@ function mark_reactions(signal, status, updated_during_traversal) {
var derived = /** @type {Derived} */ (reaction); var derived = /** @type {Derived} */ (reaction);
batch_values?.delete(derived); batch_values?.delete(derived);
mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal);
if ((flags & WAS_MARKED) === 0) {
// Only connected deriveds being executed outside the update cycle can be reliably unmarked right away
if (
flags & CONNECTED &&
(active_effect === null || (active_effect.f & REACTION_IS_UPDATING) === 0)
) {
reaction.f |= WAS_MARKED;
}
mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal);
}
} else if (not_dirty) { } else if (not_dirty) {
var effect = /** @type {Effect} */ (reaction); var effect = /** @type {Effect} */ (reaction);

@ -1,24 +1,7 @@
/** @import { Derived, Effect, Value } from '#client' */ /** @import { Effect } from '#client' */
import { CLEAN, DERIVED, DIRTY, MAYBE_DIRTY, WAS_MARKED } from '#client/constants'; import { CLEAN, DIRTY, MAYBE_DIRTY } from '#client/constants';
import { set_signal_status } from './status.js'; import { set_signal_status } from './status.js';
/**
* @param {Value[] | null} deps
*/
function clear_marked(deps) {
if (deps === null) return;
for (const dep of deps) {
if ((dep.f & DERIVED) === 0 || (dep.f & WAS_MARKED) === 0) {
continue;
}
dep.f ^= WAS_MARKED;
clear_marked(/** @type {Derived} */ (dep).deps);
}
}
/** /**
* @param {Effect} effect * @param {Effect} effect
* @param {Set<Effect>} dirty_effects * @param {Set<Effect>} dirty_effects
@ -31,10 +14,6 @@ export function defer_effect(effect, dirty_effects, maybe_dirty_effects) {
maybe_dirty_effects.add(effect); maybe_dirty_effects.add(effect);
} }
// Since we're not executing these effects now, we need to clear any WAS_MARKED flags
// so that other batches can correctly reach these effects during their own traversal
clear_marked(effect.deps);
// mark as clean so they get scheduled if they depend on pending async state // mark as clean so they get scheduled if they depend on pending async state
set_signal_status(effect, CLEAN); set_signal_status(effect, CLEAN);
} }

@ -21,7 +21,6 @@ import {
REACTION_IS_UPDATING, REACTION_IS_UPDATING,
STALE_REACTION, STALE_REACTION,
ERROR_VALUE, ERROR_VALUE,
WAS_MARKED,
MANAGED_EFFECT, MANAGED_EFFECT,
REACTION_RAN REACTION_RAN
} from './constants.js'; } from './constants.js';
@ -165,10 +164,6 @@ export function is_dirty(reaction) {
return true; return true;
} }
if (flags & DERIVED) {
reaction.f &= ~WAS_MARKED;
}
if ((flags & MAYBE_DIRTY) !== 0) { if ((flags & MAYBE_DIRTY) !== 0) {
var dependencies = /** @type {Value[]} */ (reaction.deps); var dependencies = /** @type {Value[]} */ (reaction.deps);
var length = dependencies.length; var length = dependencies.length;
@ -409,11 +404,8 @@ function remove_reaction(signal, dependency) {
) { ) {
var derived = /** @type {Derived} */ (dependency); var derived = /** @type {Derived} */ (dependency);
// If we are working with a derived that is owned by an effect, then mark it as being
// disconnected and remove the mark flag, as it cannot be reliably removed otherwise
if ((derived.f & CONNECTED) !== 0) { if ((derived.f & CONNECTED) !== 0) {
derived.f ^= CONNECTED; derived.f ^= CONNECTED;
derived.f &= ~WAS_MARKED;
} }
// In a fork it's possible that a derived is executed and gets reactions, then commits, but is // In a fork it's possible that a derived is executed and gets reactions, then commits, but is

@ -0,0 +1,41 @@
import { tick } from 'svelte';
import { test } from '../../test';
const buttons = '<button>a</button> <button>b</button> <button>shift</button> <button>pop</button>';
export default test({
async test({ assert, target }) {
await tick();
const [a, b, , pop] = target.querySelectorAll('button');
const shift = target.querySelectorAll('button')[2];
assert.htmlEqual(target.innerHTML, `${buttons} <p>a</p><p>a</p><p>aa</p><p>1</p>`);
// start two independent batches, both blocked on their awaited expression
a.click();
await tick();
assert.htmlEqual(target.innerHTML, `${buttons} <p>a</p><p>a</p><p>aa</p><p>1</p>`);
b.click();
await tick();
assert.htmlEqual(target.innerHTML, `${buttons} <p>a</p><p>a</p><p>aa</p><p>1</p>`);
// resolve the newer (b) batch first. Committing it must not commit the
// still-pending `a` batch, whose async work has not completed — `a` must
// still read 'a', and the unrelated `c` update must not be blocked
pop.click();
await tick();
assert.htmlEqual(target.innerHTML, `${buttons} <p>a</p><p>b</p><p>ab</p><p>2</p>`);
// stale promise from the `a` batch's first run — resolving it does nothing
shift.click();
await tick();
assert.htmlEqual(target.innerHTML, `${buttons} <p>a</p><p>b</p><p>ab</p><p>2</p>`);
// the `a` batch's re-run await ('bb') resolves — everything is committed
shift.click();
await tick();
assert.htmlEqual(target.innerHTML, `${buttons} <p>b</p><p>b</p><p>bb</p><p>2</p>`);
}
});

@ -0,0 +1,38 @@
<script>
let a = $state('a');
let b = $state('a');
let c = $state(0);
let n = 0;
let queued = [];
let first = true;
function push(v) {
if (first) {
first = false;
return v;
}
return new Promise((resolve) => {
queued.push(() => resolve(v));
});
}
// when a batch changing `b` commits, this writes `c` during that batch's
// flush phase — the write lands in a freshly-created batch
$effect(() => {
b;
c = ++n;
});
</script>
<button onclick={() => a = 'b'}>a</button>
<button onclick={() => b = 'b'}>b</button>
<button onclick={() => queued.shift()?.()}>shift</button>
<button onclick={() => queued.pop()?.()}>pop</button>
<p>{a}</p>
<p>{b}</p>
<p>{await push(a + b)}</p>
<p>{c}</p>

@ -0,0 +1,5 @@
import { test } from '../../test';
export default test({
html: `<p>20</p>`
});

@ -0,0 +1,7 @@
<script>
let n = $state(2);
let d = $derived(n * 10);
let x = $state.eager(d);
</script>
<p>{x}</p>
Loading…
Cancel
Save