Skip to content

Commit 98c5afd

Browse files
alxhubleonsenft
alxhub
authored and
leonsenft
committed
perf(forms): lazily instantiate signal form fields
Currently, Signal Forms eagerly instantiates all nodes in the form tree because `childrenMap` iterates over the `value` and creates a `FieldNode` for every property. This ensures validation side-effects are run early, but creates pure overhead for fields without validation logic unless explicitly accessed. This commit makes `childrenMap` lazy by default, skipping materialization for children without schema logic. This is achieved by introducing `hasLogicRules()` and `anyChildHasLogic()` across the `LogicNode` hierarchy. Fields are now only instantiated when a direct read occurs via `getChild()` (which calls the new `ensureChildrenMap()`) or if their subtree requires eager evaluation due to existing validation rules. Fixes #67212
1 parent 50e599e commit 98c5afd

File tree

4 files changed

+270
-96
lines changed

4 files changed

+270
-96
lines changed

‎packages/forms/signals/src/field/structure.ts‎

Lines changed: 138 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ export abstract class FieldNodeStructure {
8484
/** Lazily initialized injector. Do not access directly, access via `injector` getter instead. */
8585
private _injector: DestroyableInjector | undefined = undefined;
8686

87+
/** Cache whether any logic rules exist on children of this node. */
88+
private _anyChildHasLogic?: boolean;
89+
8790
/** Lazily initialized injector. */
8891
get injector(): DestroyableInjector {
8992
this._injector ??= Injector.create({
@@ -101,15 +104,43 @@ export abstract class FieldNodeStructure {
101104

102105
/** Gets the child fields of this field. */
103106
children(): readonly FieldNode[] {
107+
this.ensureChildrenMap();
104108
const map = this.childrenMap();
105109
if (map === undefined) {
106110
return [];
107111
}
108112
return Array.from(map.byPropertyKey.values()).map((child) => untracked(child.reader)!);
109113
}
110114

115+
/**
116+
* Internal method (cast to any in tests) to check if the children map has been materialized.
117+
* Useful for validating that fields without logic are lazily instantiated.
118+
*
119+
* @internal
120+
*/
121+
_areChildrenMaterialized(): boolean {
122+
return untracked(this.childrenMap) !== undefined;
123+
}
124+
125+
private ensureChildrenMap() {
126+
// If we're already materialized, there's nothing to do.
127+
if (this._areChildrenMaterialized()) {
128+
return;
129+
}
130+
131+
// We force materialization by telling the linkedSignal to re-evaluate now, but treating
132+
// its source value as having changed, or rather skipping the lazy fast-path.
133+
untracked(() => {
134+
(this.childrenMap as WritableSignal<ChildrenData | undefined>).update((current) =>
135+
this.computeChildrenMap(this.value(), current, true),
136+
);
137+
});
138+
}
139+
111140
/** Retrieve a child `FieldNode` of this node by property key. */
112141
getChild(key: PropertyKey): FieldNode | undefined {
142+
this.ensureChildrenMap();
143+
113144
const strKey = key.toString();
114145

115146
// Lookup the computed reader for this key in `childrenMap`. This lookup doesn't need to be
@@ -253,108 +284,126 @@ export abstract class FieldNodeStructure {
253284
computation: (
254285
value: unknown,
255286
previous: {source: unknown; value: ChildrenData | undefined} | undefined,
256-
): ChildrenData | undefined => {
257-
if (!isObject(value)) {
258-
// Non-object values have no children. This short-circuit path makes `childrenMap` fast
259-
// for primitive-valued fields.
260-
return undefined;
261-
}
287+
): ChildrenData | undefined => this.computeChildrenMap(value, previous?.value, false),
288+
});
289+
}
262290

263-
// Previous `ChildrenData` (immutable). This is also where we first initialize our map if
264-
// needed.
265-
const prevData: ChildrenData = previous?.value ?? {
266-
byPropertyKey: new Map(),
267-
};
291+
private computeChildrenMap(
292+
value: unknown,
293+
prevData: ChildrenData | undefined,
294+
forceMaterialize: boolean,
295+
): ChildrenData | undefined {
296+
if (!isObject(value)) {
297+
// Non-object values have no children. This short-circuit path makes `childrenMap` fast
298+
// for primitive-valued fields.
299+
return undefined;
300+
}
268301

269-
// The next `ChildrenData` object to be returned. Initialized lazily when we know there's
270-
// been a structural change to the model.
271-
let data: MutableChildrenData | undefined;
302+
// Determine if we actually need to materialize children right now.
303+
// If not forced, and NO child has any logic rules, we can safely return `undefined`
304+
// to keep instantiation lazy. However, if `prevData` is already defined, we MUST
305+
// NOT return `undefined` or we will orphan the already instantiated children.
306+
if (!forceMaterialize && prevData === undefined) {
307+
// Check if any child of this field has logic rules. This check only needs to run once per
308+
// structure since the presence of schema logic rules is static across value changes.
309+
if (!(this._anyChildHasLogic ??= this.logic.anyChildHasLogic())) {
310+
return undefined;
311+
}
312+
}
272313

273-
const parentIsArray = isArray(value);
314+
// Previous `ChildrenData` (immutable). This is also where we first initialize our map if
315+
// needed.
316+
prevData ??= {
317+
byPropertyKey: new Map(),
318+
};
274319

275-
// Remove fields that have disappeared since the last time this map was computed.
276-
if (prevData !== undefined) {
277-
if (parentIsArray) {
278-
data = maybeRemoveStaleArrayFields(prevData, value, this.identitySymbol);
279-
} else {
280-
data = maybeRemoveStaleObjectFields(prevData, value);
281-
}
282-
}
320+
// The next `ChildrenData` object to be returned. Initialized lazily when we know there's
321+
// been a structural change to the model.
322+
let materializedChildren: MutableChildrenData | undefined;
283323

284-
// Now, go through the values and add any new ones.
285-
for (const key of Object.keys(value)) {
286-
let trackingKey: TrackingKey | undefined = undefined;
287-
const childValue = value[key] as unknown;
288-
289-
// Fields explicitly set to `undefined` are treated as if they don't exist.
290-
// This ensures that `{value: undefined}` and `{}` have the same behavior for their `value`
291-
// field.
292-
if (childValue === undefined) {
293-
// The value might have _become_ `undefined`, so we need to delete it here.
294-
if (prevData.byPropertyKey.has(key)) {
295-
data ??= {...(prevData as MutableChildrenData)};
296-
data.byPropertyKey.delete(key);
297-
}
298-
continue;
299-
}
324+
const parentIsArray = isArray(value);
300325

301-
if (parentIsArray && isObject(childValue) && !isArray(childValue)) {
302-
// For object values in arrays, assign a synthetic identity. This will be used to
303-
// preserve the field instance even as this object moves around in the parent array.
304-
trackingKey = (childValue[this.identitySymbol] as TrackingKey) ??= Symbol(
305-
ngDevMode ? `id:${globalId++}` : '',
306-
) as TrackingKey;
307-
}
326+
// Remove fields that have disappeared since the last time this map was computed.
327+
if (prevData !== undefined) {
328+
if (parentIsArray) {
329+
materializedChildren = maybeRemoveStaleArrayFields(prevData, value, this.identitySymbol);
330+
} else {
331+
materializedChildren = maybeRemoveStaleObjectFields(prevData, value);
332+
}
333+
}
308334

309-
let childNode: FieldNode | undefined;
335+
// Now, go through the values and add any new ones.
336+
for (const key of Object.keys(value)) {
337+
let trackingKey: TrackingKey | undefined = undefined;
338+
const childValue = value[key] as unknown;
339+
340+
// Fields explicitly set to `undefined` are treated as if they don't exist.
341+
// This ensures that `{value: undefined}` and `{}` have the same behavior for their `value`
342+
// field.
343+
if (childValue === undefined) {
344+
// The value might have _become_ `undefined`, so we need to delete it here.
345+
if (prevData.byPropertyKey.has(key)) {
346+
materializedChildren ??= {...(prevData as MutableChildrenData)};
347+
materializedChildren.byPropertyKey.delete(key);
348+
}
349+
continue;
350+
}
310351

311-
if (trackingKey) {
312-
// If tracking is in use, then the `FieldNode` instance is always managed via its
313-
// tracking key. Create the instance if needed, or look it up otherwise.
314-
if (!prevData.byTrackingKey?.has(trackingKey)) {
315-
data ??= {...(prevData as MutableChildrenData)};
316-
data.byTrackingKey ??= new Map();
352+
if (parentIsArray && isObject(childValue) && !isArray(childValue)) {
353+
// For object values in arrays, assign a synthetic identity. This will be used to
354+
// preserve the field instance even as this object moves around in the parent array.
355+
trackingKey = (childValue[this.identitySymbol] as TrackingKey) ??= Symbol(
356+
ngDevMode ? `id:${globalId++}` : '',
357+
) as TrackingKey;
358+
}
317359

318-
data.byTrackingKey.set(
319-
trackingKey,
320-
this.createChildNode(key, trackingKey, parentIsArray),
321-
);
322-
}
360+
let childNode: FieldNode | undefined;
323361

324-
// Note: data ?? prevData is needed because we might have freshly instantiated
325-
// `byTrackingKey` only in `data` above.
326-
childNode = (data ?? prevData).byTrackingKey!.get(trackingKey)!;
327-
}
362+
if (trackingKey) {
363+
// If tracking is in use, then the `FieldNode` instance is always managed via its
364+
// tracking key. Create the instance if needed, or look it up otherwise.
365+
if (!prevData.byTrackingKey?.has(trackingKey)) {
366+
materializedChildren ??= {...(prevData as MutableChildrenData)};
367+
materializedChildren.byTrackingKey ??= new Map();
328368

329-
// Next, make sure the `ChildData` for this key in `byPropertyKey` is up to date. We need
330-
// to consider two cases:
331-
//
332-
// 1. No record exists for this field (yet).
333-
// 2. A record does exist, but the field identity at this key has changed (only possible
334-
// when fields are tracked).
335-
const child = prevData.byPropertyKey.get(key);
336-
if (child === undefined) {
337-
// No record exists yet - create one.
338-
data ??= {...(prevData as MutableChildrenData)};
339-
340-
data.byPropertyKey.set(key, {
341-
// TODO: creating a computed per-key is overkill when the field at a key can't change
342-
// (e.g. the value is not an array). Maybe this can be optimized?
343-
reader: this.createReader(key),
344-
// If tracking is in use, then it already created/found the `childNode` for this key.
345-
// Otherwise we create the child field here.
346-
node: childNode ?? this.createChildNode(key, trackingKey, parentIsArray),
347-
});
348-
} else if (childNode && childNode !== child.node) {
349-
// A record exists, but records the wrong `FieldNode`. Update it.
350-
data ??= {...(prevData as MutableChildrenData)};
351-
child.node = childNode;
352-
}
369+
materializedChildren.byTrackingKey.set(
370+
trackingKey,
371+
this.createChildNode(key, trackingKey, parentIsArray),
372+
);
353373
}
354374

355-
return data ?? prevData;
356-
},
357-
});
375+
// Note: materializedChildren ?? prevData is needed because we might have freshly instantiated
376+
// `byTrackingKey` only in `materializedChildren` above.
377+
childNode = (materializedChildren ?? prevData).byTrackingKey!.get(trackingKey)!;
378+
}
379+
380+
// Next, make sure the `ChildData` for this key in `byPropertyKey` is up to date. We need
381+
// to consider two cases:
382+
//
383+
// 1. No record exists for this field (yet).
384+
// 2. A record does exist, but the field identity at this key has changed (only possible
385+
// when fields are tracked).
386+
const child = prevData.byPropertyKey.get(key);
387+
if (child === undefined) {
388+
// No record exists yet - create one.
389+
materializedChildren ??= {...(prevData as MutableChildrenData)};
390+
391+
materializedChildren.byPropertyKey.set(key, {
392+
// TODO: creating a computed per-key is overkill when the field at a key can't change
393+
// (e.g. the value is not an array). Maybe this can be optimized?
394+
reader: this.createReader(key),
395+
// If tracking is in use, then it already created/found the `childNode` for this key.
396+
// Otherwise we create the child field here.
397+
node: childNode ?? this.createChildNode(key, trackingKey, parentIsArray),
398+
});
399+
} else if (childNode && childNode !== child.node) {
400+
// A record exists, but records the wrong `FieldNode`. Update it.
401+
materializedChildren ??= {...(prevData as MutableChildrenData)};
402+
child.node = childNode;
403+
}
404+
}
405+
406+
return materializedChildren ?? prevData;
358407
}
359408

360409
/**

‎packages/forms/signals/src/schema/logic.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,11 @@ export abstract class AbstractLogic<TReturn, TValue = TReturn> {
112112
: other.fns;
113113
this.fns.push(...fns);
114114
}
115+
116+
/** Checks if any logic rules are registered in this instance. */
117+
hasRules(): boolean {
118+
return this.fns.length > 0;
119+
}
115120
}
116121

117122
/** Logic that combines its individual logic function results with logical OR. */
@@ -282,6 +287,22 @@ export class LogicContainer {
282287
);
283288
}
284289

290+
/**
291+
* Checks whether this container has any logic rules registered in any of its categories.
292+
* @returns True if at least one logic rule exists.
293+
*/
294+
hasAnyLogic(): boolean {
295+
return (
296+
this.hidden.hasRules() ||
297+
this.disabledReasons.hasRules() ||
298+
this.readonly.hasRules() ||
299+
this.syncErrors.hasRules() ||
300+
this.syncTreeErrors.hasRules() ||
301+
this.asyncErrors.hasRules() ||
302+
this.metadata.size > 0
303+
);
304+
}
305+
285306
/** Checks whether there is logic for the given metadata key. */
286307
hasMetadata(key: MetadataKey<any, any, any>) {
287308
return this.metadata.has(key);

0 commit comments

Comments
 (0)