Repository navigation
[Perf] JSONL backend re-decodes the whole session log on every read after an append (~286 ms at 9 MB), and why skipping the prefix is impossible #9947
Replies: 3 comments
|
Confirmed, and there is a design reason for it that changes what a fix has to look like.
:199 /** Build the stat-derived best-effort change token shared by full and lightweight reads. */
:200 function fileRevision(identity: JsonlPhysicalIdentity): PersistenceRevision {
:201 return SessionPersistenceRevision([
:202 identity.dev,
:203 identity.ino,
:204 identity.size,
:205 identity.mtimeNs,
:206 identity.ctimeNs,
:207 ].join(':'))
:208 }and the only cache on the read path, Two things worth adding to the report. The memo is not a read cache that regressed; it is a two-step handoff. So raising the constant would not help — the entry is invalidated by the append itself, which is the whole point of putting The revision also carries What a real fix has to be. Either decode incrementally from a stored tail offset (which needs per-file event-boundary offsets, since a JSONL line can span a read chunk), or split the token into an identity part and a content part so a read can decode only the new tail after matching the identity. Neither is a cache-size change, and both touch If you have the numbers per phase (stat vs. decode vs. projection), put them in one table: 286 ms split across three steps is a much stronger argument than 286 ms total, because it tells the maintainer which of the two fix shapes is worth the format cost. |
|
Thanks, this is a much sharper framing than mine and I think your two corrections are both right. I re-measured against them rather than just agreeing, and one of my original numbers needed correcting. Per-phase split (what you asked for)Fresh backend per trial, real session (
So it is not distributed: For contrast, on the same handle with no append between reads: 0.3 ms. That is not a cold decode at all — it is the handle serving resident state — so it is not a baseline to subtract from the 281 ms. That mistake is exactly what produced the wrong 5.3 ms figure in my first post, and it is worth flagging because the same handle-path shortcut will silently ruin any benchmark of this code: a "fresh backend instance" is not sufficient if a resident handle survives from an earlier One measurement trap worth writing down: my first attempt at the ctime test measured 0.4 ms for a 12,369-event read and looked like a clean "no invalidation" result. It was measuring One number in my original report was wrongI reported ~286 ms and ~5.3 ms as "read after append" vs "read without append". The ~286 ms holds up (~280 ms here). The ~5.3 ms does not generalise: it was a memo hit, and a memo hit is not a cold decode, so subtracting the two does not isolate the JSON layer. The 54× framing was wrong even though the underlying claim (read-after-append re-decodes everything) is correct. Corrected numbers are the table above. Your ctime point is confirmedTested with one backend and two reads, the only variable being file metadata between them:
A pure metadata change costs a full re-decode. This matters beyond On raising
|
|
Thanks for reading it that closely — both corrections were right and I'm glad you pushed back on the memo-capacity framing before it turned into a wrong-headed patch. The thing I took most from your reply is the framing itself: that this is a format decision rather than a performance tweak. I'd been circling that without naming it, and it explains why every local fix I tried (bigger memo, skipping the decoded prefix) came out either useless or rejected by the I've posted the per-phase split and corrected the one number in my original report that didn't hold up, so the thread stands on its own as a record. Beyond that I'll leave the design question with you and the other maintainers — it's your package and your call, and I'd rather the report be accurate than persuasive. Thanks again for the time. |
Uh oh!
There was an error while loading. Please reload this page.
TL;DR
The JSONL backend re-decodes the entire session log on every read that follows an append, because
coldLogMemois keyed on a stat-derivedrevisionand every append changes it. On a real 9.2 MB / 12 369-event session this costs ~286 ms per read; the identical read with no append in between costs 5.3 ms. The read-after-append read is the one the host performs every turn, so a long session pays it repeatedly.I traced why the obvious fix does not work, and what does. Short version:
seqto be contiguous from 0, so history must be replayed. A patch that skips the prefix fails withseq gap (expected 0, got 12369).So the actionable question is not "cache better" but "may a decoder stay resident". That is a policy decision for this package, which is why I am reporting it rather than filing a patch.
1. Environment
@deepseek-ai/dsh-desktop0.2.1-rc.1 (installed build)@deepseek-ai/dsh-session-persistence-jsonl0.2.1-rc.1master@687f282e(release(dsh): 0.2.1-rc.2)~/.dsh/sessions, 9.2 MB compressed, 12 369 events,seq0…12368Medians of 5 reads on the same fixture. Source line numbers are from
master.2. The mechanism
packages/session/session-persistence-jsonl/src/index.ts:revisionincludessize,mtimeNsandctimeNs, so any append invalidates the entry.decodeStoredLogthen doesreadFile→ decode every frame →JSON.parseevery line → validate, over the whole artifact.The comment is explicit that the memo targets the cold-observation-then-resume case. I am not calling that wrong; I am pointing at the case next to it: a session that grows one batch at a time and is read after each batch.
3. Measurement
Same artifact, same backend, one append then one read:
The 54× gap between rows 1 and 2 is not I/O and not zstd — the file is in page cache. It is the JSON layer.
I also measured the session-switching case, where the 2-entry bound is usually blamed: with the bound raised to 8, the read after an append was still ~280 ms. Raising the bound does not help, because the append itself is what invalidates the entry. That rules out the cheapest suspected fix.
4. Why the obvious fix fails
I implemented "decode only the frames the append added" against
masterand measured it. It is correct but not faster (event hashes identical, 1.0×), and the reason is a hard format invariant:SessionLogScannerseeds a restore from the header and requires each row'sseqto equal the running count (dsh-session-format-v1-to-v2/src/codec.ts:103,released v2 row ${currentRow} has seq gap (expected ${eventCount}, got ${event.seq})).invalid committed event at line 1: released v2 row 0 has seq gap (expected 0, got 12369).SessionLogScanner.finish()is single-use and itsrestoreis private, so an existing scanner cannot be cloned or resumed either.Skipping history is not available at this layer. History must be read once and then kept, which is a different design:
5. What does work
A decoder that stays resident per session and is fed each append, never re-fed history:
seqcounter, committed events).This keeps
seqcontiguous because the reader has been watching from row 0 — the invariant is satisfied by construction rather than bypassed.Open question for the maintainers: residency means holding a parsed log in memory per session.
coldLogMemodeliberately bounds this at 2 entries for exactly that reason, so a resident cursor needs its own bound (entries and bytes) and an eviction policy. A community plugin ships this with a configurable byte budget (default 128 MB ≈ 1 GB heap) and 8 entries; that is a reasonable starting point to critique, not a recommendation.6. Correctness argument for the resident path
Reuse only when all hold, else fall back:
revisionis the observed one.If any check cannot be proven, the read degrades to today's behaviour — never a different answer, at worst the same cost. The fallback is total, so a wrong guess cannot corrupt a read.
7. Prior art
dsh-coldread-cache(topicdsh-plugin) implements the resident-reader approach by patching the backend instance at runtime; the 11.8 ms figure comes from it, and it installs all six of its hooks on 0.2.1-rc.1. I am reporting the mechanism upstream because a plugin has to patchcoldLogMemo,readStoredLogand four other members to do this, which is exactly the kind of thing that should not live in a third party if the behaviour is wanted.If maintaining a resident decoder here is out of scope, a cheaper and still useful outcome would be documenting that read-after-append is inherently a full decode on the current design, so users can understand long-session degradation without profiling it themselves.
8. Reproduce
Remove the
appendand the same read costs 5.3 ms — that difference is the whole finding.External PRs are not accepted at the moment, so this is a report, not a patch. The incremental-decode experiment described in §4 is available on request if it helps rule that option out.
All reactions