Prune the component index with the scrollback buffer - #265
Merged
Conversation
componentLocations maps a component name to the serials of the lines
referencing it, and removeLines deliberately left it alone:
// Intentionally leak components here. They don't exist in the main
// window, and no other windows get long enough
On a stream that does carry components, that index grew for the life of
the connection - one entry per occurrence, never reclaimed. Prune it
against the oldest buffered line instead, which ties it to a bound that
already exists.
The entries were also a Set rebuilt per occurrence
(existingLocations + serialNumber), so registering a line copied the
whole set and the cost grew with it. A deque appends in O(1) and lets
eviction drop a prefix, since serials are recorded in order. The
benchmark's append time fell from ~176-192ms to ~153-156ms per ~24k
appends: pruning costs less than the quadratic rebuild it replaces.
The memory view's "component refs" column keeps its meaning, but now
reads as a check on the pruning rather than a record of the leak.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesComponent location pruning and buffer limits
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/ComposeTextStream.kt`:
- Around line 267-299: Update removeLines() and pruneComponentLocations() to
prune only when lines are actually evicted, avoiding a full scan of
componentLocations after every append. Track the evicted cached lines and remove
locations only for component names referenced by those lines, while preserving
cleanup of empty component-location entries.
In `@core/src/commonMain/kotlin/warlockfe/warlock3/core/window/MemoryUsage.kt`:
- Around line 25-27: Update the documentation for componentReferences in
MemoryUsage so it no longer treats counts above bufferedLines as evidence that
pruning failed. Explain that a buffered line may reference multiple distinct
server components, and describe anomalously high values relative to expected
component occurrences while preserving the existing buffer-pruning context.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b648c36-4d61-40a5-86ae-2d1d11248fe5
📒 Files selected for processing (3)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/ComposeTextStream.ktcompose/src/jvmTest/kotlin/ComposeTextStreamTest.ktcore/src/commonMain/kotlin/warlockfe/warlock3/core/window/MemoryUsage.kt
A scrollback of zero or less meant an unbounded buffer, and every line is held twice over - the rendered line plus the source it was rendered from - for every window of every connection. Cap eviction at a million lines, which is far past any use for scrollback, so a setting near it is a mistake rather than a preference. Lines past it are dropped silently: there is nothing useful to tell a user who will never reach it. Kept to the pruning path. The setting is untouched, stores whatever it stored before, and the settings screen says nothing new; the buffer simply stops honouring a number it should never have honoured. The memory view reports the cap in force rather than the raw setting, so its "max" column cannot claim a bound the buffer would not keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sproctor
added a commit
that referenced
this pull request
Aug 10, 2026
removeLines now caps the buffer, so the displayed rows come out of at most about a million lines and the span they occupy is bounded with them. The escape hatch for a span too wide to fold - and the reasoning about overflowing a doubled span - was defending a case the buffer no longer allows. The comparison keeps halving the modulus instead of doubling the span: same result, and it stays that way whatever the cap becomes. Note this now leans on the buffer cap in #265. Merged the other way round, an unbounded scrollback would grow the span until the doubling overflows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/ComposeTextStream.kt`:
- Around line 760-771: The ComposeTextStream buffer construction currently uses
raw maxLines, allowing negative capacities and excessive allocation. Update the
line-buffer deque initialization in ComposeTextStream to use
effectiveMaxLines(maxLines), or no-argument constructors where appropriate, for
both deques; add stream-construction tests covering negative and oversized
settings alongside the existing effectiveMaxLines tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a0f5960-3f2e-4d47-97b1-5d0cb75517b6
📒 Files selected for processing (2)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/ComposeTextStream.ktcompose/src/jvmTest/kotlin/ComposeTextStreamTest.kt
Review points on the pruning path. removeLines pruned on every append, including the ones that evicted nothing. Guard it, which is free once the buffer is full and saves the walk during warmup. The suggested per-evicted-line prune is not taken: it re-derives that line's components on eviction and rests on the front entry belonging to that line, which partial lines - registered per increment, cached as the accumulation - make harder to verify, in exchange for dropping a walk over the small fixed set of component ids the server actually sends. The benchmark could not separate the two; the spread between runs of identical code was wider than the gap between them. Also correct the memory view's doc: a line can reference several components, so the count legitimately sits above bufferedLines, and the comment said a count above it meant pruning had failed. Test added for that, which also covers a line carrying two components being pruned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ArrayDeque allocates its initial capacity immediately and rejects a negative one outright, and the setting reached it unfiltered: a negative scrollback threw IllegalArgumentException while the stream was being built, taking the window with it, and an oversized one reserved the whole array before a single line arrived. Capping eviction made negative values meaningful rather than a synonym for unbounded, so the constructor became the one place that still could not survive them. Size from initialBufferCapacity instead, which clamps to the effective cap and then to something modest: a deque grows amortized, so starting under an unusually long buffer costs a few copies, while starting over it costs the whole allocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sproctor
added a commit
that referenced
this pull request
Aug 10, 2026
* Bound the lazy list's item key space Compose caches one CachedItemContent per distinct item key in LazyLayoutItemContentFactory and never evicts an entry - the map is only ever written to; the DisposableEffect there clears the content lambda but leaves the entry. Keying stream rows by serial number, which only counts up, therefore retained one entry plus its boxed key per line ever displayed, for as long as the window was open. Measured by replaying protocol through the app and sampling GC.class_histogram: CachedItemContent went 3,876 -> 51,782 -> 97,206 -> 140,031 over four samples a minute apart, climbing linearly, while StreamTextLine stayed flat at ~13k. The buffers were doing their job; the cache underneath them was not. Keys only have to be distinct among the rows on screen at once, so fold them into a bounded space and let a key be reused once its line is gone. The rows occupy a contiguous serial range, so any modulus wider than that range separates them; this takes twice the range, rounded up to a power of two so it settles during warmup and then stops changing (a change re-keys every row, which costs a full recomposition). Same replay after: 2,261 -> 4,082 -> 4,086 -> 4,086. Bounded by the modulus instead of growing about 45k entries a minute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep the key fold correct at any span Review catch: the modulus cap could return a value that collides. Above MAX_LAZY_ITEM_KEY_MODULUS the loop stopped at the cap even though the span exceeded it, and `serialSpan * 2` overflowed negative past Long.MAX_VALUE / 2, ending the loop immediately and handing back the 4096 floor. Either way the fold returned a modulus that maps two on-screen rows to one key, which is the one thing it must not do. Neither is reachable - both need more lines than fit in memory - but a function whose job is distinct keys should not have a size at which it quietly stops. Compare by halving the modulus instead of doubling the span, so nothing overflows, and past the point where doubling would overflow return Long.MAX_VALUE, where the modulo is the identity: no folding, but no collision either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop the guard for a serial span that cannot happen removeLines now caps the buffer, so the displayed rows come out of at most about a million lines and the span they occupy is bounded with them. The escape hatch for a span too wide to fold - and the reasoning about overflowing a doubled span - was defending a case the buffer no longer allows. The comparison keeps halving the modulus instead of doubling the span: same result, and it stays that way whatever the cap becomes. Note this now leans on the buffer cap in #265. Merged the other way round, an unbounded scrollback would grow the span until the doubling overflows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- 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.
The second leak from the memory-growth investigation. Independent of #264; they touch different files.
The leak
componentLocationsmaps a component name to the serial numbers of the lines referencing it, andremoveLinesdeliberately left it alone:On any stream that does carry components, that index grew for the life of the connection — one entry per occurrence, never reclaimed. The consumer already skipped entries below the buffer (
if (lineNumber >= 0)), so they were pure retention.It is pruned against the oldest buffered line now, which ties it to a bound that already exists rather than inventing a new one.
Also quadratic
Entries were held in a
Set<Long>rebuilt on every occurrence:So registering a line copied the entire set, and the copy grew as the leak did. A deque appends in O(1), and because serials are recorded in order, eviction is a prefix drop.
Measured with
:compose:streamNetworkBenchmark(2 connections, 6 windows, 4000 lines/sec), append time per ~24k appends:Pruning costs less than the rebuild it replaces. Not a controlled A/B — same machine and knobs, but the baseline came from the earlier investigation run — though the direction matches the mechanism, since the old cost rose with the size of the set.
Testing
The test that pinned the old behaviour is inverted rather than deleted:
memoryUsagePrunesComponentIndexWithTheBufferasserts the index tracksbufferedLines(4) instead of the 40 lines appended.Added
componentUpdatesReachRemainingLinesAfterEviction, which is the risk in this change — that pruning disturbs the component-to-line mapping. It appends 10 component lines into a 3-line buffer, updates the component, and asserts all three surviving rows render the new value.updateComponentRefreshesAllOccurrencesstill covers the unevicted path. Full suites, ktlint, and the benchmark above.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests