From a040aa522c31f5563e439d4030643b7118f4d758 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Mon, 20 Jul 2026 10:52:05 -0400 Subject: [PATCH] fix(perf): keep favicon and settings disk reads off the main thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two StrictMode disk violations on startup and on IPC, fixed differently because they are different problems. **Favicon storage was writing on the IPC handler thread**, i.e. the main looper: `BrowserIconRegistry.record` did `File.writeBytes` inline, and `init` scanned the directory with `listFiles`. Both now run on an IO scope. The directory is still published synchronously so `iconModelFor` and `record` work immediately — only the scan and the write are deferred. `keys` updates after the bytes are actually on disk, so a reader is never told an icon exists before the file backing it does, and until the scan lands an icon renders its placeholder for a frame and then recomposes, which is what the StateFlow is for. **The settings read was left synchronous on purpose, and stays that way.** `notificationServiceEnabled` reads SharedPreferences inside a lazy initialiser; the surrounding comments record why it is not hydrated asynchronously — an async hydrate reopens a window where a late disk read clobbers a user's toggle. Converting it would have looked like a StrictMode cleanup while resurrecting a settings-corruption bug. Instead the prefs file is warmed on an IO thread at startup, so the first synchronous read hits SharedPreferences' in-memory cache. Best-effort by design: if a main-thread reader wins the race it pays the disk hit once, exactly as before, and correctness is unchanged either way. Verified on device: `notificationServiceEnabled` StrictMode hits went from 9 to 0 on a cold start, no icon storage failures, 884 ms cold start, no crashes — and both pinned web-app icons (ditto.pub and Brainstorm) still render, which is the thing making the write async could plausibly have broken. Co-Authored-By: Claude Opus 4.8 --- .../com/vitorpamplona/amethyst/Amethyst.kt | 7 ++++ .../amethyst/LocalPreferences.kt | 13 ++++++ .../amethyst/favorites/BrowserIconRegistry.kt | 42 +++++++++++++++---- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/Amethyst.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/Amethyst.kt index c615047be8..2e9b8fb209 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/Amethyst.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/Amethyst.kt @@ -32,6 +32,9 @@ import com.vitorpamplona.amethyst.service.nests.AppForegroundRecycleHook import com.vitorpamplona.amethyst.ui.screen.loggedIn.embed.EmbeddedTabHost import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.LogLevel +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch import java.io.File /** @@ -95,6 +98,10 @@ class Amethyst : Application() { // Index device-local captured favicons (main process only; decorates favorites + suggestions). BrowserIconRegistry.init(this) + // Warm the global-settings prefs off-main so the first (deliberately synchronous) read of + // them does not hit disk on the main thread. See LocalPreferences.warmGlobalSettings. + CoroutineScope(Dispatchers.IO).launch { LocalPreferences.warmGlobalSettings() } + // Hydrate the per-web-client Tor routing preferences so a site opted out of Tor (some reject Tor // exits) starts on the open web without first flashing a failed Tor load. WebAppNetworkRegistry.init(this) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt index 8c5c6f9480..9603f78862 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt @@ -236,6 +236,19 @@ object LocalPreferences { // the source of truth, so there is no async hydrate that could clobber a user toggle. private fun globalSettingsPrefs(): SharedPreferences = Amethyst.instance.appContext.getSharedPreferences("amethyst_global_settings", Context.MODE_PRIVATE) + /** + * Loads the global-settings prefs file into SharedPreferences' in-memory cache, off the main + * thread, so the first synchronous read below hits memory rather than disk. + * + * The read itself is deliberately synchronous — see [setNotificationServiceEnabled]: an async + * hydrate reintroduces a window where a late disk read clobbers a user's toggle. So this warms + * the cache instead of deferring the read. Best-effort: if a main-thread reader wins the race it + * simply pays the disk hit once, exactly as before. + */ + fun warmGlobalSettings() { + globalSettingsPrefs().getBoolean(PrefKeys.NOTIFICATION_SERVICE_ENABLED, true) + } + private val notificationServiceEnabled: MutableStateFlow by lazy { MutableStateFlow(globalSettingsPrefs().getBoolean(PrefKeys.NOTIFICATION_SERVICE_ENABLED, true)) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserIconRegistry.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserIconRegistry.kt index 1623d160d7..414e44e371 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserIconRegistry.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserIconRegistry.kt @@ -22,10 +22,14 @@ package com.vitorpamplona.amethyst.favorites import android.content.Context import android.util.Log +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.update +import kotlinx.coroutines.launch import java.io.File /** @@ -50,12 +54,28 @@ object BrowserIconRegistry { @Volatile private var iconDir: File? = null - /** Binds the app context and indexes already-stored icons. Idempotent. */ + // Disk work runs here, never on the caller's thread. Both entry points are reached from threads + // that must not block: init() from app startup and record() from the broker's IPC handler, which + // is the main looper — StrictMode flagged the write, and a slow filesystem would have stalled the + // UI while a favicon was saved. + private val io = CoroutineScope(SupervisorJob() + Dispatchers.IO) + + /** + * Binds the app context and indexes already-stored icons. Idempotent. + * + * [iconDir] is published synchronously so [iconModelFor] and [record] work immediately; only the + * directory scan is deferred. Until it lands [keys] is empty, so an icon simply renders its + * placeholder for one frame and then recomposes — [keys] is a StateFlow precisely so that arrival + * drives recomposition. + */ fun init(context: Context) { if (iconDir != null) return - val dir = File(context.applicationContext.filesDir, DIR).apply { mkdirs() } + val dir = File(context.applicationContext.filesDir, DIR) iconDir = dir - _keys.value = dir.listFiles()?.mapNotNull { it.name.removeSuffix(PNG).takeIf { n -> n.isNotBlank() } }?.toSet() ?: emptySet() + io.launch { + dir.mkdirs() + _keys.value = dir.listFiles()?.mapNotNull { it.name.removeSuffix(PNG).takeIf { n -> n.isNotBlank() } }?.toSet() ?: emptySet() + } } /** Persists [bytes] as the favicon for [host] and marks it available. Called from the broker on IPC. */ @@ -66,11 +86,17 @@ object BrowserIconRegistry { val dir = iconDir ?: return if (host.isBlank() || bytes.isEmpty()) return val key = sanitize(host) - try { - File(dir, key + PNG).writeBytes(bytes) - _keys.update { it + key } - } catch (e: Exception) { - Log.w("BrowserIconRegistry", "Failed to store favicon for $host", e) + // Fire-and-forget: a favicon is a decoration, and the IPC handler must not wait on disk. + // [keys] updates only after the bytes are actually on disk, so a reader can never be told an + // icon exists before the file backing it does. + io.launch { + try { + dir.mkdirs() + File(dir, key + PNG).writeBytes(bytes) + _keys.update { it + key } + } catch (e: Exception) { + Log.w("BrowserIconRegistry", "Failed to store favicon for $host", e) + } } }