Add a memory usage view - #263
Merged
Merged
Conversation
A user reports large memory growth, and there was no way for them to say where it was going. Help > Memory usage now breaks down what each connection retains: per window, the lines shown, the lines buffered against the cap, the lines still held (including ones evicted from the buffer but still pinned by the displayed list's lazily-compacted backing), the size of the component index, and an estimated size. Plus panels, running scripts, and one heap used/max line for scale. Counts are exact. Sizes are a documented cost model - the JVM has no in-process way to size an object graph - so they rank windows against each other rather than reporting heap. "Copy report" produces a plain-text version to paste into an issue, and "Save heap dump" writes a live-objects hprof for when the breakdown is not enough. That needs HotSpotDiagnosticMXBean, so jdk.management joins the packaged runtime. Streams measure themselves on the work queue that owns their buffers, rather than racing the UI thread, which makes the registry call suspend. Both the title and usage reads are timeout-bounded, so a wedged connection degrades to "did not respond" instead of hanging the dialog. The component index count is a column because removeLines deliberately never prunes it, so on a stream that receives components it grows for the life of the connection; the new test pins that. 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)
📝 WalkthroughWalkthroughThe change adds memory usage models and APIs for streams, windows, and games. It adds stream accounting tests and a desktop dialog that displays metrics, copies reports, refreshes data, and saves live heap dumps. ChangesMemory usage reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant TitleBarView
participant WarlockApp
participant MemoryUsageDialog
participant GameViewModel
participant WindowRegistryImpl
participant ComposeTextStream
User->>TitleBarView: Select “Memory usage...”
TitleBarView->>WarlockApp: showMemoryDialog()
WarlockApp->>MemoryUsageDialog: Render with open games
MemoryUsageDialog->>GameViewModel: memoryUsage()
GameViewModel->>WindowRegistryImpl: memoryUsage()
WindowRegistryImpl->>ComposeTextStream: memoryUsage()
ComposeTextStream-->>WindowRegistryImpl: StreamMemoryUsage
WindowRegistryImpl-->>GameViewModel: WindowMemoryUsage
GameViewModel-->>MemoryUsageDialog: Memory report data
MemoryUsageDialog-->>User: Display usage and report actions
🚥 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: 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 534-537: Update memoryUsage so the queued “memory” task checks
result.isActive before invoking computeMemoryUsage, and cancel result in a
finally block around result.await(). Preserve completion for active requests
while ensuring timed-out callers cancel pending scans; already-running
non-suspending scans need not be interrupted.
In `@desktopApp/src/main/kotlin/warlockfe/warlock3/app/MemoryUsageDialog.kt`:
- Around line 276-294: Update collectReport to snapshot games with
games.toList(), launch one child coroutine per stable entry, and await all child
results together so connection reports are collected concurrently. Preserve the
existing title and memoryUsage timeouts, and keep stream reads serialized within
each connection’s coroutine.
🪄 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: 8215da8a-56bf-4786-b917-46db64f85625
📒 Files selected for processing (12)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.ktcompose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/ComposeTextStream.ktcompose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowRegistryImpl.ktcompose/src/jvmBenchmark/kotlin/warlockfe/warlock3/compose/util/StreamNetworkBenchmark.ktcompose/src/jvmTest/kotlin/ComposeTextStreamTest.ktcore/src/commonMain/kotlin/warlockfe/warlock3/core/window/MemoryUsage.ktcore/src/commonMain/kotlin/warlockfe/warlock3/core/window/WindowRegistry.ktdesktopApp/build.gradle.ktsdesktopApp/src/main/kotlin/warlockfe/warlock3/app/Main.ktdesktopApp/src/main/kotlin/warlockfe/warlock3/app/MemoryUsageDialog.ktdesktopApp/src/main/kotlin/warlockfe/warlock3/app/TitleBarView.ktdesktopApp/src/main/kotlin/warlockfe/warlock3/app/WarlockApp.kt
Two review points on the report path. Cancelling the caller does not cancel a standalone CompletableDeferred, so a dialog that timed out left its "memory" op still queued, and the op ran an O(buffer) walk on the queue that feeds the windows - the queue being busy is exactly why the caller timed out. Cancel the deferred when the caller unwinds and skip the walk when it is no longer active. A scan already under way still finishes; computeMemoryUsage never suspends. Collecting the connections was serial, so each unresponsive connection added its own timeout to the wait before the dialog showed anything. Give each connection its own coroutine: they have separate work queues, so only the streams within one connection need to serialize. The games list is snapshotted first, since it can change while the report is gathered. 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.
A user reports large memory growth and there was no way for them to tell us where it was going. Help > Memory usage breaks down what the app is retaining, in the app's own terms.
What it shows
Per connection, one row per game window:
Plus window/panel/script counts per connection, and one heap used/max line for scale.
Counts are exact. Sizes are a documented cost model (
MemoryEstimate) rather than measurements — the JVM offers no in-process way to size an object graph — so they are for comparing windows, not for reporting heap..hproffor when the breakdown is not enough. That goes throughHotSpotDiagnosticMXBean, sojdk.managementjoins the packaged runtime image.Notes
Streams measure themselves on the work queue that owns their buffers rather than racing the UI thread, which is why the registry call suspends. Both the title and the usage read are timeout-bounded, so a wedged connection degrades to "did not respond" instead of hanging the dialog.
"Component refs" is a column rather than an internal detail because
removeLinesdeliberately never prunescomponentLocations:On a stream that does receive components that index grows for the life of the connection. This PR does not change that behaviour, only surfaces it;
memoryUsageReportsUnprunedComponentIndexpins it (40 lines appended into a 4-line buffer leaves 4 buffered lines and 40 component references).Testing
Three new stream tests over the accounting, plus the existing suites. Desktop app launched and the dialog verified on screen.
jdk.managementconfirmed present in the packaged runtime image viacreateDistributable, with the packaged binary's--versionexiting 0.dumpHeap(path, live = true)checked on JDK 25, producing a validJAVA PROFILE 1.0.2file.Not exercised: the mobile UI (desktop-only feature), and the heap dump button itself was not clicked in a GUI session — the MXBean call it makes was verified separately.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests