perf(framework): drop redundant O(n) child-order copy in reconcile#35
Merged
Merged
Conversation
MultiChildRenderObjectElement._syncChildRenderObjects pre-checked child order via `owner.children`, but that getter returns List.unmodifiable — a full copy of the child list — and the check duplicated the identical `hasSameRenderChildrenInOrder` guard that `replaceAllChildren` already runs against its internal list with no copy. Every multi-child element paid a redundant O(children) copy + duplicate scan on every rebuild. Call `replaceAllChildren` directly (it no-ops unchanged order itself) and drop the now-dead `_sameRenderObjectOrder` helper. Behavior-identical (1042 widget+rendering tests green, incl. the same-order-no-op and reordered-invalidate guards); ~10% off the isolated no-op-rebuild reconcile tax at mid tree sizes. Also give `_RenderListView.replaceAllChildren` the same-order guard that every other RenderObjectWithChildren already has — it was the lone implementation missing it. This is a consistency fix, NOT a regression fix: a code review flagged that dropping the element-side pre-check would make the eager ListView re-layout on same-order rebuilds, but on verification that is masked — `_ListViewBody.updateRenderObject` marks needs-layout unconditionally every rebuild (to re-read mutable controller state), so the guard is inert for layout today. Kept for parity and to stay correct if that mark ever becomes conditional; comment documents the masking. Records the §2 retained-tree-tax investigation in the perf-pass findings: the tax is lean and sublinear (0.06 µs/node, ~85 µs even at 1465 nodes), the aggregate profiler's replaceAllChildren dominance is mount not steady-state, and the layout-skip / opt-in paint-memoization / lean-reconcile levers are all present and correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes a redundant child-order pre-check in MultiChildRenderObjectElement._syncChildRenderObjects that forced an O(n) copy via RenderObjectWithChildren.children (List.unmodifiable(...)) and duplicated the order-guard already performed inside each replaceAllChildren implementation. It also adds the missing in-render-object order guard to _RenderListView.replaceAllChildren for consistency with the other RenderObjectWithChildren implementations, and updates the perf investigation notes accordingly.
Changes:
- Remove element-level child-order comparison in
_syncChildRenderObjectsand callreplaceAllChildrendirectly (letting render objects no-op unchanged order without copying). - Add
hasSameRenderChildrenInOrderearly-return guard to_RenderListView.replaceAllChildren. - Update
perf-pass-findings.mdwith the §2 investigation results and the identified redundancy/fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/fleury/lib/src/widgets/framework.dart | Drops redundant element-side child-order pre-check that incurred an O(children) list copy and duplicated render-object guards. |
| packages/fleury/lib/src/widgets/list_view.dart | Adds missing same-order guard to _RenderListView.replaceAllChildren for parity with other multi-child render objects. |
| docs/implementation/perf-pass-findings.md | Documents the retained-tree-tax investigation outcome and the specific redundancy removed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes out the §2 retained-tree-tax investigation of the perf pass. Verdict: confirmatory — the retained-tree machinery is lean, and this lands the one genuine redundancy the investigation surfaced.
The change
MultiChildRenderObjectElement._syncChildRenderObjectspre-checked child order viaowner.children, but that getter isList.unmodifiable(_children)— a full copy of the child list — and the check duplicated the identicalhasSameRenderChildrenInOrderguard thatreplaceAllChildrenalready runs against its internal list with no copy. So every multi-child element paid a redundant O(children) copy + duplicate scan on every rebuild. Now it callsreplaceAllChildrendirectly (which no-ops unchanged order itself) and the dead_sameRenderObjectOrderhelper is gone (net −5 LOC).§2 investigation (recorded in
perf-pass-findings.md)Isolating the steady-state no-op-rebuild tax from one-time mount:
The reconcile tax is sublinear per node and tiny (85µs even at 1465 nodes ≈ 0.5% of a 60fps budget). The aggregate profiler's
replaceAllChildrendominance (27× the next symbol) is mount, not steady-state. The three big levers — layout-skip (clean subtrees), opt-in paint-memoization (RepaintBoundary), lean reconcile — are all present and correct. The earlier speculativeCellConstraints-pooling target was mis-aimed (idle/paint-only frames run 0 layouts). This change is measured ~10% off the reconcile tax at mid sizes — a hygiene win, not a workload mover._RenderListViewparity guard (not a regression fix)A
/code-reviewflagged that dropping the element-side pre-check would make the eagerListViewre-layout on same-order rebuilds —_RenderListView.replaceAllChildrenwas the loneRenderObjectWithChildrenmissing thehasSameRenderChildrenInOrderguard. On verification this is refuted as an observable regression:_ListViewBody.updateRenderObjectmarks needs-layout unconditionally every rebuild (to re-read mutable controller state), so the eager ListView relayouts regardless — measuredperformedCountidentical (7) with and without the guard. The guard is added anyway for parity with the other four implementations and to stay correct if that unconditional mark ever becomes conditional; the comment documents the masking. (The bogusperformedCount == 0regression test was dropped — it can never hold for the eager ListView.)Verification
dart analyze: clean.list_viewsuite.childListNoOpFrameUs≈223→182µs median (noisy, 20 samples).🤖 Generated with Claude Code