Skip to content

perf(store): cheaper detail storage and cached Detail.get()#2283

Merged
requilence merged 2 commits into
developfrom
perf/detail-store
Jul 7, 2026
Merged

perf(store): cheaper detail storage and cached Detail.get()#2283
requilence merged 2 commits into
developfrom
perf/detail-store

Conversation

@requilence

Copy link
Copy Markdown
Contributor

What

Follow-up to #2282. Tracing a subscription record end-to-end showed the expensive copies live downstream of protobuf decode, in S.Detail:

  • Every relation of every record was stored as a {relationKey, value, isDeleted} wrapper with its own makeObservable call (2 boxed observables each) and deep-wrapped values (arrays/objects proxied) — ~1,500 MobX-instrumented objects for a 50-record × 30-relation page.
  • S.Detail.get() rebuilt the plain object and re-ran the layout mapper (mapCommon + mapType/mapSpaceView/… coercions, translate() calls) from scratch on every call from ~246 call sites, at render rate (S.Record.getRecords alone calls it once per row per render). It also iterated all stored relations even when 3 keys were requested.

Changes

Storage — one shallow observable.map per object holding raw values:

  • One map entry per relation instead of wrapper object + 2 boxes + deep value proxies. Per-key reactivity granularity is unchanged (MobX map entries are tracked per key).
  • Drops the dead Detail.isDeleted flag — nothing anywhere ever set it to true.

get() — requested-keys iteration + per-args computed cache:

  • With withKeys, iterates the requested key set: O(requested) instead of O(total stored), and reactive callers subscribe to exactly the entries they use. Absent keys are tracked via has(), so an object/relation appearing later invalidates correctly (previously new-key adds relied on map-level key tracking).
  • In reactive contexts (observer renders, autoruns, computeds) results are cached in per-(rootId, id, args) computeds that self-evict via onBecomeUnobserved — cached between renders, recomputed only when an underlying observable actually changes. Outside reactive contexts it builds directly (a computed wouldn't cache there anyway, and no cache entries leak).
  • Callers receive a shallow copy — the old fresh-object-per-call top-level mutation semantics are preserved, and caller mutations can't corrupt the cache.
  • translate() and date formatting read observable S.Common state (config → computed, showRelativeDates/dateFormat → computed), so language/format switches invalidate cached objects automatically.

Numbers (differential harness, old implementation vs new, 2,000 records × 34 relations)

old new
set() write 118ms 11ms
heap growth on that write ~118MB ~0MB
60 re-renders of a 100-row list reading 5 keys 184ms 80ms
6,000 get() calls inside a live reaction 24ms 4.6ms

Verification

  • Differential test vs the old implementation: output equivalence across layouts (page/note/participant), withKeys/forceKeys/skipLayoutFormat combos, amend/clear updates, key/object deletes, getKeys.
  • Reactivity semantics: relevant-key updates trigger subscribers, unrelated-key updates don't (same as before), keySet-included key additions trigger, other-object updates don't, cache is shared per args and evicts to zero after disposal, non-reactive gets create no cache entries, _empty_ → populated invalidates.
  • Typecheck, lint pass; vitest shows the identical 101 pre-existing failures as clean develop (verified baseline), zero new.

Behavioral notes (intentional)

  • Result property order changes from store-insertion order (arbitrary middleware response order) to requested-keys-then-defaults order. Key sets and values are identical.
  • Stored values are no longer deep MobX proxies — get() returns plain arrays/objects. All legitimate updates flow through S.Detail.set/update (middleware events), which replace values and notify; in-place mutation of a stored value from a component was never a supported pattern.

S.Detail was the biggest post-decode amplifier of subscription data:
every relation of every record got its own makeObservable-instrumented
{relationKey, value, isDeleted} wrapper with deep-wrapped values (~1500
instrumented objects for a 50-record page), and get() rebuilt + re-ran
the layout mapper from scratch on every call from ~246 call sites at
render rate.

- Store raw values in a shallow observable map per object (one map
  entry per relation instead of a wrapper object + two boxed
  observables + deep value proxies). Per-key reactivity granularity is
  preserved by the map entries. The dead Detail.isDeleted flag (never
  set to true anywhere) is dropped.
- get() with withKeys now iterates the requested key set instead of all
  stored relations: O(requested) instead of O(total), and reactive
  callers subscribe to exactly the entries they use (absent keys are
  tracked via has(), so objects appearing later invalidate correctly).
- In reactive contexts get() results are cached in per-args computeds
  that self-evict via onBecomeUnobserved; outside reactions it builds
  directly, same as before (computeds would not cache there anyway).
  Callers get a shallow copy, preserving the old fresh-object-per-call
  mutation semantics. translate()/date formatting read observable
  S.Common state, so language/format changes invalidate cached objects.

Differential test vs the old implementation (2000 records x 34
relations): set() 118ms -> 11ms and ~118MB -> ~0MB heap growth;
60 simulated re-renders of a 100-row list reading 5 keys 184ms -> 80ms;
6000 gets inside a live reaction 24ms -> 4.6ms. Output equivalence
verified across layouts/args (result key order changes from store
insertion order to requested-keys order); fine-grained re-render
semantics verified equivalent or better.
Review findings: Number(undefined) stringifies to 'NaN' in the cache
key, so get(root, id, keys) and get(root, id, keys, false) created
duplicate computeds for identical logical args — use (forceKeys ? 1 : 0).
Also add a size bound on getCache: entries created while a suspended
computed evaluates inside a batch never fire onBecomeUnobserved (no
in-repo trigger today, but the failure mode is silent); clearing the
map is safe since live observers keep their computed instances.
@requilence

Copy link
Copy Markdown
Contributor Author

Independent adversarial review completed (differential harness vs the old implementation: 13 layouts × 7 arg combos, 12 write scenarios, in/out of reactive contexts — zero mismatches). Verdict: sound, no blockers/majors. Verified properties:

  • _isComputingDerivation() is true inside mobx-react-lite v4 renders → components hit the cache; false in runInAction/event handlers → direct build, as before.
  • No unobserve/re-observe churn across reaction re-runs; eviction fires on dispose/conditional-dep drop/aborted render; StrictMode/concurrent aborts cause only transient evict+recreate, never staleness.
  • Invalidation completeness: set() root replacement, update() (incl. clear + object-reappears), delete() whole/per-key, clear(), clearAll(), and language/date-format changes all invalidate cached results.
  • Reactivity granularity identical to old in all 12 scenarios (note: unrelated key add/remove still re-runs withKeys readers via the detailMap.size read — same as old behavior).
  • Call-site sweeps (~246 consumers): zero in-place nested mutations of results, zero deep-observable reliance (toJS/isObservable*/observe), zero order-sensitive enumeration of results.
  • Write path: same-scalar/same-reference rewrites don't notify (both versions), fresh-array replaces notify (both), batched dispatcher writes flush once.

Two minor findings fixed in 2144168's follow-up commit:

  1. Number(undefined)'NaN' in the cache key caused duplicate computeds for get(root, id, keys) vs get(root, id, keys, false) — now (forceKeys ? 1 : 0).
  2. Latent leak: entries created while a suspended computed evaluates inside a MobX batch never fire onBecomeUnobserved (no in-repo trigger today — verified every store computed getter; none reaches S.Detail.get) — added a size bound that clears the cache map, which is safe since live observers keep their computed instances.

@requilence requilence merged commit 91d04a6 into develop Jul 7, 2026
3 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant