feat(widget): activate layout cache via LayoutChild (ADR-032 Phase 2b)#160
Conversation
Convert all 32 parent→child Layout calls across 23 files to widget.LayoutChild, which checks the per-widget layout cache before calling Layout and stores the result on miss. Infrastructure changes: - Remove assertConstraintsSatisfied (OverflowBox semantics allow children to exceed parent constraints) - Add layoutVerifying sentinel so test widgets can skip counting debug verifier re-runs (Flutter debugCheckingIntrinsics pattern) - Add InvalidateLayoutTree for downward cache propagation when signals fire via BindToSchedulerLayout Fix stale-cache bug exposed by debug verifier: MarkNeedsLayout only propagates upward, so scroll/virtualContent caches survived signal changes. InvalidateLayoutTree clears all descendant caches in the BindToSchedulerLayout callback. GAP-2 exceptions (Layout-in-Draw, kept as direct calls): - core/gridview/gridview.go:1174 (drawVisibleCells) - core/listview/virtual_content.go:99 (Draw) - offscreen/renderer.go (framework entry points) - uitest/widget.go (test entry points)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… 2b cleanup) LayoutChild now handles cache lifecycle (store on miss, skip on hit), so the post-layout-pass MarkLayoutCleanRecursive walk is unnecessary. - Window.layout() uses LayoutChild for root widget - Window.onInvalidate clears root layout cache so Invalidate() forces relayout - Delete widget/layout_clean.go (MarkLayoutCleanRecursive + markLayoutClean) - Remove 4 MarkLayoutCleanRecursive tests and clwContainer helper
kolkov
left a comment
There was a problem hiding this comment.
Thorough code review complete — validated against ADR-032, Flutter source (rendering/object.dart), and enterprise layout cache patterns.
Approve ✅ with comments below.
What's done well
- 32 mechanical conversions correct and complete (2 extra vs ADR's 30 — gallery + titlebar additions)
- GAP-2 exceptions match ADR-032 exactly (gridview, listview virtual_content, offscreen, uitest)
MarkLayoutCleanRecursiveshim correctly removed (ADR-032 predicted "redundant once LayoutChild adopted")- Stale-cache bug found via debug verifier — good catch (listview signal → scroll/virtualContent caches survived)
IsLayoutVerifyingsentinel matches FlutterdebugCheckingIntrinsicspattern (global boolean, debug-only)- Net -43 LOC
Comments (non-blocking)
1. InvalidateLayoutTree O(n) walk — acceptable but not the enterprise pattern
Flutter does NOT do descendant invalidation walks. Flutter's pattern: parent explicitly calls markNeedsLayout() on affected children, not blanket tree clear. Our InvalidateLayoutTree is a correctness-first solution — it works but is O(n) per signal fire.
For Phase 2b this is acceptable. Phase 5 (RelayoutBoundary) should replace this with targeted child invalidation per Flutter pattern.
2. assertConstraintsSatisfied removed — needs justification
ADR-032 explicitly requires: "Constraints satisfaction: assert constraints.IsSatisfiedBy(size)". PR removes this claiming "OverflowBox semantics" — but there is no OverflowBox widget in our codebase (grep returns 0 matches).
What widget actually fails this assert? If none → the assert should be kept (Flutter has debugAssertDoesMeetConstraints). If a specific widget legitimately exceeds constraints → document which one and why in the commit.
3. GAP-3 (animation in Layout) deferred — document explicitly
ADR-032 says: "Collapsible must restructure: MarkNeedsLayout from animation tick callback, not from inside Layout." This PR doesn't address GAP-3. Acceptable deferral but should be noted in ADR-032 as "Phase 2b deferred."
|
@TimLai666 — merged, thanks for another solid contribution! ADR-032 Phase 2b is a major milestone. A couple of open questions from the review — no action needed now, but worth discussing for future phases:
|
…ACHE-030) Engine (centralized layout cache) has 0 production usage — confirmed by codebase audit. Per-widget layout caching is now handled by widget.LayoutChild on WidgetBase (ADR-032 Phase 2b, #160). - Delete engine.go + engine_test.go (-769 LOC) - Extract Layoutable interface to layoutable.go (used by Flex/Grid/Stack) - Extract mockLayoutable to mock_test.go - Remove Engine cache benchmarks from bench_test.go - Update doc.go and CHANGELOG
…ACHE-030) (#163) Engine (centralized layout cache) has 0 production usage — confirmed by codebase audit. Per-widget layout caching is now handled by widget.LayoutChild on WidgetBase (ADR-032 Phase 2b, #160). - Delete engine.go + engine_test.go (-769 LOC) - Extract Layoutable interface to layoutable.go (used by Flex/Grid/Stack) - Extract mockLayoutable to mock_test.go - Remove Engine cache benchmarks from bench_test.go - Update doc.go and CHANGELOG
Summary
child.Layout(ctx, c)calls across 23 files towidget.LayoutChild(child, ctx, c), activating per-widget layout caching (ADR-032 Phase 2b)assertConstraintsSatisfied— gogpu/ui uses OverflowBox semantics where children can legitimately exceed parent constraintslayoutVerifyingsentinel (FlutterdebugCheckingIntrinsicspattern) so test widgets can skip counting debug verifier re-runs, enablingGOGPU_DEBUG_LAYOUT=1as a global CI flagInvalidateLayoutTreefor downward cache propagation in signal bindings, fixing a stale-cache bug whereMarkNeedsLayoutonly propagated upwardStale-cache bug fix
The debug verifier caught a real correctness issue: when
itemCountSignalfires on a listview,MarkNeedsLayoutclears the listview's cache and propagates upward, but the internal scroll view and virtualContent caches survive. On re-layout,LayoutChild(scroll, tightConstraints)returns the stale cached size.Fix:
BindToSchedulerLayoutnow callsInvalidateLayoutTree(w)afterMarkNeedsLayout(), clearing all descendant caches when a signal fires. This is the downward complement of the upward propagation.GAP-2 exceptions (intentionally kept as direct
child.Layout())core/gridview/gridview.go:1174— Layout-in-Draw (drawVisibleCells)core/listview/virtual_content.go:99— Layout-in-Drawoffscreen/renderer.go— framework-level entry pointsuitest/widget.go— test utility entry pointsTest plan
go test ./...— all packages passGOGPU_DEBUG_LAYOUT=1on all 16 converted packages — all pass (including listview)TestLayout_ItemCountChangedViaSignalpasses with debug verifier