docs(#917): plan of record for the storage save path (A then B)
Measured diagnosis: StoredBlobs holds one JSON blob per key; saving one message decodes, rebuilds and re-encodes the whole channel history. 134 ms per message at 9230 records against 0.002 ms for an append, so 67000x more work than needed, scaling ~14 ms per 1k records. Epic chain: A moves the whole read-modify-write into one Isolate.run (relief, no data risk); B normalises to one row per message (removes the cost); C integration-tests both on hardware. Carries the cannot-tell branches table and grant terms with the basis for every budget. Also records that #335 did not fix this and that drift already runs SQL off the UI isolate. Refs #334 Agent: TealGlen (session c61ce804) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>feat/917-storage-save-path
diffwireauto
4 weeks ago
committed by
Benjamin Wiechel
parent
3055aab931
commit
6ccc4e4e96
@ -0,0 +1,118 @@
|
||||
# Feature PLAN: storage save path: remove O(history) work per message
|
||||
|
||||
| | |
|
||||
|---|---|
|
||||
| **Feature** | [#917](https://github.com/OffbandMesh/meshcore-client/issues/917) · Citadel `meshcore-open-1hf` |
|
||||
| **Serves** | No parent Initiative; a standalone Feature. Resolves the cause behind [#334](https://github.com/OffbandMesh/meshcore-client/issues/334), the originating Epic, which stays open until this Feature's integration test proves the ANRs are gone. |
|
||||
| **Status** | Draft. Approved when this PR merges. |
|
||||
| **Owner** | @Strycher |
|
||||
| **Last updated** | 2026-10-19 |
|
||||
|
||||
## At a glance
|
||||
- **What:** stop re-encoding a channel's entire message history every time one message arrives. First move the work off the UI isolate (Epic A), then delete the work by storing one row per message (Epic B).
|
||||
- **Why:** 25 ANR clusters on Play since September, all `Input dispatching timed out`. The cost grows linearly with history, so it worsens for exactly the users who use the app most. ANR rate is also the Android vitals signal Google uses to suppress an app in Play search, and Offband is currently absent from search for "meshcore" while ranking first for "offband".
|
||||
- **How:** entirely client-side. No firmware change. Three Epics in order: A isolate offload, B normalise to rows with a migration, C hardware integration test.
|
||||
- **Done when:** receiving a message costs the same at 50,000 messages of history as at 50, no message is lost in the migration, and the owner confirms on hardware on Android and Windows.
|
||||
|
||||
## 1. Diagnosis
|
||||
|
||||
All figures measured 2026-10-19 against the owner's live database and the real Dart VM. Full evidence: [#334 comment](https://github.com/OffbandMesh/meshcore-client/issues/334#issuecomment-6262727767).
|
||||
|
||||
| # | Finding | Evidence |
|
||||
|---|---|---|
|
||||
| 1 | `StoredBlobs` is `key TEXT PRIMARY KEY, value TEXT`: one JSON blob per store key | `lib/storage/drift/offband_database.dart:347` |
|
||||
| 2 | Live DB is 41.03 MB: 151 rows, 39.21 MB of JSON. Largest row is 5.38 MB holding **9,230 messages** in one channel | `select key, length(value) from stored_blobs` on a copy of the live DB |
|
||||
| 3 | Saving one message decodes the whole blob, rebuilds every record, re-encodes the whole blob | `channel_message_store.dart:80-104`, `message_store.dart:33-57` |
|
||||
| 4 | That costs **134 ms** on a desktop CPU: decode 50, rebuild 38, encode 51 | Dart benchmark, real 5.39 MB payload, median of 5 |
|
||||
| 5 | Cost scales linearly: ~14 ms per 1,000 records of history, per message | 500 recs 4 ms, 1k 12 ms, 2.5k 26 ms, 5k 83 ms, 9,230 130 ms |
|
||||
| 6 | Appending one record instead costs **0.002 ms**, so the current path does **67,000x** more work per message | Same benchmark, single-record encode, median of 200 |
|
||||
| 7 | Bursts serialize: `BlobStore.synchronized` chains futures per key, so N messages each pay full cost | `blob_store.dart:58-62` |
|
||||
| 8 | 5 messages on a 10x slower device is **6.7 s**, past the 5 s ANR watchdog | Arithmetic on finding 4. **Not reproduced on a device**; see Risks |
|
||||
| 9 | Each cycle allocates ~11 MB of strings plus 9,230 maps, which is why traces are dominated by GC frames | `ScavengerVisitor`, `MarkingVisitor`, `FreeList::TryAllocate`, `String::ConcatAllRange` in 25 clusters |
|
||||
| 10 | **#335 did not fix this.** It moved the bytes into SQLite; the JSON stayed. ANRs span version codes 65, 66 and 67, all post-drift | Play Reporting API clusters by `lastAppVersion` |
|
||||
| 11 | **SQLite was never on the UI isolate.** `drift_flutter` 0.3.1 hosts connections in a background isolate; web uses `drift_worker.js` | `drift_flutter-0.3.1/lib/src/connect.dart` |
|
||||
| 12 | Only 9 distinct users affected, 1 per cluster, consistent with cost scaling by history | `distinctUsers` on every cluster |
|
||||
|
||||
**Root cause:** O(entire history) work per message, serialized across a burst, with GC pressure compounding it.
|
||||
|
||||
## 2. Decisions
|
||||
|
||||
| Decision | Choice | Why |
|
||||
|---|---|---|
|
||||
| Fix the isolate first or normalise first | Isolate first | Stops the ANRs and the Play penalty with zero data risk, and buys time to do the migration carefully |
|
||||
| Is the isolate offload sufficient | No | It relocates ~11 MB of allocation per message rather than removing it. Total work unchanged |
|
||||
| Isolate granularity | One `Isolate.run` around the **whole** read-modify-write | Only two strings cross the boundary. Wrapping encode and decode separately would ship the object graph across twice |
|
||||
| Where the helper lives | `BlobStore` | One place, so stores change call shape instead of each growing its own isolate wrapper |
|
||||
| Migration safety | Additive. Blob retained until per-key readback matches | #355 cost 574 messages after the last storage migration shipped in a day |
|
||||
| Retype #334 | No | #917 is the Feature and carries this plan. #334 stays as the originating Epic and closes on the integration test. Less board churn |
|
||||
|
||||
## 3. Epic chain
|
||||
|
||||
### Epic A: blob read-modify-write off the UI isolate
|
||||
|
||||
One helper in `lib/storage/drift/blob_store.dart` that runs the entire read, decode, merge, encode, write inside a single `Isolate.run`, taking the raw JSON string in and returning the merged string out. Then call-shape changes in `channel_message_store.dart`, `message_store.dart` and `lib/services/storage_service.dart` (11 JSON call sites). Smaller stores follow only if measurement says they matter.
|
||||
|
||||
**Epic verification:** the benchmark harness shows main-isolate block time at or near zero against the real 5.39 MB payload, and the owner confirms a burst no longer freezes the UI.
|
||||
|
||||
### Epic B: normalise messages into rows
|
||||
|
||||
A real drift table beside `StoredBlobs`, one row per message. A save becomes an INSERT; reads page. Both message stores' read and write paths rewritten. An additive migration that never deletes a blob until readback proves the per-key count matches.
|
||||
|
||||
**Epic verification:** per-key message counts identical before and after on a copy of the real 41 MB database, and the benchmark shows per-message cost flat across history length.
|
||||
|
||||
### Epic C: Integration Testing
|
||||
|
||||
A and B together on real hardware, Android and Windows. Carries the test that validates #334 is dead, which gates closing it.
|
||||
|
||||
## 4. Interfaces
|
||||
|
||||
No protocol, wire-format or firmware change. No new dependency: `Isolate.run` is `dart:isolate`, and drift is already present. Storage schema change is internal and additive.
|
||||
|
||||
## 5. Secrets
|
||||
|
||||
None. This Feature touches no credential, key or external service.
|
||||
|
||||
## 6. Cannot-tell branches (standards#677)
|
||||
|
||||
| Branch | Behaviour |
|
||||
|---|---|
|
||||
| `Isolate.run` throws | **Stop.** Surface the error, leave the blob untouched. Never fall back to a silent main-thread encode |
|
||||
| Blob is corrupt or not a JSON list | **Stop.** Do not overwrite. Log the key and byte length, keep the original |
|
||||
| Migration readback count mismatch for a key | **Stop.** Keep the blob, delete nothing, surface loudly |
|
||||
| Row insert fails mid-migration | **Stop** inside a transaction, roll back, keep the blob |
|
||||
| Payload too large to send across the isolate | **Ask the owner.** Needs a measured threshold first; no guessed limit |
|
||||
|
||||
No fail-open paths. No branch proceeds on a condition it cannot confirm.
|
||||
|
||||
## 7. Risks
|
||||
|
||||
| Risk | Mitigation |
|
||||
|---|---|
|
||||
| Migration loses messages | Additive, blob retained, per-key readback gate, test against a copy of the real 41 MB DB |
|
||||
| The burst multiplier is arithmetic, not observed | Epic A's verification includes an owner device check. If a burst does not reproduce, the ANR cause is re-opened rather than assumed fixed |
|
||||
| Isolate transfer cost exceeds the saving on small blobs | Measure before converting the smaller stores. Epic A converts the three big ones first |
|
||||
| Play traces stay unresolvable | Out of scope here. Release builds strip Dart symbols, so no frame from this codebase appears in any trace. Retaining native debug symbols is a separate pipeline issue |
|
||||
|
||||
## 8. Owner steps
|
||||
|
||||
One, at the end of Epic A, and one at the end of Epic C: run the build on hardware and confirm the UI no longer freezes under a burst. Everything else, including the migration test, runs without him.
|
||||
|
||||
## grant-terms
|
||||
|
||||
```
|
||||
feature: Storage - remove O(history) work from the message save path
|
||||
epic_chain:
|
||||
- A: blob read-modify-write off the UI isolate
|
||||
- B: normalise messages into rows (migration)
|
||||
- C: Integration Testing (A + B on hardware, both platforms)
|
||||
verification:
|
||||
A: benchmark harness shows main-isolate block at or near zero on the real 5.39 MB payload; owner device check on burst
|
||||
B: per-key message counts identical before and after on a copy of the real 41 MB database; per-message cost flat across history length
|
||||
C: owner hardware test, Android and Windows
|
||||
merges: 3
|
||||
flash: 0
|
||||
reset: null
|
||||
duration: 72h
|
||||
```
|
||||
|
||||
**Basis for each budget.** Merges is 3: the chain is three Epics and each merges once, with no per-task PR. Flash is 0: nothing here touches a radio. Reset is null: not applicable. Duration is 72h because Epic B carries a migration test against a 41 MB database, and a grant that outlasts a careful run beats one expiring mid-chain and needing a re-mint.
|
||||
Loading…
Reference in new issue