Repository navigation
wire: replace 4 MiB script slab with tiered arena allocator - #2565
Conversation
|
Ran the benchmarks here. To reduce variance I ran a stress test with N cores in a busy loop until discover the final frequency after the CPU heats up, then used |
In this commit, we add a small bump allocator intended to replace the fixed 4 MiB script slab used to stage scripts and witness items during transaction decode. The slab approach amplifies memory use: decoding even a tiny transaction pins a full 4 MiB buffer, a pool miss allocates and zeroes 4 MiB, and the channel-based free list can retain up to 125 slabs, or roughly 524 MB, indefinitely because channels are invisible to GC pressure. The arena instead draws fixed-size chunks from a ladder of size classes (16 KiB, 128 KiB, 1 MiB, and 4 MiB) backed by sync.Pool, then bump-allocates scripts from those chunks. A decode only touches memory proportional to the scripts it contains. Typical transactions fit in one 16 KiB chunk, while larger transactions promote through the ladder. Idle chunks remain visible to the runtime across GC cycles instead of being pinned permanently. The existing decode structure provides the ownership boundary. Staged scripts are copied into one exactly-sized allocation before decode returns, so arena memory does not escape the call and reuse needs no reference counting. The arena retains the existing 4 MiB limit on total staged bytes.
d3e2def to
850492f
Compare
In this commit, we swap the 4 MiB script slab for the script arena in the transaction and block decode paths. MsgTx.BtcDecode now borrows an arena starting at the 16 KiB class, while MsgBlock.BtcDecode and DeserializeTxLoc start at the 1 MiB class and share one arena across all transactions in the block. Each btcDecode call rewinds the arena on entry, which mirrors the old behavior of handing each transaction the slab from offset zero: the previous transaction's scripts have already been copied into their final exactly-sized buffer by then. ReadTxOut previously stack-allocated an entire scriptSlab per call, and worse, returned a PkScript aliasing that 4 MiB buffer, pinning it for the lifetime of the TxOut. It now stages via the arena and copies the script into an exactly-sized allocation the caller owns. Benchmarks on an M4 Max: ReadTxOut drops from ~55 us and 4.19 MB/op to ~71 ns and 80 B/op, DeserializeTxSmall and DeserializeBlock are at parity, and ReadTxIn picks up ~3 ns from the per-iteration rewind. DeserializeTxLarge shows +12% time and two extra allocs in the benchmark harness only: its tiny heap GCs every few iterations, which drains the sync.Pool and forces chunk re-allocation, the flip side of the pools actually giving memory back.
In this commit, we tighten the script arena against caller misuse and
stabilize large chunk reuse. A released arena is now marked dead:
release is idempotent (a double release no longer double-inserts the
arena into the pool where two decodes could receive it), rewinding a
dead arena is a no-op, and any alloc through a stale reference fails
with errScriptArenaFull instead of silently carving memory out of
chunks another decode may now own. Forgetting release remains safe by
construction, the chunks are ordinary GC-visible slices, so an
abandoned arena is simply collected and only the pooling benefit is
lost.
We also front the two large chunk classes with small bounded free
lists (16x 1 MiB, 8x 4 MiB) layered over the sync.Pool. Large chunks
are the expensive ones to re-create (a pool miss zeroes the whole
chunk) and only a handful are ever live at once, so pinning a few buys
deterministic block decode performance for at most 48 MiB, and only
once block traffic has warmed them. The sync.Pool absorbs bursts past
the fixed lists and lets the GC reclaim the overflow. The hot rewind
and alloc paths are also split so their common cases inline.
benchstat vs master (M4 Max, count=10):
DeserializeTxSmallParallel 382.4n ± 1% 334.6n ± 1% -12.52%
DeserializeBlockParallel 234.7µ ± 2% 248.0µ ± 9% +5.70%
ReadTxOut 29.25µ ± 2% 73.19n ± 1% -99.75%
ReadTxIn 25.94n ± 1% 25.58n ± 1% -1.39%
DeserializeTxSmall 176.6n ± 1% 177.1n ± 1% ~
DeserializeTxLarge 205.0µ ± 1% 216.0µ ± 1% +5.33%
DeserializeBlock 903.6µ ± 2% 926.5µ ± 0% +2.53%
The parallel small transaction case (many peers relaying mempool txns
at once) is the dominant real-world decode load, and it improves 12.5%
on top of the 256x reduction in transient memory per decode. The
DeserializeTxLarge deltas come from the benchmark's tiny heap GCing
every few iterations and draining the small-class sync.Pools, the flip
side of those pools actually returning memory.
In this commit, we put the arena through property-based testing with pgregory.net/rapid. The properties cover transaction and block round trips, allocator capacity, non-overlapping live allocations, decoded message ownership, and release behavior. The generators bias script sizes toward ordinary traffic while periodically crossing the 16 KiB starting chunk class to exercise the growth ladder. They also generate witness transactions with the same shape produced by the decoder.
In this commit, we add FuzzTxDecode and FuzzBlockDecode, seeded with the existing test vectors. Both targets parse arbitrary wire data through the script arena and require every successful decode to serialize and decode to a stable fixed point. Initial runs cover approximately 1.7 million transaction executions and 4.3 million block executions without findings.
In this commit, we start block decoding from the same 16 KiB script arena tier used for standalone transactions. The block decoder rewinds the shared arena between transactions, so its live staging requirement is set by the largest individual transaction rather than the aggregate script data in the block. Larger transactions still promote the arena through the existing size classes on demand. We also tighten the arena lifecycle comments around its single-owner contract and correct the growth test description to match the old 4 MiB slab behavior.
In this commit, we report decode failures from parallel benchmark workers without calling FailNow from those worker goroutines. The testing package only permits FailNow on the goroutine running the benchmark itself, while Error is safe for concurrent use.
In this commit, we verify decoded transaction ownership by overwriting the exact arena chunks used for that decode. The prior test sampled chunks from the global pools, which could miss the recently released chunk when a fixed queue already held other entries. We also reserialize the first transaction and block after a second decode in the fuzz targets. This checks that reuse leaves the earlier decoded value stable while retaining the existing fixed-point encoding check.
In this commit, we isolate the ReadTxOut ownership test behind a dedicated script chunk pool. After ReadTxOut releases its staging arena, the test checks out and overwrites the exact returned chunk before asserting that the output script remains unchanged. This makes the exported ownership invariant fail if the final copy is removed.
In this commit, we generate transaction output values with the rapid int64 generator. The prior int generator used a bound larger than a 32-bit int, which prevented the wire tests from compiling on 32-bit targets even though the generated field itself is an int64.
In this commit, we defer allocation for length-prefixed byte sequences until the reader supplies the corresponding data. Readers that expose their remaining length retain the single-allocation path, while streaming readers grow from a bounded initial buffer. Apply the same staging rule to v1 message payloads, variable strings, and variable byte fields. Complete payloads retain their existing decode behavior and allocation counts.
In this commit, we compare each vector count with the minimum encoded element size and the bytes remaining in a buffered payload before reserving its backing slice. Readers without a measurable remainder retain the existing streaming behavior. Apply the check to transaction inputs, outputs, and witness stacks as well as the block, inventory, address, header, locator, and compact-filter vector families. The regression tests cover maximum legal counts followed by truncated payloads and verify that no corresponding maximum-capacity slice is retained.
In this commit, we add allocation benchmarks for incomplete length-prefixed fields and count-driven vectors. The cases cover outer message payloads, variable bytes, inventory, merkle hashes, and transaction inputs. The transaction case prewarms the script arena so the comparison isolates the count-sized metadata allocation. This also makes the benchmark compose directly with the staged script allocation work.
In this commit, we preserve exact-size vector allocation when a buffered reader proves that the corresponding bytes are already available. Readers that don't expose a remaining length instead grow transaction inputs, outputs, witness stacks, and compact filter checkpoints as elements arrive. This keeps the framed message path on its existing contiguous allocation strategy while bounding speculative work for callers that decode directly from a streaming reader. The streaming tests cross the initial capacity so they also exercise backing array growth and final pointer wiring. Co-authored-by: Gijs van Dam <gvandam@gmail.com>
510b054 to
12a41ab
Compare
GeorgeTsagk
left a comment
GeorgeTsagk
left a comment
There was a problem hiding this comment.
Please address the streaming allocation regression and add a block staging reuse assertion. Refresh the benchmarks for the current revision, including complete streaming reads and retained heap.
In this commit, we replace the bytes.Buffer streaming fallback with geometrically sized chunks capped by the remaining payload. bytes.Buffer.ReadFrom reserves additional space before its final EOF check, which lets a complete read retain capacity beyond the advertised length. The chunked path allocates only as bytes arrive, returns a single chunk without copying for small messages, and joins larger messages into exact-sized storage once complete. Complete framed-message benchmarks cover ping and block payloads.
In this commit, we add a block-level footprint assertion for both Deserialize and DeserializeTxLoc. The test decodes 1,000 small transactions through isolated chunk pools and requires a single 16 KiB staging chunk, so removing the per-transaction rewind now fails even when round-trip correctness remains intact.
|
Thanks, both comments were valid. I addressed the streaming path in The opaque-reader fallback previously used I reran the complete framed-message benchmark at the old PR head and the current head with Go 1.26.1 on darwin/arm64 (Apple M4 Max), using ten one-second samples. This is the direct The timing change is small for the block case, but the allocation reduction is consistent: 85% fewer bytes for ping and 32% fewer bytes for the block fixture. I also reran the retained-heap harness against the current revision. It starts 16 block decoders together, waits until each has acquired staging storage, completes the decodes, then runs two collections plus Finally, |
GeorgeTsagk
left a comment
GeorgeTsagk
left a comment
There was a problem hiding this comment.
LGTM, both comments are addressed.
Nit: please refresh the benchmark results in the description. Complete streamed block reads still allocate about 8.04 MB/op versus 5.68 MB/op on the base, down from 11.84 MB/op before the follow-up. Worth documenting the remaining staging/copying cost.
|
Going through it now... I hate that I can tell the code was written by claude. |
| str := fmt.Sprintf("too many invvect in message [%v]", count) | ||
| return messageError("MsgGetData.BtcDecode", str) | ||
| } | ||
| if err := validateElementCount( |
There was a problem hiding this comment.
I know it's nitpicking but good god this is the worst I've ever seen claude write the error handling code. It lacks artisanal detail.
There was a problem hiding this comment.
Heh, this wasn't actually Claude ;)
There was a problem hiding this comment.
Also personally I love that bots can spend their time writing non-artisanal error handling code so we can focus on bigger picture items!
| } | ||
| } | ||
|
|
||
| panic("putScriptChunk: chunk size matches no class") |
There was a problem hiding this comment.
I think this is a massive footgun that might get us in the future
There was a problem hiding this comment.
Ok so we can't trigger this right now so the code is correct and I'm ok with it. But ugh, panics are ugly in code like this
| // so repeated growth converges to the largest class in a constant number of | ||
| // steps. Any request is satisfiable because callers bound n by the arena | ||
| // capacity, which equals the largest class size. | ||
| func (a *scriptArena) grow(n int) { |
There was a problem hiding this comment.
I don't really like how class field is handled. Wouldn't it be better to just have a grow() that's something like this with no class field at all? Just remove the entire thing. The worse cas is that you're gonna be looping 4 times.
func (a *scriptArena) grow(n int) {
need := max(n, len(a.cur)+1)
class := 0
for scriptChunkClasses[class] < need {
class++
}
chunk := scriptChunkPools[class].get()
a.chunks = append(a.chunks, chunk)
a.cur = *chunk
a.off = 0
}There was a problem hiding this comment.
Removed the class field all together. grow now will determine the chunk size itself with each call. Loops 4 time max as you note
kcalvinalvin
left a comment
kcalvinalvin
left a comment
There was a problem hiding this comment.
Ok so I did an artisanal review of the entire codebase and I'm convinced that the code is correct. But the scriptArena.alloc() issue I raised should be addressed.
However, there's a lot of tests here that could use some polishing. Also, the code quailty isn't that great either. I only raised the parts that I thought were absolute musts but overall I don't think it's great.
In this commit, we remove the per-arena class field and its starting class constants entirely. grow now derives the chunk size class from the request itself on every call, choosing the smallest class that holds both the allocation and one more byte than the current chunk, so consecutive growths keep walking up the ladder without any cursor state to maintain or reset across borrows, rewinds, and releases. The worst case is four iterations of the class lookup. This also removes the borrowScriptArena starting class parameter, since without a cursor there is nothing to seed: every arena starts at the smallest class and grows on demand. The rewind path no longer needs to reposition a class cursor after retaining the largest chunk, and release no longer needs to reset one. alloc additionally rejects negative sizes with errScriptArenaFull. The decode path never passes one, but the remaining budget check alone would let a negative size through and panic on the slice bounds below it, so the guard keeps alloc total on any input.
In this commit, we strengthen the decode budget tests so each one actually verifies the property its comment claims. The truncated vector test now asserts a decode reserves zero element storage rather than comparing against an always-zero maxCap column, and it covers MsgBlock.DeserializeTxLoc in addition to BtcDecode, closing the one count-driven vector the table missed. The streaming decode tests no longer silently take the preallocation path: a bare bytes.Reader exposes its remaining length, which let the decoders preallocate from the claimed counts. Both now wrap the reader in readSizeRecorder, which hides the remaining length, and assert the element vectors grew to only about twice the delivered count. The truncated frame test now measures total bytes allocated across one hundred truncated header reads and bounds them far below the claimed payload, covering reservation as well as the already-checked read sizes. The variable length prefix test gains a loop that claims the maximum payload while delivering progressively more bytes, asserting the staged reservation tracks what was delivered rather than the claim. The readSizeRecorder helper moves next to the other test readers and gains a doc comment explaining its two jobs: hiding the reader's remaining length to force streaming, and recording the largest read request to catch oversized speculative reads.
In this commit, we fix the doc comments that still describe a scratch buffer parameter named b. The Buf variants of the readers and writers require the caller to pass a non-nil buffer, so the old 'If b is non-nil' paragraphs described behavior these functions never had; each now states simply that buf must be at least an 8-byte slice. The truncated decode benchmarks gain comments explaining what each payload prefix represents in the message encoding, such as the 4-byte transaction version before the input count or the 80-byte block header plus 4-byte transaction count before the merkle hash count. The transaction input benchmark's prewarm comment also stops referring to 'this patch' and instead describes what the prewarm isolates: it warms the chunk pool so the benchmark measures only the allocation made while decoding rather than the one-time chunk creation. The fuzz seeds are now cloned before being added to the corpus. f.Add keeps a reference to the slice it is given, so reusing one buffer across seeds overwrote the coinbase seed with the multi-input transaction before the corpus entry ever ran, and the coinbase case was silently never asserted.
|
@kcalvinalvin PTAL!
If you have any substantial ideas here to improve the quality, I'm def interested. The greater aim here fixes a ton of issues w.r.t the way the old buffer pools worked, and significantly educes memory usage, allocations, and GC churns. As noted in the OP, there's a tradeoff perf hit, but we can win that back in other ways with further optimizations. |
kcalvinalvin
left a comment
kcalvinalvin
left a comment
There was a problem hiding this comment.
I don't see any blockers now.
| } | ||
| } | ||
| // Nothing else to do: grow derives the next class from the retained | ||
| // chunk's size, so there is no class cursor to reposition. |
| } | ||
| } | ||
|
|
||
| panic("putScriptChunk: chunk size matches no class") |
There was a problem hiding this comment.
Ok so we can't trigger this right now so the code is correct and I'm ok with it. But ugh, panics are ugly in code like this
I don't see any that are huge now but just the little things you know? Stuff that's definitely discouraged for artisanal code but I guess we can assume that clankers are writing code and they wouldn't be making those mistakes so it's fine. 🎶 The times they are a-changin 🎶 |
I think the name of the game is to be able to codify what we think a mistake is. There's basic correctness, but then ofc we can write more advanced linters to catch other more fuzzy style issues. IMO it's a matter of where you want to spend your time really. |
In this commit, we clarify that successful transaction decodes copy staged scripts into owned storage, while a failed decode can leave arena-backed scripts in the partially populated receiver. Callers must discard the receiver after an error.
In this commit, we clarify that a failed transaction's contents must not be used, while allowing a later successful decode into the same receiver. This matches the witness-to-legacy retry paths in btcjson and rpcclient.
In this commit, we clear TxIn and TxOut before a failed public transaction decode releases its script arena. This prevents the partially decoded receiver from retaining slices into memory that later decodes can reuse, while keeping the block decoder's internal path unchanged. The regression covers every truncated prefix in base and witness encodings with buffered and streaming readers, then checks a successful retry into the same receiver. We also inspect the internal decoder in the allocation-budget test so error cleanup cannot hide excessive reservations.
In this commit, we validate serialized transactions with a temporary MsgTx instead of decoding back into the caller's object. TestMempoolAcceptAsync now preserves the supplied transactions on both successful validation and local decode errors. The regression checks transaction contents and input/output identity, rejection without an outbound request, and the transaction bytes and fee limit queued for a valid request.
In this PR, we replace the fixed 4 MiB script decode slab with a tiered arena allocator. Today every transaction decode borrows the full slab, even when the transaction contains only a few hundred bytes of script data. The channel-backed slab pool can also retain up to 125 slabs, or roughly 524 MB, outside the runtime's GC pressure heuristics.
ReadTxOutgoes a step further and returns a script backed by its staging slab, which keeps the full allocation live with the output.The new arena draws from 16 KiB, 128 KiB, 1 MiB, and 4 MiB chunk classes. Standalone transactions and blocks both begin at 16 KiB. Block decoding shares one arena across the block and rewinds it after each transaction has copied its scripts into owned storage, so its live staging requirement is determined by the largest transaction rather than the aggregate block. Larger transactions promote through the chunk ladder on demand. The existing 4 MiB limit on staged bytes remains unchanged.
Pooling and ownership
Each size class uses a
sync.Pool, which keeps idle chunks visible to the runtime across GC cycles. The 1 MiB and 4 MiB classes also have bounded front caches of 16 and 8 chunks. Those caches cap deterministic retention at 48 MiB. Bursts beyond the front caches fall back tosync.Pooland remain reclaimable by the GC.Arena storage has one lexical owner. A successful transaction decode copies every signature script, witness item, and output script into message-owned storage before the arena is rewound or released.
ReadTxOutnow makes the same ownership transfer and returns an exact-sized script. Release is idempotent, and use after release is rejected until the arena handle is borrowed again.Benchmarks
The following results compare public master at
ca0bb02cfwith this branch at850492f46on an Apple M4 Max using Go 1.26.1 on darwin/arm64.The cold allocation measurements ran each benchmark once in a fresh process, repeated across ten processes:
Command, repeated ten times per benchmark and revision:
The one-iteration timing samples are dominated by process and first-use noise, so the cold comparison is limited to allocation footprint.
Steady serial decode used eight one-second samples:
The parallel benchmark functions are new in this branch. For the base comparison, the same
bench_parallel_test.goharness was applied unchanged to a detached master checkout. Ten base/head process pairs were run in alternating order:These parallel results expose the main throughput tradeoff for large transactions and blocks. The steady serial large-transaction and block paths are unchanged within noise, while parallel execution pays for allocator and pool coordination. The warm
B/opfigures do not include staging chunks retained by the pools, so they should not be read as peak or retained-memory measurements.Testing
The change adds table-driven allocator tests, Rapid property tests, concurrent decode coverage, and transaction/block fuzz targets. The ownership tests overwrite the exact chunks used by a decode before reserializing its result. The fuzz targets also reserialize the first decoded object after a second decode, checking that arena reuse leaves the earlier value stable.
The full repository unit suite, the wire suite under the race detector,
go vet, andgit diff --checkpass on this branch.