db/etl: move entry index inside chunk, so Put never re-alloc by AskAlexSharov · Pull Request #23599 · erigontech/erigon

AskAlexSharov · 2026-08-26T11:40:31Z

context: reducing amount of expensive re-allocs inside SD.mu critical section (part of release/3.6 _newPayload perf jumps problem)

problem: we moved sortableBuffer to 1MB pre-allocated chunks and it works well. But we still need some metadata arrays in sortableBuffer - and this arrays have unknown size and re-alloc during exec. But this metadata was introduced to ETL when it worked with very-large-buffers (now all of them are 1MB). It means very likely we can drop metadata or embed metadata into chunk or something else.

solution: each 1MB chunk carries the index of its own entries — bytes grow up from the front, the index down from the back, and the chunk is full when they meet.

Also changed:

Buffer.Get(i) becomes Next()
Side effect: MaxKeyLen is 4094
into heap.go, which already held the one mergeSortFiles runs over data providers.

This PR concentrating on Put perf. But there is follow-up which focus on Sort/Load/Write speed: #23616

n5 (EPYC 4344P, idle, 6 interleaved A/B rounds), vs main:

	sec/op	B/op
`Sort/random_500k`	-51.7%
`PutSort/random_500k`	-51.7%	-51.1%
`Sort/random_100k`	-28.7%
`PutSortLoad/random_500k`	-18.6%	-18.5%
`PutOnly/sorted_500k`	-15.3%	-8.7%
`PutOnly/random_500k`	-11.3%	-3.4%
`Sort/sorted_*`	-12%
`LoadOnly/random_500k`	+27.7%	-8.8%
geomean	-13.8%

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.

nextChunk built make([]byte, size) before testing whether a pooled chunk would do, so every 1MB chunk was allocated twice and one copy handed straight back to the GC. BenchmarkSortableBufferPutOnly/sorted_500k allocated 51MB per op where 150KB was enough. The garbage drained both sync.Pools it depends on, so the chunk pool started missing and bufiopool handed out fresh 512KB writers, which is what made a flush inside Load allocate. On EPYC, vs the previous head: PutOnly -30 to -40%, PutSortLoad -4.5 to -24%, Sort/sorted_500k -5.5%, and allocation down 99.7% on the Put benchmarks.

entryAt walked the chunks in insertion order whenever anything had been put since the last Sort, but Sort has already permuted the in-chunk index, so after a Put the walk read the sorted part backwards: put 5,3,9,1, Sort, put 7, and Get(0..4) gave 9,5,3,1,7. Sorting instead of walking drops the second ordering entirely. Every production reader already sorts first - sortAndFlush and KeepInRAM both do - and the other two Buffer implementations return nothing at all before Sort, so key order is the contract the interface already had. Three tests read a buffer they never sorted and happened to depend on insertion order; they assert what they were after (the varint round trip, one offset per entry, the maxKeyLen edge) without it now.

maxDataChunks dated from entryLoc packing the chunk index; nothing has constrained chunk count since. Worse, the panic was reachable: the cap is 2048 and ETL_OPTIMAL=2047MB is a legal setting, since NewSortableBuffer only rejects above MaxInt32. chunkSizeFor's second return said size == dataChunkSize, which is what isPooledChunk already tests, so nextChunk can ask that itself.

Get(i) had one production caller, memoryDataProvider, and it walked i upward one step at a time. Reading a sorted buffer is a merge, so every Get had to reconcile i against the merge cursor: an index compare, a rewind branch, a catch-up loop, and for chunks already in order a starts array to search. Next drops all of it, along with the provider's own currentIndex. Sort now positions the cursor whether or not it had anything to sort, so a second read is Sort then Next again. Write on EPYC-sized data, before and after: sorted_500k -6%, random_500k -7%. Write keeps its own varint code rather than calling writeField, which does not inline.

Three things widened the diff without carrying the change: writeSortedEntries was refactored into a writeField helper that sortableBuffer.Write does not even call, the small-buffer pool grew a Prealloc call and a comment that no longer described it, and BenchmarkSortableBufferInmemLoadOneChunk was a smaller copy of the benchmark below it. Comments trimmed to the ones that carry a why: the biased key length, the big-endian prefix, the index alignment entries() relies on, the codegen notes in Put and dataChunk.sort, and siftRoot's one compare a level. The rest restated the code. Also renames BenchmarkSortableBufferGet to ...Read, since Get is gone.

Copilot

Pull request overview

Moves ETL entry indexes into fixed-size data chunks to avoid reallocations during Put.

