fix: remove `WAS_MARKED` flag in favor of `Set`

Removes the `WAS_MARKED` logic in favor of a simple `Set` heuristic: If `mark_reactions` goes beyond a certain depth or there are many reactions on a certain signal, initialize it, otherwise keep it `null`. Balances the common case of not having many transitive dependencies with the edge case of cyclic dependencies as was seen in #16658

Fixes #18123

.changeset/grumpy-pens-press.md

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

packages/svelte/src/internal/client/constants.js

@@ -44,14 +44,6 @@ export const EFFECT_PRESERVED = 1 << 19;
 export const USER_EFFECT = 1 << 20;
 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).
- */
-export const WAS_MARKED = 1 << 16;
-
 // Flags used for async
 export const REACTION_IS_UPDATING = 1 << 21;
 export const ASYNC = 1 << 22;

packages/svelte/src/internal/client/dev/debug.js

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

packages/svelte/src/internal/client/reactivity/deriveds.js

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

packages/svelte/src/internal/client/reactivity/sources.js

@@ -25,9 +25,7 @@ import {
 	MAYBE_DIRTY,
 	BLOCK_EFFECT,
 	ROOT_EFFECT,
-	ASYNC,
-	WAS_MARKED,
-	CONNECTED
+	ASYNC
 } from '#client/constants';
 import * as e from '../errors.js';
 import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js';
@@ -171,6 +169,15 @@ export function set(source, value, should_proxy = false) {
 	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;
+
 /**
  * @template V
  * @param {Source<V>} source
@@ -234,7 +241,8 @@ export function internal_set(source, value, updated_during_traversal = null) {
 
 		// For debugging, in case you want to know which reactions are being scheduled:
 		// log_reactions(source);
-		mark_reactions(source, DIRTY, updated_during_traversal);
+		seen = null;
+		mark_reactions(source, DIRTY, updated_during_traversal, 0);
 
 		// 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
@@ -321,15 +329,23 @@ export function increment(source) {
  * @param {Value} signal
  * @param {number} status should be DIRTY or MAYBE_DIRTY
  * @param {Effect[] | null} updated_during_traversal
+ * @param {number} depth
  * @returns {void}
  */
-function mark_reactions(signal, status, updated_during_traversal) {
+function mark_reactions(signal, status, updated_during_traversal, depth) {
 	var reactions = signal.reactions;
 	if (reactions === null) return;
 
 	var runes = is_runes();
 	var length = reactions.length;
 
+	if (length > 100 || depth > 100) seen = new Set();
+
+	if (seen !== null) {
+		if (seen.has(signal)) return;
+		seen.add(signal);
+	}
+
 	for (var i = 0; i < length; i++) {
 		var reaction = reactions[i];
 		var flags = reaction.f;
@@ -354,15 +370,7 @@ function mark_reactions(signal, status, updated_during_traversal) {
 			var derived = /** @type {Derived} */ (reaction);
 
 			batch_values?.delete(derived);
-
-			if ((flags & WAS_MARKED) === 0) {
-				// Only connected deriveds can be reliably unmarked right away
-				if (flags & CONNECTED) {
-					reaction.f |= WAS_MARKED;
-				}
-
-				mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal);
-			}
+			mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal, depth + 1);
 		} else if (not_dirty) {
 			var effect = /** @type {Effect} */ (reaction);
 

packages/svelte/src/internal/client/reactivity/utils.js

@@ -1,24 +1,7 @@
-/** @import { Derived, Effect, Value } from '#client' */
-import { CLEAN, DERIVED, DIRTY, MAYBE_DIRTY, WAS_MARKED } from '#client/constants';
+/** @import { Effect } from '#client' */
+import { CLEAN, DIRTY, MAYBE_DIRTY } from '#client/constants';
 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 {Set<Effect>} dirty_effects
@@ -31,10 +14,6 @@ export function defer_effect(effect, dirty_effects, maybe_dirty_effects) {
 		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
 	set_signal_status(effect, CLEAN);
 }

packages/svelte/src/internal/client/runtime.js

@@ -21,7 +21,6 @@ import {
 	REACTION_IS_UPDATING,
 	STALE_REACTION,
 	ERROR_VALUE,
-	WAS_MARKED,
 	MANAGED_EFFECT,
 	REACTION_RAN
 } from './constants.js';
@@ -160,10 +159,6 @@ export function is_dirty(reaction) {
 		return true;
 	}
 
-	if (flags & DERIVED) {
-		reaction.f &= ~WAS_MARKED;
-	}
-
 	if ((flags & MAYBE_DIRTY) !== 0) {
 		var dependencies = /** @type {Value[]} */ (reaction.deps);
 		var length = dependencies.length;
@@ -392,11 +387,8 @@ function remove_reaction(signal, 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) {
 			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

packages/svelte/tests/runtime-runes/samples/derived-dep-set-while-rendering/Child.svelte

@@ -0,0 +1,7 @@
+<script>
+    import { store } from "./store.svelte.js";
+
+    // This write marks the derived in main.svelte before it has reactions added to it.
+    // This test checks that this does not cause the WAS_MARKED logic to incorrectly skip marking the derived subsequently.
+    store.set("child-init-write", Math.random());
+</script>

packages/svelte/tests/runtime-runes/samples/derived-dep-set-while-rendering/_config.js

@@ -0,0 +1,29 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		const [show, hide] = target.querySelectorAll('button');
+
+		hide.click();
+		flushSync();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>show</button>
+			<button>hide</button>
+		`
+		);
+
+		show.click();
+		flushSync();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>show</button>
+			<button>hide</button>
+			<div>visible</div>
+		`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/derived-dep-set-while-rendering/main.svelte

@@ -0,0 +1,14 @@
+<script>
+    import Child from "./Child.svelte";
+    import { store } from "./store.svelte.js";
+
+    const visible = $derived(store.get("visible"));
+    const visible2 = $derived(visible);
+</script>
+
+<button onclick={() => store.set("visible", true)}>show</button>
+<button onclick={() => store.set("visible", false)}>hide</button>
+{#if visible2}
+	<Child />
+	<div>visible</div>
+{/if}

packages/svelte/tests/runtime-runes/samples/derived-dep-set-while-rendering/store.svelte.js

@@ -0,0 +1,13 @@
+class RawStore {
+    values = $state.raw({ visible: true });
+
+    get(key) {
+        return this.values[key];
+    }
+
+    set(key, value) {
+        this.values = { ...this.values, [key]: value };
+    }
+}
+
+export const store = new RawStore();