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 and optimize ReactiveMap and ReactiveWeakMap #704

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix and optimize ReactiveMap and ReactiveWeakMap #704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4e9ac33
fix: Remove optimizations from Map iterators
Exelord Sep 29, 2024
fb8dcbc
feat: Optimize Map initialization
Exelord Oct 4, 2024
f39dd15
feat: Optimize Set initialization
Exelord Oct 4, 2024
51a28d1
feat: Optimize Map set
Exelord Oct 4, 2024
0ef612f
feat: Optimize Map delete
Exelord Oct 4, 2024
e18c60e
feat: Fix and optimize Map clear
Exelord Oct 4, 2024
44ca697
feat: Optimize WeakMap set
Exelord Oct 4, 2024
e47d6d0
feat: Optimize WeakMap delete
Exelord Oct 4, 2024
0b1db2d
chore: Remove init optimization
Exelord Oct 4, 2024
da3985b
fix: Fix values and keys tracking in Map
Exelord Oct 4, 2024
8db3aec
fix: Always dirty values on Map delete
Exelord Oct 4, 2024
26fccc1
Merge branch 'main' into fix-map
Exelord Nov 5, 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
167 changes: 94 additions & 73 deletions packages/map/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Accessor, batch } from "solid-js";
import { TriggerCache } from "@solid-primitives/trigger";

const $KEYS = Symbol("track-keys");
const $OBJECT = Symbol("track-object");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing to object as now we need to track object keys and values in both caches


