Repository navigation
Conversation
JeanMeche
left a comment
JeanMeche
left a comment
There was a problem hiding this comment.
AGENT: Thanks for this fix! The change in markStructuresLive correctly resolves the eager traversal during the GC effect flush that was defeating the lazy instantiation from 98c5afdb02.
A few observations and questions to challenge the broader scope and tests:
-
Broader Scope of Laziness (
markAsTouched,reloadValidation,submit):
markStructuresLiveis only one of the internal tree walkers. Other methods likemarkAsTouched()(node.ts:293) andreloadValidation()(node.ts:372) still callstructure.children().
Sincesubmit()marks the root as touched, callingsubmit()on a form containing a 10,000-row model will still recursively callchildren()and trigger the exact same eager materialization / OOM freeze described in #70627. Should tree traversals have a uniform contract regarding materialized vs unmaterialized nodes? -
JSDoc on
materializedChildren():
InFieldNodeStructure(structure.ts:126), the JSDoc explicitly notes:Note: This method assumes it is called within an untracked context (or in a non-reactive context) as it reads signals without wrapping them in untracked().
createFieldManagementEffectnow callsmaterializedChildren()inside aneffect()withoutuntracked(), which intentionally subscribes the effect tochildrenMap. We should update this JSDoc note so it doesn't contradict the new usage. -
Test Assertions in
structure.spec.ts:- In the first test, replacing
_areChildrenMaterialized()withstructure.materializedChildren().toEqual([])weakens the check._areChildrenMaterialized()directly verifies that thechildrenMaplinkedSignal was not computed (undefined), whereasmaterializedChildren().toEqual([])does not distinguish between uncomputed and an empty map. - In
should not materialize unread descendants during cleanup, accessingf.a()eagerly computesroot'schildrenMap, which materializes all direct siblings at the same depth (unused). Onlyunused.nestedremained unmaterialized. It would be helpful to clarify the test description and verify whetherunuseditself was materialized. - In the array cleanup test, both elements are materialized before array modification (which passes even on
main). Adding a case where only a subset of array items are materialized before an array splice/reorder would give more robust coverage.
- In the first test, replacing
|
Thanks for the review, @JeanMeche! I updated the JSDoc to explain that reading the children map in the cleanup effect tracks structural changes. The tests now check the map directly with I think the traversal contract depends on the operation. Cleanup only needs to visit existing fields. The review updates are in f0aa9c1. |
Walk only materialized fields during cleanup so large model values do not create an unused field tree. Preserve cleanup of removed fields and identity of reordered array items. Cover lazy child maps and verify cleanup of removed array fields and their materialized descendants after reordering. Fixes angular#70627
f0aa9c1 to
ef53278
Compare
What is the current behavior?
Fixes #70627.
The field cleanup effect calls
children(), which forces unused descendants to materialize. A large model with no child rules therefore creates a complete field tree on the first effect flush and blocks the page.What is the new behavior?
Walk
materializedChildren()during cleanup. Explicit field access and schema logic continue to create fields as before.The regression tests cover laziness after effects flush and model updates, unread descendants of an accessed field, cleanup of fields first accessed after the initial sweep, identity across array reordering, and validation of fields with rules. Two tests fail with the original traversal.
Production-page measurements
Used the reporter's application with Angular 22.2.3, CLI/build 22.2.4, Node 24.22.1, and Chrome Headless 158.0.8117.0. Both optimized builds use identical instrumentation; the patched build changes only this traversal in the installed forms bundle.
For 10,000 rows, five alternating baseline/patched pairs in fresh browser contexts:
The baseline produced a 536–627 ms long task in every run; the patched run produced no long task over 50 ms. Model allocation happens before the measured action. These are local page measurements, not INP or device-independent guarantees.
Control: 50 rendered inputs with
required(), seven alternating pairs. Medians were 29.6 ms before / 28.7 ms after; both created 250 child fields, and editing updated the model in every run. The patched app also completed the reporter's original 100,000-row case in three runs without materializing children. The baseline was measured at 10,000 rows rather than run to exhaustion at 100,000.Validation
os.networkInterfaces(); that shim is outside this change.ng-dev format, Prettier, andgit diff --checkpass.Related prior work: #70633 and #71635.
PR Checklist
PR Type
Does this PR introduce a breaking change?