Repository navigation
Reduce memory use when opening and continuing REPL histories - #997
Conversation
A history that binds captured output as root values repeats that output in every later root close. The retained stream deep-copied every event for every reader and the file was read as one string, so a 508 MB two-byte history reached V8's heap limit on its first submission and a history past the longest string could not be reopened at all (#992). Retained events are now sealed once into frozen graphs that share primitive strings, and every reader gets a fresh array of the same events: readAll() slices, the append observer receives the acknowledged event, and a segment and its physical stream keep one graph. An append's operand is copied before the write suspends. Core and the projector keep detaching what they hand to documents and models, so execution still receives fresh mutable data. Entry files are read through a chunked reader that splits at the newline byte and decodes per line, sharing equal large strings across one read. A history past 536,870,888 UTF-16 code units is refused with a sentence naming the limit. Sidekick prefix binding, measurement, origin checks and the single-file-to-pair upgrade fingerprint the file's own bytes incrementally; the upgrade copies in one strict-UTF-8 pass and reuses the events already retained. The on-disk records, root-close binding maps and pair layout are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sealing treated any frozen plain container as safe to share. Freezing keeps whatever accessors a container was declared with, so a caller's frozen object with one getter stayed a live reading of the caller's own state inside the retained history: after the record was written, changing that state changed what readAll answered while the file kept what it wrote. A container is now shared only when every own property is a plain value, and is copied otherwise, which is what turns a live object into the fixed reading a retained event is. Shared graphs, primitive-string sharing, fresh membership arrays and the order of persistence before observation are unchanged. Reading a history allocated a fresh megabyte buffer for every read, so opening a 500 MB history allocated the whole of it again a megabyte at a time, on top of reading it. The reader owns one buffer and hands out views of it; every consumer reads a chunk at once, and the one that keeps bytes copies them. Across five runs of the same ten-entry journey over a 508 MB history, the compiled REPL's maximum resident size fell from a mean of 3.40 GB to 2.64 GB. A summary of a binding no longer writes the whole value out to keep twenty columns of it: strings are cut to more than any summary can show before the value is written, which leaves every summary exactly as it was. That bounds a per-frame allocation proportional to the value; it did not move the measured peak, because the right-hand pane shows the Sidekick unless a reader selects the bindings.
Shortening a value before writing its summary built the shorter object by
assigning each member into it. For the key `__proto__` that assignment reaches
`Object.prototype`'s inherited setter rather than creating a member, so on Node
and Bun a binding parsed from `{"__proto__":{"x":1}}` lost it and the row read
`held = {}` where it used to read `held = {"__proto__":{"x":1…`. A history can
hold such a member: `JSON.parse` makes one an own property, Core's parser keeps
it, and the model's `detach()` defines rather than assigns it for this reason.
The shorter object now defines its members the same way, which keeps every own
member and its order. Bounded string preparation is unchanged, and so is every
summary the previous implementation produced.
H6 gains that one parsed value beside its existing cases. With the assignment
restored it fails under Node and Bun with exactly the row the review
reproduced; it passes either way under Deno, which keeps `__proto__` as an own
property on assignment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const recorded = clone(event); | ||
| // Sealed before the write suspends, so what the caller does with its own | ||
| // object afterwards reaches neither this range nor the physical stream. | ||
| // Both keep this one graph: it is frozen, so neither side can edit it. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // Both keep this one graph: it is frozen, so neither side can edit it. |
| let start = 0; | ||
| for (let end = chunk.indexOf(NEWLINE); end >= 0; end = chunk.indexOf(NEWLINE, start)) { | ||
| // Not streamed, so a multi-byte sequence the line left unfinished is | ||
| // settled here rather than carried into the next line. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // settled here rather than carried into the next line. |
| function sealResult(result: Result, share: StringSharing | undefined): Result { | ||
| if (result.status === "ok") { | ||
| // Whether `value` is present at all is kept, because an absent value and an | ||
| // undefined one are different records to a reader that asks `in`. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // undefined one are different records to a reader that asks `in`. |
| share: StringSharing | undefined, | ||
| ): EffectDescription { | ||
| // Spread rather than rebuilt, so member order and an own `__proto__` member | ||
| // survive exactly as structured cloning kept them. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // survive exactly as structured cloning kept them. |
PR #997: Reduce memory copies when continuing REPL history12 files, +2790 / -1355 Scope🔴 PR has 4145 lines changed. Split into focused PRs. 🟡 4145 lines changed. PRs under 400 receive more thorough review. StructuralOxlint structural signals:
Slop
Static AnalysisOxlint: 96 diagnostics across 5 files (14 rules) no-shadow (51): packages/cli/tests/fixtures/repl/issue-883/recovery-phase.ts, packages/cli/src/repl/sidekick/history.ts, packages/cli/src/repl/application.ts Correctness
No extraneous code patterns detected. |
The fixture walk in the #992 suite sorted its names with `toSorted`, which is ES2023. `tsconfig.node.json` compiles with the ES2022 library, so the Typecheck step of every `test-node` shard refused the file — `error TS2550: Property 'toSorted' does not exist on type 'string[]'` — and four shards failed there before running a test. Deno's own library has the method, which is why `deno check` and all three runtime suites were green. The names are sorted in place instead, on the array `filter` has just made rather than on anything the walk still holds. What the row reads is unchanged.
| const recorded = clone(event); | ||
| // Sealed before the write suspends, so what the caller does with its own | ||
| // object afterwards reaches neither this range nor the physical stream. | ||
| // Both keep this one graph: it is frozen, so neither side can edit it. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // Both keep this one graph: it is frozen, so neither side can edit it. |
| let start = 0; | ||
| for (let end = chunk.indexOf(NEWLINE); end >= 0; end = chunk.indexOf(NEWLINE, start)) { | ||
| // Not streamed, so a multi-byte sequence the line left unfinished is | ||
| // settled here rather than carried into the next line. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // settled here rather than carried into the next line. |
| share: StringSharing | undefined, | ||
| ): EffectDescription { | ||
| // Spread rather than rebuilt, so member order and an own `__proto__` member | ||
| // survive exactly as structured cloning kept them. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // survive exactly as structured cloning kept them. |
| function sealResult(result: Result, share: StringSharing | undefined): Result { | ||
| if (result.status === "ok") { | ||
| // Whether `value` is present at all is kept, because an absent value and an | ||
| // undefined one are different records to a reader that asks `in`. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // undefined one are different records to a reader that asks `in`. |
| const location = yield* establishPair(root, execution); | ||
| if (prefix.body.length > 0) { | ||
| // One append of the whole snapshot. Every record it holds is already | ||
| // terminated, so the copy is byte-for-byte what the origin's digest covers. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // terminated, so the copy is byte-for-byte what the origin's digest covers. |
| // is what makes the execution refuse to reopen at all. | ||
| yield* writer.hold(); | ||
| // The events this process already retained, shared rather than parsed a | ||
| // second time: the writer is held, so they are the records the file holds. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // second time: the writer is held, so they are the records the file holds. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Fable 5.1 first-green review of 4657c4d
Verdict: no blocking finding. Correctness of the two new modules holds under every case I traced. Two non-blocking findings (F1 ordering, F2 minimalism) and one performance note (P1) are posted inline, each with a concrete recommendation that keeps accepted behavior.
What I checked, read-only, on the exact signed head
packages/cli/src/repl/record-file.ts: one reused buffer; lines split at the newline byte before decoding; each line decoded withstream: false, so a multi-byte sequence split across a chunk boundary completes in the next line's decode;ignoreBOM: truekeeps a per-line decoder from stripping a byte order mark at the head of every line; the limit is counted in UTF-16 code units with newlines, as the old whole-file reader's string length was; the handle is released by anensureregistered before the open and joined behind an in-flight read; an empty file reads as terminated;measureLineshashes to the end of thecountth record exactly and keeps counting. Refusal sentences match the previous reader (H2, H3, H5).packages/cli/src/repl/retained-event.ts: containers copied shallowly and frozen, strings shared by value,reusablerejecting accessors and foreign prototypes. An own__proto__member survives because spread creates an own data property and the later assignment writes to that property rather than the inherited setter.SerializedErroris flat (message,name,stack), so the shallow freeze oferroris complete.- Every
readAll()consumer: core wraps throughretainEvents, the projector detaches, the session pushes onto its own slice. None writes into a retained event, which a frozen graph would now refuse under strict mode. packages/cli/src/repl/sidekick/history.ts: prefix bindings and origin verification hash the file's own bytes; the upgrade decodes strictly so the appended text re-encodes to the hashed bytes; the writer is held through the handoff with release registered before hold.packages/cli/src/repl/application.ts: cutting strings to forty characters before writing a twenty-column summary cannot move a visible character, since a character writes as at least one character and the ellipsis path is taken either way (H6).
Performance claims. The description's numbers (baseline heap exhaustion on the first new entry over a 508.5 MB history; ten entries at 2.34–2.55 GB peak RSS) are measurements of the build at daeb4ee. The source diff from there to this head changes only how the summary cut defines an own __proto__ member, plus one test, so transferring them is sound. The evidence does not claim constant memory, and P1 says what still scales with the history.
Evidence. The PR description; packages/cli/tests/repl-retained-history.test.ts H1–H6 across Deno, Node and Bun; the green check on this head. I ran no tests, builds or benchmarks myself, and I did not re-run CI.
| // The events this process already retained, shared rather than parsed a | ||
| // second time: the writer is held, so they are the records the file holds. | ||
| const events = yield* writer.readAll(); | ||
| const location = yield* establishPair(root, execution); |
There was a problem hiding this comment.
[fable-997-F1] Refused or cancelled upgrades now leave an interrupted pair (non-blocking, correctness/ordering)
establishPair runs before the legacy file has been read. Every refusal copyEntryRecords can raise (unfinished last record, bytes that are not UTF-8 under its strict decoder, a record count other than the retained one) and a cancellation anywhere in the copy now leave v1/<execution>/ holding two empty-or-partial files and no owner record. pairedReplRepository.open then refuses that execution as an interrupted publication and, by design, never falls back to the legacy file. Before this change those three refusals fired in readTextFile / parseEntryRecords before publishPair created anything, so a refused upgrade left the execution openable; cancellation could already strand a pair, but only during the append, and the window now spans the whole read.
Of the three, only the strict-UTF-8 refusal is reachable for a file this process already opened: readRecords decodes leniently (strict unset in journal.ts), so a legacy history with one damaged byte opens, upgrades into a refusal, and cannot be opened afterwards. No row covers it; H4 upgrades valid UTF-8 only.
Recommendation, either of which keeps accepted behavior for every valid history:
- Pass
strict: truewith anundecodablerefusal inreadRecordsas well, so a history that is not UTF-8 refuses at open, before any pair exists, with the sentence the copy already uses. Both readers then agree on what a readable history is. - If lenient opening is wanted, say in the
upgradeExecutioncomment that a refused or cancelled copy leaves an interrupted pair, and add a row that upgrades a non-UTF-8 legacy file and asserts what reopening says.
| readonly origin?: RetainedOrigin; | ||
| }, | ||
| ): Operation<Publication> { | ||
| const location = yield* establishPair(root, execution); |
There was a problem hiding this comment.
[fable-997-F2] publishPair keeps a copy path no caller uses (non-blocking, minimalism)
After the upgrade moved to copyEntryRecords, every caller of publishPair passes { body: "", events: [] }: pairedReplRepository.create and the two call sites in packages/cli/tests/fixtures/repl/issue-883/recovery-phase.ts. The body and origin members, the append branch, and the doc paragraph on why bytes rather than events are copied now describe a path nothing takes; that rationale is true of copyEntryRecords instead.
Recommendation: reduce publishPair(root, execution) to establishPair followed by acknowledgePair(location, execution, [], undefined), drop the prefix parameter, and move the bytes-not-events paragraph onto copyEntryRecords. The two fixture calls lose their third argument and nothing else changes. Same behavior, roughly twenty fewer lines, one fewer way to create a pair.
Same family, smaller: copyEntryRecords passes an infinite limit, so its exceeded refusal sentence is unreachable. readRecords already bounded the file this process opened, so the option and its sentence can go.
| yield* forEachLine( | ||
| path, | ||
| { | ||
| limit, |
There was a problem hiding this comment.
[fable-997-P1] What the memory evidence establishes, and what still scales with the history (note, no action required)
Established: the reader never holds the file as a string, the one read buffer is reused, and a large string repeated across root closes is held once per read. The description's ten-entry journey (2.34–2.55 GB peak RSS over a 508.5 MB history, baseline out of memory at the first entry) was measured on the daeb4ee build; the source since then changes only the summary's __proto__ handling and a test, so the numbers transfer to this head.
Still proportional to the history: readRecords materializes every event of the file as a sealed graph before anything is projected, and every root Close carries the whole inherited root, which the accepted plan keeps. Opening alone therefore costs heap on the order of the distinct large strings plus one container graph, and each new entry still serializes and UTF-8 encodes its entire close transiently. The description says as much; this is a note so nobody reads the numbers as a constant-memory claim.
One cheap allocation remains in the open path: a freshly parsed event is never frozen, so sealEvent copies every container of it once and the parse output becomes garbage. parseDurableEvent builds that graph from JSON.parse and hands it to nobody else, so sharing strings in place and freezing the parse output would remove that transient copy. Worth doing only if a focused measurement says so: peak RSS of the open-only journey (zero submissions) over the same 508 MB history, three runs before and three after. If the difference sits inside run-to-run spread, leave it.
Integrates the two actionable comments of the first-green review on PR 997. Every caller of `publishPair` passed an empty prefix once the upgrade moved to `copyEntryRecords`, so its body, origin, append branch and the paragraph on why bytes rather than events are copied described a path nothing took. It is now `establishPair` followed by `acknowledgePair`, that rationale sits on `copyEntryRecords`, which is what copies bytes, and the three call sites lose an argument. One way to create a pair, and the copy is the only thing that copies. `copyEntryRecords` passed an infinite bound, which made its `exceeded` sentence unreachable: the file it reads is one `readRecords` has already opened under the supported limit. A bound now arrives as the units together with the refusal that names them, and a reader with no bound passes none and carries no sentence for a refusal that cannot happen. The upgrade creates the pair before it reads the older file, so a refused or cancelled copy leaves an interrupted publication that opening refuses, with the older file still holding everything. `upgradeExecution` now says so, and names the one refusal a history this process has already opened can reach: the copy decodes strictly where `readRecords` decodes leniently. A row covers it — a legacy history with a byte that begins no UTF-8 sequence opens as one replacement character, refuses its upgrade by name, keeps every byte, and reopens as an interrupted publication. Lenient opening is unchanged: a history that is not UTF-8 still opens, which is the behavior the review's other option would have changed.
| // Defined rather than assigned, as `detach()` defines the model's own | ||
| // members: `shorter[key] = …` for the key `__proto__` reaches | ||
| // `Object.prototype`'s inherited setter instead of creating a member, and | ||
| // a value parsed from `{"__proto__":{…}}` would read as `{}`. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // a value parsed from `{"__proto__":{…}}` would read as `{}`. |
| const parsed = parseDurableEvent(line); | ||
| if (!parsed.ok) { | ||
| // The record's own text stays out of this: a history holds whatever a | ||
| // person's document held. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // person's document held. |
| let start = 0; | ||
| for (let end = chunk.indexOf(NEWLINE); end >= 0; end = chunk.indexOf(NEWLINE, start)) { | ||
| // Not streamed, so a multi-byte sequence the line left unfinished is | ||
| // settled here rather than carried into the next line. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // settled here rather than carried into the next line. |
| function sealResult(result: Result, share: StringSharing | undefined): Result { | ||
| if (result.status === "ok") { | ||
| // Whether `value` is present at all is kept, because an absent value and an | ||
| // undefined one are different records to a reader that asks `in`. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // undefined one are different records to a reader that asks `in`. |
| // is what makes the execution refuse to reopen at all. | ||
| yield* writer.hold(); | ||
| // The events this process already retained, shared rather than parsed a | ||
| // second time: the writer is held, so they are the records the file holds. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // second time: the writer is held, so they are the records the file holds. |
The tracked file was measured at a4907b4, before the retained-repository suites moved from packages/git to packages/cli and before the files this branch adds. Every file it did not name was charged the heaviest weight the current corpus records, so the partition was predicting roughly twice the work each shard actually holds: deno 812,492 ms against 331,019 ms for shard 1 of 15, node 607,935 ms against 310,445 ms for shard 7 of 10, bun 743,238 ms against 341,192 ms for shard 1 of 5. These are the runner's own bytes, byte for byte, from the Measure test weights run dispatched against this branch: run 40460391084, attempt 1, commit 3b9c563, ubuntu-latest, deno 2.9.5, node v22.23.3, bun 1.4.0, SHA-256 f7818ad0168a2a06944ae7d49e28d26e76ff59a312f34f31adaf7852edc56b16. It names 505 files for deno, 396 for node and 395 for bun, this branch's new suite among them. No millisecond was edited by hand, and the provenance the file carries is the run that produced it. Shard counts are unchanged: what each one is held to is the measured procedure in AGENTS.md, not this file.
| const recorded = clone(event); | ||
| // Sealed before the write suspends, so what the caller does with its own | ||
| // object afterwards reaches neither this range nor the physical stream. | ||
| // Both keep this one graph: it is frozen, so neither side can edit it. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // Both keep this one graph: it is frozen, so neither side can edit it. |
| return scoped(function* (): Operation<number> { | ||
| const chunks = yield* useFileChunks(path); | ||
| // One decoder for the whole file. A leading byte order mark is kept, as a | ||
| // whole-file read keeps it, so a file that begins with one reads the same. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // whole-file read keeps it, so a file that begins with one reads the same. |
| let start = 0; | ||
| for (let end = chunk.indexOf(NEWLINE); end >= 0; end = chunk.indexOf(NEWLINE, start)) { | ||
| // Not streamed, so a multi-byte sequence the line left unfinished is | ||
| // settled here rather than carried into the next line. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // settled here rather than carried into the next line. |
| function sealResult(result: Result, share: StringSharing | undefined): Result { | ||
| if (result.status === "ok") { | ||
| // Whether `value` is present at all is kept, because an absent value and an | ||
| // undefined one are different records to a reader that asks `in`. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // undefined one are different records to a reader that asks `in`. |
taras
changed the title
Why
Large
xmd replhistories repeat inherited root values in later close records. Reading the whole file and deep-copying those events for each consumer multiplies memory use, so a long session can exhaust the default V8 heap when it continues.Refs #992.
What changes
Before:
After:
How it works
The reader owns one reusable byte buffer and a per-read table of repeated large strings. Consumers finish decoding or hashing before the next read. Sidekick copies, validates and hashes the same snapshot while holding the movable writer; it writes owner then ready and redirects before releasing the hold.
Review guide
Start with:
packages/cli/tests/repl-retained-history.test.ts(H1-H6).Then review:
retained-event.ts,journal.tsandentries.tsfor caller isolation, immutable sharing and acknowledgement order.record-file.tsfor UTF-8 boundaries, UTF-16 limit accounting and joined file cleanup.sidekick/history.tsfor original-byte identity, held publication and interrupted-pair refusal.application.tsfor bounded summaries;specs/repl-spec.mdfor the reopening limit.What must stay true
How to verify it
Retained focused receipts at
3b9c563creport:repl-retained-history.test.tsunder Node and Bun: 14 passed each. H6 compares summaries with whole-value serialization, including an own__proto__member.To run the focused regression:
At final head
f2fb3a7a, the five weight/measurement/workflow/discovery/exclusion suites report 19 tests / 87 steps passed, and lint passes. The existing requiredgreencheck passes for this head in run 40480639095.Memory evidence is a separate isolated experiment at
daeb4eef: a generated 508,523,892-byte history with two-byte strings opens, settles ten ordinary entries, navigates history and exits on one Control-C with the original prefix unchanged. Three default-heap compiled runs peak at 2.34-2.55 GB RSS. The original-base control exhausts its heap on the first new entry. The forced-GC import probe measures retained heap separately; it is not a native peak measurement. Later changes preserve the applicability of these historical results; they are not new measurements of the final binary.Scope
Included
Intentionally unchanged
New abstractions
retained-event.tsseals caller-owned graphs for the physical stream, segment views and parsed records, so isolation does not require payload cloning per consumer.record-file.tsowns scoped chunked I/O for decoded record reading and original-byte fingerprinting/copying. Its optional bound couples a finite limit to its refusal.Generated or mechanical changes
test-weights.jsonis byte-identical to the validated artifact from measurement run 40460391084, attempt 1, measuring parent3b9c563conubuntu-latest. No timing value was hand-edited.Risks and limitations
Scope confirmation