Make the window a limit rather than a destination, and stop it stepping
Three faults, all in how the outer bound behaved, and all of them mine. **It was a destination.** Rows were stood up towards it a couple at a time until the window was the full sixteen screens, whether or not anybody was going to read them. Two rows is not a bounded amount of work -- a row here runs to twenty-five screens of markdown -- so the transcript was laying text out continuously in the background, and that is what the 70ms measurements were. The bound is now a limit: the window grows because the reader moved, and never speculatively. `standUpSome`, `growing` and the frame loop that drove them are gone. **It moved in two-screen steps**, which is the boundary that could be felt. Crossing one moved the limit two screens at once and cut a chunk off the trailing edge in a single frame; coming back the other way needed all of it again, so oscillating around a boundary rebuilt the same rows repeatedly -- measured in the last report as twenty-two rows dropped and immediately wanted back, which is also where the flicker was coming from. Recomputed every time, the trailing edge retreats a row at a time and there is no boundary left. The step existed because this used to be a scan of every row; it has been a binary search since the last commit. **It was measured against the viewport as it is now**, so the IME animation shrank it on every one of its thirty frames -- trimming the far edge, disposing rows and recomposing the list, thirty times, for a keyboard. How much to keep alive is not a question the keyboard has any business changing: it is about how far the reader might scroll, and they can scroll just as far with it open. It is sized against the tallest the viewport has been. The bound also came down from eight screens to five. Eight was chosen when keeping a row alive was believed to be free; the report now says plainly that it is not -- three quarters of the draw phase is framework bookkeeping that grows with live nodes rather than with what is on screen. Measured on the emulator, eighty swipes oscillating across what used to be a boundary: measurements of the transcript went from 51 at 8.3ms mean and 70.3ms worst to 11 at 2.3ms mean and 4.3ms worst, and rows composed from 270 to 10. Three keyboard cycles leave the transcript at 0.36ms of the frame's draw phase. Three open/scroll/close cycles report nothing unbuilt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
0a0949ee7f
commit
b3cebd0f4c
1 file changed
+42
-105
@@ -21,7 +21,6 @@ import androidx.compose.runtime.remember
|
||||
import androidx.compose.runtime.setValue
|
||||
import androidx.compose.runtime.snapshotFlow
|
||||
import androidx.compose.runtime.snapshots.Snapshot
|
||||
import androidx.compose.runtime.withFrameNanos
|
||||
import androidx.compose.ui.Alignment
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.draw.drawWithContent
|
||||
@@ -119,7 +118,6 @@ class TranscriptScroll(internal val scroll: ScrollState) {
|
||||
if (first != null && last != null && first <= last) first..last else IntRange.EMPTY
|
||||
// Whatever it was walking towards was named in the old indices too.
|
||||
target = retained
|
||||
growing = false
|
||||
// Something has to be built before the first frame, or the first frame is blank. The window
|
||||
// is worked out from the scroll position, and there is no scroll position until this has
|
||||
// been laid out once -- so on the composition that introduces the rows, every one of them
|
||||
@@ -176,16 +174,29 @@ class TranscriptScroll(internal val scroll: ScrollState) {
|
||||
/** The window's height, for the frames before the scroll container has measured one. */
|
||||
private var screen = 0
|
||||
|
||||
/** The tallest the viewport has been, which is what the window is sized against. */
|
||||
private var widest = 0
|
||||
|
||||
/**
|
||||
* How tall to treat the visible area as.
|
||||
* How tall to treat the visible area as, for deciding how much to keep built.
|
||||
*
|
||||
* The scroll container's own measurement once there is one, and the window's height before
|
||||
* that. There is a first composition in which rows exist and no layout has happened, and
|
||||
* answering it with zero there builds a window of nothing -- so the transcript draws blank for
|
||||
* a frame, which is the flicker when a session opens.
|
||||
* The tallest it has been rather than what it is now, and that is what makes opening the
|
||||
* keyboard cheap. The IME animates over some thirty frames and shrinks the viewport on every
|
||||
* one of them, so a window measured in screens gets smaller on every frame too -- which trims
|
||||
* the far edge, disposes rows, and recomposes the list, thirty times, for a keyboard. How much
|
||||
* to keep alive is not a question the keyboard has any business changing: it is about how far
|
||||
* the reader might scroll, and they can scroll just as far with it open.
|
||||
*
|
||||
* The window's own height stands in before the scroll container has measured one. There is a
|
||||
* first composition in which rows exist and no layout has happened, and answering it with zero
|
||||
* there builds a window of nothing -- a blank frame when a session opens.
|
||||
*/
|
||||
private val visible: Int
|
||||
get() = scroll.viewportSize.takeIf { it > 0 } ?: screen
|
||||
get() {
|
||||
val now = scroll.viewportSize
|
||||
if (now > widest) widest = now
|
||||
return if (widest > 0) widest else screen
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds a screenful of rows around [index], for the frames before there is a window.
|
||||
@@ -512,97 +523,45 @@ class TranscriptScroll(internal val scroll: ScrollState) {
|
||||
/** Where the window is heading. [retained] walks towards it rather than jumping; see below. */
|
||||
private var target: IntRange = IntRange.EMPTY
|
||||
|
||||
/** Whether [retained] is still short of [target], so somebody should keep stepping it. */
|
||||
var growing: Boolean by mutableStateOf(false)
|
||||
private set
|
||||
|
||||
/**
|
||||
* Widens the built range towards its target, a few rows at a time. False when it has arrived.
|
||||
*
|
||||
* Standing rows up is not free and its cost is not recomposition -- it is laying the text out,
|
||||
* which means shaping every glyph, and that is on the thread drawing the frame. Moving the
|
||||
* window in one go meant two screens of markdown shaped inside a single frame at each step, and
|
||||
* seventeen screens of it in the frame a session opens in. The platform files that under the
|
||||
* frame's draw phase, which is why it never showed up in the counters here: nothing is being
|
||||
* *recorded*, it is being measured.
|
||||
*
|
||||
* Spreading it over frames does not make it cheaper and is not meant to. It stops it arriving
|
||||
* all at once, which is the difference between a frame that is late and a frame that is missed
|
||||
* by ten.
|
||||
*/
|
||||
internal fun standUpSome(): Boolean {
|
||||
if (target.isEmpty() || retained == target) {
|
||||
growing = false
|
||||
return false
|
||||
}
|
||||
var first = retained.first
|
||||
var last = retained.last
|
||||
var budget = STAND_UP_PER_FRAME
|
||||
while (budget > 0 && (first > target.first || last < target.last)) {
|
||||
if (first > target.first) {
|
||||
first--
|
||||
budget--
|
||||
}
|
||||
if (budget > 0 && last < target.last) {
|
||||
last++
|
||||
budget--
|
||||
}
|
||||
}
|
||||
retained = first..last
|
||||
growing = retained != target
|
||||
return growing
|
||||
}
|
||||
|
||||
internal fun trackRetained(from: Int? = null) =
|
||||
// Without subscribing whoever called it to the scroll position. This runs from the
|
||||
// composition that lays the rows out as well as from the flow that watches scrolling, and
|
||||
// a composition that reads `scroll.value` recomposes on every frame of every fling -- the
|
||||
// O(rows)-per-frame mistake this whole file exists to undo, arriving by the back door.
|
||||
// composition that lays the rows out as well as from the layout that places them, and a
|
||||
// composition that reads `scroll.value` recomposes on every frame of every fling.
|
||||
Snapshot.withoutReadObservation {
|
||||
refreshTops()
|
||||
// [from] when the caller knows better than the scroll container does; see `laidOut`.
|
||||
val viewportTop = from ?: (scroll.maxValue - scroll.value)
|
||||
// The outer bound moves lazily, because every row reads the window and moving it
|
||||
// recomposes all of them -- affordable every couple of screens, and not at every row
|
||||
// boundary a fling crosses.
|
||||
val step = (visible * RETAIN_STEP_SCREENS).coerceAtLeast(1)
|
||||
val moved = viewportTop - rangeAt
|
||||
if (target.isEmpty() || topsVersion != rangeVersion || moved > step || moved < -step) {
|
||||
rangeAt = viewportTop
|
||||
rangeVersion = topsVersion
|
||||
target = retainedRange(viewportTop, RETAIN_SCREENS)
|
||||
}
|
||||
// What is near the screen, every frame rather than only when the bound moves. This is
|
||||
// the half that cannot be lazy and the half that was: a fling crosses a screen in a
|
||||
// frame or two, so a window last widened two screens ago has already been outrun and
|
||||
// the row arriving at the edge is drawn as the spacer it still is. Standing rows up a
|
||||
// few at a time made it worse rather than causing it -- after a seed or a restore the
|
||||
// built window is a couple of screens wide and grows two rows a frame, which a fling
|
||||
// beats easily. Cheap enough to do always: one scan of the row list, and the write
|
||||
// below is skipped when the answer has not changed, so nothing recomposes.
|
||||
// Widened by a fixed number of rows as well as by screens, because a screen is a
|
||||
// number of pixels and the rows it covers are only *estimated* until they have been
|
||||
// measured -- and nothing has been measured on the frame a session opens, which is
|
||||
// where this matters. Rows shorter than the running average make a two-screen window
|
||||
// cover fewer rows than the screen actually shows, and the reader sees the difference
|
||||
// as blank.
|
||||
// Both edges every time, and the bound is a *limit* rather than a destination.
|
||||
//
|
||||
// It used to be a destination: rows were stood up towards it a couple at a time until
|
||||
// the window was the full sixteen screens, whether or not anybody was going to read
|
||||
// them. Two rows is not a bounded amount of work -- a row here runs to twenty-five
|
||||
// screens of markdown -- so the transcript was quietly laying text out forever, and
|
||||
// that is what the 70ms measurements were.
|
||||
//
|
||||
// And it used to move in two-screen steps, which is what made the boundaries the
|
||||
// reader could feel. Crossing one moved the limit by two screens at once, which cut a
|
||||
// chunk off the trailing edge in a single frame; coming back the other way needed all
|
||||
// of it again, so an oscillation around a boundary rebuilt the same rows over and
|
||||
// over. It was measured at twenty-two rows dropped and immediately wanted back.
|
||||
// Recomputed every time, the trailing edge retreats a row at a time as the reader
|
||||
// moves, and there is no boundary left to cross. The step existed because this was a
|
||||
// scan of every row; it is a binary search now.
|
||||
target = retainedRange(viewportTop, RETAIN_SCREENS)
|
||||
// What is near the screen has to be built in the frame it is on screen, so this half
|
||||
// is never deferred and never clamped away.
|
||||
val screens = retainedRange(viewportTop, RETAIN_NOW_SCREENS)
|
||||
val near =
|
||||
(screens.first - RETAIN_NOW_ROWS).coerceAtLeast(0)..(screens.last + RETAIN_NOW_ROWS)
|
||||
.coerceAtMost(order.lastIndex)
|
||||
val first = if (retained.isEmpty()) near.first else minOf(retained.first, near.first)
|
||||
val last = if (retained.isEmpty()) near.last else maxOf(retained.last, near.last)
|
||||
// Clamped to the bound, but never past what is on screen: the bound is allowed to be a
|
||||
// couple of screens out of date and this is not.
|
||||
val next =
|
||||
minOf(first.coerceAtLeast(target.first), near.first)..maxOf(
|
||||
last.coerceAtMost(target.last),
|
||||
near.last,
|
||||
)
|
||||
if (next != retained) {
|
||||
retained = next
|
||||
growing = retained != target
|
||||
}
|
||||
if (next != retained) retained = next
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -681,8 +640,6 @@ class TranscriptScroll(internal val scroll: ScrollState) {
|
||||
false
|
||||
}
|
||||
|
||||
private var rangeAt = Int.MIN_VALUE
|
||||
private var rangeVersion = -1
|
||||
private var topsVersion = 0
|
||||
|
||||
/** The height of the visible area, 0 until the first measurement. */
|
||||
@@ -792,18 +749,6 @@ fun TranscriptColumn(
|
||||
snapshotFlow { state.scroll.value to state.scroll.maxValue }
|
||||
.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)
|
||||
@@ -961,10 +906,7 @@ private fun RetainedRow(
|
||||
}
|
||||
|
||||
/** How far either side of the screen a row stays built; see [TranscriptScroll.retains]. */
|
||||
private const val RETAIN_SCREENS = 8
|
||||
|
||||
/** How far the view moves before the outer bound is worked out again; see `trackRetained`. */
|
||||
private const val RETAIN_STEP_SCREENS = 2
|
||||
private const val RETAIN_SCREENS = 5
|
||||
|
||||
/**
|
||||
* How much either side of the screen is built at once rather than a few rows at a time.
|
||||
@@ -980,11 +922,6 @@ private const val RETAIN_NOW_SCREENS = 3
|
||||
/** The same margin counted in rows, for when no height is known yet; see `trackRetained`. */
|
||||
private const val RETAIN_NOW_ROWS = 12
|
||||
|
||||
/**
|
||||
* How many rows may be laid out in one frame while the window is catching up; see `standUpSome`.
|
||||
*/
|
||||
private const val STAND_UP_PER_FRAME = 2
|
||||
|
||||
/**
|
||||
* How much is built before there is a scroll position to work a window out from; see `seedAround`.
|
||||
*/
|
||||
|
||||
Reference in new issue
Block a user