perf(desktop): coalesce read state localStorage persistence - #5591
perf(desktop): coalesce read state localStorage persistence#5591wesbillman wants to merge 3 commits into
Conversation
ReadStateManager.persistLocalState() serialized and rewrote all three read-state localStorage blobs synchronously on every context advance (~450K chars / 1,643 contexts on a real profile), producing ~880KB of sqlite WAL growth per 30s at idle. Local persistence is now coalesced behind a 1s trailing-edge timer; pending state flushes synchronously on pagehide, hidden visibilitychange, manager destroy, and before each relay publish. Hydration persists immediately as before. Publish debounce (5s), merge logic, and blob formats are unchanged. Accepted residual: a hard kill (not webview teardown) inside the 1s window can lose up to 1s of local read-state advances; relay max-merge bounds the effect to a message flickering back unread. Discussion: Buzz channel time-based-localstorage-eviction, thread 9e54016a4e0ab1f9fe0e7d808cf083e0c129985ce868c4e46a95b6f9f44d34f3. Co-authored-by: Meeseeks <2e96988f190ed1bd3c568760103aa4cadb2bc6195b832e252c984392c89039bd@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf. GitHub will not allow this account to submit CHANGES_REQUESTED on its own PR, so this is a blocking review comment instead. The coalescing direction is sound, but the implementation does not preserve the PR's stated <=1 second data-loss bound: it is an unbounded trailing-edge debounce under sustained activity. Please switch to a bounded throttle/max-wait and add a time-aware regression test.
Address review on #5591: the 1s local-persist timer was a pure trailing-edge debounce, so sustained context advances (<1s apart) postponed the write indefinitely and the documented <=1s loss bound did not hold. The timer is now a bounded max-wait throttle: the first dirty advance opens one 1s window, later advances reuse it, and the write lands no later than 1s after the first unsaved advance. A new time-aware fake-clock test advances every 200ms for 3.2s and proves one three-blob persist per window. Flush triggers, hydration, publish debounce, merge logic, and blob formats are unchanged. Discussion: Buzz channel time-based-localstorage-eviction, thread 9e54016a4e0ab1f9fe0e7d808cf083e0c129985ce868c4e46a95b6f9f44d34f3. Co-authored-by: Meeseeks <2e96988f190ed1bd3c568760103aa4cadb2bc6195b832e252c984392c89039bd@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
ca9c854 to
cb1b0c0
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
The max-wait persistence logic itself is sound, but lifecycle cleanup is incomplete: a destroyed manager can still arm a local-persist timer after an in-flight fetch resolves and later overwrite the active manager's state.
Prevent delayed relay fetches and other post-destroy entry points from mutating or persisting stale manager state. Cover replacement-manager local state against an in-flight initialization resolving after teardown. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Addressed the remaining lifecycle review in 0d7d5ee. Destroyed managers now reject new local advances/persist scheduling, and async merge paths bail after awaited parsing if teardown occurred. Added a deferred-initialize-fetch regression covering destroy → replacement manager persist → stale fetch resolution; the replacement\u2019s newer local state is preserved and no stale timer is armed. Validation on exact pushed head
|
Follow-on to #5453/#5454's localStorage work — found while investigating app-slowness reports on a real profile.
Problem
ReadStateManager.persistLocalState()serialized and rewrote all three read-state localStorage blobs (buzz.channel-read-state.v2,.publishable.v1,.source-created-at.v1) synchronously on every context advance. On a real profile (1,643 contexts, ~450K chars across the three blobs) this produced ~880KB of localStorage sqlite WAL growth per 30 seconds at idle, with writes every ~5s — steady main-thread serialization + sync IPC for no user-visible benefit. Observed WAL size on the affected profile: 94–114MB.Fix
writeStoredReadState(one write per blob).pagehide, hiddenvisibilitychange,destroy(), and before each relay publish — disk is current before any relay event goes out.DEBOUNCE_MSrenamed toPUBLISH_DEBOUNCE_MSonly).Accepted residual
A hard kill (SIGKILL/power loss — not webview teardown) inside the 1s window loses ≤1s of local read-state advances; relay max-merge bounds the effect to a message flickering back unread. On the record per review.
Validation
readStateManager.test.mjs: fake-timer/mock-storage coverage — exactly one 3-blob write per burst (zero before the timer fires), hidden-flush cancels the timer and persists, hydrate persists immediately, pre-publish flush. Suite 26/26.371a02cf(commit metadata rewritten afterward for attribution; tree identical) — all sixpersistLocalStatecall sites traced, lifecycle/leak checks (StrictMode remount, pubkey change), no external readers of the blob keys.