From db463724f764929e08f65d46c803e47dc095e795 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 22:12:36 -0700 Subject: [PATCH 1/7] ADFA-4979: Enable SQLite mmap for the documentation database 64-bit processes now map documentation.db into memory via PRAGMA mmap_size, sized to the whole file, so reads go through the OS's virtual memory instead of repeated read() syscalls. 32-bit processes are skipped (too little address space). Always logs the outcome at INFO. Co-Authored-By: Claude Sonnet 5 --- .../androidide/localWebServer/WebServer.kt | 3 + .../localWebServer/WebServerTest.kt | 11 ++++ .../utils/SqliteMmapConfiguratorTest.kt | 58 +++++++++++++++++++ .../utils/SqliteMmapConfigurator.kt | 40 +++++++++++++ .../androidide/idetooltips/ToolTipManager.kt | 3 + 5 files changed, 115 insertions(+) create mode 100644 common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt create mode 100644 common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 0b76b64d2d..738ad499d3 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -10,6 +10,7 @@ import com.google.gson.GsonBuilder import com.google.gson.ToNumberPolicy import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.DatabaseVersionResolver +import com.itsaky.androidide.utils.SqliteMmapConfigurator import io.pebbletemplates.pebble.PebbleEngine import io.pebbletemplates.pebble.loader.StringLoader import io.pebbletemplates.pebble.template.PebbleTemplate @@ -169,6 +170,7 @@ class WebServer( try { database = SQLiteDatabase.openDatabase(config.databasePath, null, SQLiteDatabase.OPEN_READONLY) + SqliteMmapConfigurator.configureMmap(database) } catch (e: Exception) { log.error("Cannot open database: {}", e.message) return @@ -336,6 +338,7 @@ class WebServer( bookshelfTemplateId = -1 database.close() database = SQLiteDatabase.openDatabase(config.debugDatabasePath, null, SQLiteDatabase.OPEN_READONLY) + SqliteMmapConfigurator.configureMmap(database) databaseTimestamp = debugDatabaseTimestamp } diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index ef1e18de8f..e8dd78fbf4 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -2,6 +2,8 @@ package com.itsaky.androidide.localWebServer import android.database.sqlite.SQLiteDatabase import android.net.TrafficStats +import android.os.Process +import android.util.Log import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -35,6 +37,15 @@ class WebServerTest { every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns mockk(relaxed = true) + + // SqliteMmapConfigurator.configureMmap(), called right after openDatabase, uses + // Process.is64Bit() and Log.i(); neither is mocked by Android's unit-test stubs + // like SQLiteDatabase is, so both throw unless stubbed here. + mockkStatic(Process::class) + every { Process.is64Bit() } returns true + + mockkStatic(Log::class) + every { Log.i(any(), any()) } returns 0 } @After diff --git a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt new file mode 100644 index 0000000000..2ef2469f94 --- /dev/null +++ b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -0,0 +1,58 @@ +package com.itsaky.androidide.utils + +import android.database.sqlite.SQLiteDatabase +import android.os.Process +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File + +@RunWith(AndroidJUnit4::class) +class SqliteMmapConfiguratorTest { + private lateinit var dbFile: File + private lateinit var db: SQLiteDatabase + + @Before + fun setUp() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + dbFile = context.getDatabasePath("sqlite_mmap_configurator_test.db") + dbFile.delete() + db = SQLiteDatabase.openOrCreateDatabase(dbFile, null) + db.execSQL("CREATE TABLE Padding (value TEXT)") + db.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) + } + + @After + fun tearDown() { + db.close() + dbFile.delete() + } + + private fun readMmapSize(): Long = + db.rawQuery("PRAGMA mmap_size", null).use { c -> + c.moveToFirst() + c.getLong(0) + } + + // This device's actual bitness decides which branch runs, matching the production + // code's own Process.is64Bit() check -- so this test is meaningful under either an + // arm64-v8a (64-bit) or armeabi-v7a (32-bit) instrumented test run. + @Test + fun setsMmapSizeToFileSize_on64BitProcess_elseLeavesItUnchanged() { + val mmapSizeBeforeCall = readMmapSize() + + SqliteMmapConfigurator.configureMmap(db) + + val mmapSizeAfterCall = readMmapSize() + + if (Process.is64Bit()) { + assertEquals(dbFile.length(), mmapSizeAfterCall) + } else { + assertEquals(mmapSizeBeforeCall, mmapSizeAfterCall) + } + } +} diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt new file mode 100644 index 0000000000..e60f0a5bca --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt @@ -0,0 +1,40 @@ +package com.itsaky.androidide.utils + +import android.database.sqlite.SQLiteDatabase +import android.os.Process +import android.util.Log +import java.io.File + +/** + * Enables SQLite's memory-mapped IO (https://sqlite.org/mmap.html) for a database, sized + * to the whole file so page reads go through the OS's virtual memory instead of repeated + * read() syscalls. Only applied on 64-bit processes -- a 32-bit process has too little + * address space to map a documentation-database-sized file. Writes, and reads of any data + * added past the original file size (e.g. by a plugin), still fall back to the slow path; + * that's inherent to how SQLite mmap works, not something this call needs to handle. + */ +object SqliteMmapConfigurator { + private const val TAG = "SqliteMmapConfigurator" + + fun configureMmap(db: SQLiteDatabase) { + val dbPath = db.path + + if (!Process.is64Bit()) { + Log.i(TAG, "Not enabling mmap for '$dbPath': running in a 32-bit process.") + return + } + + val requestedSize = File(dbPath).length() + db.execSQL("PRAGMA mmap_size=$requestedSize") + + val actualSize = + db.rawQuery("PRAGMA mmap_size", null).use { c -> + if (c.moveToFirst()) c.getLong(0) else -1L + } + + Log.i( + TAG, + "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", + ) + } +} diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt index 5254603145..5b35767a4c 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt @@ -31,6 +31,7 @@ import com.itsaky.androidide.activities.editor.HelpActivity import com.itsaky.androidide.utils.DatabaseVersionResolver import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.FeedbackManager +import com.itsaky.androidide.utils.SqliteMmapConfigurator import com.itsaky.androidide.utils.UrlManager import com.itsaky.androidide.utils.isSystemInDarkMode import com.itsaky.androidide.utils.toCssHex @@ -86,6 +87,8 @@ object TooltipManager { try { SQLiteDatabase.openDatabase(dbPath, null, SQLiteDatabase.OPEN_READONLY).use { database -> + SqliteMmapConfigurator.configureMmap(database) + val lastChange = try { DatabaseVersionResolver.resolveDatabaseVersion(database) } catch (e: Exception) { From 31697542196e8f6ed045dfdb8c154cd957fb2118 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 22:44:52 -0700 Subject: [PATCH 2/7] ADFA-4979: Don't let mmap PRAGMA failures break docdb reads Catch SQLiteException around the mmap_size PRAGMA calls so a failure there degrades to a logged warning instead of propagating out of SqliteMmapConfigurator -- previously it could abort a tooltip lookup (caught by ToolTipManager's outer catch) or fail WebServer.start() entirely (mislabeled as "Cannot open database"). Co-Authored-By: Claude Sonnet 5 --- .../utils/SqliteMmapConfigurator.kt | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt index e60f0a5bca..6bed1a79bc 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.utils import android.database.sqlite.SQLiteDatabase +import android.database.sqlite.SQLiteException import android.os.Process import android.util.Log import java.io.File @@ -25,16 +26,21 @@ object SqliteMmapConfigurator { } val requestedSize = File(dbPath).length() - db.execSQL("PRAGMA mmap_size=$requestedSize") - val actualSize = - db.rawQuery("PRAGMA mmap_size", null).use { c -> - if (c.moveToFirst()) c.getLong(0) else -1L - } + try { + db.execSQL("PRAGMA mmap_size=$requestedSize") - Log.i( - TAG, - "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", - ) + val actualSize = + db.rawQuery("PRAGMA mmap_size", null).use { c -> + if (c.moveToFirst()) c.getLong(0) else -1L + } + + Log.i( + TAG, + "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", + ) + } catch (e: SQLiteException) { + Log.w(TAG, "Could not enable mmap for '$dbPath': ${e.message}") + } } } From b1a2132a433ef5bcd5b39e9d64277d0d40c5ef06 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 13 Aug 2026 23:27:54 -0700 Subject: [PATCH 3/7] ADFA-4979: Fix mmap PRAGMA never actually applying, plus review fixes Root cause found via on-device testing: PRAGMA mmap_size=N returns the granted size as a result row, and Android's SQLiteDatabase.execSQL() rejects any statement that returns data -- it threw SQLiteException on every call, silently breaking WebServer's help content (net::ERR_CONNECTION_REFUSED) and tooltips. Switched to rawQuery(), which both sets and reads the result in one call. Also, from a second code review pass: - Broaden the catch to Exception so any failure in this best-effort path (not just SQLiteException) degrades to a warning instead of breaking the caller. - Log a warning instead of a false "Enabled mmap" success line when SQLite grants 0 bytes. - Match the androidTest's DB open mode to production's OPEN_READONLY. - Add JVM unit tests covering the exception-swallowing behavior. - Guard the androidTest's tearDown against an uninitialized lateinit. - Document the new mmap step in documentation-database.md. Co-Authored-By: Claude Sonnet 5 --- .../localWebServer/WebServerTest.kt | 7 ++- .../utils/SqliteMmapConfiguratorTest.kt | 20 +++++-- .../utils/SqliteMmapConfigurator.kt | 40 ++++++++----- .../utils/SqliteMmapConfiguratorTest.kt | 58 +++++++++++++++++++ docs/documentation-database.md | 2 +- 5 files changed, 104 insertions(+), 23 deletions(-) create mode 100644 common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index e8dd78fbf4..0f8b1c70b6 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -39,13 +39,16 @@ class WebServerTest { } returns mockk(relaxed = true) // SqliteMmapConfigurator.configureMmap(), called right after openDatabase, uses - // Process.is64Bit() and Log.i(); neither is mocked by Android's unit-test stubs - // like SQLiteDatabase is, so both throw unless stubbed here. + // Process.is64Bit() and Log.i()/Log.w() (the relaxed db mock's rawQuery() returns + // a cursor whose moveToFirst() defaults to false, taking the Log.w "not enabled" + // branch); none of that is mocked by Android's unit-test stubs like SQLiteDatabase + // is, so all three throw unless stubbed here. mockkStatic(Process::class) every { Process.is64Bit() } returns true mockkStatic(Log::class) every { Log.i(any(), any()) } returns 0 + every { Log.w(any(), any()) } returns 0 } @After diff --git a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt index 2ef2469f94..d8d99550e1 100644 --- a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -21,15 +21,25 @@ class SqliteMmapConfiguratorTest { val context = InstrumentationRegistry.getInstrumentation().targetContext dbFile = context.getDatabasePath("sqlite_mmap_configurator_test.db") dbFile.delete() - db = SQLiteDatabase.openOrCreateDatabase(dbFile, null) - db.execSQL("CREATE TABLE Padding (value TEXT)") - db.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) + + SQLiteDatabase.openOrCreateDatabase(dbFile, null).use { writable -> + writable.execSQL("CREATE TABLE Padding (value TEXT)") + writable.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) + } + + // Every production call site opens OPEN_READONLY (see docs/documentation-database.md); + // match that here rather than testing against a writable connection. + db = SQLiteDatabase.openDatabase(dbFile.absolutePath, null, SQLiteDatabase.OPEN_READONLY) } @After fun tearDown() { - db.close() - dbFile.delete() + if (::db.isInitialized) { + db.close() + } + if (::dbFile.isInitialized) { + dbFile.delete() + } } private fun readMmapSize(): Long = diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt index 6bed1a79bc..f5a17fd35e 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt @@ -1,18 +1,18 @@ package com.itsaky.androidide.utils import android.database.sqlite.SQLiteDatabase -import android.database.sqlite.SQLiteException import android.os.Process import android.util.Log import java.io.File /** - * Enables SQLite's memory-mapped IO (https://sqlite.org/mmap.html) for a database, sized - * to the whole file so page reads go through the OS's virtual memory instead of repeated - * read() syscalls. Only applied on 64-bit processes -- a 32-bit process has too little - * address space to map a documentation-database-sized file. Writes, and reads of any data - * added past the original file size (e.g. by a plugin), still fall back to the slow path; - * that's inherent to how SQLite mmap works, not something this call needs to handle. + * Enables SQLite's memory-mapped IO (see SQLite's "The Memory-Mapped I/O Extension" doc) + * for a database, sized to the whole file so page reads go through the OS's virtual + * memory instead of repeated read() syscalls. Only applied on 64-bit processes -- a + * 32-bit process has too little address space to map a documentation-database-sized + * file. Writes, and reads of any data added past the original file size (e.g. by a + * plugin), still fall back to the slow path; that's inherent to how SQLite mmap works, + * not something this call needs to handle. */ object SqliteMmapConfigurator { private const val TAG = "SqliteMmapConfigurator" @@ -28,18 +28,28 @@ object SqliteMmapConfigurator { val requestedSize = File(dbPath).length() try { - db.execSQL("PRAGMA mmap_size=$requestedSize") - + // PRAGMA mmap_size=N returns the granted size as a result row, and Android's + // execSQL() rejects any statement that returns data -- rawQuery() is required. val actualSize = - db.rawQuery("PRAGMA mmap_size", null).use { c -> + db.rawQuery("PRAGMA mmap_size=$requestedSize", null).use { c -> if (c.moveToFirst()) c.getLong(0) else -1L } - Log.i( - TAG, - "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", - ) - } catch (e: SQLiteException) { + if (actualSize > 0) { + Log.i( + TAG, + "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", + ) + } else { + Log.w( + TAG, + "mmap not enabled for '$dbPath': SQLite granted $actualSize bytes for a request of $requestedSize.", + ) + } + } catch (e: Exception) { + // This is a best-effort read-performance optimization -- no failure here (a + // missing PRAGMA, a low-memory cursor allocation failure, etc.) should ever + // break the caller's actual database access. Log.w(TAG, "Could not enable mmap for '$dbPath': ${e.message}") } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt new file mode 100644 index 0000000000..581eb2b3cc --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -0,0 +1,58 @@ +package com.itsaky.androidide.utils + +import android.database.sqlite.SQLiteDatabase +import android.database.sqlite.SQLiteException +import android.os.Process +import android.util.Log +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import io.mockk.verify +import org.junit.After +import org.junit.Before +import org.junit.Test + +// Regression coverage for the real on-device failure (ADFA-4979): Android's +// SQLiteDatabase.execSQL() rejects any statement that returns a result row, and +// `PRAGMA mmap_size=N` does exactly that -- it threw SQLiteException on every call +// until configureMmap() switched to rawQuery(). This locks in that the function +// never lets such a failure escape to the caller. +class SqliteMmapConfiguratorTest { + @Before + fun setUp() { + mockkStatic(Process::class) + every { Process.is64Bit() } returns true + + mockkStatic(Log::class) + every { Log.i(any(), any()) } returns 0 + every { Log.w(any(), any()) } returns 0 + } + + @After + fun tearDown() { + unmockkAll() + } + + @Test + fun `configureMmap swallows a SQLiteException instead of propagating`() { + val db = mockk() + every { db.path } returns "/nonexistent/documentation.db" + every { db.rawQuery(any(), any()) } throws SQLiteException("simulated failure") + + SqliteMmapConfigurator.configureMmap(db) // must not throw + + verify { Log.w(any(), any()) } + } + + @Test + fun `configureMmap swallows a non-SQLite RuntimeException too`() { + val db = mockk() + every { db.path } returns "/nonexistent/documentation.db" + every { db.rawQuery(any(), any()) } throws IllegalStateException("simulated cursor failure") + + SqliteMmapConfigurator.configureMmap(db) // must not throw + + verify { Log.w(any(), any()) } + } +} diff --git a/docs/documentation-database.md b/docs/documentation-database.md index 566703ad1b..05fa0c46d8 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -69,7 +69,7 @@ CREATE TABLE Tooltips ( ## How CoGo talks to this database -All three sites below open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)` — no writes, ever, from this app (see ADR 0001 for why raw SQLite is justified here instead of Room). +All three sites below open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)` — no writes, ever, from this app (see ADR 0001 for why raw SQLite is justified here instead of Room). `WebServer` and `ToolTipManager` (not `PluginDocumentationManager`, which opens read-write) additionally call `SqliteMmapConfigurator.configureMmap()` right after opening, so SQLite memory-maps the whole file on 64-bit processes instead of paging it in via `read()` (ADFA-4979) — a no-op on 32-bit, and any failure degrades to a logged warning rather than blocking the read. - **`app/.../localWebServer/WebServer.kt`** — serves Tier 3. On each `GET`, runs: From c75d2cdeb0c8f8b94bd3fdf9b68eb9899c03ff32 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 16 Aug 2026 16:19:45 -0700 Subject: [PATCH 4/7] ADFA-4979: Address review feedback on the mmap configurator Log through SLF4J instead of android.util.Log, per REVIEW.md - structured {} placeholders, and the throwable passed as the last arg rather than interpolating e.message. The db path stays in the message; it is an app-internal files/ path, and naming it is the point of the ticket's "emit an INFO log line to indicate what happened". Document configureMmap()'s contract: db must be open and is left open, the call does synchronous file and SQLite IO, and every failure mode is best effort. Fix a documentation-database.md contradiction the review caught - it claimed all three call sites open OPEN_READONLY while simultaneously noting PluginDocumentationManager opens read-write. Only WebServer and ToolTipManager are read-only, and they are also the only two that configure mmap. Also state that SQLite may grant less than the requested size, or nothing. The androidTest comment carried the same error. The JVM test drops its android.util.Log mocking, which the SLF4J switch made unnecessary, and now asserts the no-throw contract explicitly with Truth instead of leaning on verify { Log.w(...) }. It stays on JUnit 4: useJUnitPlatform() is configured only in gradle-plugin, so a Jupiter test in common would silently never run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y4t7fFLNPJq9EiXr9S9LxF --- .../utils/SqliteMmapConfiguratorTest.kt | 9 +++- .../utils/SqliteMmapConfigurator.kt | 37 ++++++++++++----- .../utils/SqliteMmapConfiguratorTest.kt | 41 +++++++++---------- docs/documentation-database.md | 2 +- 4 files changed, 54 insertions(+), 35 deletions(-) diff --git a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt index d8d99550e1..963832f492 100644 --- a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -11,6 +11,10 @@ import org.junit.Test import org.junit.runner.RunWith import java.io.File +/** + * On-device coverage for [SqliteMmapConfigurator]: that the PRAGMA it issues actually + * takes effect against a real SQLite connection, which no JVM test can show. + */ @RunWith(AndroidJUnit4::class) class SqliteMmapConfiguratorTest { private lateinit var dbFile: File @@ -27,8 +31,9 @@ class SqliteMmapConfiguratorTest { writable.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) } - // Every production call site opens OPEN_READONLY (see docs/documentation-database.md); - // match that here rather than testing against a writable connection. + // Both call sites that configure mmap -- WebServer and ToolTipManager -- open + // OPEN_READONLY (see docs/documentation-database.md); match that here rather than + // testing against a writable connection. db = SQLiteDatabase.openDatabase(dbFile.absolutePath, null, SQLiteDatabase.OPEN_READONLY) } diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt index f5a17fd35e..f885207c7d 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt @@ -2,7 +2,7 @@ package com.itsaky.androidide.utils import android.database.sqlite.SQLiteDatabase import android.os.Process -import android.util.Log +import org.slf4j.LoggerFactory import java.io.File /** @@ -15,13 +15,26 @@ import java.io.File * not something this call needs to handle. */ object SqliteMmapConfigurator { - private const val TAG = "SqliteMmapConfigurator" + private val logger = LoggerFactory.getLogger(SqliteMmapConfigurator::class.java) + /** + * Requests memory-mapped IO for [db], sized to its file on disk. + * + * [db] must already be open, and is left open -- this only issues a PRAGMA on the + * caller's connection. The call is synchronous and does both file and SQLite IO, so + * keep it off the main thread; in practice callers invoke it right after opening the + * database, on whatever thread that open happened. + * + * Best effort throughout: on a 32-bit process it does nothing, and any failure is + * logged and swallowed rather than propagated. SQLite may also grant less than the + * requested size, or nothing at all, which is likewise only logged. Nothing here + * changes what a subsequent query returns -- only how fast it runs. + */ fun configureMmap(db: SQLiteDatabase) { val dbPath = db.path if (!Process.is64Bit()) { - Log.i(TAG, "Not enabling mmap for '$dbPath': running in a 32-bit process.") + logger.info("Not enabling mmap for '{}': running in a 32-bit process.", dbPath) return } @@ -36,21 +49,25 @@ object SqliteMmapConfigurator { } if (actualSize > 0) { - Log.i( - TAG, - "Enabled mmap for '$dbPath': requested $requestedSize bytes, SQLite granted $actualSize bytes.", + logger.info( + "Enabled mmap for '{}': requested {} bytes, SQLite granted {} bytes.", + dbPath, + requestedSize, + actualSize, ) } else { - Log.w( - TAG, - "mmap not enabled for '$dbPath': SQLite granted $actualSize bytes for a request of $requestedSize.", + logger.warn( + "mmap not enabled for '{}': SQLite granted {} bytes for a request of {}.", + dbPath, + actualSize, + requestedSize, ) } } catch (e: Exception) { // This is a best-effort read-performance optimization -- no failure here (a // missing PRAGMA, a low-memory cursor allocation failure, etc.) should ever // break the caller's actual database access. - Log.w(TAG, "Could not enable mmap for '$dbPath': ${e.message}") + logger.warn("Could not enable mmap for '{}'.", dbPath, e) } } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt index 581eb2b3cc..85c1c00034 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -3,30 +3,29 @@ package com.itsaky.androidide.utils import android.database.sqlite.SQLiteDatabase import android.database.sqlite.SQLiteException import android.os.Process -import android.util.Log +import com.google.common.truth.Truth.assertThat import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkAll -import io.mockk.verify import org.junit.After import org.junit.Before import org.junit.Test -// Regression coverage for the real on-device failure (ADFA-4979): Android's -// SQLiteDatabase.execSQL() rejects any statement that returns a result row, and -// `PRAGMA mmap_size=N` does exactly that -- it threw SQLiteException on every call -// until configureMmap() switched to rawQuery(). This locks in that the function -// never lets such a failure escape to the caller. +/** + * Regression coverage for the real on-device failure (ADFA-4979): Android's + * `SQLiteDatabase.execSQL()` rejects any statement that returns a result row, and + * `PRAGMA mmap_size=N` does exactly that -- it threw `SQLiteException` on every call + * until [SqliteMmapConfigurator.configureMmap] switched to `rawQuery()`. These tests + * lock in that such a failure never escapes to the caller, whatever its type. + */ class SqliteMmapConfiguratorTest { @Before fun setUp() { + // Force the 64-bit branch so the PRAGMA is actually attempted; the JVM test + // runner's bitness would otherwise decide which path runs. mockkStatic(Process::class) every { Process.is64Bit() } returns true - - mockkStatic(Log::class) - every { Log.i(any(), any()) } returns 0 - every { Log.w(any(), any()) } returns 0 } @After @@ -36,23 +35,21 @@ class SqliteMmapConfiguratorTest { @Test fun `configureMmap swallows a SQLiteException instead of propagating`() { - val db = mockk() - every { db.path } returns "/nonexistent/documentation.db" - every { db.rawQuery(any(), any()) } throws SQLiteException("simulated failure") - - SqliteMmapConfigurator.configureMmap(db) // must not throw - - verify { Log.w(any(), any()) } + assertThat(configureMmapFailingWith(SQLiteException("simulated failure"))).isNull() } @Test fun `configureMmap swallows a non-SQLite RuntimeException too`() { + assertThat(configureMmapFailingWith(IllegalStateException("simulated cursor failure"))).isNull() + } + + /** Runs [SqliteMmapConfigurator.configureMmap] against a database whose `rawQuery()` + * throws [failure], returning whatever escaped -- `null` when nothing did. */ + private fun configureMmapFailingWith(failure: Throwable): Throwable? { val db = mockk() every { db.path } returns "/nonexistent/documentation.db" - every { db.rawQuery(any(), any()) } throws IllegalStateException("simulated cursor failure") - - SqliteMmapConfigurator.configureMmap(db) // must not throw + every { db.rawQuery(any(), any()) } throws failure - verify { Log.w(any(), any()) } + return runCatching { SqliteMmapConfigurator.configureMmap(db) }.exceptionOrNull() } } diff --git a/docs/documentation-database.md b/docs/documentation-database.md index 05fa0c46d8..b1edb9bc65 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -69,7 +69,7 @@ CREATE TABLE Tooltips ( ## How CoGo talks to this database -All three sites below open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)` — no writes, ever, from this app (see ADR 0001 for why raw SQLite is justified here instead of Room). `WebServer` and `ToolTipManager` (not `PluginDocumentationManager`, which opens read-write) additionally call `SqliteMmapConfigurator.configureMmap()` right after opening, so SQLite memory-maps the whole file on 64-bit processes instead of paging it in via `read()` (ADFA-4979) — a no-op on 32-bit, and any failure degrades to a logged warning rather than blocking the read. +The two read paths below — `WebServer` and `ToolTipManager` — open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)`; only `PluginDocumentationManager` opens it `OPEN_READWRITE`, to merge in plugin-contributed content (see ADR 0001 for why raw SQLite is justified here instead of Room). Those same two read paths additionally call `SqliteMmapConfigurator.configureMmap()` right after opening, requesting a memory map the size of the file so SQLite pages it in through virtual memory instead of `read()` (ADFA-4979). SQLite may grant less than the request, or nothing at all — that's logged, not enforced. The call is a no-op on 32-bit processes, and any failure degrades to a logged warning rather than blocking the read. - **`app/.../localWebServer/WebServer.kt`** — serves Tier 3. On each `GET`, runs: From a5224b547fadb2da6059c5f5a9f85cd7425f0f71 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 14:03:56 -0700 Subject: [PATCH 5/7] ADFA-4979: Drop mmap from the tooltip path and harden the configurator Review of PR #1673 found that ToolTipManager.getTooltip() opens and closes a connection per tooltip, so configureMmap() charged an mmap()/munmap() of the whole ~200 MB file plus a PRAGMA round trip to replace two small indexed queries' worth of read(). mmap only amortizes across a long-lived connection, so that call is likely a net slowdown of the exact path the ticket set out to speed up. Removed it; WebServer, which holds its connection open, keeps it. Also from that review: - Return early when the file reports a length of 0. PRAGMA mmap_size=0 is how SQLite *disables* mmap, so the old code turned it off on an unstattable path and then blamed SQLite for granting nothing - the opposite of the documented "does nothing on failure" contract. - Record that the PRAGMA binds to the connection, not the SQLiteDatabase. That is invisible only because Android caps the pool at one connection for a non-WAL database. - The androidTest asserted an exact grant, which is stronger than the contract: SQLite clamps to SQLITE_MAX_MMAP_SIZE and grants nothing when built with it set to 0. Assert "at most the request" instead, keeping the exact-match check for the non-zero case, and stop reading column 0 of a possibly empty PRAGMA cursor. - WebServerTest's android.util.Log stubs became dead when configureMmap moved to slf4j, and their comment claimed they were load-bearing. RSS growth from mapping the whole file, and mmap turning I/O errors into an uncatchable SIGBUS, are both real but are properties of the design the ticket specifies. Folded into ADFA-5136 to measure rather than changed here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y4t7fFLNPJq9EiXr9S9LxF --- .../localWebServer/WebServerTest.kt | 13 +++------- .../utils/SqliteMmapConfiguratorTest.kt | 26 ++++++++++++++----- .../utils/SqliteMmapConfigurator.kt | 14 ++++++++++ docs/documentation-database.md | 4 ++- .../androidide/idetooltips/ToolTipManager.kt | 7 +++-- 5 files changed, 45 insertions(+), 19 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index 0f8b1c70b6..f77850d5c1 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -3,7 +3,6 @@ package com.itsaky.androidide.localWebServer import android.database.sqlite.SQLiteDatabase import android.net.TrafficStats import android.os.Process -import android.util.Log import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -38,17 +37,11 @@ class WebServerTest { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns mockk(relaxed = true) - // SqliteMmapConfigurator.configureMmap(), called right after openDatabase, uses - // Process.is64Bit() and Log.i()/Log.w() (the relaxed db mock's rawQuery() returns - // a cursor whose moveToFirst() defaults to false, taking the Log.w "not enabled" - // branch); none of that is mocked by Android's unit-test stubs like SQLiteDatabase - // is, so all three throw unless stubbed here. + // SqliteMmapConfigurator.configureMmap(), called right after openDatabase, reads + // Process.is64Bit(), which Android's unit-test stubs leave throwing. Its logging + // needs no stubbing -- that goes through slf4j, not android.util.Log. mockkStatic(Process::class) every { Process.is64Bit() } returns true - - mockkStatic(Log::class) - every { Log.i(any(), any()) } returns 0 - every { Log.w(any(), any()) } returns 0 } @After diff --git a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt index 963832f492..0de315a5eb 100644 --- a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -6,6 +6,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -31,9 +32,9 @@ class SqliteMmapConfiguratorTest { writable.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) } - // Both call sites that configure mmap -- WebServer and ToolTipManager -- open - // OPEN_READONLY (see docs/documentation-database.md); match that here rather than - // testing against a writable connection. + // The one call site that configures mmap, WebServer, opens OPEN_READONLY (see + // docs/documentation-database.md); match that here rather than testing against a + // writable connection. db = SQLiteDatabase.openDatabase(dbFile.absolutePath, null, SQLiteDatabase.OPEN_READONLY) } @@ -47,10 +48,12 @@ class SqliteMmapConfiguratorTest { } } + // Returns -1 when the PRAGMA yields no row at all, which is what a build with + // SQLITE_MAX_MMAP_SIZE=0 does -- the pragma body is compiled out. Reading column 0 + // regardless would throw CursorIndexOutOfBoundsException and hide that cause. private fun readMmapSize(): Long = db.rawQuery("PRAGMA mmap_size", null).use { c -> - c.moveToFirst() - c.getLong(0) + if (c.moveToFirst()) c.getLong(0) else -1L } // This device's actual bitness decides which branch runs, matching the production @@ -65,7 +68,18 @@ class SqliteMmapConfiguratorTest { val mmapSizeAfterCall = readMmapSize() if (Process.is64Bit()) { - assertEquals(dbFile.length(), mmapSizeAfterCall) + // Only "at most what was asked for" is guaranteed: SQLite clamps the grant to + // SQLITE_MAX_MMAP_SIZE, and a build with that set to 0 grants nothing at all. + assertTrue( + "SQLite granted $mmapSizeAfterCall bytes for a ${dbFile.length()}-byte file", + mmapSizeAfterCall <= dbFile.length(), + ) + + // This file is a few KB, so on any build that supports mmap at all it fits + // entirely -- a non-zero but short grant would be a real bug, not clamping. + if (mmapSizeAfterCall > 0) { + assertEquals(dbFile.length(), mmapSizeAfterCall) + } } else { assertEquals(mmapSizeBeforeCall, mmapSizeAfterCall) } diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt index f885207c7d..bdd0235a7e 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt @@ -40,9 +40,23 @@ object SqliteMmapConfigurator { val requestedSize = File(dbPath).length() + // A zero length means an empty or unstattable file, and PRAGMA mmap_size=0 is + // SQLite's way of *disabling* mmap -- so issuing it here would turn off whatever + // the platform default was, the opposite of doing nothing on failure. + if (requestedSize <= 0L) { + logger.warn("Not enabling mmap for '{}': the file reports a length of {} bytes.", dbPath, requestedSize) + return + } + try { // PRAGMA mmap_size=N returns the granted size as a result row, and Android's // execSQL() rejects any statement that returns data -- rawQuery() is required. + // + // The PRAGMA binds to the *connection* that runs it, not to the SQLiteDatabase. + // That is invisible today only because Android caps the pool at one connection + // for a non-WAL database; opening this one with ENABLE_WRITE_AHEAD_LOGGING would + // leave the other connections un-mmap'd while the log line below still claims + // success. val actualSize = db.rawQuery("PRAGMA mmap_size=$requestedSize", null).use { c -> if (c.moveToFirst()) c.getLong(0) else -1L diff --git a/docs/documentation-database.md b/docs/documentation-database.md index b1edb9bc65..469fa7a915 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -69,7 +69,9 @@ CREATE TABLE Tooltips ( ## How CoGo talks to this database -The two read paths below — `WebServer` and `ToolTipManager` — open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)`; only `PluginDocumentationManager` opens it `OPEN_READWRITE`, to merge in plugin-contributed content (see ADR 0001 for why raw SQLite is justified here instead of Room). Those same two read paths additionally call `SqliteMmapConfigurator.configureMmap()` right after opening, requesting a memory map the size of the file so SQLite pages it in through virtual memory instead of `read()` (ADFA-4979). SQLite may grant less than the request, or nothing at all — that's logged, not enforced. The call is a no-op on 32-bit processes, and any failure degrades to a logged warning rather than blocking the read. +The two read paths below — `WebServer` and `ToolTipManager` — open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)`; only `PluginDocumentationManager` opens it `OPEN_READWRITE`, to merge in plugin-contributed content (see ADR 0001 for why raw SQLite is justified here instead of Room). + +Of those, only `WebServer` calls `SqliteMmapConfigurator.configureMmap()` right after opening, requesting a memory map the size of the file so SQLite pages it in through virtual memory instead of `read()` (ADFA-4979). SQLite may grant less than the request, or nothing at all — that's logged, not enforced. The call is a no-op on 32-bit processes, and any failure degrades to a logged warning rather than blocking the read. `ToolTipManager` deliberately does **not** mmap: it opens and closes a connection per tooltip, so mapping the whole file would cost more than the two small indexed queries it saves. mmap only amortizes across a long-lived connection. Two consequences worth knowing before touching this: mapped pages sit outside `cache_size`'s bound and count toward process RSS, and I/O errors against a mapped file surface as an uncatchable SIGBUS rather than a `SQLiteException` — both are being measured in ADFA-5136. - **`app/.../localWebServer/WebServer.kt`** — serves Tier 3. On each `GET`, runs: diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt index 5b35767a4c..96df7491dd 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt @@ -31,7 +31,6 @@ import com.itsaky.androidide.activities.editor.HelpActivity import com.itsaky.androidide.utils.DatabaseVersionResolver import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.FeedbackManager -import com.itsaky.androidide.utils.SqliteMmapConfigurator import com.itsaky.androidide.utils.UrlManager import com.itsaky.androidide.utils.isSystemInDarkMode import com.itsaky.androidide.utils.toCssHex @@ -87,7 +86,11 @@ object TooltipManager { try { SQLiteDatabase.openDatabase(dbPath, null, SQLiteDatabase.OPEN_READONLY).use { database -> - SqliteMmapConfigurator.configureMmap(database) + // Deliberately no SqliteMmapConfigurator here (ADFA-4979): this path opens + // and closes a connection per tooltip, so mmap'ing the whole ~200 MB file + // would charge an mmap()/munmap() and a PRAGMA round trip to replace two + // small indexed queries' worth of read(). mmap only pays off on a + // long-lived connection like WebServer's. val lastChange = try { DatabaseVersionResolver.resolveDatabaseVersion(database) From 3bf69f84da320b04bd00cd576a50a12cc407d025 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 16:23:17 -0700 Subject: [PATCH 6/7] ADFA-4979: Stop the mmap tests passing without running the PRAGMA The zero-length guard added in a5224b5 broke the tests that motivated it. configureMmapFailingWith pointed db.path at "/nonexistent/documentation.db", so File(dbPath).length() returned 0, configureMmap returned early, and db.rawQuery -- the call whose thrown exception is the entire point of both tests -- was never reached. Both kept passing, for the wrong reason. Point the mock at a real, non-empty temp file so the PRAGMA is actually attempted, and verify rawQuery was called so this cannot regress silently again. Confirmed the check bites: with a zero-length file both tests fail on the missing call, and with content both pass. Found by CodeRabbit on PR #1673. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y4t7fFLNPJq9EiXr9S9LxF --- .../utils/SqliteMmapConfiguratorTest.kt | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt index 85c1c00034..5001cf60e8 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt @@ -8,9 +8,11 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkAll +import io.mockk.verify import org.junit.After import org.junit.Before import org.junit.Test +import java.io.File /** * Regression coverage for the real on-device failure (ADFA-4979): Android's @@ -43,13 +45,29 @@ class SqliteMmapConfiguratorTest { assertThat(configureMmapFailingWith(IllegalStateException("simulated cursor failure"))).isNull() } - /** Runs [SqliteMmapConfigurator.configureMmap] against a database whose `rawQuery()` - * throws [failure], returning whatever escaped -- `null` when nothing did. */ + /** + * Runs [SqliteMmapConfigurator.configureMmap] against a database whose `rawQuery()` throws + * [failure], returning whatever escaped -- `null` when nothing did. + * + * The path must name a real, non-empty file: `configureMmap` returns early when the file + * reports a length of 0, so pointing this at a nonexistent path would skip the PRAGMA + * entirely and the tests would pass without ever reaching the code they cover. The + * `verify` below is what keeps that from silently regressing again. + */ private fun configureMmapFailingWith(failure: Throwable): Throwable? { - val db = mockk() - every { db.path } returns "/nonexistent/documentation.db" - every { db.rawQuery(any(), any()) } throws failure + val dbFile = File.createTempFile("sqlite_mmap_configurator_test", ".db") + return try { + dbFile.writeBytes(ByteArray(4096)) - return runCatching { SqliteMmapConfigurator.configureMmap(db) }.exceptionOrNull() + val db = mockk() + every { db.path } returns dbFile.absolutePath + every { db.rawQuery(any(), any()) } throws failure + + runCatching { SqliteMmapConfigurator.configureMmap(db) }.exceptionOrNull().also { + verify { db.rawQuery(any(), any()) } + } + } finally { + dbFile.delete() + } } } From e5e1c6491c5bcb69c54ab32502a15cbcf100ced4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 20:44:34 -0700 Subject: [PATCH 7/7] ADFA-4979: Remove SQLite mmap; benchmarking found no benefit ADFA-5136 measured this change on an arm64 device (Note 20 Ultra, Android 13) across every axis the reviewers asked about, and mmap did not improve read time in any configuration: - 200 small Kotlin stdlib pages: medians converged at ~8.4ms with mmap off, capped at 32 MB, and mapping the whole file. mmap was slightly *worse* on the first pass, which is the mapping setup plus first-touch faults. - 7 PDFs up to 9.8 MB: 30-44ms medians, no arm ahead. - A sustained 3000-page walk serving 116 MB: 6.1-6.5ms medians in all arms. - page_size 1024 vs 2048: no difference either, which is the clearest evidence that IO is not the bottleneck. Doubling the page size halves the page count, and so roughly halves the read() calls per row, and moved medians by 0.04ms. What dominates is Brotli decode plus Pebble rendering. Mapping the whole file did have one measurable effect: about 100 MB of resident mapped pages after a single walk touching roughly a tenth of the corpus, since mapped pages bypass the pager cache and sit outside cache_size's bound. Those pages are clean and file-backed, so the kernel reclaims them under pressure rather than the process being killed -- the cost is fault and reclaim churn, not an OOM risk. Still a cost with nothing on the other side of the ledger. So the whole facility goes: the configurator, its unit and instrumented tests, the WebServer call sites, and the WebServerTest stub of Process.is64Bit() that only existed because configureMmap() read it. The tooltip-path comment goes too, since it explained why that path avoided a facility that no longer exists. Kept from this branch: the correction that PluginDocumentationManager opens the database OPEN_READWRITE, which the docs previously denied outright ("no writes, ever"), and a note recording that mmap was measured and rejected so the next person does not rebuild it. The benchmark scaffolding was deliberately not retained; ADFA-5136 records the method. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/localWebServer/WebServer.kt | 3 - .../localWebServer/WebServerTest.kt | 7 -- .../utils/SqliteMmapConfiguratorTest.kt | 87 ------------------- .../utils/SqliteMmapConfigurator.kt | 87 ------------------- .../utils/SqliteMmapConfiguratorTest.kt | 73 ---------------- docs/documentation-database.md | 2 +- .../androidide/idetooltips/ToolTipManager.kt | 6 -- 7 files changed, 1 insertion(+), 264 deletions(-) delete mode 100644 common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt delete mode 100644 common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt delete mode 100644 common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 738ad499d3..0b76b64d2d 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -10,7 +10,6 @@ import com.google.gson.GsonBuilder import com.google.gson.ToNumberPolicy import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.DatabaseVersionResolver -import com.itsaky.androidide.utils.SqliteMmapConfigurator import io.pebbletemplates.pebble.PebbleEngine import io.pebbletemplates.pebble.loader.StringLoader import io.pebbletemplates.pebble.template.PebbleTemplate @@ -170,7 +169,6 @@ class WebServer( try { database = SQLiteDatabase.openDatabase(config.databasePath, null, SQLiteDatabase.OPEN_READONLY) - SqliteMmapConfigurator.configureMmap(database) } catch (e: Exception) { log.error("Cannot open database: {}", e.message) return @@ -338,7 +336,6 @@ class WebServer( bookshelfTemplateId = -1 database.close() database = SQLiteDatabase.openDatabase(config.debugDatabasePath, null, SQLiteDatabase.OPEN_READONLY) - SqliteMmapConfigurator.configureMmap(database) databaseTimestamp = debugDatabaseTimestamp } diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index f77850d5c1..ef1e18de8f 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -2,7 +2,6 @@ package com.itsaky.androidide.localWebServer import android.database.sqlite.SQLiteDatabase import android.net.TrafficStats -import android.os.Process import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -36,12 +35,6 @@ class WebServerTest { every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns mockk(relaxed = true) - - // SqliteMmapConfigurator.configureMmap(), called right after openDatabase, reads - // Process.is64Bit(), which Android's unit-test stubs leave throwing. Its logging - // needs no stubbing -- that goes through slf4j, not android.util.Log. - mockkStatic(Process::class) - every { Process.is64Bit() } returns true } @After diff --git a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt deleted file mode 100644 index 0de315a5eb..0000000000 --- a/common/src/androidTest/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ /dev/null @@ -1,87 +0,0 @@ -package com.itsaky.androidide.utils - -import android.database.sqlite.SQLiteDatabase -import android.os.Process -import androidx.test.ext.junit.runners.AndroidJUnit4 -import androidx.test.platform.app.InstrumentationRegistry -import org.junit.After -import org.junit.Assert.assertEquals -import org.junit.Assert.assertTrue -import org.junit.Before -import org.junit.Test -import org.junit.runner.RunWith -import java.io.File - -/** - * On-device coverage for [SqliteMmapConfigurator]: that the PRAGMA it issues actually - * takes effect against a real SQLite connection, which no JVM test can show. - */ -@RunWith(AndroidJUnit4::class) -class SqliteMmapConfiguratorTest { - private lateinit var dbFile: File - private lateinit var db: SQLiteDatabase - - @Before - fun setUp() { - val context = InstrumentationRegistry.getInstrumentation().targetContext - dbFile = context.getDatabasePath("sqlite_mmap_configurator_test.db") - dbFile.delete() - - SQLiteDatabase.openOrCreateDatabase(dbFile, null).use { writable -> - writable.execSQL("CREATE TABLE Padding (value TEXT)") - writable.execSQL("INSERT INTO Padding (value) VALUES (?)", arrayOf("x".repeat(4096))) - } - - // The one call site that configures mmap, WebServer, opens OPEN_READONLY (see - // docs/documentation-database.md); match that here rather than testing against a - // writable connection. - db = SQLiteDatabase.openDatabase(dbFile.absolutePath, null, SQLiteDatabase.OPEN_READONLY) - } - - @After - fun tearDown() { - if (::db.isInitialized) { - db.close() - } - if (::dbFile.isInitialized) { - dbFile.delete() - } - } - - // Returns -1 when the PRAGMA yields no row at all, which is what a build with - // SQLITE_MAX_MMAP_SIZE=0 does -- the pragma body is compiled out. Reading column 0 - // regardless would throw CursorIndexOutOfBoundsException and hide that cause. - private fun readMmapSize(): Long = - db.rawQuery("PRAGMA mmap_size", null).use { c -> - if (c.moveToFirst()) c.getLong(0) else -1L - } - - // This device's actual bitness decides which branch runs, matching the production - // code's own Process.is64Bit() check -- so this test is meaningful under either an - // arm64-v8a (64-bit) or armeabi-v7a (32-bit) instrumented test run. - @Test - fun setsMmapSizeToFileSize_on64BitProcess_elseLeavesItUnchanged() { - val mmapSizeBeforeCall = readMmapSize() - - SqliteMmapConfigurator.configureMmap(db) - - val mmapSizeAfterCall = readMmapSize() - - if (Process.is64Bit()) { - // Only "at most what was asked for" is guaranteed: SQLite clamps the grant to - // SQLITE_MAX_MMAP_SIZE, and a build with that set to 0 grants nothing at all. - assertTrue( - "SQLite granted $mmapSizeAfterCall bytes for a ${dbFile.length()}-byte file", - mmapSizeAfterCall <= dbFile.length(), - ) - - // This file is a few KB, so on any build that supports mmap at all it fits - // entirely -- a non-zero but short grant would be a real bug, not clamping. - if (mmapSizeAfterCall > 0) { - assertEquals(dbFile.length(), mmapSizeAfterCall) - } - } else { - assertEquals(mmapSizeBeforeCall, mmapSizeAfterCall) - } - } -} diff --git a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt b/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt deleted file mode 100644 index bdd0235a7e..0000000000 --- a/common/src/main/java/com/itsaky/androidide/utils/SqliteMmapConfigurator.kt +++ /dev/null @@ -1,87 +0,0 @@ -package com.itsaky.androidide.utils - -import android.database.sqlite.SQLiteDatabase -import android.os.Process -import org.slf4j.LoggerFactory -import java.io.File - -/** - * Enables SQLite's memory-mapped IO (see SQLite's "The Memory-Mapped I/O Extension" doc) - * for a database, sized to the whole file so page reads go through the OS's virtual - * memory instead of repeated read() syscalls. Only applied on 64-bit processes -- a - * 32-bit process has too little address space to map a documentation-database-sized - * file. Writes, and reads of any data added past the original file size (e.g. by a - * plugin), still fall back to the slow path; that's inherent to how SQLite mmap works, - * not something this call needs to handle. - */ -object SqliteMmapConfigurator { - private val logger = LoggerFactory.getLogger(SqliteMmapConfigurator::class.java) - - /** - * Requests memory-mapped IO for [db], sized to its file on disk. - * - * [db] must already be open, and is left open -- this only issues a PRAGMA on the - * caller's connection. The call is synchronous and does both file and SQLite IO, so - * keep it off the main thread; in practice callers invoke it right after opening the - * database, on whatever thread that open happened. - * - * Best effort throughout: on a 32-bit process it does nothing, and any failure is - * logged and swallowed rather than propagated. SQLite may also grant less than the - * requested size, or nothing at all, which is likewise only logged. Nothing here - * changes what a subsequent query returns -- only how fast it runs. - */ - fun configureMmap(db: SQLiteDatabase) { - val dbPath = db.path - - if (!Process.is64Bit()) { - logger.info("Not enabling mmap for '{}': running in a 32-bit process.", dbPath) - return - } - - val requestedSize = File(dbPath).length() - - // A zero length means an empty or unstattable file, and PRAGMA mmap_size=0 is - // SQLite's way of *disabling* mmap -- so issuing it here would turn off whatever - // the platform default was, the opposite of doing nothing on failure. - if (requestedSize <= 0L) { - logger.warn("Not enabling mmap for '{}': the file reports a length of {} bytes.", dbPath, requestedSize) - return - } - - try { - // PRAGMA mmap_size=N returns the granted size as a result row, and Android's - // execSQL() rejects any statement that returns data -- rawQuery() is required. - // - // The PRAGMA binds to the *connection* that runs it, not to the SQLiteDatabase. - // That is invisible today only because Android caps the pool at one connection - // for a non-WAL database; opening this one with ENABLE_WRITE_AHEAD_LOGGING would - // leave the other connections un-mmap'd while the log line below still claims - // success. - val actualSize = - db.rawQuery("PRAGMA mmap_size=$requestedSize", null).use { c -> - if (c.moveToFirst()) c.getLong(0) else -1L - } - - if (actualSize > 0) { - logger.info( - "Enabled mmap for '{}': requested {} bytes, SQLite granted {} bytes.", - dbPath, - requestedSize, - actualSize, - ) - } else { - logger.warn( - "mmap not enabled for '{}': SQLite granted {} bytes for a request of {}.", - dbPath, - actualSize, - requestedSize, - ) - } - } catch (e: Exception) { - // This is a best-effort read-performance optimization -- no failure here (a - // missing PRAGMA, a low-memory cursor allocation failure, etc.) should ever - // break the caller's actual database access. - logger.warn("Could not enable mmap for '{}'.", dbPath, e) - } - } -} diff --git a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt b/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt deleted file mode 100644 index 5001cf60e8..0000000000 --- a/common/src/test/java/com/itsaky/androidide/utils/SqliteMmapConfiguratorTest.kt +++ /dev/null @@ -1,73 +0,0 @@ -package com.itsaky.androidide.utils - -import android.database.sqlite.SQLiteDatabase -import android.database.sqlite.SQLiteException -import android.os.Process -import com.google.common.truth.Truth.assertThat -import io.mockk.every -import io.mockk.mockk -import io.mockk.mockkStatic -import io.mockk.unmockkAll -import io.mockk.verify -import org.junit.After -import org.junit.Before -import org.junit.Test -import java.io.File - -/** - * Regression coverage for the real on-device failure (ADFA-4979): Android's - * `SQLiteDatabase.execSQL()` rejects any statement that returns a result row, and - * `PRAGMA mmap_size=N` does exactly that -- it threw `SQLiteException` on every call - * until [SqliteMmapConfigurator.configureMmap] switched to `rawQuery()`. These tests - * lock in that such a failure never escapes to the caller, whatever its type. - */ -class SqliteMmapConfiguratorTest { - @Before - fun setUp() { - // Force the 64-bit branch so the PRAGMA is actually attempted; the JVM test - // runner's bitness would otherwise decide which path runs. - mockkStatic(Process::class) - every { Process.is64Bit() } returns true - } - - @After - fun tearDown() { - unmockkAll() - } - - @Test - fun `configureMmap swallows a SQLiteException instead of propagating`() { - assertThat(configureMmapFailingWith(SQLiteException("simulated failure"))).isNull() - } - - @Test - fun `configureMmap swallows a non-SQLite RuntimeException too`() { - assertThat(configureMmapFailingWith(IllegalStateException("simulated cursor failure"))).isNull() - } - - /** - * Runs [SqliteMmapConfigurator.configureMmap] against a database whose `rawQuery()` throws - * [failure], returning whatever escaped -- `null` when nothing did. - * - * The path must name a real, non-empty file: `configureMmap` returns early when the file - * reports a length of 0, so pointing this at a nonexistent path would skip the PRAGMA - * entirely and the tests would pass without ever reaching the code they cover. The - * `verify` below is what keeps that from silently regressing again. - */ - private fun configureMmapFailingWith(failure: Throwable): Throwable? { - val dbFile = File.createTempFile("sqlite_mmap_configurator_test", ".db") - return try { - dbFile.writeBytes(ByteArray(4096)) - - val db = mockk() - every { db.path } returns dbFile.absolutePath - every { db.rawQuery(any(), any()) } throws failure - - runCatching { SqliteMmapConfigurator.configureMmap(db) }.exceptionOrNull().also { - verify { db.rawQuery(any(), any()) } - } - } finally { - dbFile.delete() - } - } -} diff --git a/docs/documentation-database.md b/docs/documentation-database.md index 469fa7a915..a0badff4fc 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -71,7 +71,7 @@ CREATE TABLE Tooltips ( The two read paths below — `WebServer` and `ToolTipManager` — open the file with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)`; only `PluginDocumentationManager` opens it `OPEN_READWRITE`, to merge in plugin-contributed content (see ADR 0001 for why raw SQLite is justified here instead of Room). -Of those, only `WebServer` calls `SqliteMmapConfigurator.configureMmap()` right after opening, requesting a memory map the size of the file so SQLite pages it in through virtual memory instead of `read()` (ADFA-4979). SQLite may grant less than the request, or nothing at all — that's logged, not enforced. The call is a no-op on 32-bit processes, and any failure degrades to a logged warning rather than blocking the read. `ToolTipManager` deliberately does **not** mmap: it opens and closes a connection per tooltip, so mapping the whole file would cost more than the two small indexed queries it saves. mmap only amortizes across a long-lived connection. Two consequences worth knowing before touching this: mapped pages sit outside `cache_size`'s bound and count toward process RSS, and I/O errors against a mapped file surface as an uncatchable SIGBUS rather than a `SQLiteException` — both are being measured in ADFA-5136. +None of them enable SQLite's memory-mapped IO. It was implemented and benchmarked under ADFA-4979, then removed: ADFA-5136 measured it on an arm64 device across small random page reads, large PDFs, and a sustained 3000-page walk, at both 1 KB and 2 KB page sizes, and found no read-time improvement in any configuration — response time is dominated by Brotli decode and Pebble rendering, not by `read()`. Mapping the whole file did cost about 100 MB of resident mapped pages during a single walk touching roughly a tenth of the corpus, since mapped pages sit outside `cache_size`'s bound. Don't re-add it without a measurement that contradicts those numbers; if it ever returns, cap the request rather than mapping the whole file, and note that I/O errors against a mapped file surface as an uncatchable SIGBUS rather than a `SQLiteException`. - **`app/.../localWebServer/WebServer.kt`** — serves Tier 3. On each `GET`, runs: diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt index 96df7491dd..5254603145 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt @@ -86,12 +86,6 @@ object TooltipManager { try { SQLiteDatabase.openDatabase(dbPath, null, SQLiteDatabase.OPEN_READONLY).use { database -> - // Deliberately no SqliteMmapConfigurator here (ADFA-4979): this path opens - // and closes a connection per tooltip, so mmap'ing the whole ~200 MB file - // would charge an mmap()/munmap() and a PRAGMA round trip to replace two - // small indexed queries' worth of read(). mmap only pays off on a - // long-lived connection like WebServer's. - val lastChange = try { DatabaseVersionResolver.resolveDatabaseVersion(database) } catch (e: Exception) {