diff --git a/plans/sh-testing-design.md b/plans/sh-testing-design.md index 18c149dcb3..afed77ce6b 100644 --- a/plans/sh-testing-design.md +++ b/plans/sh-testing-design.md @@ -4,138 +4,239 @@ Produce one human-readable Markdown report that documents the performance problems found in the SimpleX Chat Android/Desktop Kotlin code (`apps/multiplatform/`) and states a concrete fix for each. The report is the -deliverable; it changes no application code. +deliverable. It changes no application code. -## Origin +## Request and prior feedback The request has two parts: 1. Review the Kotlin client code and determine whether performance issues exist. -2. Write the result as a structured, human-readable report — styled like the - project's own docs (table of contents, plain technical prose), with a BRIEF - at the top and one section per issue in the form `## Issue N: `, - followed by a description and a `### Fix` subsection. +2. Write the result as a structured, human-readable report: each finding is a + section in the exact form `## Issue N: <title>`, followed by a prose + description, then a `### Fix` subsection. -The review has already been carried out against the checked-out worktree; the -findings below are grounded in specific files and line numbers. This brief and -the plan define how those findings become the report. +Prior feedback: an earlier attempt deleted the findings and produced content +that did not follow the `## Issue N` / `### Fix` structure. This revision +restores every grounded finding and fixes the structure. Each issue below is +verified against the checked-out worktree with a real `file:line`. ## Deliverable A single Markdown file, `apps/multiplatform/PERFORMANCE_REPORT.md`, containing: -- A short **Brief** (scope, what was reviewed, method, how severities are used). -- A **Table of contents** linking to every issue. -- One `## Issue N: <title>` section per finding, each with a description and a - `### Fix`. -- A closing **Not defects / verified-good patterns** section that records the - places already doing the right thing (stable `LazyColumn` keys, off-main JSON - decode, existing caches and index maps), so the report is balanced and does - not imply the whole codebase is slow. +- A short **Brief** (scope: `apps/multiplatform/` Kotlin; method: static read of + the checked-out worktree; how severity is used). +- A **Table of contents** linking to every issue, ordered by severity. +- One `## Issue N: <title>` section per finding, each with a prose description + and a `### Fix` subsection. This structure is mandatory. +- A closing **Verified-good patterns / not defects** section recording the + places already doing the right thing, so the report is balanced. -## Audience -SimpleX maintainers familiar with the codebase. Assume they read Kotlin and -Compose; do not explain language basics. Every claim must point at a concrete -`file:line` in `apps/multiplatform/`. +## Audience and voice +SimpleX maintainers who read Kotlin and Compose. Do not explain language basics. +Every claim points at a concrete `file:line` under `apps/multiplatform/`. +Plain and formal engineering prose. No hedging, no marketing adjectives, no +emoji, no AI tells. Each issue states: what the code does, the `file:line` +evidence (short excerpt where it clarifies), why it costs time or memory (name +the complexity or "runs on the UI thread"), when it triggers (scroll, incoming +message batch, recomposition, startup, tap), and a severity label. -## Content and voice -- Structure exactly as requested: `## Issue N: ...` → description → `### Fix`. -- Each issue states: what the code does, the file:line evidence (with a short - excerpt where it clarifies), why it costs time or memory (name the complexity, - e.g. O(n) per add / O(n^2) per batch, or "runs on the UI thread"), when it is - triggered (scroll, incoming message batch, recomposition), and its severity. -- The fix is specific and grounded in this repo — prefer pointing at patterns the - codebase already uses (`groupMembersIndexes` id->index map at - `ChatModel.kt:139`; the async `Base64AsyncImage` at `Images.kt:36`; the desktop - `loadedImageCache` / `base64BitmapCache`; the `mapItemsToIds` Set in - `ChatItemsLoader.kt`). -- Plain and formal. No LLM/AI tells: no "As an AI", no hedging ("it seems", - "might possibly"), no marketing adjectives, no emoji, no per-section filler. - Write as an engineer filing findings. -- Severity labels: High (blocks the UI thread or scales O(n^2) on common paths), - Medium (per-event or per-recomposition waste on hot paths), Low (bounded or - startup-only). +Severity labels: +- **High**: blocks the UI thread, or scales O(n^2) on a common path. +- **Medium**: per-event or per-recomposition waste on a hot path. +- **Low**: bounded, startup-only, or already partly mitigated. -## Issues to cover (grounded findings) -Data model / collections (`model/ChatModel.kt`, `views/chat/ChatItemsLoader.kt`): -- `addToChatItems` allocates a new `SnapshotStateList` and copies the whole list - on every single insert — O(n) per add, O(n^2) for the per-item batch loop in - `CR.NewChatItems` (`ChatModel.kt:500-506`; `SimpleXAPI.kt:2899-2928`). -- `addChatItem` dedupes with `none { it.id == ... }` — O(n) scan per add before - the O(n) copy (`ChatModel.kt:582-590`). -- `upsertChatItem` / `updateChatItem` locate items with - `indexOfFirst { it.id == ... }` — O(n) per update, O(m*n) per status/reaction - batch (`ChatModel.kt:638-639, 661-662`). -- Chat lookup by id (`getChatIndex` / `getChat` / `hasChat`) is a linear scan - called per incoming item (`ChatModel.kt:377-379`, used at 460, 533, 615, 677 …). -- `getChatItemIndexOrNull` linear scan is called from render-time composables - (`ChatModel.kt:1122-1145`; callers `ChatItemView.kt:887`, `ChatView.kt:3333`, - `CIChatFeatureView.kt:79`, `MarkedDeletedItemView.kt:46`). -- `upsertGroupMember` maps the entire `chatItems` list and does an O(n) equality - check on every member update (`ChatModel.kt:941-955`). -- Pagination duplicate-removal uses `List.indexOf`/`contains` on `splits` inside - per-item `removeAll` predicates — O(items*splits) - (`ChatItemsLoader.kt:196-, 341-364`). +## Corrections folded in from verification +Three claims from the earlier review were inaccurate and are corrected here so +the report does not repeat them: +- `Base64AsyncImage` is at `platform/Images.kt:36` and **is used** (one caller, + `views/helpers/ChatInfoImage.kt:104`). It is not "unused". Frame it as an + existing off-thread decode pattern that the media item views do not reuse. +- `CIImageView.kt:45`, `CIVideoView.kt:42`, and `ImageFullScreenView.kt:157` + **do** wrap `base64ToBitmap` in `remember`, so they do not re-decode on every + recomposition. The real cost is the first synchronous decode on the composing + thread. Do not claim "no remember" for these three. +- `getChatItemIndexOrNull` at `ChatItemView.kt:887` is inside an `onClick` + handler (tap-time), not render-time. The render-time linear scan is + `EventItemView` (`ChatItemView.kt:640-643`). Do not claim 887 is per-frame. -Compose recomposition (`views/chatlist/*`, `views/chat/*`): -- `ChatList` re-runs `filteredChats(... allChats.value.toList() ...)` inline on - every recomposition — full copy + full filter, not memoized - (`ChatListView.kt:936-943`, `1443-1471`). -- The per-row `remember(chat.id, chats)` is keyed on the freshly allocated - filtered list, so it is invalidated for every visible row every pass - (`ChatListView.kt:1001-1006`). -- Whole-list reassignment: `replaceAll` and `reorderChat` throw away and rebuild - the `SnapshotStateList`, invalidating all readers (`ChatModel.kt:3526-3528`, - `406-413`). -- Unstable `selectChatItem` lambda allocated per item makes `ChatItemView` - non-skippable (`ChatView.kt:1950`). -- `EventItemView` reads `chatsCtx.chatItems.value` and reverses/scans the list in - composition, recomputing text unmemoized (`ChatItemView.kt:605-643`). -- Chat-list rows read model-wide `deletedChats`/`chatRunning` in the row body, so - one deletion recomposes every row (`ChatListNavLinkView.kt:42`). -- Toolbar rebuilds a mutable list of `@Composable` lambdas each recomposition - (`ChatListView.kt:483-568`). +## Issues to cover (grounded and verified) +Order in the report by severity, then group related items. Each entry below +maps to one `## Issue N` / `### Fix` section. -Main-thread blocking (`views/chat/ChatView.kt`, helpers, platform): -- `runBlocking { getLoadedImage(...) }` decrypts + decodes a full image on the - composition thread in the fullscreen gallery — High - (`ChatView.kt:3599`; `getMedia` at `ImageFullScreenView.kt:49, 97`). -- `Thread.sleep(10)` busy-wait in `ModalManager.runAtomically` on the UI thread - (`ModalView.kt:241-247`). -- `runBlocking { progressJob?.cancelAndJoin() }` in audio recorder `stop()` - (`RecAndPlay.android.kt:82-84`, `RecAndPlay.desktop.kt:80`). -- `Thread.sleep(50)` file-existence polling at desktop startup — Low - (`SingleInstance.kt:50-53`). +### High severity +- **Full image decoded on the composition thread in the fullscreen gallery.** + `ImageGalleryProvider.getMedia` calls `runBlocking { getLoadedImage(item.file) }` + (`views/chat/ChatView.kt:3599`), and `ImageFullScreenView.kt:97` calls + `provider.getMedia(index)` directly in the composable `Content` body (also in a + `LaunchedEffect` at `:49`). This decrypts and decodes a full-resolution image + synchronously, blocking the UI thread on open and page change. + Fix: move the load off the main thread — decode in `produceState` / + `LaunchedEffect` on `Dispatchers.IO`, mirroring `Base64AsyncImage` + (`platform/Images.kt:36`); never call the blocking provider from composition. -Image / media decoding and memory: -- Chat-list previews call `base64ToBitmap` in the composable body with no - `remember`, re-decoding on every recomposition (`ChatPreviewView.kt:320, 357`). -- Media items decode the base64 preview synchronously during composition - (`CIImageView.kt:45`, `CIVideoView.kt:42`, `ImageFullScreenView.kt:157`) while - the async `Base64AsyncImage` (`Images.kt:36`) exists and is unused. -- Android `base64ToBitmap` has no cache and decodes the byte array twice at full - resolution, never setting `inSampleSize` (`Images.android.kt:26-42`), unlike the - desktop cache (`Images.desktop.kt:24-50`). -- Android `getLoadedImage` has no cache and re-reads the whole file with - `readBytes()` on every call (`Utils.android.kt:172-194`), unlike the desktop +- **O(n) list copy per chat-item insert, O(n^2) per incoming batch.** + `addToChatItems` rebuilds a fresh `SnapshotStateList` and `addAll`s the whole + list on every insert (`model/ChatModel.kt:500-506`); `addChatItem` first scans + with `none { it.id == cItem.id }` (`:584`) then calls that O(n) copy. + `CR.NewChatItems` runs `r.chatItems.forEach { addChatItem(...) }` + (`model/SimpleXAPI.kt:2899-2900`), so a batch of m items costs O(m*n). + Triggered on every incoming message and batch. + Fix: mutate the existing `SnapshotStateList` in place (`add`/`add(index, …)`) + instead of reallocating; maintain an id->index map for the dedupe, mirroring + `groupMembersIndexes` (`ChatModel.kt:139`) and `mapItemsToIds` in + `ChatItemsLoader.kt`. + +- **O(n) `indexOfFirst` per upsert/update, O(m*n) per status/reaction batch.** + `upsertChatItem` (`ChatModel.kt:639`) and `updateChatItem` (`:662`) locate the + item with `indexOfFirst { it.id == cItem.id }`; a status/reaction batch runs + `r.chatItems.forEach { upsertChatItem(...) }` (`SimpleXAPI.kt:2930-2935`). + Fix: back `chatItems` with an id->index map so upsert/update are O(1) lookups. + +### Medium severity +- **Linear chat lookup per incoming item.** `getChatIndex`, `getChat`, and + `hasChat` are linear scans (`ChatModel.kt:377-379`) called on the add/upsert + paths (e.g. `:533`, `:615`, `:677`). + Fix: maintain an id->index map for `chats`, invalidated on reorder/replace. + +- **Chat list re-filters and re-copies the whole list every recomposition.** + `ChatList` calls `filteredChats(..., allChats.value.toList(), ...)` inline in + composition (`views/chatlist/ChatListView.kt:943`; definition `:1443-1471`), + copying then filtering all chats each pass. + Fix: memoize with `remember`/`derivedStateOf` keyed on the inputs + (search text, active filter, chats) so it recomputes only when they change. + +- **Per-row `remember` keyed on the freshly allocated filtered list.** + `remember(chat.id, chats)` (`ChatListView.kt:1002`) is keyed on `chats`, the + new filtered list, so every visible row's `derivedStateOf` is invalidated each + pass. + Fix: key the row `remember` on stable values only (`chat.id` and the neighbour + id), not the list identity. + +- **Whole-list reassignment invalidates all readers.** `replaceAll` + (`ChatModel.kt:3526-3528`) and `reorderChat` (`:406-413`, two full allocations + per reorder) discard and rebuild the `SnapshotStateList`. + Fix: mutate in place (`move`/`add`/`removeAt`) so only affected rows recompose. + +- **`upsertGroupMember` maps the whole `chatItems` list per member update.** + It builds a full `map` copy, does an O(n) structural `!=` comparison, then + `replaceAll` rebuilds the list (`ChatModel.kt:941-955`), on every member update. + Fix: update only the affected items in place; skip the full map + compare + + rebuild. + +- **Unstable `selectChatItem` lambda makes `ChatItemView` non-skippable.** + A new closure is allocated inline per item at `views/chat/ChatView.kt:1950`. + Fix: hoist/`remember` the lambda so the parameter is stable and Compose can + skip unchanged items. + +- **`EventItemView` reverses and scans `chatItems` in composition.** + `chatsCtx.chatItems.value.asReversed()` then `mergedGroupEventText(...)` scans + it and computes text unmemoized (`ChatItemView.kt:640-643`, `625-638`). + Fix: memoize the merged event text with `remember` keyed on the relevant ids. + +- **Chat-list rows read model-wide state in the row body.** + `ChatListNavLinkView` reads `chatModel.chatRunning.value` and + `chatModel.deletedChats.value` in every row (`ChatListNavLinkView.kt:42`), so a + single change recomposes all rows. + Fix: derive a per-chat boolean outside the row, or scope the read so only the + affected row recomposes. + +- **Toolbar rebuilds a list of `@Composable` lambdas each recomposition.** + `ChatListToolbar` allocates `arrayListOf<@Composable RowScope.() -> Unit>()` + and re-adds lambdas each pass (`ChatListView.kt:484-624`). + Fix: extract stable composables or `remember` the button set. + +- **Chat-list previews decode base64 in the composable body with no `remember`.** + `ChatPreviewView.kt:320` and `:357` call `base64ToBitmap(...)` inline, so the + preview re-decodes on every recomposition while scrolling. + Fix: wrap in `remember(image)` or use `Base64AsyncImage`. + +- **Android `base64ToBitmap` has no cache, double-decodes, no `inSampleSize`.** + `platform/Images.android.kt:26-42` runs `decodeByteArray` twice (bounds then + full) at full resolution and caches nothing, unlike the desktop + `base64BitmapCache` (`Images.desktop.kt:24-49`). + Fix: add a bounded cache like desktop and set `inSampleSize` for the target size. + +- **Android `getLoadedImage` has no cache and re-reads the file each call.** + `views/helpers/Utils.android.kt:172-194` calls `readBytes()` and decodes on + every call (`clearImageCaches()` at `:169` is a no-op), unlike the desktop `loadedImageCache` (`Utils.desktop.kt:128-146`). -- Full undecoded file bytes are retained per visible media item and passed to - Coil with `Size.ORIGINAL` (`CIImageView.kt:167-213`, `CIImageView.android.kt:30-34`). -- Desktop `resizeImageToStrSize` recomputes `hasAlpha()` (full pixel scan) on each - shrink iteration (`Images.desktop.kt:57-68, 105-115, 143-155`). + Fix: add a bounded cache mirroring desktop and wire `clearImageCaches()` to it. + +### Low severity +- **Media preview decode is synchronous on the composing thread (first pass).** + `CIImageView.kt:45`, `CIVideoView.kt:42`, and `ImageFullScreenView.kt:157` + wrap `base64ToBitmap` in `remember` (so no re-decode on recomposition), but the + first decode runs on the composing thread. `Base64AsyncImage` + (`platform/Images.kt:36`, used once at `ChatInfoImage.kt:104`) already decodes + off-thread on `Dispatchers.IO`. + Fix: reuse the off-thread decode pattern for these media previews. + +- **`getChatItemIndexOrNull` is a linear scan.** `ChatModel.kt:1122-1125` + (`indexOfFirst`). The per-recomposition caller is `EventItemView` + (`ChatItemView.kt:640-643`); the call at `ChatItemView.kt:887` is a tap-time + delete handler, not render-time. + Fix: resolve index via the id->index map introduced for `chatItems`. + +- **Pagination dedupe scans `splits` per item.** The loader runs + `splits.value.indexOf(it.id)` and `splits.contains(...)` inside per-item passes + and `removeAll { newIds.contains(it.id) }` + (`views/chat/ChatItemsLoader.kt:204, 246, 284, 318, 336, 354-356, 384, 410`), + giving O(items*splits) where `splits` is a `List`. Note `newIds` is already a + Set, so credit that. + Fix: index `splits` by id (Set/Map) for O(1) membership within these loops. + +- **Full undecoded file bytes retained per visible media item, Coil + `Size.ORIGINAL`.** `CIImageView.kt` (image data path) and + `CIImageView.android.kt` request original size, holding full bytes per item. + Fix: request a bounded target size rather than original for list/preview use. + +- **Desktop `resizeImageToStrSize` recomputes `hasAlpha()` each iteration.** + The resize loop calls `compressImageStr` per iteration + (`platform/Images.desktop.kt:57-68`), and `compressImageStr` calls + `bitmap.hasAlpha()` (`:105-106`), a full pixel scan (`:143-155`), every time. + Fix: compute `usePng`/`hasAlpha` once before the loop and pass it in, as + `resizeImageToDataSize` already does. + +- **`Thread.sleep(10)` busy-wait on the UI thread in `ModalManager`.** + `runAtomically` spins with `Thread.sleep(10)` until a CAS succeeds + (`views/helpers/ModalView.kt:241-247`). + Fix: replace the spin-wait with a coroutine `Mutex` or a suspend guard. + +- **`runBlocking { progressJob?.cancelAndJoin() }` in audio recorder `stop()`.** + `platform/RecAndPlay.android.kt:82-84` and `RecAndPlay.desktop.kt:80` block the + caller; the player path in the same files uses non-blocking `cancel()`. + Fix: use non-blocking `cancel()` (or a suspend `stop()`), matching the player. + +- **`Thread.sleep(50)` file-existence polling at desktop startup.** + `SingleInstance.kt:50-53` polls with `Thread.sleep(50)` for up to 1s. + Fix: acceptable as startup/background, but a `WatchService` or shorter bounded + wait removes the poll. Mark Low. + +## Verified-good patterns (must appear in the closing section) +- `groupMembersIndexes` id->index map (`ChatModel.kt:139`) — the pattern the + other collections should adopt. +- `Base64AsyncImage` off-thread decode (`platform/Images.kt:36`). +- Desktop `base64BitmapCache` (`Images.desktop.kt:24-49`) and `loadedImageCache` + (`Utils.desktop.kt:128-146`). +- `newIds` Set-based membership in the pagination loader (`ChatItemsLoader.kt`). +- `PopChatCollector.throttlePopChat` throttling the chat-pop hot path + (`ChatModel.kt`). +- Stable `LazyColumn` keys in `ChatList` (`ChatListView.kt:1001`). ## Success criteria -- The report file exists at the path above and follows the requested structure - (Brief, TOC, `## Issue N` / description / `### Fix`). +- `apps/multiplatform/PERFORMANCE_REPORT.md` exists and follows the structure: + Brief, TOC, one `## Issue N: <title>` / description / `### Fix` per finding, + then the verified-good section. - Every issue above is covered, each with a real `file:line`, a stated cost, a - trigger, a severity, and a concrete fix. -- Issues are ordered by severity/impact and grouped by theme in the TOC. -- The "verified-good patterns" section is present. -- The prose is plain and free of LLM/AI tells; no application code is modified. + trigger, a severity, and a concrete fix grounded in this repo. +- Issues are ordered by severity and grouped by theme in the TOC. +- The three corrections above are honoured; no corrected claim is restated in its + wrong form. +- Prose is plain and free of AI tells; no application code is modified. ## Edge cases and constraints -- Do not assert unverified claims. Findings the review could not confirm at the - call site (WebRTC signaling decode in `CallView.*:786/290`, the `rememberSaveable` - Saver decode in `ComposeView.kt:235`) are excluded, not stated as defects. -- Distinguish Android-only from desktop-only from shared issues; several problems - are platform-specific (the caches exist on desktop but not Android). -- Keep the report descriptive: it recommends fixes but must not edit code, and - must not overstate — note where a hot path is already throttled (e.g. - `PopChatCollector`) or already optimized (Set-based dedupe, stable keys). +- Do not assert unverified claims. Findings that could not be confirmed at the + call site (WebRTC signaling decode in `CallView.*`, the `rememberSaveable` + Saver decode in `ComposeView.kt`) are excluded, not stated as defects. +- Distinguish Android-only, desktop-only, and shared issues; several image/cache + problems exist on Android but are already solved on desktop. +- Keep the report descriptive: it recommends fixes but edits no code, and does + not overstate — note where a hot path is already throttled or optimized. - Length follows coverage; do not pad. One tight section per issue.