From dc787be55059590c600c782a291cd77b85de29ac Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 14 Aug 2024 13:05:01 +0100 Subject: [PATCH] chore: improve signal perf by using Set rather than array for reactions (#12831) * chore: improve signal perf by using Set rather than array for reactions * tweak * simplify * lint * address feedback --- .changeset/calm-clocks-raise.md | 5 +++ .../src/internal/client/reactivity/sources.js | 4 +-- .../src/internal/client/reactivity/types.d.ts | 2 +- .../svelte/src/internal/client/runtime.js | 36 +++++-------------- packages/svelte/tests/signals/test.ts | 16 ++++----- 5 files changed, 24 insertions(+), 39 deletions(-) create mode 100644 .changeset/calm-clocks-raise.md diff --git a/.changeset/calm-clocks-raise.md b/.changeset/calm-clocks-raise.md new file mode 100644 index 0000000000..dbcfabe197 --- /dev/null +++ b/.changeset/calm-clocks-raise.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +chore: improve signal perf by using Set rather than array for reactions diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 86427a7509..3425d6dadc 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -139,10 +139,8 @@ function mark_reactions(signal, status) { if (reactions === null) return; var runes = is_runes(); - var length = reactions.length; - for (var i = 0; i < length; i++) { - var reaction = reactions[i]; + for (var reaction of reactions) { var flags = reaction.f; // Skip any effects that are already dirty diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts index 1e347c011f..4a53a70043 100644 --- a/packages/svelte/src/internal/client/reactivity/types.d.ts +++ b/packages/svelte/src/internal/client/reactivity/types.d.ts @@ -9,7 +9,7 @@ export interface Signal { export interface Value extends Signal { /** Signals that read from this signal */ - reactions: null | Reaction[]; + reactions: null | Set; /** Equality function */ equals: Equals; /** The latest value for this signal */ diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 89c607cb72..f113e339e7 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/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 ??= []).push(reaction); + (dependencies[i].reactions ??= new Set()).add(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?.includes(reaction)) { + if (!current_skip_reaction && !dependency?.reactions?.has(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 ??= []).push(reaction); + (dependency.reactions ??= new Set()).add(reaction); } } } @@ -323,17 +323,7 @@ export function update_reaction(reaction) { if (!current_skip_reaction) { for (i = skipped_deps; i < deps.length; i++) { - dependency = deps[i]; - var reactions = dependency.reactions; - - if (reactions === null) { - dependency.reactions = [reaction]; - } else if ( - reactions[reactions.length - 1] !== reaction && - !reactions.includes(reaction) - ) { - reactions.push(reaction); - } + (deps[i].reactions ??= new Set()).add(reaction); } } } else if (deps !== null && skipped_deps < deps.length) { @@ -358,24 +348,16 @@ export function update_reaction(reaction) { * @returns {void} */ function remove_reaction(signal, dependency) { - const reactions = dependency.reactions; - let reactions_length = 0; + let reactions = dependency.reactions; if (reactions !== null) { - reactions_length = reactions.length - 1; - const index = reactions.indexOf(signal); - if (index !== -1) { - if (reactions_length === 0) { - dependency.reactions = null; - } else { - // Swap with last element and then remove. - reactions[index] = reactions[reactions_length]; - reactions.pop(); - } + reactions.delete(signal); + if (reactions.size === 0) { + reactions = dependency.reactions = null; } } // If the derived has no reactions, then we can disconnect it from the graph, // allowing it to either reconnect in the future, or be GC'd by the VM. - if (reactions_length === 0 && (dependency.f & DERIVED) !== 0) { + if (reactions === null && (dependency.f & DERIVED) !== 0) { set_signal_status(dependency, MAYBE_DIRTY); // If we are working with a derived that is owned by an effect, then mark it as being // disconnected. diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index d14d5fe964..7a68e92bc7 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/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?.length, 1); + assert.deepEqual(count.reactions?.size, 1); flushSync(() => set(count, 2)); // Ensure we're not leaking consumers - assert.deepEqual(count.reactions?.length, 1); + assert.deepEqual(count.reactions?.size, 1); flushSync(() => set(count, 3)); // Ensure we're not leaking consumers - assert.deepEqual(count.reactions?.length, 1); + assert.deepEqual(count.reactions?.size, 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?.length, 1); - assert.deepEqual(calc.reactions?.length, 1); + assert.deepEqual(count.reactions?.size, 1); + assert.deepEqual(calc.reactions?.size, 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?.length, 1); + assert.equal(state?.reactions?.size, 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?.length, 1); + assert.equal(count.reactions?.size, 1); assert.equal(d.deps?.length, 1); set(count, 1); assert.equal($.get(d), 2); - assert.equal(count.reactions?.length, 1); + assert.equal(count.reactions?.size, 1); assert.equal(d.deps?.length, 1); destroy();