From e7a90c38f3935bc2bab37352b11ef32d6b746224 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 12:32:18 +0000 Subject: [PATCH 01/11] fix: addresses memory leak when creating deriveds inside untrack --- .../internal/client/reactivity/deriveds.js | 42 +++++++++++++------ .../src/internal/client/reactivity/types.d.ts | 5 ++- .../svelte/src/internal/client/runtime.js | 13 ++++-- packages/svelte/tests/signals/test.ts | 26 ++++++++++++ 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 98fbfb0f5242..50669d170130 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -42,6 +42,11 @@ export function derived(fn) { active_effect.f |= EFFECT_HAS_DERIVED; } + var parent_derived = + active_reaction !== null && (active_reaction.f & DERIVED) !== 0 + ? /** @type {Derived} */ (active_reaction) + : null; + /** @type {Derived} */ const signal = { children: null, @@ -53,12 +58,11 @@ export function derived(fn) { reactions: null, v: /** @type {V} */ (null), version: 0, - parent: active_effect + parent: parent_derived || active_effect }; - if (active_reaction !== null && (active_reaction.f & DERIVED) !== 0) { - var derived = /** @type {Derived} */ (active_reaction); - (derived.children ??= []).push(signal); + if (parent_derived !== null) { + (parent_derived.children ??= []).push(signal); } return signal; @@ -104,6 +108,21 @@ function destroy_derived_children(derived) { */ let stack = []; +/** + * @param {Derived} derived + * @returns {Effect | null} + */ +export function get_derived_parent_effect(derived) { + var parent = derived.parent; + if (parent !== null) { + if ((parent.f & DERIVED) === 0) { + return /** @type {Effect} */ (parent); + } + parent = parent.parent; + } + return null; +} + /** * @template T * @param {Derived} derived @@ -113,7 +132,7 @@ export function execute_derived(derived) { var value; var prev_active_effect = active_effect; - set_active_effect(derived.parent); + set_active_effect(get_derived_parent_effect(derived)); if (DEV) { let prev_inspect_effects = inspect_effects; @@ -162,14 +181,13 @@ export function update_derived(derived) { } /** - * @param {Derived} signal + * @param {Derived} derived * @returns {void} */ -export function destroy_derived(signal) { - destroy_derived_children(signal); - remove_reactions(signal, 0); - set_signal_status(signal, DESTROYED); +export function destroy_derived(derived) { + destroy_derived_children(derived); + remove_reactions(derived, 0); + set_signal_status(derived, DESTROYED); - // TODO we need to ensure we remove the derived from any parent derives - signal.v = signal.children = signal.deps = signal.ctx = signal.reactions = null; + derived.v = derived.children = derived.deps = derived.ctx = derived.reactions = null; } diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts index 2cef49eb2d80..395070fedd1b 100644 --- a/packages/svelte/src/internal/client/reactivity/types.d.ts +++ b/packages/svelte/src/internal/client/reactivity/types.d.ts @@ -23,7 +23,6 @@ export interface Reaction extends Signal { fn: null | Function; /** Signals that this signal reads from */ deps: null | Value[]; - parent: Effect | null; } export interface Derived extends Value, Reaction { @@ -31,6 +30,8 @@ export interface Derived extends Value, Reaction { fn: () => V; /** Reactions created inside this signal */ children: null | Reaction[]; + /** Parent effect or derived */ + parent: Effect | Derived | null; } export interface Effect extends Reaction { @@ -58,6 +59,8 @@ export interface Effect extends Reaction { first: null | Effect; /** Last child effect created inside this signal */ last: null | Effect; + /** Parent effect */ + parent: Effect | null; /** Dev only */ component_function?: any; } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 5b5b488824e6..9810c1856780 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -29,7 +29,12 @@ import { import { flush_tasks } from './dom/task.js'; import { add_owner } from './dev/ownership.js'; import { mutate, set, source } from './reactivity/sources.js'; -import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js'; +import { + destroy_derived, + execute_derived, + get_derived_parent_effect, + update_derived +} from './reactivity/deriveds.js'; import * as e from './errors.js'; import { lifecycle_outside_component } from '../shared/errors.js'; import { FILENAME } from '../../constants.js'; @@ -766,10 +771,10 @@ export function get(signal) { } } else if (is_derived && /** @type {Derived} */ (signal).deps === null) { var derived = /** @type {Derived} */ (signal); - var parent = derived.parent; + var parent_effect = get_derived_parent_effect(derived); - if (parent !== null && !parent.deriveds?.includes(derived)) { - (parent.deriveds ??= []).push(derived); + if (parent_effect !== null && !parent_effect.deriveds?.includes(derived)) { + (parent_effect.deriveds ??= []).push(derived); } } diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index e2d27267a3f5..2df3ca9e5dc0 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -739,4 +739,30 @@ describe('signals', () => { assert.deepEqual(a.reactions, null); }; }); + + test.only('nested deriveds clean up the releationships when used with untrack', () => { + return () => { + let a = render_effect(() => {}); + + const destroy = effect_root(() => { + a = render_effect(() => { + $.untrack(() => { + const b = derived(() => { + const c = derived(() => {}); + $.untrack(() => { + $.get(c); + }); + }); + $.get(b); + }); + }); + }); + + assert.deepEqual(a.deriveds?.length, 1); + + destroy(); + + assert.deepEqual(a.deriveds, null); + }; + }); }); From 9d60721800b55c044f7c4f0f3ef134869190abe9 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 12:35:23 +0000 Subject: [PATCH 02/11] fix: addresses memory leak when creating deriveds inside untrack --- packages/svelte/tests/signals/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 2df3ca9e5dc0..d8aece2d96e8 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -740,7 +740,7 @@ describe('signals', () => { }; }); - test.only('nested deriveds clean up the releationships when used with untrack', () => { + test('nested deriveds clean up the releationships when used with untrack', () => { return () => { let a = render_effect(() => {}); From 8a7dec8c5f27eb3bb7baa2946f418b7a002bd1d2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:09:01 +0000 Subject: [PATCH 03/11] changeset --- .changeset/great-crabs-rhyme.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/great-crabs-rhyme.md diff --git a/.changeset/great-crabs-rhyme.md b/.changeset/great-crabs-rhyme.md new file mode 100644 index 000000000000..9f22d6583508 --- /dev/null +++ b/.changeset/great-crabs-rhyme.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: addresses memory leak when creating deriveds inside untrack From 0953c0a85dedec755331b0e664b8739a6fbb9c05 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:15:06 +0000 Subject: [PATCH 04/11] Update packages/svelte/src/internal/client/reactivity/deriveds.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- packages/svelte/src/internal/client/reactivity/deriveds.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 50669d170130..66cb8a882b33 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -114,7 +114,7 @@ let stack = []; */ export function get_derived_parent_effect(derived) { var parent = derived.parent; - if (parent !== null) { + while (parent !== null) { if ((parent.f & DERIVED) === 0) { return /** @type {Effect} */ (parent); } From af875b3f03b905b17c55c14858e1f8e42b52554b Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:22:35 +0000 Subject: [PATCH 05/11] fix --- .../src/internal/client/reactivity/deriveds.js | 2 +- packages/svelte/src/internal/client/runtime.js | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 66cb8a882b33..9d1b0d22ee96 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -112,7 +112,7 @@ let stack = []; * @param {Derived} derived * @returns {Effect | null} */ -export function get_derived_parent_effect(derived) { +function get_derived_parent_effect(derived) { var parent = derived.parent; while (parent !== null) { if ((parent.f & DERIVED) === 0) { diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 9810c1856780..b1d75f8763ab 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -29,12 +29,7 @@ import { import { flush_tasks } from './dom/task.js'; import { add_owner } from './dev/ownership.js'; import { mutate, set, source } from './reactivity/sources.js'; -import { - destroy_derived, - execute_derived, - get_derived_parent_effect, - update_derived -} from './reactivity/deriveds.js'; +import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js'; import * as e from './errors.js'; import { lifecycle_outside_component } from '../shared/errors.js'; import { FILENAME } from '../../constants.js'; @@ -771,9 +766,14 @@ export function get(signal) { } } else if (is_derived && /** @type {Derived} */ (signal).deps === null) { var derived = /** @type {Derived} */ (signal); - var parent_effect = get_derived_parent_effect(derived); + var parent = derived.parent; - if (parent_effect !== null && !parent_effect.deriveds?.includes(derived)) { + if ( + parent !== null && + (parent.f & DERIVED) === 0 && + !(/** @type {Effect} */ (parent).deriveds?.includes(derived)) + ) { + var parent_effect = /** @type {Effect} */ (parent); (parent_effect.deriveds ??= []).push(derived); } } From ac98ee75baa182315c0c4a26f1c6e4aca042dcf6 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:25:40 +0000 Subject: [PATCH 06/11] fix --- .../svelte/src/internal/client/runtime.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index b1d75f8763ab..c64a3d6b3854 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -767,14 +767,19 @@ export function get(signal) { } else if (is_derived && /** @type {Derived} */ (signal).deps === null) { var derived = /** @type {Derived} */ (signal); var parent = derived.parent; + var target = derived; - if ( - parent !== null && - (parent.f & DERIVED) === 0 && - !(/** @type {Effect} */ (parent).deriveds?.includes(derived)) - ) { - var parent_effect = /** @type {Effect} */ (parent); - (parent_effect.deriveds ??= []).push(derived); + while (parent !== null) { + if ((parent.f & DERIVED) !== 0) { + target = /** @type {Derived} */ (parent); + parent = /** @type {Derived} */ (parent.parent); + } else { + if (!(/** @type {Effect} */ (parent).deriveds?.includes(target))) { + var parent_effect = /** @type {Effect} */ (parent); + (parent_effect.deriveds ??= []).push(target); + } + break; + } } } From 628183671b00554a9ee7748306a536568b7d5f88 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:26:47 +0000 Subject: [PATCH 07/11] fix --- packages/svelte/src/internal/client/runtime.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index c64a3d6b3854..c267e2157459 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -771,11 +771,14 @@ export function get(signal) { while (parent !== null) { if ((parent.f & DERIVED) !== 0) { - target = /** @type {Derived} */ (parent); - parent = /** @type {Derived} */ (parent.parent); + var parent_derived = /** @type {Derived} */ (parent); + + target = parent_derived; + parent = parent_derived.parent; } else { - if (!(/** @type {Effect} */ (parent).deriveds?.includes(target))) { - var parent_effect = /** @type {Effect} */ (parent); + var parent_effect = /** @type {Effect} */ (parent); + + if (!parent_effect.deriveds?.includes(target)) { (parent_effect.deriveds ??= []).push(target); } break; From 22ae61b55e2ab5958784a9b7c148502950cc2dd1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 14:27:31 +0000 Subject: [PATCH 08/11] comment --- packages/svelte/src/internal/client/runtime.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index c267e2157459..8931d02009a6 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -770,6 +770,8 @@ export function get(signal) { var target = derived; while (parent !== null) { + // Attach the derived to the nearest parent effect, if there are deriveds + // in between then we also need to attach them too if ((parent.f & DERIVED) !== 0) { var parent_derived = /** @type {Derived} */ (parent); From 91e7e5becbc9b892420db65c9d5d39d3d3289d79 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 26 Nov 2024 14:10:18 -0500 Subject: [PATCH 09/11] Update packages/svelte/tests/signals/test.ts --- packages/svelte/tests/signals/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index d8aece2d96e8..db061bfb5543 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -740,7 +740,7 @@ describe('signals', () => { }; }); - test('nested deriveds clean up the releationships when used with untrack', () => { + test('nested deriveds clean up the relationships when used with untrack', () => { return () => { let a = render_effect(() => {}); From c909b1d7637527ab2a8c5352238d4caad5ec3710 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 19:15:37 +0000 Subject: [PATCH 10/11] Update packages/svelte/src/internal/client/reactivity/deriveds.js Co-authored-by: Rich Harris --- packages/svelte/src/internal/client/reactivity/deriveds.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 9d1b0d22ee96..a679d307c40d 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -58,7 +58,7 @@ export function derived(fn) { reactions: null, v: /** @type {V} */ (null), version: 0, - parent: parent_derived || active_effect + parent: parent_derived ?? active_effect }; if (parent_derived !== null) { From b16eb5f8adfa7fb70afe6162c6622a625fb5f336 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 26 Nov 2024 19:15:42 +0000 Subject: [PATCH 11/11] Update .changeset/great-crabs-rhyme.md Co-authored-by: Rich Harris --- .changeset/great-crabs-rhyme.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/great-crabs-rhyme.md b/.changeset/great-crabs-rhyme.md index 9f22d6583508..b02f7bc9b296 100644 --- a/.changeset/great-crabs-rhyme.md +++ b/.changeset/great-crabs-rhyme.md @@ -2,4 +2,4 @@ 'svelte': patch --- -fix: addresses memory leak when creating deriveds inside untrack +fix: prevent memory leak when creating deriveds inside untrack