Repository navigation
Use Pebble batch directly in SS - #4300
Conversation
alrevuelta
commented
Sep 22, 2026
- Batching is used when inserting keys into SS. Meaning we aggregate tons on updates into a single batch.
- However the current code was using an intermediate buffer for the batch. This means that we were allocating a buffer for the whole batch (which can be eg 500k of keys) to then copy that again to Pebble "native" batch.
- This produced many many unnecessary memory allocations.
- This PR reduces allocations (for a 500k keys batch) from 1,011,200 to only 11,300 and insertion time (of an ordered batch) from 90ms to 79 ms.
- It simply uses the native Pebble batch type directly, without using an intermediate batch.
PR SummaryMedium Risk Overview
Tests/benchmarks were updated for zero-allocation Reviewed by Cursor Bugbot for commit 2be5dc4. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4300 +/- ##
==========================================
- Coverage 67.42% 66.32% -1.10%
==========================================
Files 2179 2058 -121
Lines 167292 155265 -12027
==========================================
- Hits 112797 102985 -9812
+ Misses 54485 52270 -2215
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8246527. Configure here.
There was a problem hiding this comment.
Swapping the intermediate op buffer for a native pebble.Batch is a solid allocation win, but moving the sort out of the batch and into ApplyChangesetSync now mutates caller-owned changeset slices from the async writer goroutine, racing the synchronous commit-store apply and hash logging. The new Write()-closes-the-batch lifecycle is also undocumented and leaves one error path operating on a released pebble batch.
Findings: 1 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
NewBatchnow acquires apebble.Batcheagerly, but onlyWrite()releases it. Every earlyreturn errinApplyChangesetSync(db.go:710,713) and the error exits inDeleteKeysAtVersionabandon the batch withoutClose(), so it is never returned to pebble'sbatchPool. Adeferthat closes the batch unlessWrite()already did (or aClose()/Release()method onBatch) would make the lifecycle a property of the type rather than a convention each caller has to remember. - [suggestion] Sorting is now done only in
ApplyChangesetSync;Importlost it. PreviouslywriteBatchOpssorted every batch before commit, so eachImportCommitBatchSizechunk hit the memtable in comparer order. Import feeds from a channel consumed byImportNumWorkersgoroutines, so its batches are no longer ordered — by this PR's own premise (ordered insertion is measurably faster) that is a regression on the import path. Worth either sorting in the import worker or noting the measured impact. - [suggestion] Test coverage for the two behaviours the refactor actually changed is missing: (1) nothing asserts that
ApplyChangesetSyncleaves its input[]*proto.NamedChangeSetunmodified — a test on that would have caught the in-place sort above, and it is cheap (snapshot the pair order, apply, compare); (2) nothing pins that aBatchis single-use afterWrite().TestSortBatchOps(which checked the sort agreed withMVCCComparerin both version directions) was replaced byTestSortChangesetPairs, which only re-testsbytes.Compareon three keys and covers none of the new surface. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
8246527 to
2be5dc4
Compare
|
@seidroid review |
There was a problem hiding this comment.
Replacing the intermediate []batchOp buffer with a native pebble.Batch is a clean allocation win, and the three findings from the previous review (the cross-goroutine sort race, the undocumented single-use lifecycle, and the Batch godoc) are all genuinely fixed — with Close()/errBatchClosed, errors.Join cleanup at every exit, and new tests pinning both. Two non-blocking points remain: the batch is no longer sorted before it reaches Pebble on any path, and a new godoc overstates what DeleteKeysAtVersion removes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Encoding directly into the
pebble.Batchremoves the sort thatwriteBatchOpspreviously applied on every write path, and nothing replaces it (SortChangesetPairsis deleted with no caller). Base always handed Pebble a batch in comparer order; head hands it whatever order the caller produced, andPopChangeSetproduces write order, not key order. Pebble's memtable apply usesarenaskl.Inserter, whose splice cache only pays off for ascending input, so unordered application loses that. The PR describes the 90ms→79ms number as "insertion time (of an ordered batch)", i.e. the case where the sort was already a no-op scan — the unordered case, which is whatApplyChangesetSync,ImportandDeleteKeysAtVersionactually feed, is the one that could regress.BenchmarkBatchWritealready builds random (unordered) EVM keys, so a base-vs-head run of it would settle this; worth posting that number, or sorting at the producer, before merging. - [suggestion]
Importnow passesbufSize: 0(db.go:1272,db.go:1295), so each 10k-record batch grows its buffer by doubling from scratch. Base sized the ops slice byImportCommitBatchSizeand then derived an exact pebble buffer from it viapebbleBatchBufSize, so the import path previously reserved once. Given the PR's premise is eliminating regrowth, an estimatedbatchRecordSize-based reservation (or reusing one batch viaResetacross chunks) would keep that property on the import path. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
DeleteKeysAtVersion(sei-db/db_engine/pebbledb/mvcc/db.go:1379) drives its deletes fromRawIterate, whichcontinues past any entry whose value is tombstoned (db.go:1359). Keys deleted at the target version therefore keep their tombstone records after the "physical delete" runs. Present on the base branch.
Superseded: latest AI review found no blocking issues.
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
