Skip to content

perf(web): a thread loaded from the cache holds each item once - #31483

Open
Pyseph wants to merge 4 commits into
pingdotgg:mainfrom
Pyseph:perf/thread-cache-items-once
Open

Pyseph wants to merge 4 commits into
pingdotgg:mainfrom
Pyseph:perf/thread-cache-items-once

Conversation

@Pyseph

@Pyseph Pyseph commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

After a restart, 12 threads of 30 MB each hold 595 MB of JS heap instead of 948 MB, and their saved cache takes 361 MB instead of 717 MB. A thread of first-open size barely changes.

Problem

A thread projection holds its timeline items in two lists, turnItems and visibleTurnItems. From the server they are one object in both lists, and a message record and its item share one body string (boundedSnapshotProjection, dedupeSnapshotBodies). The web and desktop cache writes both lists in full (apps/web/src/connection/storage.ts), so a thread decoded from IndexedDB holds every item twice and every message body three times. That lasts as long as the thread is in memory, and after an app restart every opened thread takes this path.

How I established it: heap snapshots through CDP after opening 8 threads from IndexedDB. Per thread, the 300 command outputs of the fixture exist as 601 strings with the full record and 301 with this change, and answer bodies as 312 and 112 (gist, snapshot-items-twice.txt).

#24265 fixed the same duplication on disk for mobile (threadCacheEncoding.ts, record version 6). Web and desktop stayed on version 5.

Change

  • apps/web/src/connection/storage.ts uses the shared encodeStoredThreadSnapshot and decodeStoredThreadSnapshot, so web and desktop write the version 6 record too and still read version 5.
  • threadCacheEncoding.ts: decoding now also points each item's body at its record's copy, which version 6 did not do. That reuses dedupeSnapshotBodies and restoreSnapshotBodies, so only equal bodies are shared.
  • A version 5 record gets the same sharing on load. A visible row shares the turnItems object only when the two copies are structurally equal, so a record whose copies differ loads exactly as saved.
  • A version 6 record that could not be compacted gets that sharing too. Compaction needs turnItems to start with the visible rows, and a retained request card followed by newer items breaks that, so such a record is written with both copies.

Source: 46 lines added and 12 removed, plus tests. This PR does not depend on #31464.

Behaviour differences:

  1. An older client reading a version 6 record discards it and fetches the thread again, as older mobile clients do since perf(mobile): compact persisted thread snapshots #24265. Two open tabs of different builds can do this to each other. It costs a refetch and loses nothing.
  2. Mobile keeps its format and gains the body sharing on decode.

Not changed: message bodies are still on disk twice, in the record and in the item, because OrchestrationV2TurnItemJson drops the textOmitted marker, so blanking them would lose the text. They are shared after decoding. A record that cannot be compacted is the same size on disk as today.

