chore: refactor internal signal dependency heuristic (#12881)

* chore: remove redundant signal logic

* more tweaks

* more tweaks

* refactor

* tweak

Author: trueadm

.changeset/hot-kangaroos-invite.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+chore: refactor internal signal dependency heuristic

packages/svelte/src/internal/client/dom/template.js

@@ -39,7 +39,7 @@ export function template(content, flags) {
 			return hydrate_node;
 		}
 
-		if (!node) {
+		if (node === undefined) {
 			node = create_fragment_from_html(has_start ? content : '<!>' + content);
 			if (!is_fragment) node = /** @type {Node} */ (get_first_child(node));
 		}

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

@@ -375,8 +375,10 @@ export function destroy_effect(effect, remove_dom = true) {
 	remove_reactions(effect, 0);
 	set_signal_status(effect, DESTROYED);
 
-	if (effect.transitions) {
-		for (const transition of effect.transitions) {
+	var transitions = effect.transitions;
+
+	if (transitions !== null) {
+		for (const transition of transitions) {
 			transition.stop();
 		}
 	}

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

@@ -139,8 +139,10 @@ function mark_reactions(signal, status) {
 	if (reactions === null) return;
 
 	var runes = is_runes();
+	var length = reactions.length;
 
-	for (var reaction of reactions) {
+	for (var i = 0; i < length; i++) {
+		var reaction = reactions[i];
 		var flags = reaction.f;
 
 		// Skip any effects that are already dirty

packages/svelte/src/internal/client/reactivity/types.d.ts

@@ -9,7 +9,7 @@ export interface Signal {
 
 export interface Value<V = unknown> extends Signal {
 	/** Signals that read from this signal */
-	reactions: null | Set<Reaction>;
+	reactions: null | Reaction[];
 	/** Equality function */
 	equals: Equals;
 	/** The latest value for this signal */

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

@@ -169,7 +169,7 @@ export function check_dirtiness(reaction) {
 
 			if ((flags & DISCONNECTED) !== 0) {
 				for (i = 0; i < dependencies.length; i++) {
-					(dependencies[i].reactions ??= new Set()).add(reaction);
+					(dependencies[i].reactions ??= []).push(reaction);
 				}
 
 				reaction.f ^= DISCONNECTED;
@@ -188,11 +188,11 @@ export function check_dirtiness(reaction) {
 
 				if (is_unowned) {
 					// TODO is there a more logical place to do this work?
-					if (!current_skip_reaction && !dependency?.reactions?.has(reaction)) {
+					if (!current_skip_reaction && !dependency?.reactions?.includes(reaction)) {
 						// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
 						// and the version hasn't changed, we still need to check that this reaction
 						// if linked to the dependency source – otherwise future updates will not be caught.
-						(dependency.reactions ??= new Set()).add(reaction);
+						(dependency.reactions ??= []).push(reaction);
 					}
 				}
 			}
@@ -291,26 +291,9 @@ export function update_reaction(reaction) {
 		var deps = reaction.deps;
 
 		if (new_deps !== null) {
-			var dependency;
 			var i;
 
-			if (deps !== null) {
-				/** All dependencies of the reaction, including those tracked on the previous run */
-				var array = skipped_deps === 0 ? new_deps : deps.slice(0, skipped_deps).concat(new_deps);
-
-				// If we have more than 16 elements in the array then use a Set for faster performance
-				// TODO: evaluate if we should always just use a Set or not here?
-				var set = array.length > 16 ? new Set(array) : null;
-
-				// Remove dependencies that should no longer be tracked
-				for (i = skipped_deps; i < deps.length; i++) {
-					dependency = deps[i];
-
-					if (set !== null ? !set.has(dependency) : !array.includes(dependency)) {
-						remove_reaction(reaction, dependency);
-					}
-				}
-			}
+			remove_reactions(reaction, skipped_deps);
 
 			if (deps !== null && skipped_deps > 0) {
 				deps.length = skipped_deps + new_deps.length;
@@ -323,7 +306,7 @@ export function update_reaction(reaction) {
 
 			if (!current_skip_reaction) {
 				for (i = skipped_deps; i < deps.length; i++) {
-					(deps[i].reactions ??= new Set()).add(reaction);
+					(deps[i].reactions ??= []).push(reaction);
 				}
 			}
 		} else if (deps !== null && skipped_deps < deps.length) {
@@ -350,9 +333,16 @@ export function update_reaction(reaction) {
 function remove_reaction(signal, dependency) {
 	let reactions = dependency.reactions;
 	if (reactions !== null) {
-		reactions.delete(signal);
-		if (reactions.size === 0) {
-			reactions = dependency.reactions = null;
+		var index = reactions.indexOf(signal);
+		if (index !== -1) {
+			var new_length = reactions.length - 1;
+			if (new_length === 0) {
+				reactions = dependency.reactions = null;
+			} else {
+				// Swap with last element and then remove.
+				reactions[index] = reactions[new_length];
+				reactions.pop();
+			}
 		}
 	}
 	// If the derived has no reactions, then we can disconnect it from the graph,
@@ -377,19 +367,8 @@ export function remove_reactions(signal, start_index) {
 	var dependencies = signal.deps;
 	if (dependencies === null) return;
 
-	var active_dependencies = start_index === 0 ? null : dependencies.slice(0, start_index);
-	var seen = new Set();
-
 	for (var i = start_index; i < dependencies.length; i++) {
-		var dependency = dependencies[i];
-
-		if (seen.has(dependency)) continue;
-		seen.add(dependency);
-
-		// Avoid removing a reaction if we know that it is active (start_index will not be 0)
-		if (active_dependencies === null || !active_dependencies.includes(dependency)) {
-			remove_reaction(signal, dependency);
-		}
+		remove_reaction(signal, dependencies[i]);
 	}
 }
 
@@ -726,16 +705,10 @@ export function get(signal) {
 		// rather than updating `new_deps`, which creates GC cost
 		if (new_deps === null && deps !== null && deps[skipped_deps] === signal) {
 			skipped_deps++;
-		}
-
-		// Otherwise, create or push to `new_deps`, but only if this
-		// dependency wasn't the last one that was accessed
-		else if (deps === null || skipped_deps === 0 || deps[skipped_deps - 1] !== signal) {
-			if (new_deps === null) {
-				new_deps = [signal];
-			} else if (new_deps[new_deps.length - 1] !== signal) {
-				new_deps.push(signal);
-			}
+		} else if (new_deps === null) {
+			new_deps = [signal];
+		} else {
+			new_deps.push(signal);
 		}
 
 		if (

packages/svelte/tests/signals/test.ts

@@ -236,13 +236,13 @@ describe('signals', () => {
 		return () => {
 			flushSync(() => set(count, 1));
 			// Ensure we're not leaking consumers
-			assert.deepEqual(count.reactions?.size, 1);
+			assert.deepEqual(count.reactions?.length, 1);
 			flushSync(() => set(count, 2));
 			// Ensure we're not leaking consumers
-			assert.deepEqual(count.reactions?.size, 1);
+			assert.deepEqual(count.reactions?.length, 1);
 			flushSync(() => set(count, 3));
 			// Ensure we're not leaking consumers
-			assert.deepEqual(count.reactions?.size, 1);
+			assert.deepEqual(count.reactions?.length, 1);
 			assert.deepEqual(log, [0, 1, 2, 3]);
 		};
 	});
@@ -304,8 +304,8 @@ describe('signals', () => {
 			flushSync(() => set(count, 4));
 			flushSync(() => set(count, 0));
 			// Ensure we're not leaking consumers
-			assert.deepEqual(count.reactions?.size, 1);
-			assert.deepEqual(calc.reactions?.size, 1);
+			assert.deepEqual(count.reactions?.length, 2);
+			assert.deepEqual(calc.reactions?.length, 1);
 			assert.deepEqual(log, [0, 2, 'limit', 0]);
 			destroy();
 			// Ensure we're not leaking consumers
@@ -484,7 +484,7 @@ describe('signals', () => {
 		return () => {
 			flushSync();
 			assert.equal(a?.deps?.length, 1);
-			assert.equal(state?.reactions?.size, 1);
+			assert.equal(state?.reactions?.length, 1);
 			destroy();
 			assert.equal(a?.deps?.length, 1);
 			assert.equal(state?.reactions, null);
@@ -658,12 +658,12 @@ describe('signals', () => {
 			});
 
 			assert.equal($.get(d), 0);
-			assert.equal(count.reactions?.size, 1);
+			assert.equal(count.reactions?.length, 1);
 			assert.equal(d.deps?.length, 1);
 
 			set(count, 1);
 			assert.equal($.get(d), 2);
-			assert.equal(count.reactions?.size, 1);
+			assert.equal(count.reactions?.length, 1);
 			assert.equal(d.deps?.length, 1);
 
 			destroy();