diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index b3adafa..4bd4fe8 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -86,7 +86,11 @@ class FakeMailRepository( val movedToFolder = mutableListOf, String>>() val replyDrafts = mutableListOf>() - override fun observeMessages(): Flow> = flowOf(messages) + override fun observeFolderMessages(accountId: String, folder: String): Flow> = + flowOf(messages.filter { it.accountId == accountId && it.folder == folder }) + + override fun observeUnifiedFolderMessages(folder: String): Flow> = + flowOf(messages.filter { it.folder == folder }) override fun observeFolders(accountId: String): Flow> = flowOf( folders.filter { diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index b0399cc..ef86725 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -24,6 +24,36 @@ interface MessageDao { ) fun observeSummaries(): Flow> + /** + * Mailbox-list projection scoped in SQL to one account's [folder], newest-first. Unlike + * [observeSummaries] this pulls only that folder's rows and re-emits only when they actually + * change, so the mailbox list's cost scales with what's shown, not the whole cache (issue #86). + * `inInbox` is *not* filtered here so the one query serves both the normal list (caller keeps + * `inInbox = 1` rows) and search (caller keeps rows matching the query, including transient + * `inInbox = 0` server-search hits). Served by the existing `(accountId, folder, uid)` index (its + * `(accountId, folder)` prefix), so no new index — and no schema migration — is needed. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE accountId = :accountId AND folder = :folder ORDER BY timestampMillis DESC", + ) + fun observeFolderSummaries(accountId: String, folder: String): Flow> + + /** + * Unified-inbox projection: [folder] across every account, newest-first. Scoped in SQL like + * [observeFolderSummaries] but without an account predicate (issue #86). No existing index leads + * with `folder`, so this still scans in timestamp order — far cheaper than [observeSummaries] (it + * materializes only this folder's rows, not the whole cache) but not O(1); a large multi-account + * unified inbox is a candidate for a `(folder, inInbox, timestampMillis)` index + paging. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE folder = :folder ORDER BY timestampMillis DESC", + ) + fun observeUnifiedFolderSummaries(folder: String): Flow> + /** * Live per-(account, folder) unread counts for the drawer's folder badges and the bold styling of * accounts with unread mail. Counts only folder-synced rows (`inInbox = 1`), so transient diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index 61ff531..68bc9ed 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -60,9 +60,11 @@ class MailRepositoryImpl @Inject constructor( private val signatureRepository: SignatureRepository, ) : MailRepository { - override fun observeMessages(): Flow> = messageDao.observeSummaries().map { rows -> - rows.map { it.toDomain() } - } + override fun observeFolderMessages(accountId: String, folder: String): Flow> = + messageDao.observeFolderSummaries(accountId, folder).map { rows -> rows.map { it.toDomain() } } + + override fun observeUnifiedFolderMessages(folder: String): Flow> = + messageDao.observeUnifiedFolderSummaries(folder).map { rows -> rows.map { it.toDomain() } } override fun observeFolders(accountId: String): Flow> = folderDao.observeForAccount(accountId).map { rows -> diff --git a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt index 4126fc6..8a4adc0 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt @@ -17,7 +17,15 @@ import java.io.File * The UI always reads from here; the cache is the single source of truth. */ interface MailRepository { - fun observeMessages(): Flow> + /** + * The cached messages of one account's [folder], newest-first — the scoped source for the mailbox + * list (issue #86). Includes transient server-search rows so search can surface them; the caller + * filters `inInbox`/query. Re-emits only when that folder's rows change, not on every cache write. + */ + fun observeFolderMessages(accountId: String, folder: String): Flow> + + /** Like [observeFolderMessages] but for [folder] across every account (the unified inbox). */ + fun observeUnifiedFolderMessages(folder: String): Flow> /** The account's cached IMAP folders for the navigation drawer. */ fun observeFolders(accountId: String): Flow> diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt index d2d60fd..fe319ed 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt @@ -117,21 +117,25 @@ class MailboxViewModel @Inject constructor( val searchQuery: StateFlow = _searchQuery.asStateFlow() val messages: StateFlow> = - combine( - mailRepository.observeMessages(), - _selectedAccountId, - _selectedFolder, - _searchQuery, - ) { all, accountId, folder, query -> - val q = query.trim() - all.filter { message -> - (accountId == null || message.accountId == accountId) && - message.folder == folder && - // Outside of search show only synced rows; while searching show every match in this - // folder, including transient server-search hits that aren't synced. - (if (q.isEmpty()) message.inInbox else message.matchesSearch(q)) - } - }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + combine(_selectedAccountId, _selectedFolder) { accountId, folder -> accountId to folder } + .distinctUntilChanged() + .flatMapLatest { (accountId, folder) -> + // Scope the query in SQL to the viewed account+folder (or [folder] across accounts for + // the unified inbox) so Room only re-queries/re-emits when those rows change, and the + // cost scales with the folder, not the whole cache (issue #86). + val scoped = if (accountId == null) { + mailRepository.observeUnifiedFolderMessages(folder) + } else { + mailRepository.observeFolderMessages(accountId, folder) + } + // The only remaining client-side pass distinguishes the normal list (synced rows) from + // an active search (any matching row, including transient server-search hits) — a match + // over the small folder-scoped set, never the whole cache. + combine(scoped, _searchQuery) { rows, query -> + val q = query.trim() + rows.filter { if (q.isEmpty()) it.inInbox else it.matchesSearch(q) } + } + }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) val draftCount: StateFlow = mailRepository.observeDrafts() .map { it.size } diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index f89f909..2e43f53 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -74,18 +74,19 @@ class MailRepositoryImplTest { ) @Test - fun `observeMessages is empty when the cache is empty`() = runTest { - every { messageDao.observeSummaries() } returns flowOf(emptyList()) - repository.observeMessages().test { + fun `observeFolderMessages is empty when the folder has no cached rows`() = runTest { + every { messageDao.observeFolderSummaries("acct", "INBOX") } returns flowOf(emptyList()) + repository.observeFolderMessages("acct", "INBOX").test { assertTrue(awaitItem().isEmpty()) awaitComplete() } } @Test - fun `observeMessages maps cached entities`() = runTest { - every { messageDao.observeSummaries() } returns flowOf(listOf(messageSummary("1", "INBOX"))) - repository.observeMessages().test { + fun `observeFolderMessages maps the folder's cached entities`() = runTest { + every { messageDao.observeFolderSummaries("acct", "INBOX") } returns + flowOf(listOf(messageSummary("1", "INBOX"))) + repository.observeFolderMessages("acct", "INBOX").test { val items = awaitItem() assertEquals(1, items.size) assertEquals("Ada", items.first().sender) @@ -94,6 +95,16 @@ class MailRepositoryImplTest { } } + @Test + fun `observeUnifiedFolderMessages maps the folder's rows across accounts`() = runTest { + every { messageDao.observeUnifiedFolderSummaries("INBOX") } returns + flowOf(listOf(messageSummary("1", "INBOX"))) + repository.observeUnifiedFolderMessages("INBOX").test { + assertEquals(1, awaitItem().size) + awaitComplete() + } + } + @Test fun `observeFolders maps cached folders with their roles and server special-use flag`() = runTest { every { folderDao.observeForAccount("acct") } returns flowOf( diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt index 02d2699..e9d3da2 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt @@ -167,7 +167,6 @@ class MailboxViewModelTest { @Test fun `folder list lags a drawer-account switch until the new folders arrive`() = runTest(testDispatcher) { val repo = mockk(relaxed = true) - every { repo.observeMessages() } returns MutableStateFlow(emptyList()) every { repo.observeDrafts() } returns flowOf(emptyList()) every { repo.observeOutbox() } returns flowOf(emptyList()) every { repo.observeFolders("imap:a") } returns @@ -483,7 +482,17 @@ class MailboxViewModelTest { repo: MailRepository = mockk(relaxed = true), initialAccountId: String? = null, ): MailboxViewModel { - every { repo.observeMessages() } returns MutableStateFlow(messages) + // Mimic the SQL scoping: return the seed list narrowed to the queried account+folder / folder, + // exactly as MessageDao.observeFolderSummaries / observeUnifiedFolderSummaries do (issue #86). + every { repo.observeFolderMessages(any(), any()) } answers { + val accountId = firstArg() + val folder = secondArg() + MutableStateFlow(messages.filter { it.accountId == accountId && it.folder == folder }) + } + every { repo.observeUnifiedFolderMessages(any()) } answers { + val folder = firstArg() + MutableStateFlow(messages.filter { it.folder == folder }) + } every { repo.observeDrafts() } returns flowOf(emptyList()) every { repo.observeOutbox() } returns flowOf(emptyList()) every { repo.observeUnreadCounts() } returns MutableStateFlow(unreadCounts) diff --git a/docs/perf/issue-86-profiling.md b/docs/perf/issue-86-profiling.md new file mode 100644 index 0000000..e51dbf7 --- /dev/null +++ b/docs/perf/issue-86-profiling.md @@ -0,0 +1,127 @@ + +# Issue #86 — mailbox message-loading profiling + +Empirical validation of the issue-#86 hypothesis that the mailbox list is slow because its data +path scales with the **total** cached message count, not with what's visible. Measured on an +emulator against a large seeded cache **before** committing a fix (as the ticket asked). + +## TL;DR / verdict + +- **The SQL-scoping theory holds, decisively.** The current path + (`MessageDao.observeSummaries()` → whole `messages` table → `MailRepositoryImpl.observeMessages()` + maps every row → `MailboxViewModel` `.filter{}`) is **O(total cache size)**: ~5 ms at 1k rows, + ~25 ms at 5k, **~125 ms at 20k**. A SQL-scoped `WHERE accountId = ? AND folder = ?` query is + **~1.5 ms, flat** regardless of cache size — **~80× faster at 20k**. This cost is paid on **every** + re-emission, i.e. on every unrelated write to `messages` (IDLE delivery, a read/star toggle, a + backfill page, a sync of any other folder), because Room re-runs the whole-table query on any write + to the table. +- **No new index is needed, and no schema migration.** The account-scoped query is already served by + the **existing** `(accountId, folder, uid)` index (added in v13 for backfill). The ticket's proposed + `(accountId, folder, inInbox, timestampMillis)` composite index changes the timing only within noise + (80× → 87×) — and SQLite's planner doesn't even prefer it when both are present. So the fix is pure + SQL WHERE-scoping; **the v14→v15 migration (and the #118 coordination) is avoided entirely.** +- **Second contributor found: the unified "All inboxes" view.** With no account selected the query is + folder-only (`WHERE folder = 'INBOX'`), which no existing index leads with, so it still scans in + timestamp order — better than the whole-table path (it materializes only INBOX rows, ~5.6× at 20k) + but still **O(N)** and it returns the whole unified inbox (3.7k rows at 20k, vs ~15 on screen). The + real fix there is **paging** (± a `(folder, timestampMillis)` index), deferred below. Notably the + ticket's `accountId`-leading index would **not** help this case at all. +- **IMAP latency (the ticket's other suspected contributor) was not measured** — it is a separate + network path (`MailSyncer.syncFolder` on folder open) independent of the cached-list rendering this + benchmark covers. The cached-list bottleneck is real and confirmed on its own. + +## Setup & method + +- Device: Gradle-managed AVD `libremail_api29` (API 29, x86, google_apis), headless, animations off. +- Harness: a throwaway instrumented probe built the real `LibreMailDatabase` in-memory (real entities, + real indices, real generated `MessageDao`), seeded it, and timed each query variant with **8 warmup + + 25 measured iterations**, reporting **median and min** wall-clock ms with a GC between phases. + `androidx.benchmark` was deliberately *not* used: on an emulator only the **relative** A/B result is + meaningful (absolute nanos aren't representative of a real device), and it would have forced the + module's global instrumentation runner to `AndroidBenchmarkRunner`, changing what CI's E2E jobs run + under. Warmup + median/min-of-many is robust to emulator scheduling/GC noise for an order-of- + magnitude comparison. (The probe was removed after measuring; the code path it exercised is now the + shipped `observeFolderSummaries` / `observeUnifiedFolderSummaries`.) +- Dataset: a fixed **150-row visible page** (`acct0` / `INBOX`) held constant while the total cache + grows to 1 000 / 5 000 / 20 000 rows, so the visible page is a shrinking fraction of the whole cache + (15 % → 3 % → 0.75 %) — modelling a long-lived, fully-backfilled account (#12/#13). The remaining + rows are distractors spread across 3 accounts × {INBOX, Sent, Archive, Spam}. Bodies are stored so + rows are realistically sized, but the summary projection never selects them. +- Variants per size: (a) **CURRENT** = `observeSummaries()` (whole table) + map-to-domain + the + `MailboxViewModel` filter down to the page; (b) **SCOPED acct+folder** = `WHERE accountId=? AND + folder=?`; (c) **UNIFIED folder-only** = `WHERE folder=?`. (b)/(c) were measured with the existing + indices and again after adding the candidate indexes, and each query's `EXPLAIN QUERY PLAN` was + captured with existing indices only. + +## Results (median / min ms, lower is better) + +| total rows | visible page | CURRENT (whole-table + filter) | SCOPED acct+folder (existing → +composite) | UNIFIED folder-only (existing → +folder idx) | +|-----------:|-------------:|-------------------------------:|:------------------------------------------:|:--------------------------------------------:| +| 1 000 | 150 | **4.98** / 4.20 | 1.83 → 1.61 | 2.48 → 1.89 | +| 5 000 | 150 | **25.17** / 22.37 | 1.43 → 1.45 | 5.46 → 4.75 | +| 20 000 | 150 | **127.6** / 122.8 | 1.62 → 1.46 | 23.0 → 18.6 | + +Speedups @ 20 000 rows (vs current): **acct+folder 79× (existing index), 87× (+composite)**; unified +5.6× (existing), 6.9× (+folder index). The current path's cost is the whole-table SQL scan + cursor +materialization of all N rows plus mapping all N to domain objects — both eliminated by scoping. +(A second run against the exact shipped SQL reproduced these figures; its 20k *median* was GC-inflated +to 254 ms while its *min* stayed at 125 ms — the min is the stable floor, hence both are reported.) + +## Query plans (`EXPLAIN QUERY PLAN`, existing indices only) + +``` +CURRENT whole-table : SCAN TABLE messages USING INDEX index_messages_timestampMillis +SCOPED acct+folder : SEARCH TABLE messages USING INDEX index_messages_accountId_folder_uid + (accountId=? AND folder=?) ; USE TEMP B-TREE FOR ORDER BY +UNIFIED folder-only : SCAN TABLE messages USING INDEX index_messages_timestampMillis +``` + +- CURRENT walks the timestamp index to avoid a sort but **reads every row** → O(N). +- SCOPED seeks the ~150 matching rows via the existing `(accountId, folder, uid)` index's + `(accountId, folder)` prefix, then sorts them with a temp b-tree — trivial for 150 rows (hence the + composite index, which would remove that tiny sort, makes no measurable difference). After adding the + composite `idx_afit`, **SQLite keeps choosing `index_messages_accountId_folder_uid`** — confirming the + new index is redundant. +- UNIFIED has no `folder`-leading index → scans. A `(folder, timestampMillis)` index (not the ticket's + `(folder, inInbox, …)`, which still needs the sort for the no-`inInbox` unified query) would turn it + into a seek — deferred. + +## Answers to the ticket's questions + +1. **Root cause confirmed via profiling on a large cache?** Yes — the whole-table observe + map + filter + is O(total cache) and dominates at scale; the SQL-scoped query is flat. +2. **Scope the list query in SQL instead of Kotlin?** Yes — implemented (below). +3. **Composite index needed?** **No.** The existing `(accountId, folder, uid)` index already captures + the account-scoped win; the ticket's index adds only noise and isn't even preferred by the planner. + No schema change / migration is made. +4. **Second contributor?** Yes — the unified inbox stays O(N) (folder-only scan) and returns the full + unified inbox; its real fix is paging (± a `(folder, timestampMillis)` index). Separately, IMAP + round-trip latency on folder open remains a distinct, unmeasured network concern. + +## What was implemented + +- `MessageDao.observeFolderSummaries(accountId, folder)` and `observeUnifiedFolderSummaries(folder)` — + the SQL-scoped, newest-first list projections (`inInbox`-agnostic so one query serves both the normal + list and search). +- `MailRepository.observeFolderMessages` / `observeUnifiedFolderMessages` replace the whole-table + `observeMessages()`. +- `MailboxViewModel.messages` now `flatMapLatest`es over the selected account+folder to the scoped + flow; the only remaining client-side pass distinguishes the normal list (`inInbox`) from an active + search (`matchesSearch`) over the small folder-scoped set — never the whole cache. `StateFlow`'s + built-in equality de-dup means an unrelated write now costs one cheap scoped re-query and no + recomposition. +- `observeSummaries()` is retained as the #51 CursorWindow regression-guard target in the DB tests. + +## Deferred follow-ups + +- **Unified inbox**: add Room `PagingSource`/Paging3 so the unified list's cost scales with the screen, + not the total inbox count; optionally a `(folder, timestampMillis)` index to turn the scan into a + seek. (The per-account views are already flat, so Paging3 there is lower priority.) +- **IMAP latency on folder open** is out of scope for this cached-render fix. + +## Reproduce + +Seed a large `messages` cache and compare `observeSummaries()` (+ a client filter) against +`observeFolderSummaries(account, folder)` on an emulator, timing first-emit at 1k/5k/20k rows and +inspecting `EXPLAIN QUERY PLAN`. The scoped query should be flat (~1.5 ms) and use +`index_messages_accountId_folder_uid`; the whole-table query should grow linearly.