Pending questions (#26435)

The compact record leaves visible items out of turnItems, including a visible question's card, so it matters when #26435's hasMissingPendingQuestionCard looks. It reads the decoded projection, after restoreLocalVisibleTurnItems, so the card is back by then. threads-sync.test.ts runs real records through makeEnvironmentThreadState for each place a card can be:

the card is record result
inside the visible window compact, card left out of turnItems answerable, no refetch
older than the window compact, card kept after the visible rows answerable, no refetch
older than the window, with newer items after it not compacted, both copies answerable, no refetch, items shared
missing compact discarded and fetched again, as #26435 does

Scope and approval

A focused memory fix with no intended visible change, following a maintainer's own mobile change. I think it fits as a small fix; a triaged issue is fine if you prefer one.

Verification

Tests on this branch, on main at 337c5858c3 (gist, tests-cache-only.log):

  • packages/client-runtime/src/platform/threadCacheEncoding.test.ts (new, 5 tests): an item written once, one object per item after decoding, a record that could not be compacted, a version 5 record shared when equal and kept when different. Against main's threadCacheEncoding.ts, the tests for the uncompacted record and the version 5 record fail, with the third pending-question case below (gist, tests-against-main.log). The rest pass there, because version 6 already shared items.
  • apps/web/src/connection/storage.test.ts: "saves each item once and loads it as one object". It fails against main's storage.ts, with the output written twice.
  • threads-sync.test.ts, the "a pending question in a saved record" block above (4 tests). Without the sharing for uncompacted records, the third case and the new encoding test fail.
  • Unchanged and passing: orchestrationCache.test.ts, threadDetail.test.ts, threads-atoms.test.ts, the rest of threads-sync.test.ts, threadHistoryMerge.test.ts (client-runtime 92 tests in all), apps/mobile/src/connection/environment-cache-store.test.ts (8), ProjectionStore.test.ts and threadHistoryPaging.test.ts (79, the server tests fix: keep older pending questions answerable #26435 touched), web ChatView.logic.test.ts and its timeline neighbours (160 with storage.test.ts), ChatView.browser.test.tsx (1).
  • effect-tsgo tsrs --noEmit for packages/client-runtime and apps/web: 0 errors. vp fmt --check and vp lint on the changed files pass, and so did the pre-commit hook (fmt, lint, knip) under Node 26. Web tests ran on Windows with system Chrome and a local vite.config.ts workaround for a path with spaces, which is not part of this change.

Measurements: the whole AppRoot on the in-process fake environment with synthetic threads already in IndexedDB, opened as after a restart, headless Chrome, two forced GCs per reading, Vite dev build. The fake answers a resume with no new snapshot, as a real server does when nothing changed.

Harness: the harness-- files in gist. apply.mjs puts the measurement-only switches and renderer.ram.tsx on a checkout (BRANCH1=1 for this branch), one.sh runs one scenario, matrix.ps1 alternates the arms, agg.mjs builds the tables. They carry placeholders for my paths and expect the folder layout they ran in, so they need small changes to rerun.

Re-check on this branch, on current main, n=3 alternating, R30 at 12 threads (gist, recheck-cache.ndjson):

main this branch
JS heap after opening all 12, MB, median (min) 948.4 (948.4) 594.9 (591.0)
saved cache, MB 717.4 361.2

"main" is this branch with the old record format and no sharing on decode, switched by a measurement-only switch. This branch has neither #31464 nor the closure fix, and the saving is the same as in the table below.

The fuller table below was measured before the rebase, 5 alternating runs per arm, on a base that already had #31464. The "this change" arm also had the two-closure fix from #31484. While every thread in the table is still warm, that fix frees nothing, because the copy it lets go of is still retained.

threads opened, size of each JS heap, before JS heap, this change saved cache
20, W10 (10 turns, about a first-open server window) 248.2 (244.5) 246.8 (241.7) 3.1 -> 2.0 MB
20, W100 (100 turns as the wire sends them) 276.8 (276.3) 260.6 (260.4) 31.5 -> 19.7 MB
12, W500 (500 turns as the wire sends them) 336.6 (336.0) 288.5 (283.1) 95.1 -> 59.3 MB
12, R30 (30 MB of large bodies each) 949.7 (948.2) 594.1 (594.0) 717.4 -> 361.2 MB

MB, median (min), n=5 (gist, heap-seeded.ndjson, tables-heap.txt). The shell-only phase varies between about 192 and 200 MB across runs, so the W10 row is within noise.

First paint does not change: opening from the cache takes 582.5 -> 589.0 ms at W100 and 863.7 -> 841.9 ms at R30 (medians, n=30 and n=20, exclusive timing lock, before the rebase; gist, cold.ndjson). I expected decoding half the data to show here, and it does not.

A record above 133 MB cannot be saved at all: IndexedDB fails it with "The serialized keys and/or value are too large (size=215123491 bytes, max=133169152 bytes)" (gist, heap-seeded.ndjson). The compact record doubles how large a thread fits.

Not checked

The real server and wire path, a production build, Safari or Firefox storage, and mobile on a device. The R30 sizes stand in for large text, because the real wire omits command output and diffs (projectTurnItemForWire). How often real threads fall back to an uncompacted record: the sharing covers them either way, but they get no saving on disk.

Claude Opus 5.5 in T3 Code (Claude Code harness).

Web and desktop now write the compact record mobile already uses, so a saved thread takes about half the space. Decoding also shares equal item copies and message bodies again, including in records saved before this change and in records that could not be compacted, so a thread from the cache costs what a thread from the server costs.

A pending question's card survives the round trip whether it sits inside the visible window, older than it, or older with newer items after it, and a record with no card is still fetched again.
@github-actions github-actions Bot added vouch:unvouched Author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 5, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 437259b

Macroscope's review found this PR approvable — This reduces cached-thread memory duplication by using the shared compact codec and sharing equivalent decoded items and bodies. The effects are confined to local caches, with compatibility checks and invalid-cache discard retained.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Thread cache decoding now restores matching references between visible rows and turn items, and restores shared snapshot bodies. Web storage uses the platform decoder and compact snapshot encoding.

Changes: Thread cache persistence

Layer / File(s) Summary
Restore shared references during decoding
packages/client-runtime/src/platform/threadCacheEncoding.ts, packages/client-runtime/src/platform/threadCacheEncoding.test.ts
Full and compact decoding restore matching item and body references. Tests cover retained local visible items, matching copies, and copies with different command outputs.
Use compact snapshots in web storage
apps/web/src/connection/storage.ts
Web storage imports the platform snapshot decoder, removes its local snapshot schema and decoder, and encodes snapshots in compact format.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to a2388

Cached threads now load with fewer duplicate items in memory, and older version 5 records still load. No concrete problem was found, so the change looks ready to merge.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "perf(web): a thread loaded from the cache holds each item once" matches the main change. The PR makes web and desktop cache decoding share item objects and message bodies, so a thread resto…
Description check Passed The description follows the repository template. It has a Problem section that gives the duplication cause and measurements, and a Change section that lists the storage and decode edits. It has a Scop…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "perf(web): a thread loaded from the cache holds each item once" matches the main change. The PR makes web and desktop cache decoding share item objects and message bodies, so a thread restored from IndexedDB holds each item once. It is a conventional commit title in plain language, as the repository template asks.

Full details: Description check

Explanation

The description follows the repository template. It has a Problem section that gives the duplication cause and measurements, and a Change section that lists the storage and decode edits. It has a Scope section that explains why the change is a bug fix and not a feature. It has a Verification section that lists tests, measurements, and unchecked items. It names the model used, as the template asks. The Problem section states the bug in plain terms. The Scope section links no issue, but it gives a reason for omitting one, which the template allows for small fixes.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 584b414 and a2388a4.

Files selected for processing: 1

  • packages/client-runtime/src/platform/threadCacheEncoding.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Merged current main (no rebase) and resolved the conflicts with #33503, which replaced encodeStoredThreadSnapshot with the chunked makeStoredThreadSnapshotEncoder and added a 4 MB record cap.

  • storage.ts: web and desktop now use main's encoder with format: "compact" instead of "full", and load through the shared decodeStoredThreadSnapshot. That keeps this PR's intent on top of main's write ordering and cap.
  • threadCacheEncoding.ts: kept main's encoder and this PR's decode-side sharing (shareFullRecordItems, shareBodiesWithRecords, the version 5 path).
  • threadCacheEncoding.test.ts (both sides added it): kept main's cases and moved this PR's cases into their own describe, encoding through the compact encoder. threads-sync.test.ts does the same.

Effect of the cap on the numbers in the description: a record over 4 MB is no longer cached on main, so the R30 and W500 rows no longer apply as measured. The compact record still roughly halves the size of what is saved, so more threads fit under the cap.

Checked: vp run check:changed (format, lint, knip, typecheck of 7 packages, the 4 changed test files), plus orchestrationCache.test.ts and mobile environment-cache-store.test.ts. Not re-measured: heap and cache size on the merged branch.

@juliusmarminge juliusmarminge added the takeover:in-progress A takeover lead is working on this PR label Nov 15, 2026
juliusmarminge and others added 2 commits November 15, 2026 02:11
…out decode sharing

The storage and thread-sync cases repeated the compact round-trip one layer
up; mobile already exercised that path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge

Copy link
Copy Markdown
Member

The automated pipeline stopped on this pull request and will not merge it:

the capture step could not show the problem on the base commit, so there is nothing to prove fixed.

Seeded eight threads per commit through thread start/send using devReplay command-output, twenty turns each, with deterministic unique 50,000-character user bodies. Opened all eight threads and Settings to populate each browser's cache. Alternated base and head for five browser restarts, opening all eight cached threads through sidebar buttons. Measured click-event to the final assistant paragraph appearing inside the viewport, then requested two garbage collections, recorded Runtime.getHeapUsage, and counted heap-snapshot body strings with self_size >= 40,000 bytes. Repeated five fresh-profile server-control runs per commit. Compared message-content hashes, item counts, and local/visible object identity. Recorded an additional identical navigation pass per commit, excluded from statistics; stills required two byte-identical screenshots 500 ms apart.

It stays open for a person to finish or close.

@juliusmarminge juliusmarminge added takeover:blocked Takeover blocked; reason in the PR body and removed takeover:in-progress A takeover lead is working on this PR labels Nov 15, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). takeover:blocked Takeover blocked; reason in the PR body vouch:unvouched Author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants