Skip to content

perf: remove eager expandNode from ArrayView#5605

Open
Karavil wants to merge 1 commit intorocicorp:mainfrom
Karavil:fix/remove-eager-expand-node
Open

perf: remove eager expandNode from ArrayView#5605
Karavil wants to merge 1 commit intorocicorp:mainfrom
Karavil:fix/remove-eager-expand-node

Conversation

@Karavil
Copy link
Contributor

@Karavil Karavil commented Feb 25, 2026

Problem

PR #5462 introduced expandNode/expandChange to support change buffering in ArrayView. These functions eagerly materialize ALL relationship thunks recursively before passing nodes to applyChange. For queries with wide/deep relationships, this turned into the single most expensive operation in the IVM pipeline.

Profiled on a page with 45 parent rows, ~200 related rows per parent, and 135 IVM pipelines (3 queries per parent row), expandNode became the dominant cost at 60.7% inclusive time (10,672ms), a 4.3x regression over the pre-#5462 baseline.

The root cause: expandNode recursively calls Array.from(skipYields(v()), expandNode) on every relationship key of every node, eagerly walking entire relationship trees. Combined with mapValues/mapEntries object allocation and generator overhead through skipYields, this created O(tree_size * pipeline_count) work on every push() and #hydrate() call.

Profiled numbers (Chrome DevTools, React DEV mode)

Test scenario: page rendering 45 parent rows with ~200 related rows each, 135 IVM pipelines (3 queries per parent row).

Before (0.26-canary.8 with expandNode) vs After (this PR)

Metric                     Before          After           Change
-----------------------------------------------------------------------
Longest task               12,900ms        7,686ms         40% faster
Long tasks (>50ms)         11              6               45% fewer

Function-level breakdown (inclusive time)

Function                   Before               After                Change
---------------------------------------------------------------------------------
expandNode                 10,672ms (60.7%)      53ms (0.3%)         ELIMINATED
mapValues/mapEntries       10,616ms (60.3%)      106ms (0.6%)        ELIMINATED
skipYields                  5,984ms (34.0%)      108ms (0.7%)        93% reduction
#pushChild                 11,895ms (67.6%)     6,792ms (44.9%)      43% reduction
filterPush                  5,789ms (32.9%)     2,085ms (13.8%)      64% reduction
compareUTF8                   402ms (2.3%)        121ms (0.8%)        70% reduction
GC                            776ms (4.4%)        848ms (5.6%)        similar

Self-time (leaf functions)

Function                   Before               After
------------------------------------------------------
run (React DEV scheduler)  7,653ms (43.5%)      8,310ms (54.9%)   [constant, dev-only]
expandNode                 ~2,600ms (~15%)       negligible        ELIMINATED
skipYields                   450ms (2.6%)         108ms (0.7%)     76% reduction
compareUTF8                  402ms (2.3%)         121ms (0.8%)     70% reduction
#fetch                       530ms (3.0%)         189ms (1.2%)     64% reduction

Production estimate

The run function (React DEV mode scheduler) accounts for ~48% of total time but disappears in production builds. Subtracting dev-only overhead:

  • Before (with expandNode): ~6.5s estimated production blocking time
  • After (this PR): ~2.5 to 3s estimated production blocking time
  • Remaining time dominated by N+1 pipeline pattern (135 pipelines), not ArrayView

Data flow: before vs after

BEFORE (0.26 with expandNode):
                                                              
  push(change) ──> expandChange(change)                       
                        |                                     
                        ├── expandNode(node)                  
                        |     ├── mapValues(relationships)    
                        |     |     ├── skipYields(thunk())   
                        |     |     ├── Array.from(...)       
                        |     |     └── expandNode (recurse)  x N children
                        |     └── { row, relationships }      
                        |                                     
                        ├── expandNode(oldNode)  [for edits]  
                        |     └── (same recursive walk)       
                        |                                     
                        └── ExpandedChange                    
                              |                               
                     buffer in #pendingChanges                
                              |                               
                     flush() ──> applyChanges(expanded)       
                                    └── getChildNodes(array)  


