MOB-128: Android :scroll composes only what is on screen - #46
Merged
Conversation
Measured on a Moto G Power, one render at a time so no frame queues behind another: a 200-row screen cost 134 ms of main-thread work per update, 107 ms of it Compose recomposition, against 45 ms for the entire BEAM-plus-NIF pipeline. The native rebuild is the dominant cost, and :scroll composed every child regardless of the viewport — 200 rows composed to show about ten. Vertical :scroll now renders through MobLazyList when its content can be lazified. Two things had to be solved and they are the substance of this change. Mob screens are written scroll > column > rows, so a scroll node usually has exactly ONE child; lazifying its direct children buys nothing because the column underneath still composes every row. When the sole child is a column whose own props are layout-neutral, its children become the items. Only fill_width and fill_height count as neutral — a LazyColumn already spans its container's width and takes its height from the scroll node. Padding, background, align, or an id the harness addresses would be silently dropped by flattening, so those keep the eager path. A lazy container must be measured with a bounded main axis. Column only measures a child against the remaining space when that child is weighted, so fill_height alone left it unbounded. verticalScroll tolerates that; LazyColumn does not — it composes zero items and renders an empty screen. A fill_height scroll child now gets Modifier.weight(1f) from its parent column. MobLazyList is reused rather than a fresh LazyColumn so :scroll inherits the list-state hoisting already there: rememberLazyListState resets to 0 on every BEAM re-render. Main-thread frame cost, p50 / max: 50 rows eager 77.8 / 106.7 ms lazy 83.2 / 120.4 ms 200 rows eager 140.9 / 162.1 ms lazy 81.5 / 107.5 ms 500 rows eager 104.0 / 307.9 ms lazy 91.6 / 110.9 ms The shape matters more than the percentage: lazy cost is flat in list length while eager grows. At 50 rows lazy is slightly worse, which is expected — the list nearly fits on screen so there is little to skip and LazyColumn has setup cost. At 500 rows the eager p50 is unreliable (under backpressure the app draws less often, so fewer heavy frames are sampled); its max of 308 ms against 111 ms is the honest comparison. This does not make Compose skip anything — skipping is separately broken, and a composable with a literally constant argument still recomposes every frame here. Laziness works by composing fewer nodes, not by reusing unchanged ones. Verified by generating an app from the patched template and building it, and by screenshotting every measurement. That last part is not ceremony: an earlier version of this change reported 1617 -> 24 composables and 312 -> 78 ms while rendering a blank list. Both numbers looked like a triumph. Only pixels caught it. No version bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gnosis An adversarial review found one critical bug, one silent API loss, and — most usefully — disproved the justification for half this change from the Compose bytecode. Reworked accordingly. ## Laziness is now opt-in via `lazy: true` The review established that making it the default silently degrades the test harness for apps that never asked: rows below the fold are never composed, so they never register a frame and Mob.Test.element_frames / tap_id cannot address them, and scroll position becomes index-based rather than pixel-based. `lazy_list` already exists and makes exactly that trade explicitly. So the author asks for it. This also answers the review's observation that the guard is narrow enough that the optimisation would rarely fire on real screens — the generator's own starter templates use `scroll > column padding: background:`, which the guard correctly refuses. Opt-in makes that a documented instruction (move those props onto the scroll) rather than silent non-application. ## The critical bug: a detached ScrollState left registered `scrollInfo` checks `scrollState` before `lazyState`, and the `rememberScrollState()` above the branch is never attached to a `verticalScroll` on the lazy path — so its maxValue keeps its Int.MAX_VALUE default. A lazified `:scroll` with an id therefore reported kind "pixel" with max_y ~2.1e9, which made `scroll_to(:bottom)` return :ok without moving the list, and `screenshot_tour` compute roughly a million pages. That is a silent false pass and an unbounded hang, on the exact node shape this change targets. The lazy path now clears it. ## The silent API loss: weight A flattened column's children lose `weight` — it comes from ColumnScope and LazyColumn items have none. `weight` is documented public API. Any child carrying it now forces the eager path. ## The wrong diagnosis, removed The first version gave a `fill_height` scroll child `Modifier.weight(1f)`, justified as "Column only measures a child against the remaining space when that child is weighted". The review disproved this from the shipped foundation-layout bytecode: a non-weighted child of a bounded Column is measured with mainAxisMax = remaining. Bounded. weight only raises min to equal max. The weight was also actively harmful — inside an unbounded Column (a nested scroll) the weighted branch computes targetSpace = mainAxisMin = 0, so the child measured at zero height and vanished; and it changed weight distribution for apps that never touched the lazy path. Removed entirely. The blank list that motivated it is explained by MOB-136: on Android a text_field in a row under an unbounded-height container blows the row height to ~500 px so its siblings are centred off screen. The eager path rendered nothing for the same reason, which only a screenshot of the eager baseline established. Verified on device that removing the weight does not reintroduce the blank list. ## Evidence Single-variable A/B, same build and fixture, toggling only `lazy: true`, 500 rows on a Moto G Power: main-thread frame cost p50 498.9 ms -> 115.8 ms, worst frame 1385.9 ms -> 164.8 ms. That also proves the flag switches paths. Five template-assertion tests pin the opt-in check, the neutral-prop set, the weight guard, the ScrollState clearing, and the absence of the removed weight hunk. No version bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Android half of MOB-128, from the MOB-124 rendering-performance epic. No version bump.
Companion PR in
mob: GenericJam/mob#118 (iOS half plus MOB-125/133/135).What ships
A vertical
:scrollrenders throughMobLazyList(LazyColumn) when the author opts in withlazy: trueand its content can be lazified. Otherwise it keeps the eagerColumn+verticalScroll.Opt-in on purpose. Laziness is not free of observable consequences: rows below the fold are never composed, so they never register a frame —
Mob.Test.element_framesandtap_idcannot address them — and scroll position becomes index-based rather than pixel-based.lazy_listalready makes exactly that trade explicitly; making it the silent default for every:scrollwould change harness behaviour under apps that never asked.Flattening, narrowly guarded. Mob screens are written
scroll > column > rows, so a scroll usually has one child and lazifying its direct children buys nothing. When that column's own props are all in{fill_width, fill_height}— genuinely droppable, since aLazyColumnalready spans its width andfill_heightis a no-op under an unbounded main axis — its children become the items. Anything else (padding, background, align, anid, a tap handler) keeps the eager path. A child carryingweightalso forces the eager path:weightcomes fromColumnScopeandLazyColumnitems have none, so it would vanish without a trace.Result
Single-variable A/B — same build, same fixture, toggling only
lazy: true. 500 rows, Moto G Power, main-thread frame cost:lazy: true4.3x on p50, 8.4x on the worst frame. Lazy cost is flat in list length where eager grows; short lists are marginally slower, which is another reason it is opt-in.
Changed by adversarial review
This PR was reworked after review. Three things it caught:
A critical harness bug.
scrollInfochecksscrollStatebeforelazyState, and therememberScrollState()above the branch is never attached to averticalScrollon the lazy path — so itsmaxValuekeeps itsInt.MAX_VALUEdefault. A lazified:scrollwith anidreportedkind: "pixel"withmax_y ≈ 2.1e9, which madescroll_to(:bottom)return:okwithout moving the list andscreenshot_tourcompute roughly a million pages. Fixed: the lazy path clears it.A silent API loss. Flattened children lost
weight. Now guarded.A fix built on a wrong diagnosis, removed. An earlier revision gave a
fill_heightscroll childModifier.weight(1f), justified as "Columnonly measures a child against the remaining space when that child is weighted". The review disproved that from the shippedfoundation-layoutbytecode — a non-weighted child of a boundedColumnis measured withmainAxisMax = remaining. The weight was also harmful: inside an unboundedColumnthe weighted branch computestargetSpace = mainAxisMin= 0, so the child measured at zero height, and it changed weight distribution for apps that never touched the lazy path. Removed entirely, and verified on device that removing it does not reintroduce the blank list that motivated it — that was MOB-136 (atext_fieldunder an unbounded-height container blows the row height to ~500 px, which broke the eager path identically).Tests
Five template-assertion tests pin the opt-in check, the neutral-prop set, the weight guard, the ScrollState clearing, and the absence of the removed weight hunk.
mix test→ 351/353. The two failures (--python project compiles cleanly,liveview_generate/3 ... --local) fail identically onmaster— confirmed by the reviewer in a clean worktree at34831c4. CI is green.Verification
Generated an app from the patched template and built + deployed it to an emulator; screenshotted every measurement to confirm rows actually render. That is not ceremony — an earlier revision reported 1617 → 24 composables and 312 → 78 ms while rendering a blank list. Only pixels caught it.