db/etl: store an entry as one packed offset - #23576
Closed
AskAlexSharov wants to merge 24 commits into
Closed
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%.
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.
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.
- 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.
entryLoc spent 16 bytes on four int32s. insertionOrder was redundant: offsets rise monotonically with insertion order, so comparing them orders duplicate keys and unstable SortFunc is still enough. valLen moves into the chunk as a native uint32, which is a load rather than a parse. What is left fits one uint64 - keyLen in the high half, the packed chunk offset in the low half - so an entry costs 8 bytes of array plus 4 in its chunk instead of 16, and the array keeps a power-of-two stride. Sort still reads keyLen straight from the array. Every entry now writes its valLen, so entries whose key and value are both empty get distinct offsets instead of all colliding at 0.
AskAlexSharov
force-pushed
the
alex/etl_entry_offset_37
branch
from
August 26, 2026 05:32
ea2902e to
a8ff5f7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Packs ETL entry metadata to reduce allocation overhead in sortable buffers.
Changes:
- Packs key length and offset into one
uint64. - Stores value length alongside chunk data.
- Adds empty-entry and chunk-boundary tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
db/etl/buffers.go |
Implements packed entry locations and value-length headers. |
db/etl/etl_test.go |
Tests empty entries and chunk boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Size() charges entryLocSize per entry against the same byte budget as the key/value bytes, so the entry count a buffer reaches before it flushes is bounded by both. Pool the buffers with that count already allocated instead of climbing to it in 1.25x steps, which copies several times the final size on the way. The estimate is deliberately below the observed average: bigger entries flush before they reach the predicted count, smaller ones run past it and re-grow.
maxKeyLen keeps a key inside a pooled chunk beside its header, so only a value can outgrow one - which is what Sort assumes when it reads keys straight out of a chunk. It also keeps len(k) clear of the int32 the length is stored as, where a wrap would read back as nil. Both new tests fed Sort input that was already in comparator order, so IsSortedFunc short-circuited and SortFunc never ran. They now insert out of order, the chunk-boundary case covers value lengths that land an entry's last byte exactly on a chunk edge, and the empty-entry case asserts the invariant the tie-break rests on: offsets rise strictly with insertion order.
maxKeyLen bounds a key to 4096, so the length needs 16 of the 32 bits it had. Narrowing it leaves the top 16 spare and makes the packing's budget explicit.
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.
# Conflicts: # db/etl/buffers.go
AskAlexSharov
marked this pull request as ready for review
August 26, 2026 08:24
AskAlexSharov
requested review from
lupin012,
taratorio and
yperbasis
as code owners
August 26, 2026 08:24
AskAlexSharov
requested review from
anacrolix,
bloxster,
domiwei,
lystopad,
mh0lt,
mriccobene and
sudeepdino008
as code owners
August 26, 2026 08:24
…ex/etl_entry_offset_37
awskii
reviewed
Aug 26, 2026
awskii
left a comment
awskii
left a comment
Member
There was a problem hiding this comment.
Four invariants this change touches aren't pinned. All four check out — by inspection and by throwaway tests I ran and deleted — so this is coverage, not defects.
Sort()is never run against an oversized/private-chunk entry. That's the intersection of both halves of this change: the packed offset space for private chunks, andcmpreading through the new accessors.TestSortableBufferOversizedEntry(1607) only does Get/Write.- Nothing sorts a maximal 4096-byte key.
TestSortableBufferRejectsOversizedKey(1764) Puts and Gets one, never sorts. - No
Reset()→ second Put/Sort generation with duplicate keys.TestSortableBufferResetReleasesChunks(1638) puts one entry post-reset and never sorts. Size()'s 12-bytes-per-entry accounting — the headline claim — isn't asserted numerically. Existing tests only checkSize() == 0on an empty buffer.
Separately, buffers.go:91: "they are-preallocated", stray hyphen.
# Conflicts: # db/etl/buffers.go # db/etl/etl_test.go
Collaborator
Author
|
closing in favor on #23599 |
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.
Stacked on #23518.
context: reducing amount of expensive
re-allocsinsideSD.mucritical section (part ofrelease/3.6_newPayloadperf jumps problem)problem:
entriesarray in ETL re-alloc on growSolution:
insertionOrderwas redundant — offsets rise monotonically with insertion order.valLento another array (todata)keyLenandoffsetinto u64entriesarray always pre-allocSide effect: every entry now writes its
valLen, so entries whose key and value are both empty get distinct offsets instead of all colliding at 0.n5 (AMD EPYC 4344P, idle), 3 interleaved A/B rounds of
-count=2, vs the #23518 head after its key-fold. Both arms built from the same tree with onlydb/etlswapped. All rows +-0..2%:An earlier revision of this body quoted ~-6% on the random
Sortrows. Thatwas measuring the un-inlined key closure, which #23518 has since folded into
the comparator - the win belonged to the parent, not here. On its own the u64
packing costs ~1.5%.
What remains is memory: 12 bytes of metadata per entry (8 in
entries+ 4 inthe chunk) instead of 16, so a buffer holds more entries before it flushes.
On the pre-alloc: at
Prealloc(512, ...)the entries slice still climbs in1.25x steps, ~35 grows to reach ~1M entries, so the re-alloc cost in the
contextline above is not actually removed. Sized to the entry count abuffer reaches, n5
stage_execreportsetl_buffer_entries_grow_total0 over2291 blocks. Measured cost of that sizing:
db/state -shortcreates 54 pooledbuffers at 9.14MB each, with peak RSS unchanged (751MB vs 758MB) and wall time
unchanged (13.3s vs 13.4s).