[r3.6] db/etl: split the sortable buffer into pooled 1MB chunks - #23519
[r3.6] db/etl: split the sortable buffer into pooled 1MB chunks#23519AskAlexSharov wants to merge 8 commits into
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%. Claude-Session: https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
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. Claude-Session: https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
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. Claude-Session: https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
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. Claude-Session: https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
release/3.6 does not carry the lazy-draw test that pulled the import in on main, and TestCloseDisposesProvidersBeforeBuffer builds its own pool. Claude-Session: https://claude.ai/code/session_01WFkAYPPhqPe1NXg41Nph78
awskii
left a comment
awskii
left a comment
There was a problem hiding this comment.
Reviewed at 3fa8075. Re-checked against the current head aa7cb36, whose only delta is dropping .Prealloc(...) from SmallSortableBuffers — that touches none of the points below, and line numbers are unchanged.
One thing that has no line to sit on: two test docstrings still name sortableBuffer.data, a field this change deleted — etl_test.go:620-621 ("GetRef returns zero-copy slices into sortableBuffer.data — appending to prevV without copying would corrupt adjacent entries in the buffer") and etl_test.go:923. The bytes now live in sortableBuffer.chunks, so grepping sortableBuffer.data to find what these tests protect returns nothing. The claim also understates the new failure mode: chunks are shared across collectors via dataChunks, so it is not only adjacent entries in one buffer. The first test builds NewAppendBuffer buffers, which never had a data field at all. (The GetRef half was already stale — the method is Get. The .data half is what this change broke.)
Worth noting the parent #23518 is still open on main, so this backport can still drift under review.
| // nextChunk starts a chunk able to hold n bytes. An entry never straddles | ||
| // chunks, so Get can hand out direct references. | ||
| func (b *sortableBuffer) nextChunk(n int) { | ||
| if len(b.chunks) >= maxDataChunks { |
There was a problem hiding this comment.
The chunk-count guard is reachable, so the comment at :108-110 ("one buffer addresses 2GB — the ceiling NewSortableBuffer already puts on optimalSize") does not hold.
maxDataChunks is 2048, nextChunk panics on the 2049th, and extractNextFunc calls Put before CheckFlushSize, so the panic fires before the collector can flush.
The band is narrow. Size() charges the full chunk for every filled chunk, so the only slack is the current chunk's tail: the cheapest way to hold 2048 chunks is 2047 entries of exactly 1MB plus one 1-byte entry, giving Size()==2,146,467,841. The panic therefore needs optimalSize in (2,146,467,841, 2,147,483,647] — a 1,015,806-byte band at 2047.03 MiB, the top 0.05% of what NewSortableBuffer accepts. ETL_OPTIMAL=2047MB is safe; ETL_OPTIMAL=2GB already panics in NewSortableBuffer. No production caller goes near it — every one uses BufferOptimalSize or etlSmallBufRAM.
Crash rather than silent corruption, and only under a hand-picked config. Worth adjusting the comment's claim rather than the guard.
There was a problem hiding this comment.
Fixed on main in a997860a01 (#23518): corrected the comment to note nextChunk's panic is reachable near the top of optimalSize's range, not fully precluded by NewSortableBuffer's MaxInt32 bound.
| var BufferOptimalSize = dbg.EnvDataSize("ETL_OPTIMAL", 256*datasize.MB) /* var because we want to sometimes change it from tests or command-line flags */ | ||
|
|
||
| // 3_domains * 2 + 3_history * 1 + 4_indices * 2 = 17 etl collectors, 17*(256Mb/8) = 512Mb - for all collectros | ||
| // etlSmallBufRAM (BufferOptimalSize/8) bounds the flush threshold so a full |
There was a problem hiding this comment.
The same rationale is now written three times in this diff: here, on dataChunks at :113-114 ("a buffer takes chunks as it fills and gives them back on Reset, instead of pinning its peak size forever"), and on Allocator.Put in collector.go:48.
.claude/rules/comments.md names this case: state the why once at the canonical place, terse pointers elsewhere. The canonical place is the dataChunks var 27 lines below — etlSmallBufRAM is a flush-threshold knob and pooling is not its subject.
The rewrite also drops 3_domains * 2 + 3_history * 1 + 4_indices * 2 = 17 etl collectors, 17*(256Mb/8) = 512Mb, which was the one thing a reader could not re-derive from the code. It is now "~17 per batch writer" with no way to check the number as the domain set changes.
There was a problem hiding this comment.
Fixed on main in a997860a01 (#23518): kept the canonical explanation at dataChunks, reduced etlSmallBufRAM's and Allocator.Put's comments to terse pointers, and restored the 17-collector derivation (3_domains2 + 3_history1 + 4_indices2 = 17, 17(256Mb/8) = 512Mb).
| } | ||
|
|
||
| func (b *sortableBuffer) Size() int { return len(b.data) + len(b.entries)*entryLocSize } | ||
| // Size counts the stored bytes plus the tails wasted by the chunks already filled. |
There was a problem hiding this comment.
"Size counts the stored bytes plus the tails wasted by the chunks already filled" omits the third term. The body returns b.chunkBytes - (len(b.cur) - int(b.curOff)) + len(b.entries)*entryLocSize — 16 bytes of entryLoc metadata per entry.
It moves the flush point, since CheckFlushSize compares Size() against optimalSize. With many small entries it dominates: a 32MB etlSmallBufRAM buffer holding 8-byte keys and 8-byte values flushes at roughly 1M entries, where metadata is 16MB of the 32MB budget. Anyone tuning ETL_SMALL from this comment mis-predicts the flush point by 2x.
This is the comment added to answer Copilot's round-1 note on #23518 that the old "tracks RAM held" wording did not match the accounting. It still doesn't fully.
There was a problem hiding this comment.
Fixed on main in a997860a01 (#23518): Size()'s comment now names all three terms, including the entryLocSize-per-entry metadata.
| func getDataChunk() []byte { return *dataChunks.Get().(*[]byte) } | ||
|
|
||
| func putDataChunk(c []byte) { | ||
| if len(c) != dataChunkSize { // private chunk of an oversized entry |
There was a problem hiding this comment.
if len(c) != dataChunkSize { return } is the only thing keeping an oversized chunk out of the process-wide dataChunks pool, and no test exercises it.
If it regressed — guard deleted or comparison flipped — a chunk of len dataChunkSize+k gets recycled. A later nextChunk sets b.cur to it, and Put's bounds check int(off)+n > len(b.cur) then lets off reach dataChunkSize and beyond. e.offset = b.curBase | off is an OR, not an add, so an off with bit 20 set flips the chunk-index field and entryData reads from chunks[idx|1] at the wrong position. Silently mis-sorted or mis-written ETL output on the state and commitment path — not a crash.
Existing coverage doesn't reach it. TestSortableBufferOversizedEntry is the only test that Puts an entry larger than one chunk and it never calls Reset, so putDataChunk is never invoked with a private chunk; TestSortableBufferResetReleasesChunks uses 16KB values. Deleting the guard leaves the whole db/etl suite green.
There was a problem hiding this comment.
Fixed on main in a997860a01 (#23518): added TestDataChunkPoolRoundTrip and TestPutDataChunkRejectsOversized. The latter goes red if the size guard in putDataChunk is removed (verified locally before adding it).
| require.Equal(t, wantV, gotV) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The docstring says "Reset hands the chunks back to the shared pool, so an idle pooled buffer doesn't pin the RAM it once needed", but every assertion is about the buffer's own state — Empty(buf.chunks), Zero(buf.Size()), Zero(buf.Len()), and a reuse round-trip. None observes dataChunks.
Delete the putDataChunk(c) call from Reset and this stays green: chunks get dropped for the GC, every buffer allocates fresh 1MB chunks each cycle, and the allocation win this PR exists for is gone with no test failing.
TestReuseCollectorAfterLoad has the same gap — its new comment says "data chunks went back to the pool" while the assertions only check buf.chunks is empty and the scalars were cleared.
There was a problem hiding this comment.
Fixed on main in a997860a01 (#23518): reworded TestSortableBufferResetReleasesChunks's and TestReuseCollectorAfterLoad's comments to only claim what their assertions check (the buffer's own state); the pool round-trip is now covered separately by TestDataChunkPoolRoundTrip.
|
Addressed on main in a997860a01 (#23518) — replies posted on each inline thread. Also fixed the two stale This backport (#23519) is left untouched, since fixing review points on a backport rather than on main would make the release branch carry a variant main doesn't have. #23518 is still open on main, so the fix landed directly on its branch rather than a separate follow-up PR. |
|
Correction: my earlier follow-up added |
2 participants
Cherry-pick of #23518 to release/3.6.
r3.6-specific adaptations
TestCollectorWithAllocatorDrawsBufferLazily— it pins the lazy-draw contract from db/etl, tools: draw pooled ETL buffers lazily to cut race-shard memory #22929, which release/3.6 does not carry (NewCollectorWithAllocatorstill draws its buffer eagerly).syncimport toetl_test.go; on main that test file already had it.LargeSortableBuffersPrealloc(1_024, etlLargeBufRAM/32)call, which main dropped in db/etl, tools: draw pooled ETL buffers lazily to cut race-shard memory #22929.SmallSortableBufferstakes the new arguments from the parent PR.