/**
* A reactive version of `Map` data structure. All the reads (like `get` or `has`) are signals, and all the writes (`delete` or `set`) will cause updates to appropriate signals.
Expand All @@ -21,100 +21,114 @@ const $KEYS = Symbol("track-keys");
* userPoints.set(user1, { foo: "bar" });
*/
export class ReactiveMap<K, V> extends Map<K, V> {
#keyTriggers = new TriggerCache<K | typeof $KEYS>();
#valueTriggers = new TriggerCache<K>();
#keyTriggers = new TriggerCache<K | typeof $OBJECT>();
#valueTriggers = new TriggerCache<K | typeof $OBJECT>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Values will have an object trigger as well to support and values, entires and forEach


constructor(initial?: Iterable<readonly [K, V]> | null) {
super();
if (initial) for (const v of initial) super.set(v[0], v[1]);
[Symbol.iterator](): MapIterator<[K, V]> {
return this.entries();
}

// reads
has(key: K): boolean {
this.#keyTriggers.track(key);
return super.has(key);
}
get(key: K): V | undefined {
this.#valueTriggers.track(key);
return super.get(key);
constructor(entries?: Iterable<readonly [K, V]> | null) {
super();
if (entries) for (const entry of entries) super.set(...entry);
}

get size(): number {
this.#keyTriggers.track($KEYS);
this.#keyTriggers.track($OBJECT);
return super.size;
}

*keys(): MapIterator<K> {
this.#keyTriggers.track($OBJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, keys invalidate whenever a key has been added, changed or removed. No matter if a key was used or not.

This is Removing the same keys optimization as in Set, as we cannot assume a break will happen consistently on each computation.


for (const key of super.keys()) {
this.#keyTriggers.track(key);
yield key;
}
this.#keyTriggers.track($KEYS);
}

*values(): MapIterator<V> {
for (const [key, v] of super.entries()) {
this.#valueTriggers.track(key);
yield v;
this.#valueTriggers.track($OBJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Values are no longer tracking changes in keys changes. Instead they track only values changes.

This is also Removing the same keys optimization as in Set, as we cannot assume a break will happen consistently on each computation.


for (const value of super.values()) {
yield value;
}
this.#keyTriggers.track($KEYS);
}

*entries(): MapIterator<[K, V]> {
this.#keyTriggers.track($OBJECT);
this.#valueTriggers.track($OBJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entries are now tracking global changes to keys and values, ensuring they will be dirtied whenever anything has changed.

This is also Removing the same keys optimization as in Set, as we cannot assume a break will happen consistently on each computation.


for (const entry of super.entries()) {
this.#valueTriggers.track(entry[0]);
yield entry;
}
this.#keyTriggers.track($KEYS);
}

// writes
forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to entires, forEach is now tracking global changes to keys and values, ensuring they will be dirtied whenever anything has changed.

This is also Removing the same keys optimization as in Set, as we cannot assume a break will happen consistently on each computation.

this.#keyTriggers.track($OBJECT);
this.#valueTriggers.track($OBJECT);
super.forEach(callbackfn, thisArg);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now support as well thisArg, that wasn't supported

}

has(key: K): boolean {
this.#keyTriggers.track(key);
return super.has(key);
}

get(key: K): V | undefined {
this.#valueTriggers.track(key);
return super.get(key);
}

set(key: K, value: V): this {
batch(() => {
if (super.has(key)) {
if (super.get(key)! === value) return;
} else {
this.#keyTriggers.dirty(key);
this.#keyTriggers.dirty($KEYS);
}
this.#valueTriggers.dirty(key);
super.set(key, value);
});
return this;
const hadNoKey = !super.has(key);
const hasChanged = super.get(key) !== value;
const result = super.set(key, value);

if (hasChanged || hadNoKey) {
batch(() => {
if (hadNoKey) {
this.#keyTriggers.dirty($OBJECT);
this.#keyTriggers.dirty(key);
}
if (hasChanged) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set is now invalidating values only if they changed

this.#valueTriggers.dirty($OBJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Global values are dirtied for values, entries and forEach support

this.#valueTriggers.dirty(key);
}
});
}

return result;
}

delete(key: K): boolean {
const r = super.delete(key);
if (r) {
const isDefined = super.get(key) !== undefined;
const result = super.delete(key);

if (result) {
batch(() => {
this.#keyTriggers.dirty($OBJECT);
this.#valueTriggers.dirty($OBJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is important: we do dirty global values trigger always, as values() may depend on order. Even though the actual value of the key may not change, removing it, will have impact on amount of iterations in *values()

this.#keyTriggers.dirty(key);
this.#keyTriggers.dirty($KEYS);
this.#valueTriggers.dirty(key);

if (isDefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Value trigger is only dirtied if the value actually has changed.

this.#valueTriggers.dirty(key);
}
});
}
return r;

return result;
}

clear(): void {
if (super.size) {
super.clear();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clear is now fixed, calling .clear() at before dirtying


batch(() => {
for (const v of super.keys()) {
this.#keyTriggers.dirty(v);
this.#valueTriggers.dirty(v);
}
super.clear();
this.#keyTriggers.dirty($KEYS);
this.#keyTriggers.dirtyAll();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clear is now optimized using new dirtyAll()

this.#valueTriggers.dirtyAll();
});
}
}

// callback
forEach(callbackfn: (value: V, key: K, map: this) => void) {
this.#keyTriggers.track($KEYS);
for (const [key, v] of super.entries()) {
this.#valueTriggers.track(key);
callbackfn(v, key, this);
}
}

[Symbol.iterator](): MapIterator<[K, V]> {
return this.entries();
}
}

/**
Expand All @@ -137,9 +151,9 @@ export class ReactiveWeakMap<K extends object, V> extends WeakMap<K, V> {
#keyTriggers = new TriggerCache<K>(WeakMap);
#valueTriggers = new TriggerCache<K>(WeakMap);

constructor(initial?: Iterable<readonly [K, V]> | null) {
constructor(entries?: Iterable<readonly [K, V]> | null) {
super();
if (initial) for (const v of initial) super.set(v[0], v[1]);
if (entries) for (const entry of entries) super.set(...entry);
}

has(key: K): boolean {
Expand All @@ -151,24 +165,31 @@ export class ReactiveWeakMap<K extends object, V> extends WeakMap<K, V> {
return super.get(key);
}
set(key: K, value: V): this {
batch(() => {
if (super.has(key)) {
if (super.get(key)! === value) return;
} else this.#keyTriggers.dirty(key);
this.#valueTriggers.dirty(key);
super.set(key, value);
});
return this;
const hadNoKey = !super.has(key);
const hasChanged = super.get(key) !== value;
const result = super.set(key, value);

if (hasChanged || hadNoKey) {
batch(() => {
if (hadNoKey) this.#keyTriggers.dirty(key);
if (hasChanged) this.#valueTriggers.dirty(key);
});
}

return result;
}
delete(key: K): boolean {
const r = super.delete(key);
if (r) {
const isDefined = super.get(key) !== undefined;
const result = super.delete(key);

if (result) {
batch(() => {
this.#keyTriggers.dirty(key);
this.#valueTriggers.dirty(key);
if (isDefined) this.#valueTriggers.dirty(key);
});
}
return r;

return result;
}
}

Expand Down
50 changes: 19 additions & 31 deletions packages/map/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ describe("ReactiveMap", () => {
[1, "a"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

const captured: unknown[][] = [];
Expand All @@ -220,7 +219,6 @@ describe("ReactiveMap", () => {
const run: unknown[] = [];
for (const key of map.keys()) {
run.push(key);
if (key === 3) break; // don't iterate over all keys
}
captured.push(run);
});
Expand All @@ -233,12 +231,12 @@ describe("ReactiveMap", () => {
map.set(1, "e");
expect(captured, "value change").toHaveLength(1);

map.set(5, "f");
expect(captured, "not seen key change").toHaveLength(1);
map.set(4, "f");
expect(captured, "new key added").toHaveLength(2);

map.delete(1);
expect(captured, "seen key change").toHaveLength(2);
expect(captured[1]).toEqual([2, 3]);
expect(captured, "seen key change").toHaveLength(3);
expect(captured[2]).toEqual([2, 3, 4]);

dispose();
});
Expand All @@ -248,19 +246,15 @@ describe("ReactiveMap", () => {
[1, "a"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

const captured: unknown[][] = [];

const dispose = createRoot(dispose => {
createEffect(() => {
const run: unknown[] = [];
let i = 0;
for (const v of map.values()) {
run.push(v);
if (i === 2) break; // don't iterate over all keys
i += 1;
}
captured.push(run);
});
Expand All @@ -275,14 +269,12 @@ describe("ReactiveMap", () => {
expect(captured[1]).toEqual(["e", "b", "c"]);

map.set(4, "f");
expect(captured, "not seen value change").toHaveLength(2);
expect(captured, "new key added").toHaveLength(3);
expect(captured[2]).toEqual(["e", "b", "c", "f"]);

map.delete(4);
expect(captured, "not seen key change").toHaveLength(2);

map.delete(1);
expect(captured, "seen key change").toHaveLength(3);
expect(captured[2]).toEqual(["b", "c"]);
expect(captured, "key removed").toHaveLength(4);
expect(captured[3]).toEqual(["e", "b", "c"]);

dispose();
});
Expand All @@ -292,19 +284,15 @@ describe("ReactiveMap", () => {
[1, "a"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

const captured: unknown[][] = [];

const dispose = createRoot(dispose => {
createEffect(() => {
const run: unknown[] = [];
let i = 0;
for (const e of map.entries()) {
run.push(e);
if (i === 2) break; // don't iterate over all keys
i += 1;
}
captured.push(run);
});
Expand All @@ -327,14 +315,18 @@ describe("ReactiveMap", () => {
]);

map.set(4, "f");
expect(captured, "not seen value change").toHaveLength(2);
expect(captured, "new key added").toHaveLength(3);
expect(captured[2]).toEqual([
[1, "e"],
[2, "b"],
[3, "c"],
[4, "f"],
]);

map.delete(4);
expect(captured, "not seen key change").toHaveLength(2);

map.delete(1);
expect(captured, "seen key change").toHaveLength(3);
expect(captured[2]).toEqual([
expect(captured, "key removed").toHaveLength(4);
expect(captured[3]).toEqual([
[1, "e"],
[2, "b"],
[3, "c"],
]);
Expand All @@ -347,7 +339,6 @@ describe("ReactiveMap", () => {
[1, "a"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

const captured: unknown[][] = [];
Expand All @@ -368,7 +359,6 @@ describe("ReactiveMap", () => {
[1, "a"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

map.set(1, "e");
Expand All @@ -377,15 +367,13 @@ describe("ReactiveMap", () => {
[1, "e"],
[2, "b"],
[3, "c"],
[4, "d"],
]);

map.delete(4);
map.delete(3);
expect(captured).toHaveLength(3);
expect(captured[2]).toEqual([
[1, "e"],
[2, "b"],
[3, "c"],
]);

dispose();
Expand Down
Loading
Loading