Skip to content

Commit 269bd8d

Browse files
VarixoVarixo
committed
fix(perf): use sets instead of arrays for effects
1 parent 2739ddc commit 269bd8d

File tree

8 files changed

+88
-78
lines changed

8 files changed

+88
-78
lines changed

‎packages/qwik/src/core/shared/component-execution.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,7 @@ export const executeComponent = (
8888
if (!isInlineComponent) {
8989
container.setHostProp(renderHost, ELEMENT_SEQ_IDX, null);
9090
container.setHostProp(renderHost, USE_ON_LOCAL_SEQ_IDX, null);
91-
if (container.getHostProp(renderHost, ELEMENT_PROPS) !== props) {
92-
container.setHostProp(renderHost, ELEMENT_PROPS, props);
93-
}
91+
container.setHostProp(renderHost, ELEMENT_PROPS, props);
9492
}
9593

9694
if (vnode_isVNode(renderHost)) {

‎packages/qwik/src/core/shared/scheduler.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ export const createScheduler = (
498498
const target = chore.$target$ as ComputedSignal<unknown> | WrappedSignal<unknown>;
499499
const forceRunEffects = target.$forceRunEffects$;
500500
target.$forceRunEffects$ = false;
501-
if (!target.$effects$?.length) {
501+
if (!target.$effects$?.size) {
502502
break;
503503
}
504504
returnValue = retryOnPromise(() => {

‎packages/qwik/src/core/shared/shared-serialization.ts‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,13 @@ import { type DomContainer } from '../client/dom-container';
88
import type { VNode } from '../client/types';
99
import { vnode_getNode, vnode_isVNode, vnode_locate, vnode_toString } from '../client/vnode';
1010
import { NEEDS_COMPUTATION } from '../signal/flags';
11-
import { ComputedSignal, EffectPropData, Signal, WrappedSignal } from '../signal/signal';
11+
import {
12+
ComputedSignal,
13+
EffectPropData,
14+
Signal,
15+
WrappedSignal,
16+
type EffectSubscriptions,
17+
} from '../signal/signal';
1218
import type { Subscriber } from '../signal/signal-subscriber';
1319
import {
1420
STORE_ARRAY_PROP,
@@ -233,7 +239,7 @@ const inflate = (
233239
task.$flags$ = v[1];
234240
task.$index$ = v[2];
235241
task.$el$ = v[3] as HostElement;
236-
task.$effectDependencies$ = v[4] as Subscriber[] | null;
242+
task.$effectDependencies$ = v[4] as Set<Subscriber> | null;
237243
task.$state$ = v[5];
238244
break;
239245
case TypeIds.Resource:
@@ -268,20 +274,27 @@ const inflate = (
268274
}
269275
case TypeIds.Signal: {
270276
const signal = target as Signal<unknown>;
271-
const d = data as [unknown, ...any[]];
277+
const d = data as [unknown, Set<EffectSubscriptions>];
272278
signal.$untrackedValue$ = d[0];
273-
signal.$effects$ = d.slice(1);
279+
signal.$effects$ = d[1];
274280
break;
275281
}
276282
case TypeIds.WrappedSignal: {
277283
const signal = target as WrappedSignal<unknown>;
278-
const d = data as [number, unknown[], Subscriber[], unknown, HostElement, ...any[]];
284+
const d = data as [
285+
number,
286+
unknown[],
287+
Set<Subscriber>,
288+
unknown,
289+
HostElement,
290+
Set<EffectSubscriptions>,
291+
];
279292
signal.$func$ = container.getSyncFn(d[0]);
280293
signal.$args$ = d[1];
281294
signal.$effectDependencies$ = d[2];
282295
signal.$untrackedValue$ = d[3];
283296
signal.$hostElement$ = d[4];
284-
signal.$effects$ = d.slice(5);
297+
signal.$effects$ = d[5];
285298
break;
286299
}
287300
case TypeIds.ComputedSignal: {
@@ -843,7 +856,7 @@ export const createSerializationContext = (
843856
// WrappedSignal uses syncQrl which has no captured refs
844857
if (obj instanceof WrappedSignal) {
845858
if (obj.$effectDependencies$) {
846-
discoveredValues.push(...obj.$effectDependencies$);
859+
discoveredValues.push(obj.$effectDependencies$);
847860
}
848861
if (obj.$args$) {
849862
discoveredValues.push(...obj.$args$);
@@ -1175,7 +1188,7 @@ function serialize(serializationContext: SerializationContext): void {
11751188
value.$effectDependencies$,
11761189
v,
11771190
value.$hostElement$,
1178-
...(value.$effects$ || []),
1191+
value.$effects$,
11791192
]);
11801193
} else if (value instanceof ComputedSignal) {
11811194
const out = [
@@ -1188,7 +1201,7 @@ function serialize(serializationContext: SerializationContext): void {
11881201
}
11891202
output(TypeIds.ComputedSignal, out);
11901203
} else {
1191-
output(TypeIds.Signal, [v, ...(value.$effects$ || [])]);
1204+
output(TypeIds.Signal, [v, value.$effects$]);
11921205
}
11931206
} else if (value instanceof URL) {
11941207
output(TypeIds.URL, value.href);

‎packages/qwik/src/core/signal/signal-subscriber.ts‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { isPropsProxy } from '../shared/jsx/jsx-runtime';
1313
import { _CONST_PROPS, _VAR_PROPS } from '../internal';
1414

1515
export abstract class Subscriber {
16-
$effectDependencies$: (Subscriber | TargetType)[] | null = null;
16+
$effectDependencies$: Set<Subscriber | TargetType> | null = null;
1717
}
1818

1919
export function isSubscriber(value: unknown): value is Subscriber {
@@ -40,13 +40,23 @@ export function clearVNodeEffectDependencies(container: Container, value: VNode)
4040

4141
export function clearSubscriberEffectDependencies(container: Container, value: Subscriber): void {
4242
if (value.$effectDependencies$) {
43-
for (let i = value.$effectDependencies$.length - 1; i >= 0; i--) {
44-
const subscriber = value.$effectDependencies$[i];
45-
clearEffects(subscriber, value, value.$effectDependencies$, i, container);
43+
for (const subscriber of value.$effectDependencies$) {
44+
let subscriptionRemoved = false;
45+
const seenSet = new Set();
46+
if (subscriber instanceof WrappedSignal) {
47+
subscriptionRemoved = clearSignalEffects(subscriber, value, seenSet);
48+
} else if (container.$storeProxyMap$.has(subscriber)) {
49+
const store = container.$storeProxyMap$.get(subscriber)!;
50+
const handler = getStoreHandler(store)!;
51+
subscriptionRemoved = clearStoreEffects(handler, value);
52+
}
53+
if (subscriptionRemoved) {
54+
value.$effectDependencies$.delete(subscriber);
55+
}
4656
}
4757

48-
if (value.$effectDependencies$.length === 0) {
49-
value.$effectDependencies$ = null;
58+
if (value.$effectDependencies$.size === 0) {
59+
value.$effectDependencies$.clear();
5060
}
5161
}
5262
}
@@ -81,10 +91,9 @@ function clearSignalEffects(
8191

8292
let subscriptionRemoved = false;
8393
if (effectSubscriptions) {
84-
for (let i = effectSubscriptions.length - 1; i >= 0; i--) {
85-
const effect = effectSubscriptions[i];
94+
for (const effect of effectSubscriptions) {
8695
if (effect[EffectSubscriptionsProp.EFFECT] === value) {
87-
effectSubscriptions.splice(i, 1);
96+
effectSubscriptions.delete(effect);
8897
subscriptionRemoved = true;
8998
}
9099
}
@@ -114,14 +123,13 @@ function clearStoreEffects(storeHandler: StoreHandler, value: Subscriber | VNode
114123
let subscriptionRemoved = false;
115124
for (const key in effectSubscriptions) {
116125
const effects = effectSubscriptions[key];
117-
for (let i = effects.length - 1; i >= 0; i--) {
118-
const effect = effects[i];
126+
for (const effect of effects) {
119127
if (effect[EffectSubscriptionsProp.EFFECT] === value) {
120-
effects.splice(i, 1);
128+
effects.delete(effect);
121129
subscriptionRemoved = true;
122130
}
123131
}
124-
if (effects.length === 0) {
132+
if (effects.size === 0) {
125133
delete effectSubscriptions[key];
126134
}
127135
}

‎packages/qwik/src/core/signal/signal.ts‎

Lines changed: 22 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ export class Signal<T = any> implements ISignal<T> {
148148
$untrackedValue$: T;
149149

150150
/** Store a list of effects which are dependent on this signal. */
151-
$effects$: null | EffectSubscriptions[] = null;
151+
$effects$: null | Set<EffectSubscriptions> = null;
152152

153153
$container$: Container | null = null;
154154

@@ -184,7 +184,7 @@ export class Signal<T = any> implements ISignal<T> {
184184
}
185185
const effectSubscriber = ctx.$effectSubscriber$;
186186
if (effectSubscriber) {
187-
const effects = (this.$effects$ ||= []);
187+
const effects = (this.$effects$ ||= new Set());
188188
// Let's make sure that we have a reference to this effect.
189189
// Adding reference is essentially adding a subscription, so if the signal
190190
// changes we know who to notify.
@@ -227,7 +227,9 @@ export class Signal<T = any> implements ISignal<T> {
227227
toString() {
228228
return (
229229
`[${this.constructor.name}${(this as any).$invalid$ ? ' INVALID' : ''} ${String(this.$untrackedValue$)}]` +
230-
(this.$effects$?.map((e) => '\n -> ' + pad(qwikDebugToString(e[0]), ' ')).join('\n') || '')
230+
(Array.from(this.$effects$ || [])
231+
.map((e) => '\n -> ' + pad(qwikDebugToString(e[0]), ' '))
232+
.join('\n') || '')
231233
);
232234
}
233235
toJSON() {
@@ -245,19 +247,13 @@ export const ensureContains = (array: any[], value: any): boolean => {
245247
};
246248

247249
export const ensureContainsEffect = (
248-
array: EffectSubscriptions[],
250+
array: Set<EffectSubscriptions>,
249251
effectSubscriptions: EffectSubscriptions
250252
) => {
251-
for (let i = 0; i < array.length; i++) {
252-
const existingEffect = array[i];
253-
if (
254-
existingEffect[0] === effectSubscriptions[0] &&
255-
existingEffect[1] === effectSubscriptions[1]
256-
) {
257-
return;
258-
}
253+
if (array.has(effectSubscriptions)) {
254+
return;
259255
}
260-
array.push(effectSubscriptions);
256+
array.add(effectSubscriptions);
261257
};
262258

263259
export const ensureEffectContainsSubscriber = (
@@ -266,36 +262,35 @@ export const ensureEffectContainsSubscriber = (
266262
container: Container | null
267263
) => {
268264
if (isSubscriber(effect)) {
269-
effect.$effectDependencies$ ||= [];
270-
271-
if (subscriberExistInSubscribers(effect.$effectDependencies$, subscriber)) {
265+
effect.$effectDependencies$ ||= new Set();
266+
if (effect.$effectDependencies$.has(subscriber)) {
272267
return;
273268
}
274269

275-
effect.$effectDependencies$.push(subscriber);
270+
effect.$effectDependencies$.add(subscriber);
276271
} else if (vnode_isVNode(effect) && !vnode_isTextVNode(effect)) {
277-
let subscribers = vnode_getProp<(Subscriber | TargetType)[]>(
272+
let subscribers = vnode_getProp<Set<Subscriber | TargetType>>(
278273
effect,
279274
QSubscribers,
280275
container ? container.$getObjectById$ : null
281276
);
282-
subscribers ||= [];
277+
subscribers ||= new Set();
283278

284-
if (subscriberExistInSubscribers(subscribers, subscriber)) {
279+
if (subscribers.has(subscriber)) {
285280
return;
286281
}
287282

288-
subscribers.push(subscriber);
283+
subscribers.add(subscriber);
289284
vnode_setProp(effect, QSubscribers, subscribers);
290285
} else if (isSSRNode(effect)) {
291-
let subscribers = effect.getProp(QSubscribers) as (Subscriber | TargetType)[];
292-
subscribers ||= [];
286+
let subscribers = effect.getProp(QSubscribers) as Set<Subscriber | TargetType>;
287+
subscribers ||= new Set();
293288

294-
if (subscriberExistInSubscribers(subscribers, subscriber)) {
289+
if (subscribers.has(subscriber)) {
295290
return;
296291
}
297292

298-
subscribers.push(subscriber);
293+
subscribers.add(subscriber);
299294
effect.setProp(QSubscribers, subscribers);
300295
}
301296
};
@@ -304,18 +299,6 @@ const isSSRNode = (effect: Effect): effect is ISsrNode => {
304299
return 'setProp' in effect && 'getProp' in effect && 'removeProp' in effect && 'id' in effect;
305300
};
306301

307-
const subscriberExistInSubscribers = (
308-
subscribers: (Subscriber | TargetType)[],
309-
subscriber: Subscriber | TargetType
310-
) => {
311-
for (let i = 0; i < subscribers.length; i++) {
312-
if (subscribers[i] === subscriber) {
313-
return true;
314-
}
315-
}
316-
return false;
317-
};
318-
319302
export const addQrlToSerializationCtx = (
320303
effectSubscriber: EffectSubscriptions,
321304
isMissing: boolean,
@@ -341,7 +324,7 @@ export const addQrlToSerializationCtx = (
341324
export const triggerEffects = (
342325
container: Container | null,
343326
signal: Signal | TargetType,
344-
effects: EffectSubscriptions[] | null
327+
effects: Set<EffectSubscriptions> | null
345328
) => {
346329
const isBrowser = isDomContainer(container);
347330
if (effects) {
@@ -503,7 +486,7 @@ export class WrappedSignal<T> extends Signal<T> implements Subscriber {
503486
// We need a separate flag to know when the computation needs running because
504487
// we need the old value to know if effects need running after computation
505488
$invalid$: boolean = true;
506-
$effectDependencies$: Subscriber[] | null = null;
489+
$effectDependencies$: Set<Subscriber> | null = null;
507490
$hostElement$: HostElement | null = null;
508491
$forceRunEffects$: boolean = false;
509492

‎packages/qwik/src/core/signal/store.ts‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export const getOrCreateStore = <T extends object>(
7777
};
7878

7979
export class StoreHandler implements ProxyHandler<TargetType> {
80-
$effects$: null | Record<string | symbol, EffectSubscriptions[]> = null;
80+
$effects$: null | Record<string | symbol, Set<EffectSubscriptions>> = null;
8181

8282
constructor(
8383
public $flags$: StoreFlags,
@@ -223,7 +223,7 @@ function addEffect<T extends Record<string | symbol, any>>(
223223
const effectsMap = (store.$effects$ ||= {});
224224
const effects =
225225
(Object.prototype.hasOwnProperty.call(effectsMap, prop) && effectsMap[prop]) ||
226-
(effectsMap[prop] = []);
226+
(effectsMap[prop] = new Set());
227227
// Let's make sure that we have a reference to this effect.
228228
// Adding reference is essentially adding a subscription, so if the signal
229229
// changes we know who to notify.
@@ -260,17 +260,25 @@ function setNewValueAndTriggerEffects<T extends Record<string | symbol, any>>(
260260
function getEffects<T extends Record<string | symbol, any>>(
261261
target: T,
262262
prop: string | symbol,
263-
storeEffects: Record<string | symbol, EffectSubscriptions[]> | null
263+
storeEffects: Record<string | symbol, Set<EffectSubscriptions>> | null
264264
) {
265-
let effectsToTrigger = storeEffects
266-
? Array.isArray(target)
267-
? Object.values(storeEffects).flatMap((effects) => effects)
268-
: storeEffects[prop]
269-
: null;
265+
let effectsToTrigger: Set<EffectSubscriptions> | null = null;
266+
267+
if (storeEffects) {
268+
if (Array.isArray(target)) {
269+
for (const effects of Object.values(storeEffects)) {
270+
effectsToTrigger ||= new Set();
271+
effects.forEach((effect) => effectsToTrigger!.add(effect));
272+
}
273+
} else {
274+
effectsToTrigger = storeEffects[prop];
275+
}
276+
}
277+
270278
const storeArrayValue = storeEffects?.[STORE_ARRAY_PROP];
271279
if (storeArrayValue) {
272-
effectsToTrigger ||= [];
273-
effectsToTrigger.push(...storeArrayValue);
280+
effectsToTrigger ||= new Set();
281+
storeArrayValue.forEach((effect) => effectsToTrigger!.add(effect));
274282
}
275283
return effectsToTrigger;
276284
}

‎starters/apps/perf.prod/src/root.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import "./global.css";
44

55
export default () => {
66
return (
7-
<html>
7+
<>
88
<head>
99
<meta charset="utf-8" />
1010
<title>Qwik Blank App</title>
@@ -13,6 +13,6 @@ export default () => {
1313
<body>
1414
<App />
1515
</body>
16-
</html>
16+
</>
1717
);
1818
};

‎starters/dev-server.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ export {
216216
disableVendorScan: true,
217217
vendorRoots: enableRouterServer ? [qwikRouterMjs] : [],
218218
entryStrategy: {
219-
type: "single",
219+
type: "segment",
220220
},
221221
client: {
222222
manifestOutput(manifest) {

0 commit comments

Comments
 (0)