From f497d1f3ade4a2de4b83a07ce3411f13858654e7 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Mon, 31 Aug 2026 03:57:11 -0400 Subject: [PATCH] Actually run the amortization, and stop paying for history twice Two things committed in 9052e5f and 94b1509 were never wired up. The row count was tracked and nothing observed it, so the fix for the gap when sending was inert; `standUpSome` and `growing` were written and nothing called them, so the window still jumped in one step. Both are connected now, which is what the last two commit messages already claimed. The third thing is what the counters were pointing at all along. The frame's draw phase sat at a flat 7.7ms -- p99 only 1.8x p50, so not shaping, which is spiky -- while the transcript's own recording ran 43 times in 3638 frames and accounted for a fortieth of it. A stood-down row drew nothing but was still a layout node, and the framework's per-frame bookkeeping after a scroll walks live nodes rather than visible ones. So the cost grew with the conversation instead of with the screen, which is exactly the step the reader felt at each page loaded. The rows outside the window are now two spacers rather than one per row, which they can be because the window is a range: what is not in it is one run before and one run after. Live nodes are bounded by the window now. The gaps between collapsed rows are added into the spacer, because `spacedBy` puts a gap between children and a run drawn as one child is short by all of them -- which would not read as a spacing bug, it would move everything under the reader. Measure and place are timed separately now. The draw phase carries Compose's measurement, so "draw is high while nothing is recorded" could not say which half, and the two have different fixes. Diagnosis from the ai-app-2-6d session: foundation already places scrolled content with `placeRelativeWithLayer`, so the scroll is a layer transform today and re-placing children was never the cost -- the node-count walk is, and collapsing the runs is the experiment that settles it. Co-Authored-By: Claude Opus 5 --- .../com/example/aiapp/TranscriptScroll.kt | 111 ++++++++++++++---- 1 file changed, 88 insertions(+), 23 deletions(-) diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/TranscriptScroll.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/TranscriptScroll.kt index 0fbc552..b62cf9e 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/TranscriptScroll.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/TranscriptScroll.kt @@ -22,6 +22,7 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.runtime.snapshotFlow +import androidx.compose.runtime.withFrameNanos import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.drawWithContent @@ -281,7 +282,35 @@ class TranscriptScroll(internal val scroll: ScrollState) { var retained: IntRange by mutableStateOf(IntRange.EMPTY) private set - fun retains(seq: Long): Boolean = rowIndex[seq]?.let { it in retained } ?: true + /** + * How tall rows [run] stand in for as a single spacer, the gaps between them included. + * + * The gaps have to be counted here because collapsing a run removes children from the column, + * and `Arrangement.spacedBy` puts a gap between children rather than after each one -- so a run + * of `k` rows drawn as one spacer is `k - 1` gaps shorter than the rows were unless it says so. + * Getting that wrong does not look like a spacing bug; it shortens the content, which moves + * everything the reader is looking at. + */ + fun runHeight(run: IntRange): Int { + if (run.isEmpty() || order.isEmpty()) return 0 + var total = 0 + for (index in run) total += assumed(order[index]) + return total + (run.last - run.first) * spacing + } + + /** + * [retained] clamped to a list of [count] rows, or empty if nothing should be built yet. + * + * The range is worked out against the order from the last layout, and a page of history landing + * changes that order in the same composition that reads this -- so an index from before it can + * be past the end. Clamping here rather than at the read sites keeps one answer to it. + */ + fun window(count: Int): IntRange { + if (count == 0 || retained.isEmpty()) return IntRange.EMPTY + val first = retained.first.coerceIn(0, count - 1) + val last = retained.last.coerceIn(first, count - 1) + return first..last + } /** * Recomputes [retained] if the view has moved far enough to be worth it. @@ -407,9 +436,6 @@ class TranscriptScroll(internal val scroll: ScrollState) { private var rangeVersion = -1 private var topsVersion = 0 - /** The height a row was last measured at, for the spacer that stands in for it. */ - fun heightOf(seq: Long): Int = assumed(seq) - /** The height of the visible area, 0 until the first measurement. */ val viewport: Int get() = scroll.viewportSize @@ -497,10 +523,25 @@ fun TranscriptColumn( layoutDirection } // One evaluation of the window a frame, for the whole list; see [TranscriptScroll.retained]. + // The row count is in here because a row arriving is the one case that does not move the view: + // without it a sent message falls outside the window and stands in as a spacer, which is a + // screen of blank between the last message and the box it was typed in. LaunchedEffect(state) { - snapshotFlow { state.scroll.value to state.scroll.maxValue } + snapshotFlow { Triple(state.scroll.value, state.scroll.maxValue, state.rowCount) } .collect { state.trackRetained() } } + // Walks the window towards its target a few rows a frame; see [TranscriptScroll.standUpSome]. + // Driven from a frame callback rather than a plain loop so the rows stand up between frames + // instead of all inside one, which is the entire point of doing it gradually. + LaunchedEffect(state) { + snapshotFlow { state.growing } + .collect { + while (state.growing) { + withFrameNanos {} + state.standUpSome() + } + } + } Column( modifier .verticalScroll(state.scroll, reverseScrolling = true) @@ -514,10 +555,22 @@ fun TranscriptColumn( // the whole transcript was being subcomposed again for each of those frames, which is // what made bringing the keyboard up cost more than anything else on the screen. Read // here it is a relayout, and the rows keep the measurements they already have. + // + // Timed in two halves because the frame's draw phase is where Compose's measurement + // lands, and "draw is high while nothing is being recorded" does not say which half. + // Measuring the transcript and placing it are different costs with different fixes: + // one is shaping text that has changed, the other is O(rows) whether or not anything + // has. .layout { measurable, constraints -> + val started = System.nanoTime() val placeable = measurable.measure(constraints.copy(minHeight = state.scroll.viewportSize)) - layout(placeable.width, placeable.height) { placeable.place(0, 0) } + DebugStats.record("measure: the whole transcript", System.nanoTime() - started) + layout(placeable.width, placeable.height) { + val placing = System.nanoTime() + placeable.place(0, 0) + DebugStats.record("place: the whole transcript", System.nanoTime() - placing) + } } .fillMaxWidth() // Once for the whole list, not once per row: this is where a saved position is put @@ -545,11 +598,25 @@ fun TranscriptColumn( }, verticalArrangement = Arrangement.spacedBy(spacing, Alignment.Bottom), ) { - rows.forEach { item -> + // Everything outside the window is two spacers rather than one per row, and that is what + // stops the cost of a frame growing with the conversation. A stood-down row was still a + // layout node, and the framework's own per-frame bookkeeping after a scroll -- the position + // dispatch, the cached screen rect each node keeps -- walks live nodes rather than visible + // ones. So the transcript got slower with every page loaded even though the extra rows drew + // nothing at all: on a Pixel 9 Pro XL, a flat 7.7ms in the frame's draw phase while our own + // recording accounted for a fortieth of it. Two spacers, because [retained] is a range: the + // rows that are not in it are always one run before it and one run after. + val window = state.window(rows.size) + val ahead = if (window.isEmpty()) rows.indices else 0.. Unit, ) { - // Derived, so a row hears about the scroll only when its own answer changes rather than on - // every frame; see [TranscriptScroll.retains]. - if (!state.retains(item.startSeq)) { - DebugStats.count("row stood down") - Spacer( - Modifier.fillMaxWidth() - .height(with(LocalDensity.current) { state.heightOf(item.startSeq).toDp() }) - ) - return - } Column( Modifier.fillMaxWidth() // A layer of its own, which is the piece of a lazy list this had not rebuilt.