Repository navigation
rpcv2: O(tx) allocation for by-hash reads — pooled decode scratch, copy-out, pinned read - #961
Conversation
|
Added |
There was a problem hiding this comment.
Pull request overview
Reduces hot-tier transaction lookup allocations by reusing decode buffers, pinned RocksDB reads, and compact transaction copies. It also adds close-time memoization.
Changes:
- Adds zero-copy pinned RocksDB reads and reusable ledger decoding.
- Pools decode buffers and copies transaction views out safely.
- Adds bounded close-time caching and regression tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
stores/txhash/read_assembly.go |
Pools ledger scratch and compacts views. |
stores/txhash/read_assembly_test.go |
Tests isolation, reuse, and ownership. |
stores/ledger/hot_store.go |
Adds reusable pinned decoding. |
stores/ledger/hot_store_test.go |
Tests decode reuse and growth. |
rocksdb/rocksdb.go |
Adds callback-based pinned reads. |
rocksdb/rocksdb_test.go |
Tests pinned-read lifecycle. |
adapters/transaction_reader.go |
Routes scratch-capable reads. |
adapters/transaction_reader_test.go |
Tests wiring and allocation shape. |
adapters/ledger_reader.go |
Adds close-time memoization. |
adapters/ledger_reader_test.go |
Tests memoization and concurrency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ct, ok := cachedCloseTime(seq); ok { | ||
| return ct, nil |
There was a problem hiding this comment.
This reviewed an earlier revision: the process-wide close-time memo was replaced wholesale by the current design — close times live on the registry's own stamps as an optional CloseTime value, so there is no cross-registry cache to pollute. The concern was valid against that revision and is moot in the current one.
| // readCloseTime returns one ledger's close time, memoized. On a miss it | ||
| // point-reads the ledger and decodes only the close time off the raw bytes — no | ||
| // full LedgerCloseMeta unmarshal, though the point read itself still | ||
| // decompresses the whole ledger, which is exactly why the answer is kept. which | ||
| // names the window edge ("oldest"/"latest") in the missing-ledger error. |
There was a problem hiding this comment.
Fixed in a later revision (the fragment was repaired, then the comment-pruning pass shortened the doc further).
360ee28 to
b9eab0c
Compare
|
Force-pushed the reviewed redesign (v2), replacing the earlier 8-commit shape with 5: the borrow is now a scope-bounded callback ( |
| // the source take the bytes back with nothing to release and nothing to leak. | ||
| type LedgerSource interface { | ||
| GetLedgerRaw(seq uint32) ([]byte, error) | ||
| WithLedger(seq uint32, fn func(raw []byte) error) error |
There was a problem hiding this comment.
The description has been rewritten for the current design and now matches: WithLedger on both tiers, per-store pooling, ReadView as the ledger source, and the cold-read changes are all documented.
| // The loan contract, shared by ReadItem, ReadItems and ReadRange: the bytes | ||
| // belong to the reader for the duration of the call, and are capacity-clipped | ||
| // so appending to them allocates rather than writing into the record buffer | ||
| // they came from. A borrower may read and append freely; it may not retain. |
There was a problem hiding this comment.
Addressed: the loan doc now states the three lifetimes distinctly (ReadItem — until fn returns; ReadItems — until that item's callback returns, records are reused between callbacks; ReadRange — until the loop body ends, break included).
| // store's lifecycle read lock is held across fn, so fn must not block on | ||
| // anything that could close the store. |
There was a problem hiding this comment.
Addressed: the contract now forbids fn from calling back into lock-taking Store methods, naming the nested-RLock-behind-waiting-Close deadlock.
8e18de0 to
38eb76e
Compare
| rerr := c.r.ReadItem(int(seq-h.firstSeq), func(b []byte) error { | ||
| fnErr = fn(b) | ||
| return nil | ||
| }) | ||
| if rerr != nil { | ||
| return nil, translateReaderErr(rerr) | ||
| switch { | ||
| case fnErr != nil: | ||
| return fnErr |
There was a problem hiding this comment.
Addressed with TestColdReader_WithLedgerPassesCallbackErrorThrough — sentinel asserted via errors.Is and by identity, proving it bypasses translateReaderErr; fn invocation count pinned.
| if seq < chunk.FirstLedgerSeq { | ||
| return stores.ErrNotFound |
There was a problem hiding this comment.
Addressed with TestReadViewWithLedger_SubGenesisIsNotFound — seq=0 and FirstLedgerSeq-1 both return stores.ErrNotFound, fn never invoked, no panic. Good catch — the guard had no coverage.
38eb76e to
4a3c7dc
Compare
Get copies every value it returns, which is the right default: the pinned block dies with the read, so anything a caller keeps has to be its own. The hot ledger read keeps nothing — it decompresses the value once and drops it — and at roughly 2MB of compressed ledger per lookup that defensive copy is pure garbage. GetPinned hands the pinned block to a callback instead. Get is now that same body plus the copy, the way Iterate and IterateAsOf already share one, so the two reads cannot drift apart. The contract is narrow on purpose: the callback may not retain, append to, or mutate what it is given. Holding the lifecycle read lock across a callback is safe because nothing that reclaims the store queues behind readers — CloseIfIdle tries the write lock and gives up — which the mutex now says out loud, since the callback in question runs a multi-millisecond decode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit f48ea5e2faa978dc32d384b0eecf42fc50a3340f)
4a3c7dc to
5b49c3b
Compare
| pooled, _ := h.scratch.Get().(*[]byte) | ||
| assert.GreaterOrEqual(t, cap(*pooled), len(big), "the grown buffer must be the one recycled") |
There was a problem hiding this comment.
Addressed: the keep-or-drop decision is now a pure poolable() helper with its own table test, and the WithLedger test asserts only decode/growth behavior inside fn — no pool-identity assertions remain (same treatment an earlier flake of this class got).
| // Loans are capacity-clipped, so a borrower may read and append freely; it may | ||
| // not retain. Lifetimes differ: ReadItem's until fn returns, ReadItems' until | ||
| // that item's callback returns (records are reused between callbacks), and | ||
| // ReadRange's until the loop body ends, break included. |
There was a problem hiding this comment.
Addressed: ReadRange's GoDoc now matches the summary — valid only until the loop body ends, break included.
A point read of a ledger needs the bytes for as long as it takes to find one transaction in them, and never again. GetLedgerRaw cannot express that: it allocates a whole uncompressed ledger — megabytes on a full chunk — hands it over, and watches it become garbage. Per call. WithLedger lends instead, for the duration of a call. The hot store decodes into a buffer from a pool it owns and takes it back as the callback returns, so the steady state is one buffer per serving thread rather than one per request; the decode reads straight out of RocksDB's pinned block, so the compressed value is not copied either. Scoping the loan to a call is what makes the contract enforceable rather than merely stated — there is no handle to forget to release and no way to hold the bytes past the point they are reused — and it lets the store cap the loan to the ledger once, on the way in. The pool is a field of the store, not a global, because the buffers belong to the store that decodes into them; IterateLedgers draws from it too, instead of growing an unbounded scratch of its own. Capacity only ratchets upward, so a buffer grown past maxPooledLedgerBytes is dropped rather than parked for the life of the store. The cold reader has the shape already and did not know it: the packfile reader decompresses a record into a buffer it pools and lends a slice of it for the duration of a callback. WithLedger passes that loan straight through instead of copying out of it, so a cold point read stops allocating a ledger too. Its slices are clipped, point read and scan alike, because a record's capacity runs on into the ledgers packed after it; and a caller's error is carried out around the packfile rather than through it, so reader failures and callback failures never get confused for one another. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 8d80912e1673fa437a956baa91deab9fc9d8ed52)
The lend is mandatory on LedgerReader rather than something a caller type-asserts for: both tiers implement it, so there is nothing to probe and no fallback to get wrong or to leave silently taken. And the view routes it. A sequence does not name its chunk, so every caller that has only a sequence — the by-hash lookup verifying a candidate, the close-time edges, the point read — would otherwise repeat the same resolve. It sits beside Ledgers, which is the resolve it wraps, and it is what lets the by-hash probe take the read view outright instead of a wrapper that exists to spell this out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d652ab50db270c4b224be0be8b66d1370a85e5d9)
A found by-hash lookup cost one ledger-sized allocation and returned a view aliasing it, so the response pinned megabytes until it was serialized. Both halves are gone. The probe borrows the candidate ledger for one call, locates the transaction, and copies its bytes into a single compact transaction-sized array before the loan ends. That copy is what makes the loan returnable, and it is the difference between a lookup that allocates a ledger and one that allocates a transaction. Verification moves into its own function, which is also where the two candidate failures stay honestly apart: a ledger that could not be read may simply be gone, and for an exact index that is an inconsistency, while one that could not be parsed never is. LedgerSource becomes the one method every tier implements, which lands with its callers because an interface and its implementations cannot change apart. Both the served path and the bench had wrapped the read view to route a ledger read; the view routes and lends by itself now, so they pass it directly. And with the probe returning a view backed by its own compact array, transactionFromView stops cloning on the way to the serving contract. That cloning was load-bearing when the view aliased a whole ledger buffer; afterwards it duplicated every field of every served transaction to own what was already owned. Nil and empty are unchanged, and the invariant the type cannot carry — that only a compactView-produced view may be passed — is written where the next producer will look. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 328e5d8c98fbb84c874ea56a5044151db7af5f4c)
Every response reports the servable window's edge close times, and the registry stamps them so no request has to read a ledger for them: startup seeds both edges before serving, ingestion restamps the tip on every commit, and the oldest stamp carries its sequence so a moved retention floor invalidates it by construction. That design was already right. The sentinel underneath it was not. The registry stored a bare int64 and read zero as "not known yet". Zero is a legal timestamp. A ledger genuinely closed at the epoch therefore read back as unknown forever, and unknown is not a cheap state: it sends every request that reports a window edge to a point read, which decompresses an entire ledger to recover eight bytes the stamp already held. Synthetic corpora close their ledgers at zero routinely, which is enough to make a benchmark measure this instead of the read path it was pointed at. So a close time is a value that may be absent — CloseTimeAt(unix) or UnknownCloseTime() — and SetLatestLedger takes one. A value rather than a pointer, so the absent case cannot be dereferenced, and so both states are named at the call site that produces them rather than inferred from what was passed. One setter, because the two states are one argument. The two remaining ledger reads on this path lend rather than allocate. Both wanted almost nothing out of the ledger they decompressed — eight bytes of close time, or an unmarshal that copies what it keeps — so both do that work inside the loan. They were the last callers reading to own a ledger, so the owning read goes: off the routing interface, and off both stores. Nothing served keeps a whole ledger, and leaving that API beside the pooled one only invited the allocation back. The tests that do assert on a whole ledger copy it themselves. Three tests asserted the old sentinel or stamped a known epoch where they meant the boot state, and now say what they mean. Two that duplicated registry-level close-time assertions are gone, covered where the registry is tested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every item the reader lends — ReadItem's callback, ReadItems' callback, ReadRange's yield — is sliced out of one shared record buffer, and until now carried capacity to the end of it. A borrower that appended to what it was lent therefore wrote over the items packed after it, and the damage surfaced on the next read of a neighbour rather than at the append. Clipping belongs here, at the lender that owns the buffer, not at each consumer that happens to remember. All three paths slice through record.item, so one clip covers them; the ledger cold reader's own clips go, and the events store — which was receiving unclipped loans and had no clip of its own — is covered without touching it. The hot ledger store keeps its clips: that pool is its own, and it is the lender there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 880c58b3e9765e6a69f74fdcb6fc15ef5d092c96)
5b49c3b to
fb2dd17
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Upstream #954 and #961 moved the read-view context helper to query.WithView, replaced LedgerReader.GetLedgerRaw with the loaned WithLedger callback, and made SetLatestLedger take a query.CloseTime. The cold fixture now stamps the latest ledger with an unknown close time and seeds it with adapters.SeedCloseTimes, the same way startup.go does, so served requests do not decode a ledger to learn it.
Upstream #954 and #961 moved the read-view context helper to query.WithView, replaced LedgerReader.GetLedgerRaw with the loaned WithLedger callback, and made SetLatestLedger take a query.CloseTime. The cold fixture now stamps the latest ledger with an unknown close time and seeds it with adapters.SeedCloseTimes, the same way startup.go does, so served requests do not decode a ledger to learn it.
Upstream #954 and #961 moved the read-view context helper to query.WithView, replaced LedgerReader.GetLedgerRaw with the loaned WithLedger callback, and made SetLatestLedger take a query.CloseTime. The cold fixture now stamps the latest ledger with an unknown close time and seeds it with adapters.SeedCloseTimes, the same way startup.go does, so served requests do not decode a ledger to learn it.
What
By-hash and by-sequence ledger reads no longer allocate or retain whole ledgers, and the response envelope no longer decodes ledgers to report window close times.
rocksdb.Store.GetPinned— a read for values consumed in place: the pinned block is handed to a callback and released after;getWith(the allocating read) is nowgetPinnedWith+bytes.Clone.WithLedger(seq, fn)on both ledger tiers — the store lends the raw ledger bytes to a scope-bounded callback. The hot store decodes into an internal boundedsync.Pool(64 MiB cap per buffer) and recycles afterfn; the cold reader passes throughpackfile.ReadItem's existing loan, so cold point reads stop allocating too. Every lender clips its loans (cap == len): the packfile clips at its single choke point (record.item(), coveringReadItem/ReadRange/ReadItemsand closing a pre-existing unclipped exposure in the events cold reader), and the hot store clips its pooled decode buffer — so an appending caller reallocates instead of corrupting shared memory, enforced at the source rather than per consumer.query.ReadViewis itself the ledger source — one routed point read (sub-genesis guard included) serves the tx-hash probe,getLedger, andreadCloseTime; the per-caller wrapper types are gone, and the interface method is compiler-enforced on every tier.txhash.TxReaderborrows the candidate ledger, locates the transaction via the SDK view, andcompactViewcopies its byte sections once into a single exactly-sized backing;transactionFromViewaliases that copy directly. Nothing the caller receives references pooled memory.knownflag —closeTime == 0previously doubled as "unknown" — but the epoch is a legal close time: pubnet's genesis ledger closes at 1970-01-01 (ledger 1), so a node serving full history from genesis has its oldest window edge on such a ledger permanently. Under the sentinel such a ledger read back as unknown forever and sent every request — hits and misses — into a fallback that fully decompresses a ledger per window edge to read an 8-byte timestamp. Close times are an optional value type (CloseTime, constructed viaCloseTimeAt/UnknownCloseTime), so the stamp API is a single setter and absence is a state of the value, not a method name or a sentinel.readCloseTimeremains as a rare fallback and now borrows instead of allocating.Iterator contracts are documented as loans (valid until the loop body ends, break included), and
GetLedgerRawis deleted from both tiers and the interface — no production caller remained, and it was the ergonomic path back to megabytes of per-request garbage.Numbers
At Phase 3 target load — sac-6000 stress profile (6,000 tx/ledger), 600 ms closes, 1,000 rps of getTransaction — the read p99 is 22–24 ms with this PR + #902's ingest, and 30–32 ms with this PR alone. Before this PR the load was not servable at all: 200 rps OOM-killed the daemon. The 10 ms objective is not yet met — the remainder is raw per-request CPU (≈12 ms per lookup ⇒ ~83 req/s per core), addressed by the follow-up options doc, not this PR.
And ingest, measured in the same cells (per-ledger pace lag behind the 600 ms schedule, under the same 1,000 rps of reads; budget 80 ms p99):
HTTP rows are server-side hold-window percentiles; ranges span 2 reps × during-ingest/static legs. Peak RSS in the in-process harness drops 10.4 → 6.4 GB.
Measurement protocol, per-rep tables, client-observed columns, ingest detail, capacity math, and flagged anomalies
Measurement protocol
Dataset: sac-6000 — the stress profile: 6,000 transactions per ledger, ~18 MB raw
LedgerCloseMeta, ~2.1 MB stored (zstd), 2,500 ledgers in the hot tier. Load model: Phase 3 — 600 ms ledger closes, read RPS = 10% of TPS ⇒ 1,000 rps. Box: c6id.8xlarge (16 physical cores / 32 vCPU, 61 GB).getTransactionby hash. The HTTP corpus is 5,000 real transaction hashes drawn from a 379-ledger band (seqs 10002–10380; ~800 MB stored / ~5 GB decoded working set — far past any reuse, and no decoded-ledger cache exists), so every request is a found-hash hit paying a full ledger decode + walk; misses are not exercised over HTTP (minimum hold-window handler duration across ~2.4 M requests: 5.5 ms). The separate in-process bench-query corpus carries a 12 % miss fraction — that is where the µs miss figures come from.Full tables
In-process floor (bench-query, single-threaded, solo box — the clean per-request read cost, no HTTP/load):
The pre-#961 build cannot be measured at target load over HTTP at all: by-hash reads allocated ~20 MB per request, and 200 rps OOM-killed the daemon on this 61 GB box.
Over HTTP at 1,000 rps (2 reps — ranges span the reps; server-side = exact percentiles over ~300 k hold-window handler durations; client-observed = the blaster's whole-run blend for the same legs, ramp included):
¹ The layer a caller of this rig experiences. Per the methodology above, the client stack itself accounts for ≈ +0.9–1.1 ms p50 / +3.5–4.1 ms p99 of the gap (disjoint-core bound) and SMT sharing for ≈ +4–6 ms p99 more; the rest of the client-column tails is the flagged windows below.
² Blends poisoned by the flagged anomalous client-side windows below — reported as measured, not averaged away; the same legs' server-side distributions are the adjacent columns.
³ Rep 2's blend carries its failed request and two elevated windows (132 / 160.9 ms); rep 1's blend p99 is 33.6.
Flagged (client timelines + error gate — the #961-standalone legs are noisier than a single event): during legs carry 1 failed request each (~1 in 360 k, client-side EOF, zero server-side failures) and a pervasively elevated client tail — 60/85 and 56/84 five-second windows above 100 ms client p99, worst windows 503 / 356 ms, whole-run client blend p95 198 / 173 ms — over server-side hold distributions that stay at the table's values. Static legs show a recurring ~55 s-periodic stall pattern: six episodes per rep, with 25 % / 31 % of client requests in windows above 500 ms (worst-window p99 1.26 / 1.42 s). Server-side hold percentiles stay at the table's values, but whole-leg server maxima are 415 / 237 ms and, inside the episodes, server p50/p99 rise to ~15–19 / ~41 ms while request throughput dips then overshoots. Root-caused (gctrace replica cell): the base-ingest daemon retains a ~4.3 GB pointer-dense live heap after replay, so at serving allocation (~65 MB/s, GOGC=100) a concurrent GC fires every ~55–70 s with a 5.5–5.9 s mark phase; during mark, background workers plus assists cut request intake ~15–25 %, a ~1,000-request backlog builds upstream of the handlers and then drains — six marks aligned one-to-one with the six episodes. The joint build is clean because #902's ingest leaves no such heap behind. The episode latency is pre-handler queueing, which the logged duration excludes (it also excludes post-handler response marshal/write), so the table's server-side values are unaffected. The #961+#902 rep 2 during-leg carries one failed request (t ≈ 129 s) and two elevated windows (132 / 160.9 ms); rep 1 is clean.
Ingest under read load (per-ledger pace lag: how far each of the 2,500 replayed ledgers committed behind its 600 ms schedule; one aggregate per replay — no per-ledger series is recorded — so the numbers span the whole 25:00 replay; budget 80 ms p99):
⁴ No fully read-free paced rep exists for the joint build; these reps, whose load ran at full rate for only ~1 of the 25 minutes (a 3-minute window including its 2-minute ramp — 12 % of the replay wall, ~4 % at full rate), bound the unloaded baseline from above. Three reps used an earlier narrower read corpus; the fourth used the headline cells' corpus and sits highest (p99 42.7, max 68.5) — ingest-side numbers only.
Replay wall time is 25:00 within ±0.4 s in every rep (measured 1,499.8–1,500.3 s for 2,500 × 600 ms) — zero cumulative drift. Read load does not move the median ledger; the 5-minute hold lifts the whole-replay p99 from ~36–43 (the ~1-minute-load band) to ~50–51 ms, and since the hold covers ~20 % of the replayed ledgers, the within-hold p99 lies between that and the run max — every ledger committed ≤ 76.2 ms behind schedule, inside the 80 ms budget. The base-ingest row is the background the #961-standalone read cells ran against: the pre-#902 pipeline is far over budget on this profile with or without reads (unpaced solo build of the same DB: p50 144.6 / p99 335.6 per ledger) — that is what #902 fixes.
Capacity. A found lookup costs ≈ 12 ms of CPU (zstd decode ~4 ms + SDK locate/walk ~8 ms) ⇒ ~83 req/s per core, so 1,000 rps needs ~12 cores for reads plus ~4 for ingest and runtime — the 16-core box at redline. Halving the daemon to 8 physical cores at 1,000 rps collapses (client-observed p50 7.6 s, 31 % errors; the daemon's in-handler p50 that leg was 24.8 ms — the collapse lives in queueing and shed load, which handler durations exclude; same protocol otherwise). The p99 at target load is saturation-shaped, not GC-shaped: this PR removes the allocation catastrophe and makes target load survivable and stable; the remaining gap to the 10 ms objective (p99 ≈ 22–24 ms in the joint configuration) is raw per-request CPU. Shrinking that — a per-tx offset header (≈ 4 ms floor, ~2.4× capacity) or per-tx re-materialization (~0.1–0.3 ms, ~40×) — is scoped in the follow-up options doc, not this PR.
Compatibility
The changes are ingestion-agnostic: they reshape read-side materialization only. #902 rebases over this with two known seams, both mechanical and pre-resolved in the verification build:
ledger/hot_store.go(this PR's read side vs the campaign's write-side encoder — disjoint halves of one file) and the campaign's in-memory tx-hash index superseding the index-CF point read. Recommended landing order: this PR, then rebase #902.Verification
Differential tests pin byte-identical responses (including nil-vs-empty wire semantics) against a buffer-poisoning source with pool churn; concurrency isolation under
-race; fat-buffer reuse across lopsided ledgers; panic-path release for the pinned read; a packfile corruption-regression test (a callback appending to its loan must leave neighboring items byte-exact); allocation-shape guards onGetTransactionandgetLedgerRangeso the optimizations cannot silently regress. Independently reviewed across multiple simplify and adversarial-review rounds to a clean report; all 23rpcv2packages green.🤖 Generated with Claude Code