Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve reactive Map and Set implementations #11827

Merged
merged 4 commits into from
May 31, 2024
Merged

fix: improve reactive Map and Set implementations #11827

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74d18ad
fix: improve reactive Map and Set implementations
trueadm May 29, 2024
81e150e
fix bug
trueadm May 29, 2024
d1b974c
add test
trueadm May 29, 2024
ac921d8
address feedback
trueadm May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eight-jeans-compare.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: improve reactive Map and Set implementations
10 changes: 0 additions & 10 deletions packages/svelte/src/internal/client/dev/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
82 changes: 46 additions & 36 deletions packages/svelte/src/reactivity/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ 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
* @template V
* @extends {Map<K, V>}
*/
export class ReactiveMap extends Map {
/** @type {Map<K, import('#client').Source<V>>} */
/** @type {Map<K, import('#client').Source<symbol>>} */
#sources = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could use a WeakMap here to allow for object keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah WeakMaps don't give us anything here really. It means we'd have to duplicate data as we need to store the value on the super and in the WeakMap, which means the WeakMap never does anything as the value is strongly held. Also WeakMaps are very slow.

#version = source(0);
#size = source(0);
Expand All @@ -28,7 +27,8 @@ export class ReactiveMap extends Map {
var sources = this.#sources;

for (var [key, v] of value) {
sources.set(key, source(v));
sources.set(key, source(Symbol()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check if the key is an object here as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I'm not sure if we need to fill the sources Map here, since we now generate signals in has and get

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed.

super.set(key, v);
}

this.#size.v = sources.size;
Expand All @@ -41,18 +41,24 @@ 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;
// If we're working with a non-primitive then we can generate a signal for it
if (typeof key !== 'object' || key === null) {
s = source(UNINITIALIZED);
sources.set(key, s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still has the same problem as #11504 and #11287. We need a way to remove these signals when we are no longer listening to them. Otherwise it would create a memory leak (see #11504 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Unfortunately, this means that we can't retain primitives either. So they won't be fine-grain, as there's no way to do so without leaking.

} 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);
return true;
return super.has(key);
}

/**
Expand All @@ -62,23 +68,25 @@ 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);

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;
// Re-use the has code-path to init the signal if needed and also
// register to the version if needed.
this.has(key);
s = this.#sources.get(key);
if (s === undefined) {
return undefined;
}
}

return get(s);
get(s);
return super.get(key);
}

/**
Expand All @@ -88,32 +96,37 @@ 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));
// Remove non-primitive keys
if (typeof key === 'object' && key !== null) {
sources.delete(key);
}
set(this.#size, super.size);
set(s, UNINITIALIZED);
this.#increment_version();
return removed;
}

return false;
return res;
}

clear() {
Expand All @@ -122,31 +135,28 @@ export class ReactiveMap extends Map {
if (sources.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();
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]() {
Expand Down
34 changes: 33 additions & 1 deletion packages/svelte/src/reactivity/map.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ test('map.values()', () => {
map.clear();
});

assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, false, []]);
assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, []]);

cleanup();
});
Expand Down Expand Up @@ -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, 1]);

cleanup();
});

test('Map.instanceOf', () => {
assert.equal(new ReactiveMap() instanceof Map, true);
});
60 changes: 34 additions & 26 deletions packages/svelte/src/reactivity/set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'];
Expand Down Expand Up @@ -32,6 +31,7 @@ export class ReactiveSet extends Set {

for (var element of value) {
sources.set(element, source(true));
super.add(element);
}

this.#size.v = sources.size;
Expand All @@ -51,23 +51,17 @@ 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);
};
}

for (const method of set_like_methods) {
// @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<T>} */ (set_proto[method].apply(clone, v));
var set = /** @type {Set<T>} */ (set_proto[method].apply(this, v));
return new ReactiveSet(set);
};
}
Expand All @@ -79,46 +73,57 @@ 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;
// If we're working with a non-primitive then we can generate a signal for it
if (typeof value !== 'object' || value === null) {
s = source(false);
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() {
Expand All @@ -133,19 +138,22 @@ export class ReactiveSet extends Set {
}

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]() {
Expand Down
Loading
Loading