Changes:

Packs entry metadata into chunk-local indexes.
Adds per-chunk sorting and heap-based merging.
Replaces indexed Get access with cursor-based Next.

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 chunk-local indexes, sorting, merging, and cursor reads.
`db/etl/dataprovider.go`	Reads in-memory buffers through `Next`.
`db/etl/etl_test.go`	Updates tests and benchmarks for the new buffer design.

Suppressed comments (1)

db/etl/buffers.go:780

Next returns EOF after entries are added unless Sort was called explicitly, and a new unique key added after sorting is omitted from the iteration. This violates the new Buffer.Next auto-sort contract; mark successful inserts dirty and have Next sort/rewind when dirty.

func (b *oldestEntrySortableBuffer) Next() ([]byte, []byte, bool) {
	if b.at >= len(b.sortedBuf) {
		return nil, nil, false

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Buffer.Next says it sorts first if it has to, but only sortableBuffer did. appendSortableBuffer and oldestEntrySortableBuffer read sortedBuf straight out, so Next returned nothing at all until the caller called Sort, and returned the previous run after a later Put. Not a regression - Get(i) read the same stale sortedBuf - but the three implementations have to agree now that the interface promises it. Their len(entries) cannot stand in for a dirty flag, since appendSortableBuffer Put appends to a key already present.

Making Next a cursor introduced two ways to lose or repeat entries. Write drove the same cursor Next does, so a flush after a partial read emitted only what was left: ten entries put, two read, eight written. sortAndFlush happened to be safe because it sorts first, and the short file would have made mergeSortFiles panic on a provider whose first Next gives EOF. Write now sorts, which also rewinds, so it always writes the whole buffer however the cursor was left. Next called Sort itself when anything had been put since the last one, and Sort rewinds - so a Put between two Next calls silently restarted the read: reading 3 of 5 then putting one gave 0 1 2 0 1 2 3 4 9. A loadFunc feeding its own collector would duplicate rows. Next now panics instead, which also makes the map-backed buffers loud rather than returning nothing at all, and drops the auto-Sort the two production callers never needed.

chunksInOrder skipped a chunk holding nothing, which also skipped the comparison between the chunks on either side of it, so concat mode could be entered on chunks that were out of order and read them unsorted. It now carries the last non-empty chunk forward. Unreachable today, since nextChunk is always followed by a Put that succeeds. rewind reslices the cursors through slices.Grow, which overwrites only the new length: a buffer that once held 300 chunks and now holds 3 kept 297 cursors pointing at chunks it no longer owns. Reset handed the chunks to the shared pool before dropping the cursors that alias them. nextChunk's overflow message named MaxInt32-4, but the condition trips at MaxInt32-6; it now reports the chunk size that overflowed.

An entry's key length shares a uint32 with its chunk-local offset, so Put panics above 4094 bytes. seg turns dictionary patterns into etl keys in extractPatternsInSuperstrings, bounded by cfg.MaxPatternLen - which defaults to 128 but is operator-overridable through the MaxPatternLen env var, so raising it past the bound would panic mid-compression. Exports the bound as etl.MaxKeyLen and rejects the config in NewCompressor, where every compression run passes and an error can still be returned.

putDataChunk took the slice by value and put &c back, so returning a chunk heap-allocated a slice header: 256 of them per Reset on a full 256MB buffer. The chunk now keeps the *[]byte the pool gave out. That also replaces isPooledChunk: a chunk with no ref is one that was sized for a single oversized entry, and only a ref can go back, so the pool can no longer be handed a chunk of the wrong size by a length test that happened to match. Adds a test for entries() over a private chunk, whose length comes from chunkSizeFor rather than being a round 1MB - unsafe.Slice was uncovered there. Folds dataChunk.len into its one caller, inlines keyLenBits, and splits entryLocAlign out of entryLocSize, which was serving as both a slot size and an alignment.

cmp spelled out keyOf's three lines twice, under a comment saying a separate function would never inline. That was written about an earlier closure which captured chunks; keyOf is a leaf the compiler does inline. The pre-scan calls bytes.Compare directly for the same reason - equal keys leave the offsets descending, which it already accepts, so it does not need cmp's tie-break. Sort/random_100k on M4, 5 rounds of 20: 9.025ms before, 9.016ms after. Taken for the ten lines, not the time.

heap.go held one k-way merge already - the one mergeSortFiles runs over data providers - so the merge over a buffer's sorted chunks belongs beside it rather than in the middle of buffers.go. Moves merger, cursor and keyPrefix across; buffers.go loses 180 lines and keeps the chunk format. siftRoot now takes the index to sift from, which is what rewind needed siftDown for: sinking a hole to a leaf and climbing back is also the bottom-up heapify step, so one function does both. The climb stops at the subtree root it started from - a mutation dropping that bound still passes the suite, so the bound is there for the contract, not because a current caller reaches it. Adds TestMergerMatchesReferenceSort: random keys over many chunks, checked against a stable sort of the same pairs and against the heap invariant directly, since a broken heapify need not show in the order.

Write sorts before it writes, so it always emits the whole buffer. For sortableBuffer that is nearly free - the chunk sort sees sortedN == n and only the cursor is rewound - but appendSortableBuffer and oldestEntrySortableBuffer had no such guard: every Sort flattened the map into sortedBuf and sorted it again. sortAndFlush calls Sort and then Write, so each flush of those two did all of it twice. Sort now rebuilds only when a Put has marked the buffer unsorted, and otherwise just puts the cursor back. Prealloc replaces the entry map but kept the run flattened out of the old one, the read cursor into it, and the byte count: Len reported 0 while Next handed back the previous contents and CheckFlushSize measured a map that no longer existed. Adds a test per bug; both fail against the code without the fix.

…king Sort both ordered the entries and moved the read cursor, which is what made Write consume a read in progress and what let a Sort in the middle of one silently restart it. Sort now only orders, Rewind only positions, and Write does Sort then Rewind so it always writes the whole buffer. KeepInRAM rewinds for the same reason - it hands the buffer to a provider that reads from the start. Re-reading is Rewind, not a second Sort. Collect returns an error for a key past MaxKeyLen rather than letting Put panic: it sits under Load and the stage loop, which return errors, so a long key should fail the stage and not the process. seg keeps its own check, moved to Cfg.Validate so DictionaryBuilderFromCollectors - the other entry point that builds a pattern-keyed collector - inherits it. Also: - chunkSizeFor bounds n before rounding. Both additions wrap for n near math.MaxInt, and a wrapped size came back small enough to pass for a pooled chunk. It returns 0 now, and nextChunk panics on that. - nextChunk asserted the chunk pointer's alignment, which the allocator already guarantees for anything this size. What entries() depends on is len(buf) being a multiple of the slot, since entTop starts there and only moves by it. That is what it checks now. - Reset dropped curBuf three lines after handing the chunks to the pool; it aliases one of them, same as the cursors above it. - flushBuffer sized the next buffer from SizeLimit, a constant, so the reserve never tracked the load. Size is now on the Buffer interface, where CheckFlushSize already implied it.

Validating seg's MaxPatternLen against etl's key bound made a compressor config answer for an etl storage detail. Collect returns an error for a long key now, which reaches every caller rather than the two entry points seg happens to have, so the seg-side check is gone and db/seg is back to untouched. maxKeyLen is unexported again with nothing outside needing it. The guard itself cost 3.7% on BenchmarkCollect/10k_largebuf and 5% on 100k_largebuf, because building the error inline spent extractNextFunc's inlining budget on a path that never runs. Out of line it measures as noise: 74.6us against 74.3us, and 757us against 768us.

chunkSizeFor returns dataChunkSize or a size already rounded up to entryLocAlign, so the panic guarding entries()' unsafe.Slice could not fire. TestChunkSizeFor asserts the property on every size the function can return, which covers more than the branch did. Renames mustBeSorted to panicIfUnsorted, so the name matches what the argument says.

A buffer fills, then Sorts, then is read. Put after Sort panics, and so does Next or Write before it. That removes the three ways the old shape could go wrong quietly - a Put restarting a read, a Write consuming one, a read handing back the previous run - without needing a Rewind method on the interface or a Sort call inside Write. Sort positions the cursor, so Sorting again is how a buffer is read twice. Write asserts rather than sorting, as on main, where sortAndFlush already sorts before it writes. The Put guard costs nothing: it folds into the single test Put already makes before going out of line. PutOnly/random_500k 2.823ms against 2.829ms, sorted_500k 2.515ms against 2.531ms. Also drops Size from the Buffer interface - it counts something different in each implementation - which puts flushBuffer's Prealloc call back to main's line, and renames Put's locals to main's: kLen and vLen for the lengths, keyLen and valLen for what gets stored.

The merge compared cached 8-byte key prefixes before full keys, and its sift sank a hole to a leaf and climbed back to spend one compare a level instead of two. Both are speed, not correctness, and they belong with the same two tricks #23616 gives the provider merge - one PR about the chunk format, one about making both merges fast. less does a bytes.Compare now, and siftRoot is the plain top-down sift. On EPYC that costs PutSortLoad/random_500k 19.7% and random_100k 9.9%; keys arriving in order are unaffected, since chunks already in order end to end skip the heap. Sort, Put and Collect do not change.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

AskAlexSharov · 2026-08-27T06:31:42Z

+	// less one for the bias. Sort slices a key out of its chunk, so only a
+	// value may outgrow one. Collect turns a longer key into an error before
+	// it reaches Put, which panics.
+	maxKeyLen = 1<<(32-dataChunkBits) - 2


Not adding it back — that coupling was removed deliberately after review: a seg.Cfg should not have to answer for an etl storage detail, and validating it in NewCompressor only ever covered the entry points seg happens to have.

The guard now lives where every caller reaches it: Collector.Collect returns an error for a key past the bound, so any collector with long keys fails rather than panicking. That is strictly wider coverage than a compressor-side check, and it is why maxKeyLen is unexported again — nothing outside etl needs to know the number.

Two corrections to the finding. The PR body does not advertise a compressor guard; it says a key is capped and Collect errors above it. And the "silently dropped, may flood logs" shape is extractPatternsInSuperstrings logging any Collect error and continuing, which predates this PR and applies to every error Collect can return — worth fixing in db/seg on its own terms, not by adding a config check here.

For what it is worth I did verify the mechanism before removing it: l <= maxPatternLen at parallel_compress.go:1085 and dictKey[:l] is the etl key at :1129, so patterns really do become keys. That is the argument for guarding Collect, not Cfg.

Put added the bias and keyLen subtracted it, so half the encoding lived at the call site. makeEntryLoc takes the key length as main states it, -1 for nil, and biases it itself. Put's two lengths become symmetric. PutOnly/sorted_500k over alternating rounds: 2.557ms before, 2.550ms after.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…wo benches The allocation test skipped itself under -race, which the repo forbids outside the testing.Short partition. It was copied from a test that needs it; this one does not - the region it measures never touches sync.Pool, and it passes under -race unchanged. panicIfUnsorted guards Write as well as Next, so "Next before Sort" named the wrong call. It says "read before Sort" now. BenchmarkSortableBufferRead did the real per-chunk sort inside its first timed iteration, which dominates when b.N is small; it sorts once before the loop, so an iteration is the rewind plus the read. And BenchmarkSortableBufferPutOnlyCold is only cold on its first iteration, since Reset hands the chunks straight back to the pool - its docstring says so rather than claiming a cold-start cost.

…x/etl_chunk_sort_37 # Conflicts: # db/etl/buffers.go

readFieldBufioU16 and U32 read the length prefix into a local array and hand it to io.ReadFull, which takes an io.Reader - so the array escapes and every field read costs an allocation. The mmap readers next to them slice the mapping directly and pay nothing, so the comparison the file exists to make was handing the bufio side a penalty that is not about buffered I/O at all. The caller owns lenBuf now. BenchmarkSequentialRead/val32/bufio_u16, two alternating rounds each: 57.3ms and 59.5ms before, 42.8ms and 42.0ms after - 8.4MB over 3947589 allocations becomes 525KB over 7.

w.Write takes an io.Writer, so the local array escaped and Write cost an allocation every time it was called - once per flush, which a stage_exec heap profile shows as half a megabyte over the run. The array lives on the buffer now, which is on the heap already. BenchmarkSortableBufferWrite goes to 0 allocs/op on all three cases. PutOnly/random_500k is unmoved by the extra ten bytes in the struct: 2.885ms against 2.885ms over alternating rounds.

curBuf/curEnd/curTop said neither "chunk" nor which one, and "cur" read as the cursor the merge keeps. The hoisted copy is a dataChunk, so it is one now: openChunk, the last of chunks and the only one still being written. dataChunk gains the end field the copy needed, which fits in padding it already had. syncOpen writes the whole value back rather than reaching in for entTop, so it cannot be half-synced by a later field being added. Holding it by value keeps Put's fields inline in the buffer - a pointer here cost 8-10% when it was tried. PutOnly over alternating rounds: random_500k 2.854ms against 2.870ms, sorted_500k 2.580ms against 2.526ms.

openChunk and syncOpen needed a glossary. The field is the current chunk and the method completes it - when it runs out of room, and when Sort ends the filling phase. Also restores the field's comment, which the previous rename dropped.

The field said what ref holds, not why it is a *[]byte: sync.Pool stores any, so putting a []byte in boxes its header and allocates. Holding the pointer the pool gave out means Reset gives the chunk back for free.

AskAlexSharov added 5 commits August 27, 2026 09:38

AskAlexSharov force-pushed the alex/etl_chunk_sort_37 branch from 7d306df to 9c7dbb3 Compare August 27, 2026 02:38

AskAlexSharov changed the base branch from alex/etl_entry_offset_37 to main August 27, 2026 02:38

AskAlexSharov changed the title ~~db/etl: keep each chunk index inside the chunk, sort chunks apart~~ db/etl: hold the entry index inside each chunk, so Put never re-allocates Aug 27, 2026

AskAlexSharov requested a balanced review from Copilot August 27, 2026 02:46

Copilot started reviewing on behalf of AskAlexSharov August 27, 2026 02:47 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread db/etl/buffers.go

AskAlexSharov added 7 commits August 27, 2026 09:52

AskAlexSharov force-pushed the alex/etl_chunk_sort_37 branch from 7eb5b86 to 5265f1b Compare August 27, 2026 04:08

AskAlexSharov mentioned this pull request Aug 27, 2026

db/etl: give both merges the prefix, the sift and the in-place advance #23616

Draft

AskAlexSharov added 7 commits August 27, 2026 11:18

Merge remote-tracking branch 'origin/main' into alex/etl_chunk_sort_37

a59ae81

AskAlexSharov changed the title ~~db/etl: hold the entry index inside each chunk, so Put never re-allocates~~ db/etl: hold the entry index inside each chunk, so Put never re-alloc Aug 27, 2026

AskAlexSharov changed the title ~~db/etl: hold the entry index inside each chunk, so Put never re-alloc~~ db/etl: move entry index inside chunk, so Put never re-alloc Aug 27, 2026

AskAlexSharov requested a balanced review from Copilot August 27, 2026 06:14

AskAlexSharov marked this pull request as ready for review August 27, 2026 06:14

AskAlexSharov requested a review from sudeepdino008 as a code owner August 27, 2026 06:14

Copilot started reviewing on behalf of AskAlexSharov August 27, 2026 06:14 View session

AskAlexSharov marked this pull request as draft August 27, 2026 06:17

Copilot AI reviewed Aug 27, 2026

View reviewed changes

AskAlexSharov mentioned this pull request Aug 27, 2026

db/etl: store an entry as one packed offset #23576

Closed

AskAlexSharov and others added 2 commits August 27, 2026 13:23

Merge remote-tracking branch 'origin/main' into alex/etl_chunk_sort_37

56bfdfb

Potential fix for pull request finding

576428e

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

AskAlexSharov marked this pull request as ready for review August 27, 2026 06:26

AskAlexSharov added 3 commits August 27, 2026 13:26

Merge remote-tracking branch 'origin/alex/etl_chunk_sort_37' into ale…

8b08523

…x/etl_chunk_sort_37 # Conflicts: # db/etl/buffers.go

AskAlexSharov mentioned this pull request Aug 27, 2026

db/etl: stop the bufio read benchmarks allocating per field #23618

Closed

AskAlexSharov added 6 commits August 27, 2026 13:32

Merge branch 'alex/etl_bench_lenbuf_37' into alex/etl_chunk_sort_37

2654e6e

db/etl: currentChunk and completeCurrentChunk

3e52250

openChunk and syncOpen needed a glossary. The field is the current chunk and the method completes it - when it runs out of room, and when Sort ends the filling phase. Also restores the field's comment, which the previous rename dropped.

save

ad3c9ac

db/etl: say why dataChunk keeps the pool's pointer

114d702

The field said what ref holds, not why it is a *[]byte: sync.Pool stores any, so putting a []byte in boxes its header and allocates. Holding the pointer the pool gave out means Reset gives the chunk back for free.

AskAlexSharov force-pushed the alex/etl_chunk_sort_37 branch from 0577441 to 114d702 Compare August 27, 2026 07:33

AskAlexSharov marked this pull request as draft August 27, 2026 07:35

Navigation Menu

Uh oh!

db/etl: move entry index inside chunk, so Put never re-alloc - #23599

db/etl: move entry index inside chunk, so Put never re-alloc#23599
AskAlexSharov wants to merge 32 commits into
mainfrom
alex/etl_chunk_sort_37

AskAlexSharov commented Aug 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

AskAlexSharov Aug 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

AskAlexSharov commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

AskAlexSharov Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AskAlexSharov commented Aug 26, 2026 •

edited

Loading