From 98c5afdb02192f99c886fc3fda13ec6f39018f23 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 26 Feb 2026 14:42:28 -0800 Subject: [PATCH] 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 --- packages/forms/signals/src/field/structure.ts | 251 +++++++++++------- packages/forms/signals/src/schema/logic.ts | 21 ++ .../forms/signals/src/schema/logic_node.ts | 72 ++++- .../signals/test/node/api/structure.spec.ts | 46 ++++ 4 files changed, 282 insertions(+), 108 deletions(-) diff --git a/packages/forms/signals/src/field/structure.ts b/packages/forms/signals/src/field/structure.ts index 01ef055a02d..8caa8d0b59d 100644 --- a/packages/forms/signals/src/field/structure.ts +++ b/packages/forms/signals/src/field/structure.ts @@ -84,6 +84,9 @@ export abstract class FieldNodeStructure { /** Lazily initialized injector. Do not access directly, access via `injector` getter instead. */ private _injector: DestroyableInjector | undefined = undefined; + /** Cache whether any logic rules exist on children of this node. */ + private _anyChildHasLogic?: boolean; + /** Lazily initialized injector. */ get injector(): DestroyableInjector { this._injector ??= Injector.create({ @@ -101,6 +104,7 @@ export abstract class FieldNodeStructure { /** Gets the child fields of this field. */ children(): readonly FieldNode[] { + this.ensureChildrenMap(); const map = this.childrenMap(); if (map === undefined) { return []; @@ -108,8 +112,35 @@ export abstract class FieldNodeStructure { return Array.from(map.byPropertyKey.values()).map((child) => untracked(child.reader)!); } + /** + * Internal method (cast to any in tests) to check if the children map has been materialized. + * Useful for validating that fields without logic are lazily instantiated. + * + * @internal + */ + _areChildrenMaterialized(): boolean { + return untracked(this.childrenMap) !== undefined; + } + + private ensureChildrenMap() { + // If we're already materialized, there's nothing to do. + if (this._areChildrenMaterialized()) { + return; + } + + // We force materialization by telling the linkedSignal to re-evaluate now, but treating + // its source value as having changed, or rather skipping the lazy fast-path. + untracked(() => { + (this.childrenMap as WritableSignal).update((current) => + this.computeChildrenMap(this.value(), current, true), + ); + }); + } + /** Retrieve a child `FieldNode` of this node by property key. */ getChild(key: PropertyKey): FieldNode | undefined { + this.ensureChildrenMap(); + const strKey = key.toString(); // Lookup the computed reader for this key in `childrenMap`. This lookup doesn't need to be @@ -253,110 +284,128 @@ export abstract class FieldNodeStructure { computation: ( value: unknown, previous: {source: unknown; value: ChildrenData | undefined} | undefined, - ): ChildrenData | undefined => { - if (!isObject(value)) { - // Non-object values have no children. This short-circuit path makes `childrenMap` fast - // for primitive-valued fields. - return undefined; - } - - // Previous `ChildrenData` (immutable). This is also where we first initialize our map if - // needed. - const prevData: ChildrenData = previous?.value ?? { - byPropertyKey: new Map(), - }; - - // The next `ChildrenData` object to be returned. Initialized lazily when we know there's - // been a structural change to the model. - let data: MutableChildrenData | undefined; - - const parentIsArray = isArray(value); - - // Remove fields that have disappeared since the last time this map was computed. - if (prevData !== undefined) { - if (parentIsArray) { - data = maybeRemoveStaleArrayFields(prevData, value, this.identitySymbol); - } else { - data = maybeRemoveStaleObjectFields(prevData, value); - } - } - - // Now, go through the values and add any new ones. - for (const key of Object.keys(value)) { - let trackingKey: TrackingKey | undefined = undefined; - const childValue = value[key] as unknown; - - // Fields explicitly set to `undefined` are treated as if they don't exist. - // This ensures that `{value: undefined}` and `{}` have the same behavior for their `value` - // field. - if (childValue === undefined) { - // The value might have _become_ `undefined`, so we need to delete it here. - if (prevData.byPropertyKey.has(key)) { - data ??= {...(prevData as MutableChildrenData)}; - data.byPropertyKey.delete(key); - } - continue; - } - - if (parentIsArray && isObject(childValue) && !isArray(childValue)) { - // For object values in arrays, assign a synthetic identity. This will be used to - // preserve the field instance even as this object moves around in the parent array. - trackingKey = (childValue[this.identitySymbol] as TrackingKey) ??= Symbol( - ngDevMode ? `id:${globalId++}` : '', - ) as TrackingKey; - } - - let childNode: FieldNode | undefined; - - if (trackingKey) { - // If tracking is in use, then the `FieldNode` instance is always managed via its - // tracking key. Create the instance if needed, or look it up otherwise. - if (!prevData.byTrackingKey?.has(trackingKey)) { - data ??= {...(prevData as MutableChildrenData)}; - data.byTrackingKey ??= new Map(); - - data.byTrackingKey.set( - trackingKey, - this.createChildNode(key, trackingKey, parentIsArray), - ); - } - - // Note: data ?? prevData is needed because we might have freshly instantiated - // `byTrackingKey` only in `data` above. - childNode = (data ?? prevData).byTrackingKey!.get(trackingKey)!; - } - - // Next, make sure the `ChildData` for this key in `byPropertyKey` is up to date. We need - // to consider two cases: - // - // 1. No record exists for this field (yet). - // 2. A record does exist, but the field identity at this key has changed (only possible - // when fields are tracked). - const child = prevData.byPropertyKey.get(key); - if (child === undefined) { - // No record exists yet - create one. - data ??= {...(prevData as MutableChildrenData)}; - - data.byPropertyKey.set(key, { - // TODO: creating a computed per-key is overkill when the field at a key can't change - // (e.g. the value is not an array). Maybe this can be optimized? - reader: this.createReader(key), - // If tracking is in use, then it already created/found the `childNode` for this key. - // Otherwise we create the child field here. - node: childNode ?? this.createChildNode(key, trackingKey, parentIsArray), - }); - } else if (childNode && childNode !== child.node) { - // A record exists, but records the wrong `FieldNode`. Update it. - data ??= {...(prevData as MutableChildrenData)}; - child.node = childNode; - } - } - - return data ?? prevData; - }, + ): ChildrenData | undefined => this.computeChildrenMap(value, previous?.value, false), }); } + private computeChildrenMap( + value: unknown, + prevData: ChildrenData | undefined, + forceMaterialize: boolean, + ): ChildrenData | undefined { + if (!isObject(value)) { + // Non-object values have no children. This short-circuit path makes `childrenMap` fast + // for primitive-valued fields. + return undefined; + } + + // Determine if we actually need to materialize children right now. + // If not forced, and NO child has any logic rules, we can safely return `undefined` + // to keep instantiation lazy. However, if `prevData` is already defined, we MUST + // NOT return `undefined` or we will orphan the already instantiated children. + if (!forceMaterialize && prevData === undefined) { + // Check if any child of this field has logic rules. This check only needs to run once per + // structure since the presence of schema logic rules is static across value changes. + if (!(this._anyChildHasLogic ??= this.logic.anyChildHasLogic())) { + return undefined; + } + } + + // Previous `ChildrenData` (immutable). This is also where we first initialize our map if + // needed. + prevData ??= { + byPropertyKey: new Map(), + }; + + // The next `ChildrenData` object to be returned. Initialized lazily when we know there's + // been a structural change to the model. + let materializedChildren: MutableChildrenData | undefined; + + const parentIsArray = isArray(value); + + // Remove fields that have disappeared since the last time this map was computed. + if (prevData !== undefined) { + if (parentIsArray) { + materializedChildren = maybeRemoveStaleArrayFields(prevData, value, this.identitySymbol); + } else { + materializedChildren = maybeRemoveStaleObjectFields(prevData, value); + } + } + + // Now, go through the values and add any new ones. + for (const key of Object.keys(value)) { + let trackingKey: TrackingKey | undefined = undefined; + const childValue = value[key] as unknown; + + // Fields explicitly set to `undefined` are treated as if they don't exist. + // This ensures that `{value: undefined}` and `{}` have the same behavior for their `value` + // field. + if (childValue === undefined) { + // The value might have _become_ `undefined`, so we need to delete it here. + if (prevData.byPropertyKey.has(key)) { + materializedChildren ??= {...(prevData as MutableChildrenData)}; + materializedChildren.byPropertyKey.delete(key); + } + continue; + } + + if (parentIsArray && isObject(childValue) && !isArray(childValue)) { + // For object values in arrays, assign a synthetic identity. This will be used to + // preserve the field instance even as this object moves around in the parent array. + trackingKey = (childValue[this.identitySymbol] as TrackingKey) ??= Symbol( + ngDevMode ? `id:${globalId++}` : '', + ) as TrackingKey; + } + + let childNode: FieldNode | undefined; + + if (trackingKey) { + // If tracking is in use, then the `FieldNode` instance is always managed via its + // tracking key. Create the instance if needed, or look it up otherwise. + if (!prevData.byTrackingKey?.has(trackingKey)) { + materializedChildren ??= {...(prevData as MutableChildrenData)}; + materializedChildren.byTrackingKey ??= new Map(); + + materializedChildren.byTrackingKey.set( + trackingKey, + this.createChildNode(key, trackingKey, parentIsArray), + ); + } + + // Note: materializedChildren ?? prevData is needed because we might have freshly instantiated + // `byTrackingKey` only in `materializedChildren` above. + childNode = (materializedChildren ?? prevData).byTrackingKey!.get(trackingKey)!; + } + + // Next, make sure the `ChildData` for this key in `byPropertyKey` is up to date. We need + // to consider two cases: + // + // 1. No record exists for this field (yet). + // 2. A record does exist, but the field identity at this key has changed (only possible + // when fields are tracked). + const child = prevData.byPropertyKey.get(key); + if (child === undefined) { + // No record exists yet - create one. + materializedChildren ??= {...(prevData as MutableChildrenData)}; + + materializedChildren.byPropertyKey.set(key, { + // TODO: creating a computed per-key is overkill when the field at a key can't change + // (e.g. the value is not an array). Maybe this can be optimized? + reader: this.createReader(key), + // If tracking is in use, then it already created/found the `childNode` for this key. + // Otherwise we create the child field here. + node: childNode ?? this.createChildNode(key, trackingKey, parentIsArray), + }); + } else if (childNode && childNode !== child.node) { + // A record exists, but records the wrong `FieldNode`. Update it. + materializedChildren ??= {...(prevData as MutableChildrenData)}; + child.node = childNode; + } + } + + return materializedChildren ?? prevData; + } + /** * Creates a "reader" computed for the given key. * diff --git a/packages/forms/signals/src/schema/logic.ts b/packages/forms/signals/src/schema/logic.ts index c9d8813c0df..02cded06e45 100644 --- a/packages/forms/signals/src/schema/logic.ts +++ b/packages/forms/signals/src/schema/logic.ts @@ -112,6 +112,11 @@ export abstract class AbstractLogic { : other.fns; this.fns.push(...fns); } + + /** Checks if any logic rules are registered in this instance. */ + hasRules(): boolean { + return this.fns.length > 0; + } } /** Logic that combines its individual logic function results with logical OR. */ @@ -282,6 +287,22 @@ export class LogicContainer { ); } + /** + * Checks whether this container has any logic rules registered in any of its categories. + * @returns True if at least one logic rule exists. + */ + hasAnyLogic(): boolean { + return ( + this.hidden.hasRules() || + this.disabledReasons.hasRules() || + this.readonly.hasRules() || + this.syncErrors.hasRules() || + this.syncTreeErrors.hasRules() || + this.asyncErrors.hasRules() || + this.metadata.size > 0 + ); + } + /** Checks whether there is logic for the given metadata key. */ hasMetadata(key: MetadataKey) { return this.metadata.has(key); diff --git a/packages/forms/signals/src/schema/logic_node.ts b/packages/forms/signals/src/schema/logic_node.ts index be3bfa1b33d..1001e34f9cf 100644 --- a/packages/forms/signals/src/schema/logic_node.ts +++ b/packages/forms/signals/src/schema/logic_node.ts @@ -61,6 +61,18 @@ export abstract class AbstractLogicNodeBuilder { */ abstract hasLogic(builder: AbstractLogicNodeBuilder): boolean; + /** + * Checks whether this builder or any of its children have any logic rules defined. + * @returns True if rules exist, false otherwise. + */ + abstract hasRules(): boolean; + + /** + * Checks whether any of the children of this builder have any logic rules defined. + * @returns True if rules exist on any child, false otherwise. + */ + abstract anyChildHasLogic(): boolean; + /** * Builds the `LogicNode` from the accumulated rules and child builders. * @returns The constructed `LogicNode`. @@ -155,6 +167,14 @@ export class LogicNodeBuilder extends AbstractLogicNodeBuilder { return this.all.some(({builder: subBuilder}) => subBuilder.hasLogic(builder)); } + override hasRules(): boolean { + return this.all.length > 0; + } + + override anyChildHasLogic(): boolean { + return this.all.some(({builder}) => builder.anyChildHasLogic()); + } + /** * Merges logic from another `LogicNodeBuilder` into this one. * If a `predicate` is provided, all logic from the `other` builder will only apply @@ -266,6 +286,19 @@ class NonMergeableLogicNodeBuilder extends AbstractLogicNodeBuilder { override hasLogic(builder: AbstractLogicNodeBuilder): boolean { return this === builder; } + + override hasRules(): boolean { + return this.logic.hasAnyLogic() || this.children.size > 0; + } + + override anyChildHasLogic(): boolean { + for (const child of this.children.values()) { + if (child.hasRules()) { + return true; + } + } + return false; + } } /** @@ -291,6 +324,18 @@ export interface LogicNode { * @returns True if the builder has been merged, false otherwise. */ hasLogic(builder: AbstractLogicNodeBuilder): boolean; + + /** + * Checks whether this node or any of its children have any logic rules defined. + * @returns True if rules exist, false otherwise. + */ + hasRules(): boolean; + + /** + * Checks whether any of the children of this node have any logic rules defined. + * @returns True if rules exist on any child, false otherwise. + */ + anyChildHasLogic(): boolean; } /** @@ -349,14 +394,19 @@ class LeafLogicNode implements LogicNode { } } - /** - * Checks whether the logic from a particular `AbstractLogicNodeBuilder` has been merged into this - * node. - * @param builder The builder to check for. - * @returns True if the builder has been merged, false otherwise. - */ hasLogic(builder: AbstractLogicNodeBuilder): boolean { - return this.builder?.hasLogic(builder) ?? false; + if (!this.builder) { + return false; + } + return this.builder.hasLogic(builder); + } + + hasRules(): boolean { + return this.builder ? this.builder.hasRules() : false; + } + + anyChildHasLogic(): boolean { + return this.builder ? this.builder.anyChildHasLogic() : false; } } @@ -399,6 +449,14 @@ class CompositeLogicNode implements LogicNode { hasLogic(builder: AbstractLogicNodeBuilder): boolean { return this.all.some((node) => node.hasLogic(builder)); } + + hasRules(): boolean { + return this.all.some((node) => node.hasRules()); + } + + anyChildHasLogic(): boolean { + return this.all.some((child) => child.anyChildHasLogic()); + } } /** diff --git a/packages/forms/signals/test/node/api/structure.spec.ts b/packages/forms/signals/test/node/api/structure.spec.ts index cf2e7f5f7a5..fb85b8e6756 100644 --- a/packages/forms/signals/test/node/api/structure.spec.ts +++ b/packages/forms/signals/test/node/api/structure.spec.ts @@ -84,4 +84,50 @@ describe('structure APIs', () => { expect(f.b().errors()).toEqual([jasmine.objectContaining({message: 'each'})]); }); }); + + describe('lazy materialization', () => { + it('should stay unmaterialized when children have no schema logic', () => { + const s = schema<{a: {b: string}}>((p) => { + // No logic applied + }); + + const data = signal({a: {b: 'test'}}); + const f = form(data, s, {injector: TestBed.inject(Injector)}); + + // Cast to any to access the private helper method + expect((f() as any).structure._areChildrenMaterialized()).toBeFalse(); + }); + + it('should materialize children when explicitly accessed', () => { + const s = schema<{a: {b: string}}>((p) => { + // No logic applied + }); + + const data = signal({a: {b: 'test'}}); + const f = form(data, s, {injector: TestBed.inject(Injector)}); + + // Initially false + expect((f() as any).structure._areChildrenMaterialized()).toBeFalse(); + + // Accessing 'a' forces materialization of root's children + const childA = f.a; + expect((f() as any).structure._areChildrenMaterialized()).toBeTrue(); + + // 'a' itself should not have materialized its children ('b') yet + expect((childA() as any).structure._areChildrenMaterialized()).toBeFalse(); + }); + + it('should eagerly materialize children when they have schema logic', () => { + const s = schema<{a: {b: string}}>((p) => { + required(p.a.b, {message: 'required'}); + }); + + const data = signal({a: {b: 'test'}}); + const f = form(data, s, {injector: TestBed.inject(Injector)}); + + // Because 'a.b' has rules, 'a' and 'b' must be materialized up the tree + expect((f() as any).structure._areChildrenMaterialized()).toBeTrue(); + expect((f.a() as any).structure._areChildrenMaterialized()).toBeTrue(); + }); + }); });