perf(pebble): unlink spill chunks during merge, sort resources on import - #1107
Open
kans wants to merge 10 commits into
Open
perf(pebble): unlink spill chunks during merge, sort resources on import#1107kans wants to merge 10 commits into
kans wants to merge 10 commits into
Conversation
The two k-way merge helpers held every sorted chunk file open until the staging directory was torn down, so for the length of each merge the staging directory carried the same entries twice: once in the chunks and once in the SST being built from them. Chunks are now closed and unlinked as soon as the merge reads their last entry, bounding the overlap to the chunks still in flight. Unlinking is confined to the exhausted-chunk path, where the file is provably fully consumed; a merge that fails partway closes its descriptors and leaves the rest to the staging-dir teardown that already owned them. Both helpers back the bulk import, the deferred grant index, the id-index migration, the segment layer, and the digest build, and the index builds run at EndSync on every pebble sync, so this bounds peak staging for all of them. AddResources also no longer requires rows pre-sorted by (resource_type_id, resource_id); it routes them through the same spill sorter entitlements already use. A converter scanning SQLite with a matching ORDER BY satisfied the old precondition for free, but a producer that rewrites resource ids cannot — the c1z sanitizer HMACs them, so its output order is unrelated to the order it read. Sorting inside the importer keeps every producer on one path instead of making sortedness a precondition each one has to re-establish, and it holds when a single resource type is too large to sort in memory.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
General PR Review: perf(pebble): unlink spill chunks during merge, sort resources on importBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit is documentation only: it widens Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
The digest build's mergeGrantHashChunksToSST and the migration's mergeGrantPrimaryMigrationChunksToSST were hand-rolled copies of the merge prologue and still held every chunk open until teardown, so the previous commit's claim that the unlink bounded staging for them was wrong. Both now read through openSpillChunks, which puts all four of the engine's k-way merges on one cursor and leaves readSpillEntry with a single caller. Only the bulk import spills 8MiB chunks; these two use the 128MiB deferredIndexSpillChunkBytes, so they are where the doubling cost most. Two consequences of that consolidation: os.Open on a spill chunk now happens in exactly one place, collapsing four os-IO allowlist entries into one, and ingestSynthLayerSegment's post-merge os.Remove loop is dead work the merge already did. Also corrects the BulkSyncImport type doc, which still told callers resources must arrive in strictly increasing key order and fail with ErrBulkImportOutOfOrder, and guards advance against being called on an exhausted chunk — releasing a chunk nils its reader, where the inline readSpillEntry calls this replaced returned false idempotently. Adds spill_merge_test.go, the first coverage of any of this. A sorter only cuts a second chunk past 8MiB/128MiB and no test drives that much, so every merge under test was single-chunk; these build sorted runs directly with writeSortedSpillChunk and cover an empty chunk, a single-entry chunk, staggered exhaustion, and a duplicate key spanning two runs through both the strict and resolving merges. Confirmed non-vacuous by mutation: removing the unlink fails three of the four, and dropping one entry per advance fails all four. Co-authored-by: Cursor <cursoragent@cursor.com>
mergeGrantPrimaryMigrationChunksToSST was the only one of the four merges sharing spillChunkCursors with no test driving more than one chunk. TestIDIndexMigrationSemantics runs it end to end through Open, but with few enough rows to fit a single chunk, so the path that matters was never reached: advanceMigrationChunk fires once for a duplicate group's leader and again for every same-key follower, so one group can drain several chunks. It is also the merge where a mistake is worst, since it runs on the open-time id-index migration and a row it drops or double-counts stays sorted -- bulkSSTWriter.add does not notice -- and is written into the c1z. The gap predates this branch. What makes the cross-chunk fold safe is that heap items are owned copies, which the cursor refactor did not change, so this is coverage for a path the refactor touched rather than a fix for one it broke. The rows are laid out globally sorted and dealt round-robin so every chunk stays internally sorted while the duplicate group spans three of them; the test asserts that layout instead of assuming it, since nothing in the assertions would reveal a deal that stopped splitting the group. Beyond the folded primary rows it also checks the derived index sorters, because a fold bug can emit a correct primary row alongside one index entry per duplicate, leaving by_principal rows dangling against a primary that no longer has them. Confirmed non-vacuous by mutation: dropping followers from the fold, emitting the index per row, skipping a primary row, and removing the unlink each fail it. Also narrows the spill_merge_test.go header, which claimed to be the only coverage of multi-chunk exhaustion and release-on-exhaustion. TestGrantDigestSpillMerge forces a 512-byte chunk size through a real digest build and reaches both, so the header now names it to keep the two comments from drifting apart again. Co-authored-by: Cursor <cursoragent@cursor.com>
TestMergeGrantPrimaryMigrationChunksFoldsAcrossChunks built the two derived-index sorters and left them open, so nothing waited out their background chunk sorts before t.TempDir removed the directory holding them. teardown() documents that ordering as a requirement -- a chunk sort racing the removal can re-create a file mid-walk and strand the directory -- and production honors it by waiting on every sorter before removeStagingDir. The test cannot actually flake as written, since six keys of ~50 bytes never reach the 8MiB threshold that makes add() cut a chunk, but it would arm itself the moment the fixture grows. Closes them with finalize rather than abort. abort waits on a WaitGroup that, for these sorters, nothing has ever added to, so it would satisfy the ordering on paper while leaving the test with no dispatched sort to wait for. finalize cuts the pending arena, so the background sort really runs and the wait is load-bearing; asserting it flushed a chunk pins that, since only that goroutine writes one. Co-authored-by: Cursor <cursoragent@cursor.com>
Finish's teardown comment still described two ordered SST writers and the failing-one-leaves-the-other scenario; with resource types the only ordered writer left, finish() runs first and closes its handle even on error, so that clause is gone and the sorter-race rationale stands alone. advance's doc claimed merges depend on EOF idempotency in one breath and that no caller reaches it in the next; it now says what the test pins. The resources field comment led with an unconditional key-order mismatch that its own second paragraph walks back for ORDER BY converters, so the mismatch is now scoped to entitlements with resources presented as producer-dependent. The spillChunkCursors type doc drops its change-justification half — the subsystem enumeration and cost analysis live in the commit history — keeping the invariant, the ownership rules, and the only-caller fact that keeps the remaining claim verifiable. ingestSynthLayerSegment's cleanup note said the final dir cleanup "keeps" the SST path when it removes it. Also has the migration merge test capture the duplicate group's key while building the fixture rather than marshaling a throwaway record to recover it. Co-authored-by: Cursor <cursoragent@cursor.com>
…fast path A pebble-destination sanitize run now writes its four record families through BulkSyncImport (sorted SST construction + ingest) instead of per-batch Put upserts, the same split the SQLite->pebble converter uses: assets, EndSync, and supports_diff stay on the writer path. The bulk contract holds by construction — resumable+pebble and multi-sync +pebble are already rejected up front, so the destination sync is always fresh and checkpoints are no-ops. Computed stats (plus the asset count, which rides outside the import) are stashed so EndSync persists the sidecar without re-scanning the ingested keyspaces. Adds Options.TmpDir and a --tmp-dir sanitize flag (matching to-pebble) for spill/unpack staging, and a stats-sidecar assertion to the pebble end-to-end test. Verified by mutation: dropped resource/grant rows and an omitted asset-count stash each fail the parity suites. Co-authored-by: Cursor <cursoragent@cursor.com>
…g spills The resources and entitlements sorters spilled full marshaled records at bulkSpillKeyChunkBytes (8MiB), the size meant for key-only index spills; grants.go records what that does at whale scale (a 3,400-way merge with ~3.4GB of read buffers). Both are single sorters filled in sequential phases, so they move to deferredIndexSpillChunkBytes (128MiB) like the other record-carrying sorters; grant shards and index sorters stay key-sized because their arenas multiply across shards x families. The field comment now states the fd/buffer curve and the ~2x transient staging spill-sorting costs over the ordered writer it replaced. Also from review: ErrBulkImportOutOfOrder's doc now names AddResourceTypes as the only caller-reachable source (resources/ entitlements/grants re-sort internally; their duplicates surface at Finish), the redundant finished flag in the sanitizer's bulk sink is gone (Abort is already a no-op once Finish marks itself done), and the cross-engine parity test pins full record identity for resource types, resources, and entitlements — normalizing the two fields pebble's v3 schema stores as identity-only refs — verified non-vacuous by a field-corruption mutation no prior assertion could see. Co-authored-by: Cursor <cursoragent@cursor.com>
The grant shards and per-shard index sorters were the last spill sorters cutting 8MiB chunks. Chunk size sets Finish's merge fan-in — every chunk stays open behind a 1MiB reader for the whole merge — so a whale-sized grant family (~17GB) meant a ~2,100-way merge with ~2GB of read buffers and as many open files. A 3.7M-grant sanitize run merged 135 grant chunks; the same run now merges 9. All of the import's sorters now share one bounded arena freelist (the same pairing every other 128MiB spill user already has), which also closes the gap where resources and entitlements adopted the big chunk size without arena reuse. Verified compute-neutral on the 3.7M-grant run (+1.7% instructions retired) with identical output counts. Co-authored-by: Cursor <cursoragent@cursor.com>
…cing docs The duplicate-key sentinel moved into the public contract when resources stopped surfacing duplicates through ErrBulkImportOutOfOrder (they now fail at Finish), so export it for errors.Is. Also: the TmpDir doc now states the ~2x merge-peak staging cost instead of ~1x, the WithConvertParallelism doc notes the ~384MiB of spill arenas each lane pins, and the cross-engine parity oracle fails loudly if a fixture ever outgrows its single-page reads instead of silently narrowing. Co-authored-by: Cursor <cursoragent@cursor.com>
ErrBulkImportDuplicateKey surfaces from every spill merge (deferred index and digest builds, synth layer, id-index migration), not just BulkSyncImport.Finish; and the WithConvertParallelism memory budget missed the three lane-independent sorters that stay live through the scan, so the honest figure is ~384MiB x (lanes + 1). Co-authored-by: Cursor <cursoragent@cursor.com>
Comment on lines
+111
to
+116
| // scales with fan-out: each lane pins up to three 128MiB spill arenas | ||
| // (its grant sorter plus two index sorters), and the import's three | ||
| // lane-independent sorters (resources, entitlements, parent index) pin | ||
| // another ~384MiB alongside the scan since none finalize until Finish — | ||
| // so budget ~384MiB × (lanes + 1) of sort memory (see the bulk import's | ||
| // arenaFree). |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: the widened budget counts one live arena per sorter, but the freelist and the sort semaphore each hold arenas on top of that. cutAndDispatch detaches the producer's arena before handing it to a background sort, and the producer allocates a replacement on its next add, so up to sortSem (min(4, max(2, GOMAXPROCS/2))) arenas are in flight beyond the per-sorter ones, plus up to sorters+2 idle arenas parked in arenaFree. At lanes=4 that is ~25 × 128MiB worst case rather than the stated ~1.9GB. Worth saying "≥ 384MiB × (lanes + 1), plus up to ~sortSem + sorters+2 arenas in flight/recycled" so a caller sizing a memory-capped host has the right headroom. (Low confidence on the exact figure; the direction is what matters.)
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
1 participant
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.
perf(pebble): unlink spill chunks during merge, sort resources on import
The two k-way merge helpers held every sorted chunk file open until the
staging directory was torn down, so for the length of each merge the
staging directory carried the same entries twice: once in the chunks and
once in the SST being built from them. Chunks are now closed and unlinked
as soon as the merge reads their last entry, bounding the overlap to the
chunks still in flight. Unlinking is confined to the exhausted-chunk path,
where the file is provably fully consumed; a merge that fails partway
closes its descriptors and leaves the rest to the staging-dir teardown
that already owned them. Both helpers back the bulk import, the deferred
grant index, the id-index migration, the segment layer, and the digest
build, and the index builds run at EndSync on every pebble sync, so this
bounds peak staging for all of them.
AddResources also no longer requires rows pre-sorted by
(resource_type_id, resource_id); it routes them through the same spill
sorter entitlements already use. A converter scanning SQLite with a
matching ORDER BY satisfied the old precondition for free, but a producer
that rewrites resource ids cannot — the c1z sanitizer HMACs them, so its
output order is unrelated to the order it read. Sorting inside the
importer keeps every producer on one path instead of making sortedness a
precondition each one has to re-establish, and it holds when a single
resource type is too large to sort in memory.