mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
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
This commit is contained in:
Alex Rickabaugh
committed by
Leon Senft
Leon Senft
parent
50e599e73e
commit
98c5afdb02
@@ -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<ChildrenData | undefined>).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.
|
||||
*
|
||||
|
||||
@@ -112,6 +112,11 @@ export abstract class AbstractLogic<TReturn, TValue = TReturn> {
|
||||
: 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<any, any, any>) {
|
||||
return this.metadata.has(key);
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user