JMR-dev/LibreMail
1
0
Fork 0
Code Issues 40 Pull Requests 2 Actions Packages Projects Releases 2 Wiki Activity

Merge pull request #429 from JMR-dev/perf-322-batched-persistbatch

Browse Source 
perf(data): route MailBackfiller.persistBatch through batched updateHeaderContents
This commit was merged in pull request #429.
This commit is contained in:
mergify[bot]
2026-07-08 06:04:46 +00:00
committed by GitHub
parent d55a1c3fae 1d17f76b15
commit d9e0d6410c
3 changed files with 146 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt
+56
View File
@@ -320,6 +320,62 @@ class MessageDaoTest {
assertEquals("cached-2", two.body)
}
/**
* The batched [MessageDao.updateHeaderContents] must write byte-for-byte the same row as the per-row
* [MessageDao.updateHeaderContent] it replaces on the backfill path (issue #322): the same refreshed
* fields and casefold columns, and the same untouched flags/body/membership. Two rows seeded
* identically and refreshed by the two paths with the same values must end up identical.
*/
@Test
fun updateHeaderContentsWritesTheSameResultAsThePerRowUpdate() = runBlocking {
dao.insertNew(
listOf(
message("perRow", isStarred = true, body = "cached"),
message("batch", isStarred = true, body = "cached"),
),
)
// Old path: the single-row update. New path: the batch, carrying identical field values.
dao.updateHeaderContent(
id = "perRow",
sender = "Refreshed",
senderEmail = "refreshed@example.org",
subject = "Fresh",
timestampMillis = 9_000L,
uid = 7L,
)
dao.updateHeaderContents(
listOf(
message(
"batch",
sender = "Refreshed",
senderEmail = "refreshed@example.org",
subject = "Fresh",
timestampMillis = 9_000L,
uid = 7L,
),
),
)
// Every stored column — refreshed and preserved alike — matches between the two paths.
val perRow = requireNotNull(dao.getById("perRow"))
val batch = requireNotNull(dao.getById("batch"))
assertEquals(perRow.copy(id = "id"), batch.copy(id = "id"))
}
/** An empty batch is a no-op — issue #322's empty-batch boundary at the DAO transaction level. */
@Test
fun updateHeaderContentsWithAnEmptyBatchWritesNothing() = runBlocking {
dao.insertNew(listOf(message("acct:1", subject = "Original", isRead = true, uid = 5)))
dao.updateHeaderContents(emptyList())
val row = requireNotNull(dao.getById("acct:1"))
assertEquals("Original", row.subject)
assertEquals(5L, row.uid)
assertTrue(row.isRead)
}
@Test
fun markSyncedPromotesSearchOnlyRowsIntoTheFolder() = runBlocking {
dao.insertNew(
app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt
+8 -10
View File
@@ -198,17 +198,15 @@ class MailBackfiller @Inject constructor(
val toRefresh = entities.filter { it.id in preexisting }
if (toRefresh.isNotEmpty()) {
messageDao.markSynced(toRefresh.map { it.id })
toRefresh.forEach {
messageDao.updateHeaderContent(
id = it.id,
sender = it.sender,
senderEmail = it.senderEmail,
subject = it.subject,
timestampMillis = it.timestampMillis,
uid = it.uid,
)
}
// Refresh every pre-existing row's header in ONE transaction (issue #322) rather than a per-row
// UPDATE each in its own implicit transaction — the same batched path foreground sync uses
// (issue #310). N per-row commits fsync the journal once per message (amplified on the
// encrypted cache); routing the whole batch through updateHeaderContents collapses them into a
// single commit per page. Semantically identical: it applies updateHeaderContent to each row in
// list order, so the same rows get the same values (and the same casefold columns).
messageDao.updateHeaderContents(toRefresh)
}
AppLog.d(TAG, "backfill persist: fetched=${entities.size} refreshed=${toRefresh.size}")
}
private suspend fun markComplete(accountId: String, folder: String, nextBeforeUid: Long) {
app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt
+82 -3
View File
@@ -378,10 +378,12 @@ class MailBackfillerTest {
/**
* A backfilled page whose ids already exist (e.g. former search-only rows) must be *refreshed*
* (markSynced + header update), not just IGNORE-inserted — this covers persistBatch's
* pre-existing-row branch, which the all-brand-new happy paths above never hit.
* pre-existing-row branch, which the all-brand-new happy paths above never hit. The refresh is
* routed through the BATCHED [MessageDao.updateHeaderContents] (issue #322), never the per-row
* [MessageDao.updateHeaderContent].
*/
@Test
fun `re-inserting a pre-existing header refreshes it rather than only inserting`() = runTest {
fun `re-inserting a pre-existing header refreshes it through the batched update`() = runTest {
appendMessages(60)
seedForegroundWindow()
val backfiller = backfiller(AccountSettings("acct"))
@@ -391,11 +393,88 @@ class MailBackfillerTest {
backfiller.runBackfill()
coVerify(atLeast = 1) { lastMessageDao!!.markSynced(any()) }
coVerify(atLeast = 1) {
// The whole batch is refreshed in one transaction — the per-row path is never taken (issue #322).
coVerify(atLeast = 1) { lastMessageDao!!.updateHeaderContents(any()) }
coVerify(exactly = 0) {
lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any())
}
}
// --- issue #322: batched persist ------------------------------------------------------------
/**
* persistBatch routes its pre-existing-row refreshes through the BATCHED
* [MessageDao.updateHeaderContents] — one transaction per page (issue #322) — and refreshes ONLY
* the rows that already existed: brand-new rows are written whole by insertNew, so re-updating them
* would be redundant. A partial page (some ids already present, some brand-new) exercises exactly
* that split, in one batched call, never the per-row [MessageDao.updateHeaderContent].
*/
@Test
fun `a partial page refreshes only its pre-existing rows through one batched update`() = runTest {
// One cached window row makes INBOX a backfill target with boundary UID 20.
cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX")
// A single older page (UIDs 10..19); the next fetch (boundary 10) returns empty → folder complete.
val page = (10..19).map { fetchedMessage(uid = it.toString()) }
val imapClient = mockk<ImapClient>()
coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers {
if (thirdArg<Long>() > 10L) page else emptyList()
}
val backfiller = backfiller(AccountSettings("acct"), imapClient = imapClient)
// Only UIDs 10..13 are reported as already present — the partial pre-existing subset.
val preexistingIds = (10..13).map { "acct:INBOX:$it" }.toSet()
coEvery { lastMessageDao!!.existingIds(any()) } answers {
firstArg<List<String>>().filter { it in preexistingIds }
}
val refreshedBatches = mutableListOf<List<MessageEntity>>()
coEvery { lastMessageDao!!.updateHeaderContents(any()) } answers {
refreshedBatches += firstArg<List<MessageEntity>>()
}
backfiller.runBackfill()
// Exactly one batched call for the page, carrying exactly the pre-existing subset (not the 6 new rows).
assertEquals(1, refreshedBatches.size, "one batched update per page")
assertEquals(preexistingIds, refreshedBatches.single().mapTo(HashSet()) { it.id })
// markSynced promotes exactly that subset; the per-row update path is never taken (issue #322).
coVerify(exactly = 1) { lastMessageDao!!.markSynced(match { it.toSet() == preexistingIds }) }
coVerify(exactly = 0) {
lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any())
}
// Counts-only persist breadcrumb (PII-free): 10 fetched, 4 refreshed.
assertTrue(
logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=4" },
"the persist breadcrumb logs page + refresh counts only",
)
}
/**
* The empty-refresh boundary: a page whose rows are ALL brand-new must skip the header-refresh
* transaction entirely — insertNew writes them whole, so neither markSynced nor the batched
* updateHeaderContents runs (issue #322 preserves persistBatch's isNotEmpty guard).
*/
@Test
fun `an all-new page skips the batched header update entirely`() = runTest {
cached += fetchedMessage(uid = "20").toEntity("acct", "INBOX")
val page = (10..19).map { fetchedMessage(uid = it.toString()) }
val imapClient = mockk<ImapClient>()
coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers {
if (thirdArg<Long>() > 10L) page else emptyList()
}
// existingIds stays at the relaxed default (empty) → every fetched row is brand-new.
backfiller(AccountSettings("acct"), imapClient = imapClient).runBackfill()
coVerify(exactly = 0) { lastMessageDao!!.updateHeaderContents(any()) }
coVerify(exactly = 0) { lastMessageDao!!.markSynced(any()) }
coVerify(exactly = 0) {
lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any())
}
// The persist breadcrumb still records the page, with zero refreshed.
assertTrue(
logBuffer.snapshot().any { it.message == "backfill persist: fetched=10 refreshed=0" },
"an all-new page logs refreshed=0",
)
}
// --- issue #329: AppLog breadcrumbs ---------------------------------------------------------
@Test