db/etl: split the sortable buffer into pooled 1MB chunks - #23518
Merged
Conversation
sortableBuffer held every key/value byte in one slice, so a Collect that outgrew it re-allocated and copied everything collected so far - for a 256MB buffer that is ~256MB of memmove and ~512MB live at the peak. Store the bytes in 1MB chunks drawn from a shared sync.Pool: Put never copies what is already there, Reset hands the chunks back, and an idle pooled buffer no longer pins its busiest run's RAM. entryLoc.offset packs the chunk index with the offset inside the chunk; an entry never straddles a chunk, so Get still returns direct references. An entry larger than one chunk gets a private chunk of its own. BenchmarkCollect (no Prealloc, the path collectors actually take): 10k_largebuf 518.8µs -> 116.8µs -77% 8.7MB/op -> 0.7MB/op 100k_largebuf 3.286ms -> 1.137ms -65% 87MB/op -> 8.5MB/op Cost of the extra indirection, both on prealloc'd buffers that never grow: Put +11%, Sort +8..24%.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors db/etl’s sortableBuffer to avoid costly grow-and-copy behavior by storing key/value bytes in fixed-size pooled chunks, reducing peak allocations and memmove overhead during Collect growth. It also adjusts pooling behavior so idle buffers don’t retain peak-run data capacity.
Changes:
- Replace the monolithic
dataslice insortableBufferwith 1MB pooled chunks and packed offsets for chunk+in-chunk addressing. - Ensure pooled buffers release chunk memory on
Reset(including when returned to anAllocatorpool). - Add unit tests that pin the chunked layout behavior, oversized-entry handling, and chunk release on
Reset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/etl/buffers.go | Implements chunked storage, packed offsets, and sync.Pool chunk reuse; updates Size, Get, Sort, and Write to work with chunked layout. |
| db/etl/collector.go | Resets buffers on allocator Put to release chunk memory before pooling. |
| db/etl/etl_test.go | Updates reuse assertions and adds new tests validating chunking behavior and reset/pool release semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AskAlexSharov
marked this pull request as ready for review
August 23, 2026 10:03
Close released the buffer first, and a KeepInRAM provider reads straight from it. That was harmless while Reset only truncated the buffer's own slice; now Reset hands the chunks to a pool other collectors draw from, so any read after Close would race a different goroutine's writes.
Look up chunk bytes through the chunks slice itself. The parallel []unsafe.Pointer bought ~12 points of Sort geomean by saving a slice-header load per key, but it duplicated ownership of every chunk to buy back part of a cost the chunking itself creates.
AskAlexSharov
marked this pull request as draft
August 23, 2026 10:34
20 and 1<<20 read as magic: the power of two is what lets entryLoc.offset pack the chunk index with the in-chunk offset, and the index range is what caps a buffer at 2GB.
AskAlexSharov
marked this pull request as ready for review
August 24, 2026 02:20
- Correct maxDataChunks' comment: the panic is reachable near the top of optimalSize's range, not fully ruled out by NewSortableBuffer's bound - Consolidate the pooling rationale at dataChunks; restore the 17-collector derivation dropped from etlSmallBufRAM's comment - Complete Size()'s comment with the entryLoc metadata term - Fix stale sortableBuffer.data/GetRef references in test docstrings - Add TestDataChunkPoolRoundTrip and TestPutDataChunkRejectsOversized to cover putDataChunk's oversized-chunk guard, which had zero test coverage
TestDataChunkPoolRoundTrip assumed a same-goroutine Put/Get pair on sync.Pool round-trips deterministically. It doesn't: under -race CI it failed even pinned to GOMAXPROCS(1) with GC disabled, so the pool is dropping or missing the item for reasons outside our control (Go's own sync.Pool tests reach for an internal-only pin hook to make this reliable, which isn't available outside the sync package). TestPutDataChunkRejectsOversized stays: since a rejected oversized chunk never reaches dataChunks.Put, whatever getDataChunk returns afterwards is necessarily dataChunkSize-length regardless of how the pool internally schedules retrieval, so that assertion is not subject to the same flake.
AskAlexSharov
force-pushed
the
alex/etl_chunked_buf_37
branch
from
August 24, 2026 08:08
dc2fd9d to
3ebbb54
Compare
This was referenced Aug 24, 2026
Open
awskii
reviewed
Aug 26, 2026
awskii
left a comment
awskii
left a comment
Member
There was a problem hiding this comment.
Reviewed 3ebbb54. Ran both arms myself on an M5 Max, base 0d4a82d, -benchtime=50x -count=10, benchstat:
| case | base | PR | |
|---|---|---|---|
| 10k_largebuf | 522.8µs / 9.08MB | 120.4µs / 0.74MB | -77% / -92% |
| 100k_largebuf | 3.616ms / 90.9MB | 1.166ms / 9.00MB | -68% / -90% |
| 100k_smallbuf | 16.81ms ± 4% | 19.27ms ± 4% | +14.7% (p=0.000) |
The largebuf numbers reproduce and are slightly better than the body claims. The configs that actually flush to disk go the other way, and it isn't flush overhead: a 4MB-buffer case regresses +14.3% with 4 flushes against +14.0% for the 256KB case with 61 flushes — same percentage, 15x fewer flushes. It's Sort; details inline.
go test ./db/etl/... passes with and without -race, go vet clean.
pdqsortCmpFunc calls the comparator indirectly, so a separate key closure never inlines there and costs a real call per key, twice per comparison. Inlining it into cmp on n5: Sort/random_100k -14.38%, Sort/random_500k -12.47%, geomean -6.99%. Also fix Size's docstring, which named the one term Size subtracts: the tails of the chunks already filled are counted, the tail of the chunk still filling is not. And say in Prealloc that predictDataSize only sizes the chunk-pointer slice, since the chunks themselves are still taken one at a time.
awskii
approved these changes
Aug 26, 2026
awskii
left a comment
awskii
left a comment
Member
There was a problem hiding this comment.
Reviewed 4cdecf2. The fold is verbatim the arm I benchmarked — +3.34% (p=0.016) vs base on 100k_smallbuf, down from +14.08%. The residual is the chunk indirection, fair price. Size and Prealloc read right now.
Approving. Three non-blocking things inline, plus a pre-existing one the new Close comment leans on.
Review follow-ups: correct the combined ETL budget to 544Mb, test the sort comparator past chunk 0, assert the pool-eligibility decision instead of pool internals, and wait for the async flush before Dispose returns on a nil file.
AskAlexSharov
added a commit
that referenced
this pull request
Aug 27, 2026
Put grew the entry index by appending to one array per buffer, so it re-allocated and copied as the buffer filled. Each 1MB chunk now carries the index of the entries it holds: data grows up from the front, the index down from the back, and the chunk is full when they meet. Put allocates nothing, and a buffer's whole footprint is the chunks it took. An index slot is a uint32 - 20 bits of chunk-local offset and 12 bits of key length, biased so nil and empty both fail the comparator's test - so it addresses only bytes inside its own chunk. Sort therefore orders each chunk on its own and reading the buffer merges the runs, over a heap of chunk ids keyed on an 8-byte big-endian key prefix. Chunks that already run in order end to end, which ascending keys produce, skip the heap. Squashed from the work on alex/etl_entry_offset_37 and the first half of alex/etl_chunk_sort_37: those commits were written against #23518 before it was squash-merged, so they no longer apply to main one by one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
problem:
etl.Collectdoes expensive re-alloc inside. It's critical because now it happening insideSD.mucritical section.Solution:
dataarray of sortable buf to 1mb chunkschunk, never growchunkBenchmarkCollect(noPrealloc— the path collectors actually take):context: reducing amount of expensive
re-allocsinsideSD.mucritical section (part ofrelease/3.6_newPayloadperf jumps problem)