AFTER (this PR):

  push(change) ──> applyChange(root, change)
                        |
                        └── getChildNodes(node.relationships)
                              |
                              ├── if Array.isArray  ──> yield* array
                              └── else (thunk)      ──> yield* skipYields(thunk())
                                                        evaluated lazily,
                                                        only for relationships
                                                        that applyChange
                                                        actually touches

The key insight: getChildNodes() in view-apply-change.ts already handles both expanded arrays AND lazy thunks natively. It uses Array.isArray(children) to distinguish the two cases and evaluates thunks on demand. The eager expandNode was doing redundant work that getChildNodes would do anyway, but recursively across the ENTIRE tree instead of just the nodes being changed.

Fix

Remove expandNode/expandChange and the change buffering (#pendingChanges, #applyPendingChanges, auto-flush in .data). Apply each change immediately in push() using the immutable applyChange, exactly as the original code did before #5462 but preserving the immutable return semantics that #5462 introduced.

This is safe because:

  1. Thunks evaluated at the right time. push() evaluates thunks immediately while source state is current. No stale captures.
  2. getChildNodes() handles lazy thunks natively. It uses Array.isArray(children) to distinguish expanded arrays from lazy generator thunks. No expansion needed.
  3. Listener contract preserved. Listeners still only fire on flush(), so the push/flush batching contract is maintained.
  4. Immutable applyChange preserved. Returns new Entry objects, preserves identity for unchanged nodes. React.memo / shallow comparison on view data still works correctly.

Changes

  • Remove expandNode and expandChange functions
  • Remove mapValues import (only used by expandNode)
  • Remove ExpandedNode type import (no longer needed in array-view)
  • Remove change buffering (#pendingChanges, #applyPendingChanges, auto-flush in .data)
  • Apply changes immediately via this.#root = applyChange(...) in push()
  • Update test: singular format error now throws at push time, not flush time

Test results

All 2,212 ZQL tests pass across 132 test files (2 pre-existing skips unrelated to this change).

Follows up on #5462.

Remove the recursive eager expansion of relationship thunks that was
introduced in rocicorp#5462. Instead of buffering expanded changes and applying
them in batch at flush time, apply each change immediately in push()
using the immutable applyChange. This preserves correctness (thunks are
evaluated while source state is current) and eliminates the O(tree)
recursive expansion overhead.

The getChildNodes() helper in view-apply-change.ts already handles
lazy thunks natively, so no changes are needed there.

* Remove expandNode and expandChange functions
* Remove mapValues import (only used by expandNode)
* Remove change buffering (#pendingChanges array, #applyPendingChanges)
* Apply changes immediately in push() via applyChange (immutable)
* Update test: singular format error now throws at push time, not flush
* All 2212 ZQL tests pass (132 files, 2 pre-existing skips)
@vercel
Copy link

vercel bot commented Feb 25, 2026

Someone is attempting to deploy a commit to the Rocicorp Team on Vercel.

A member of the Team first needs to authorize it.

@arv arv self-requested a review February 25, 2026 09:31
Copy link
Contributor

@arv arv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

When looking at the previous PR I was also looking into removing the data getter but it turned out to be a larger change. I still believe we should get rid of that but in a separate PR.

Karavil pushed a commit to Karavil/mono that referenced this pull request Feb 26, 2026
Adds comprehensive tests verifying that the immutable applyChange pipeline
preserves object identity for unchanged nodes (enabling React.memo) while
correctly bubbling new references up for changed descendants.

NOTE: Several tests fail on main due to expandNode eagerly materializing
all relationships (breaking identity). These pass with PR rocicorp#5605 which
removes expandNode. The two PRs should be merged together.

* ArrayView flat list: edit/add/remove preserve sibling identity
* Relationship propagation: child edit/add/remove bubble new refs to parent
* 3-level deep: grandchild edit bubbles through entire ancestor chain
* React snapshot identity: getSnapshot stability, sentinel reuse
* React.memo render counting: only changed row children re-render
* Data flash prevention: no empty snapshots between data updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants