From 3c84c21a033636ae6fd3a65158a13847da833782 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Fri, 31 May 2024 14:30:51 +0100 Subject: [PATCH 1/4] fix: improve controlled each block cleanup performance (#11839) --- .changeset/gentle-ties-fetch.md | 5 +++++ .../svelte/src/internal/client/dom/blocks/each.js | 11 +++++++---- .../svelte/src/internal/client/reactivity/effects.js | 7 ++++--- packages/svelte/src/internal/client/runtime.js | 5 +++-- 4 files changed, 19 insertions(+), 9 deletions(-) create mode 100644 .changeset/gentle-ties-fetch.md diff --git a/.changeset/gentle-ties-fetch.md b/.changeset/gentle-ties-fetch.md new file mode 100644 index 0000000000..8302785811 --- /dev/null +++ b/.changeset/gentle-ties-fetch.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve controlled each block cleanup performance diff --git a/packages/svelte/src/internal/client/dom/blocks/each.js b/packages/svelte/src/internal/client/dom/blocks/each.js index 8c59e3aeba..bef42ea5c4 100644 --- a/packages/svelte/src/internal/client/dom/blocks/each.js +++ b/packages/svelte/src/internal/client/dom/blocks/each.js @@ -68,17 +68,20 @@ function pause_effects(items, controlled_anchor, callback) { pause_children(items[i].e, transitions, true); } + var is_controlled = length > 0 && transitions.length === 0 && controlled_anchor !== null; // If we have a controlled anchor, it means that the each block is inside a single // DOM element, so we can apply a fast-path for clearing the contents of the element. - if (length > 0 && transitions.length === 0 && controlled_anchor !== null) { - var parent_node = /** @type {Element} */ (controlled_anchor.parentNode); + if (is_controlled) { + var parent_node = /** @type {Element} */ ( + /** @type {Element} */ (controlled_anchor).parentNode + ); clear_text_content(parent_node); - parent_node.append(controlled_anchor); + parent_node.append(/** @type {Element} */ (controlled_anchor)); } run_out_transitions(transitions, () => { for (var i = 0; i < length; i++) { - destroy_effect(items[i].e); + destroy_effect(items[i].e, !is_controlled); } if (callback !== undefined) callback(); diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 6411e8109a..f7db38f3d1 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -311,16 +311,17 @@ export function execute_effect_teardown(effect) { /** * @param {import('#client').Effect} effect + * @param {boolean} [remove_dom] * @returns {void} */ -export function destroy_effect(effect) { +export function destroy_effect(effect, remove_dom = true) { var dom = effect.dom; - if (dom !== null) { + if (dom !== null && remove_dom) { remove(dom); } - destroy_effect_children(effect); + destroy_effect_children(effect, remove_dom); remove_reactions(effect, 0); set_signal_status(effect, DESTROYED); diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 1f0a0bfdab..8fe016532d 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -478,16 +478,17 @@ export function remove_reactions(signal, start_index) { /** * @param {import('#client').Reaction} signal + * @param {boolean} [remove_dom] * @returns {void} */ -export function destroy_effect_children(signal) { +export function destroy_effect_children(signal, remove_dom = true) { let effect = signal.first; signal.first = null; signal.last = null; var sibling; while (effect !== null) { sibling = effect.next; - destroy_effect(effect); + destroy_effect(effect, remove_dom); effect = sibling; } } From 2382eb08c56b81b2d5699ad644a5697fb513e227 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Fri, 31 May 2024 14:40:08 +0100 Subject: [PATCH 2/4] fix: improve reactive Map and Set implementations (#11827) Closes #11727. This PR aims to tackle issues around our reactive Map/Set implementations. Notably: - We now store the values on the backing Map/Set, allowing for much better introspection in console/dev tools - We no longer store the values inside the source signals, instead we use Symbols and booleans only as markers There's one limitation around `.has(x)` when `x` is not in the Map/Set yet - it's not fine-grained. Making it so could create too much memory pressure when e.g. iterating a big list of items with few of them being in the set (`has` returns `false` most of the time). --- .changeset/eight-jeans-compare.md | 5 + .../svelte/src/internal/client/dev/inspect.js | 10 -- packages/svelte/src/reactivity/map.js | 95 ++++++++++--------- packages/svelte/src/reactivity/map.test.ts | 32 +++++++ packages/svelte/src/reactivity/set.js | 71 +++++++------- packages/svelte/src/reactivity/set.test.ts | 51 ++++++++++ 6 files changed, 176 insertions(+), 88 deletions(-) create mode 100644 .changeset/eight-jeans-compare.md diff --git a/.changeset/eight-jeans-compare.md b/.changeset/eight-jeans-compare.md new file mode 100644 index 0000000000..fbd71dd1c2 --- /dev/null +++ b/.changeset/eight-jeans-compare.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve reactive Map and Set implementations diff --git a/packages/svelte/src/internal/client/dev/inspect.js b/packages/svelte/src/internal/client/dev/inspect.js index 3e01dc5b44..fb37f99df2 100644 --- a/packages/svelte/src/internal/client/dev/inspect.js +++ b/packages/svelte/src/internal/client/dev/inspect.js @@ -62,16 +62,6 @@ export function inspect(get_value, inspector = console.log) { */ function deep_snapshot(value, visited = new Map()) { if (typeof value === 'object' && value !== null && !visited.has(value)) { - if (DEV) { - // When dealing with ReactiveMap or ReactiveSet, return normal versions - // so that console.log provides better output versions - if (value instanceof Map && value.constructor !== Map) { - return new Map(value); - } - if (value instanceof Set && value.constructor !== Set) { - return new Set(value); - } - } const unstated = snapshot(value); if (unstated !== value) { diff --git a/packages/svelte/src/reactivity/map.js b/packages/svelte/src/reactivity/map.js index 59fcfc1cf0..ece3d1845d 100644 --- a/packages/svelte/src/reactivity/map.js +++ b/packages/svelte/src/reactivity/map.js @@ -2,7 +2,6 @@ import { DEV } from 'esm-env'; import { source, set } from '../internal/client/reactivity/sources.js'; import { get } from '../internal/client/runtime.js'; import { UNINITIALIZED } from '../constants.js'; -import { map } from './utils.js'; /** * @template K @@ -10,7 +9,7 @@ import { map } from './utils.js'; * @extends {Map} */ export class ReactiveMap extends Map { - /** @type {Map>} */ + /** @type {Map>} */ #sources = new Map(); #version = source(0); #size = source(0); @@ -25,13 +24,10 @@ export class ReactiveMap extends Map { if (DEV) new Map(value); if (value) { - var sources = this.#sources; - for (var [key, v] of value) { - sources.set(key, source(v)); + super.set(key, v); } - - this.#size.v = sources.size; + this.#size.v = super.size; } } @@ -41,14 +37,20 @@ export class ReactiveMap extends Map { /** @param {K} key */ has(key) { - var s = this.#sources.get(key); + var sources = this.#sources; + var s = sources.get(key); if (s === undefined) { - // We should always track the version in case - // the Set ever gets this value in the future. - get(this.#version); - - return false; + var ret = super.get(key); + if (ret !== undefined) { + s = source(Symbol()); + sources.set(key, s); + } else { + // We should always track the version in case + // the Set ever gets this value in the future. + get(this.#version); + return false; + } } get(s); @@ -62,23 +64,29 @@ export class ReactiveMap extends Map { forEach(callbackfn, this_arg) { get(this.#version); - var bound_callbackfn = callbackfn.bind(this_arg); - this.#sources.forEach((s, key) => bound_callbackfn(s.v, key, this)); + return super.forEach(callbackfn, this_arg); } /** @param {K} key */ get(key) { - var s = this.#sources.get(key); + var sources = this.#sources; + var s = sources.get(key); if (s === undefined) { - // We should always track the version in case - // the Set ever gets this value in the future. - get(this.#version); - - return undefined; + var ret = super.get(key); + if (ret !== undefined) { + s = source(Symbol()); + sources.set(key, s); + } else { + // We should always track the version in case + // the Set ever gets this value in the future. + get(this.#version); + return undefined; + } } - return get(s); + get(s); + return super.get(key); } /** @@ -88,65 +96,63 @@ export class ReactiveMap extends Map { set(key, value) { var sources = this.#sources; var s = sources.get(key); + var prev_res = super.get(key); + var res = super.set(key, value); if (s === undefined) { - sources.set(key, source(value)); - set(this.#size, sources.size); + sources.set(key, source(Symbol())); + set(this.#size, super.size); this.#increment_version(); - } else { - set(s, value); + } else if (prev_res !== value) { + set(s, Symbol()); } - return this; + return res; } /** @param {K} key */ delete(key) { var sources = this.#sources; var s = sources.get(key); + var res = super.delete(key); if (s !== undefined) { - var removed = sources.delete(key); - set(this.#size, sources.size); - set(s, /** @type {V} */ (UNINITIALIZED)); + sources.delete(key); + set(this.#size, super.size); + set(s, UNINITIALIZED); this.#increment_version(); - return removed; } - return false; + return res; } clear() { var sources = this.#sources; - if (sources.size !== 0) { + if (super.size !== 0) { set(this.#size, 0); for (var s of sources.values()) { - set(s, /** @type {V} */ (UNINITIALIZED)); + set(s, UNINITIALIZED); } this.#increment_version(); + sources.clear(); } - - sources.clear(); + super.clear(); } keys() { get(this.#version); - return this.#sources.keys(); + return super.keys(); } values() { get(this.#version); - return map(this.#sources.values(), get, 'Map Iterator'); + return super.values(); } entries() { get(this.#version); - return map( - this.#sources.entries(), - ([key, source]) => /** @type {[K, V]} */ ([key, get(source)]), - 'Map Iterator' - ); + return super.entries(); } [Symbol.iterator]() { @@ -154,6 +160,7 @@ export class ReactiveMap extends Map { } get size() { - return get(this.#size); + get(this.#size); + return super.size; } } diff --git a/packages/svelte/src/reactivity/map.test.ts b/packages/svelte/src/reactivity/map.test.ts index 7777392cf9..3cdba4bb38 100644 --- a/packages/svelte/src/reactivity/map.test.ts +++ b/packages/svelte/src/reactivity/map.test.ts @@ -183,3 +183,35 @@ test('map handling of undefined values', () => { cleanup(); }); + +test('not invoking reactivity when value is not in the map after changes', () => { + const map = new ReactiveMap([[1, 1]]); + + const log: any = []; + + const cleanup = effect_root(() => { + render_effect(() => { + log.push(map.get(1)); + }); + + render_effect(() => { + log.push(map.get(2)); + }); + + flushSync(() => { + map.delete(1); + }); + + flushSync(() => { + map.set(1, 1); + }); + }); + + assert.deepEqual(log, [1, undefined, undefined, undefined, 1, undefined]); + + cleanup(); +}); + +test('Map.instanceOf', () => { + assert.equal(new ReactiveMap() instanceof Map, true); +}); diff --git a/packages/svelte/src/reactivity/set.js b/packages/svelte/src/reactivity/set.js index 0a4963f839..809109846a 100644 --- a/packages/svelte/src/reactivity/set.js +++ b/packages/svelte/src/reactivity/set.js @@ -1,7 +1,6 @@ import { DEV } from 'esm-env'; import { source, set } from '../internal/client/reactivity/sources.js'; import { get } from '../internal/client/runtime.js'; -import { map } from './utils.js'; var read_methods = ['forEach', 'isDisjointFrom', 'isSubsetOf', 'isSupersetOf']; var set_like_methods = ['difference', 'intersection', 'symmetricDifference', 'union']; @@ -28,13 +27,10 @@ export class ReactiveSet extends Set { if (DEV) new Set(value); if (value) { - var sources = this.#sources; - for (var element of value) { - sources.set(element, source(true)); + super.add(element); } - - this.#size.v = sources.size; + this.#size.v = super.size; } if (!inited) this.#init(); @@ -51,11 +47,8 @@ export class ReactiveSet extends Set { // @ts-ignore proto[method] = function (...v) { get(this.#version); - // We don't populate the underlying Set, so we need to create a clone using - // our internal values and then pass that to the method. - var clone = new Set(this.values()); // @ts-ignore - return set_proto[method].apply(clone, v); + return set_proto[method].apply(this, v); }; } @@ -63,11 +56,8 @@ export class ReactiveSet extends Set { // @ts-ignore proto[method] = function (...v) { get(this.#version); - // We don't populate the underlying Set, so we need to create a clone using - // our internal values and then pass that to the method. - var clone = new Set(this.values()); // @ts-ignore - var set = /** @type {Set} */ (set_proto[method].apply(clone, v)); + var set = /** @type {Set} */ (set_proto[method].apply(this, v)); return new ReactiveSet(set); }; } @@ -79,73 +69,86 @@ export class ReactiveSet extends Set { /** @param {T} value */ has(value) { - var s = this.#sources.get(value); + var sources = this.#sources; + var s = sources.get(value); if (s === undefined) { - // We should always track the version in case - // the Set ever gets this value in the future. - get(this.#version); - - return false; + var ret = super.has(value); + if (ret) { + s = source(true); + sources.set(value, s); + } else { + // We should always track the version in case + // the Set ever gets this value in the future. + get(this.#version); + return false; + } } - return get(s); + get(s); + return super.has(value); } /** @param {T} value */ add(value) { var sources = this.#sources; + var res = super.add(value); + var s = sources.get(value); - if (!sources.has(value)) { + if (s === undefined) { sources.set(value, source(true)); - set(this.#size, sources.size); + set(this.#size, super.size); this.#increment_version(); + } else { + set(s, true); } - return this; + return res; } /** @param {T} value */ delete(value) { var sources = this.#sources; var s = sources.get(value); + var res = super.delete(value); if (s !== undefined) { - var removed = sources.delete(value); - set(this.#size, sources.size); + sources.delete(value); + set(this.#size, super.size); set(s, false); this.#increment_version(); - return removed; } - return false; + return res; } clear() { var sources = this.#sources; - if (sources.size !== 0) { + if (super.size !== 0) { set(this.#size, 0); for (var s of sources.values()) { set(s, false); } this.#increment_version(); + sources.clear(); } - - sources.clear(); + super.clear(); } keys() { get(this.#version); - return map(this.#sources.keys(), (key) => key, 'Set Iterator'); + return super.keys(); } values() { - return this.keys(); + get(this.#version); + return super.values(); } entries() { - return map(this.keys(), (key) => /** @type {[T, T]} */ ([key, key]), 'Set Iterator'); + get(this.#version); + return super.entries(); } [Symbol.iterator]() { diff --git a/packages/svelte/src/reactivity/set.test.ts b/packages/svelte/src/reactivity/set.test.ts index 5226014518..97d0869c9e 100644 --- a/packages/svelte/src/reactivity/set.test.ts +++ b/packages/svelte/src/reactivity/set.test.ts @@ -106,3 +106,54 @@ test('set.forEach()', () => { cleanup(); }); + +test('not invoking reactivity when value is not in the set after changes', () => { + const set = new ReactiveSet([1, 2]); + + const log: any = []; + + const cleanup = effect_root(() => { + render_effect(() => { + log.push('has 1', set.has(1)); + }); + + render_effect(() => { + log.push('has 2', set.has(2)); + }); + + render_effect(() => { + log.push('has 3', set.has(3)); + }); + }); + + flushSync(() => { + set.delete(2); + }); + + flushSync(() => { + set.add(2); + }); + + assert.deepEqual(log, [ + 'has 1', + true, + 'has 2', + true, + 'has 3', + false, + 'has 2', + false, + 'has 3', + false, + 'has 2', + true, + 'has 3', + false + ]); + + cleanup(); +}); + +test('Set.instanceOf', () => { + assert.equal(new ReactiveSet() instanceof Set, true); +}); From 9d2ecc1d7a8583e777db7e37a097504802f088c3 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Fri, 31 May 2024 14:45:02 +0100 Subject: [PATCH 3/4] chore: improve event error handling (#11840) I noticed that we spend a lot of time dealing with a recursive function when propagating events. Let's avoid that overhead and move back to a much faster while loop. We can also stack the errors and throw them at the end. --- .../internal/client/dom/elements/events.js | 78 ++++++++++++------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/elements/events.js b/packages/svelte/src/internal/client/dom/elements/events.js index 1f94162064..ea2fb351d4 100644 --- a/packages/svelte/src/internal/client/dom/elements/events.js +++ b/packages/svelte/src/internal/client/dom/elements/events.js @@ -173,38 +173,60 @@ export function handle_event_propagation(handler_element, event) { } }); - /** @param {Element} next_target */ - function next(next_target) { - current_target = next_target; - /** @type {null | Element} */ - var parent_element = next_target.parentNode || /** @type {any} */ (next_target).host || null; - - try { - // @ts-expect-error - var delegated = next_target['__' + event_name]; - - if (delegated !== undefined && !(/** @type {any} */ (next_target).disabled)) { - if (is_array(delegated)) { - var [fn, ...data] = delegated; - fn.apply(next_target, [event, ...data]); - } else { - delegated.call(next_target, event); + try { + /** + * @type {unknown} + */ + var throw_error; + /** + * @type {unknown[]} + */ + var other_errors = []; + yield_event_updates(() => { + while (current_target !== null) { + /** @type {null | Element} */ + var parent_element = + current_target.parentNode || /** @type {any} */ (current_target).host || null; + + try { + // @ts-expect-error + var delegated = current_target['__' + event_name]; + + if (delegated !== undefined && !(/** @type {any} */ (current_target).disabled)) { + if (is_array(delegated)) { + var [fn, ...data] = delegated; + fn.apply(current_target, [event, ...data]); + } else { + delegated.call(current_target, event); + } + } + } catch (error) { + if (throw_error) { + other_errors.push(error); + } else { + throw_error = error; + } + } + if ( + event.cancelBubble || + parent_element === handler_element || + parent_element === null || + current_target === handler_element + ) { + break; } + current_target = parent_element; } - } finally { - if ( - !event.cancelBubble && - parent_element !== handler_element && - parent_element !== null && - next_target !== handler_element - ) { - next(parent_element); + }); + if (throw_error) { + for (let error of other_errors) { + // Throw the rest of the errors, one-by-one on a microtask + queueMicrotask(() => { + throw error; + }); } + throw throw_error; } - } - - try { - yield_event_updates(() => next(/** @type {Element} */ (current_target))); } finally { // @ts-expect-error is used above event.__root = handler_element; From f411f776ca63704398c198e0bdc68184bd70bc38 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 31 May 2024 15:55:36 +0200 Subject: [PATCH 4/4] Version Packages (next) (#11853) Co-authored-by: github-actions[bot] --- .changeset/pre.json | 2 ++ packages/svelte/CHANGELOG.md | 8 ++++++++ packages/svelte/package.json | 2 +- packages/svelte/src/version.js | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.changeset/pre.json b/.changeset/pre.json index ba66791944..4e35411992 100644 --- a/.changeset/pre.json +++ b/.changeset/pre.json @@ -110,6 +110,7 @@ "eight-carrots-hunt", "eight-cougars-watch", "eight-hornets-punch", + "eight-jeans-compare", "eight-pianos-raise", "eight-steaks-shout", "eighty-bikes-camp", @@ -170,6 +171,7 @@ "gentle-dolls-juggle", "gentle-sheep-hug", "gentle-spies-happen", + "gentle-ties-fetch", "gentle-toys-chew", "gentle-trees-exercise", "gentle-wasps-pull", diff --git a/packages/svelte/CHANGELOG.md b/packages/svelte/CHANGELOG.md index 48c4bcb453..6d65ffa79f 100644 --- a/packages/svelte/CHANGELOG.md +++ b/packages/svelte/CHANGELOG.md @@ -1,5 +1,13 @@ # svelte +## 5.0.0-next.147 + +### Patch Changes + +- fix: improve reactive Map and Set implementations ([#11827](https://github.com/sveltejs/svelte/pull/11827)) + +- fix: improve controlled each block cleanup performance ([#11839](https://github.com/sveltejs/svelte/pull/11839)) + ## 5.0.0-next.146 ### Patch Changes diff --git a/packages/svelte/package.json b/packages/svelte/package.json index d6715d4bdb..fa2f7daffe 100644 --- a/packages/svelte/package.json +++ b/packages/svelte/package.json @@ -2,7 +2,7 @@ "name": "svelte", "description": "Cybernetically enhanced web apps", "license": "MIT", - "version": "5.0.0-next.146", + "version": "5.0.0-next.147", "type": "module", "types": "./types/index.d.ts", "engines": { diff --git a/packages/svelte/src/version.js b/packages/svelte/src/version.js index e425bd0cbe..48147b2b9a 100644 --- a/packages/svelte/src/version.js +++ b/packages/svelte/src/version.js @@ -6,5 +6,5 @@ * https://svelte.dev/docs/svelte-compiler#svelte-version * @type {string} */ -export const VERSION = '5.0.0-next.146'; +export const VERSION = '5.0.0-next.147'; export const PUBLIC_VERSION = '5';