Bound the lazy list's item key space - #264
Merged
Merged
Conversation
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>
|
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)
📝 WalkthroughWalkthroughLazy-list item keys now use a bounded modulus based on the displayed serial range. Helpers select power-of-two moduli. JVM tests cover modulus bounds, uniqueness, stability, and key reuse. ChangesLazy-list key management
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 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/WindowViewScaffold.kt`:
- Around line 655-658: Update the modulus calculation around the visible while
loop to prevent duplicate LazyColumn keys when serialSpan exceeds
MAX_LAZY_ITEM_KEY_MODULUS: enforce a displayed-span limit before calculating the
modulus or otherwise ensure the active key range remains below the cap, and
avoid serialSpan * 2 overflow by using overflow-safe bounds. Preserve unique
keys for spans of MAX_LAZY_ITEM_KEY_MODULUS + 1 and overflow-sized spans, and
add regression tests covering both cases.
🪄 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: 93e36265-ec1a-48df-a5bb-6e318e05d25c
📒 Files selected for processing (2)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowViewScaffold.ktcompose/src/jvmTest/kotlin/LazyItemKeyTest.kt
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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compose/src/jvmTest/kotlin/LazyItemKeyTest.kt (1)
35-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the required key reuse distance.
Lines 35-39 only require
modulus > span. A regression that returns a modulus wherespan < modulus <= 2 * spanwill pass this test, but it will recycle keys earlier thanlazyItemKeyModuluspermits.Assert exact boundary values, such as
2_047L -> 4_096Land2_048L -> 8_192L. Also assertmodulus / 2 > spanwhen the modulus is notLong.MAX_VALUE.Proposed test change
- for (span in listOf(0L, 1L, 100L, 2_000L, 4_095L, 4_096L, 10_000L, 250_000L)) { + for ((span, expectedModulus) in listOf( + 0L to 4_096L, + 2_000L to 4_096L, + 2_047L to 4_096L, + 2_048L to 8_192L, + 4_095L to 8_192L, + 4_096L to 16_384L, + )) { val modulus = lazyItemKeyModulus(span) - assertTrue(modulus > span, "modulus $modulus must exceed span $span to keep keys distinct") + assertEquals(expectedModulus, modulus) + assertTrue(modulus / 2 > span) }🤖 Prompt for 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. In `@compose/src/jvmTest/kotlin/LazyItemKeyTest.kt` around lines 35 - 39, Strengthen modulus assertions in modulusExceedsTheSpanItHasToSeparate by verifying exact boundary mappings such as 2_047L to 4_096L and 2_048L to 8_192L, and assert modulus / 2 > span whenever the result is not Long.MAX_VALUE. Retain coverage of the existing span values while checking the required key reuse distance rather than only modulus > span.
🤖 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.
Outside diff comments:
In `@compose/src/jvmTest/kotlin/LazyItemKeyTest.kt`:
- Around line 35-39: Strengthen modulus assertions in
modulusExceedsTheSpanItHasToSeparate by verifying exact boundary mappings such
as 2_047L to 4_096L and 2_048L to 8_192L, and assert modulus / 2 > span whenever
the result is not Long.MAX_VALUE. Retain coverage of the existing span values
while checking the required key reuse distance rather than only modulus > span.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80ebef49-7c4f-472e-bf35-49ef28447481
📒 Files selected for processing (2)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowViewScaffold.ktcompose/src/jvmTest/kotlin/LazyItemKeyTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowViewScaffold.kt
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>
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.
Fixes the unbounded growth found while investigating the memory-growth report.
The leak
Compose caches one
CachedItemContentper distinct item key inLazyLayoutItemContentFactory, and never evicts an entry — the map is only ever written to, and theDisposableEffectthere clears the content lambda but leaves the entry behind. That is fine for a list with finite keys. Stream rows were keyed by line serial number, which only counts up, so every line ever displayed left an entry (plus its boxedLongkey) for as long as the window stayed open.Measured by replaying protocol through the real app and sampling
jcmd GC.class_histogram, four samples about a minute apart:CachedItemContentjava.lang.LongStreamTextLineLinear, no plateau, while the line buffers themselves stayed flat — the scrollback caps were working; the cache underneath them was not.
Not a regression from any recent change: v3.0.166 keyed rows the same way, and
LazyLayoutItemContentFactory.ktis byte-identical between Compose 1.10.3 and 1.11.1. What varies is the rate, which scales with how many windows are composing lines.The fix
Item keys only have to be distinct among the rows present at once — a key reused after its line is gone just reuses the cache entry, which is the point. So the serial numbers are folded into a bounded space.
Displayed rows occupy a contiguous serial range (a name filter only removes lines from inside it), so any modulus wider than that range keeps them distinct.
lazyItemKeyModulustakes twice the range, floored at 4096 and rounded up to a power of two, so with the default 2000-line scrollback it settles during warmup and then never changes — a change would re-key every row, costing one full recomposition of the list.Same replay after the fix: 2,261 → 4,082 → 4,086 → 4,086. Bounded by the modulus rather than growing ~45k entries a minute.
Why not just drop the key
Index keys are bounded too, but the keys are what anchor the view when lines are trimmed off the front of the buffer. Without them, a reader scrolled up in the scrollback drifts by a line every time one is evicted, and per-row remembered state gets reused for the wrong line.
Testing
LazyItemKeyTestcovers the invariant the fix depends on — that no two rows on screen share a key:Plus the existing suites, ktlint, and the replay measurement above.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests