perf: make Rewrite transaction fragment handling O(n) instead of O(groups × fragments) - #22
Conversation
…oups * fragments) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Tested this end-to-end by building pylance wheels from this branch and from the parent commit (851925c) and A/B-comparing every rewrite-commit path. Correctness — byte-identical behavior vs parent commit
Silk Node committer (the motivating consumer): installed the PR wheel into the silk_node venv and ran Perf (debug builds, manifest with 1,000,000 fragments, 2,000 rewrite groups × 100):
The ~30s algorithmic term is eliminated; the remainder is manifest serialization common to both. Not covered: concurrent-append rebase mid-compaction (stale plans surface as conflicts before that path in the Python API). Tested by Devin — session: https://app.devin.ai/sessions/53d00624d93f48cebd023412401b9ea2 |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
fc98c59 to
535ec12
Compare
Summary
Makes
Transaction::handle_rewrite_fragmentsO(n + groups) instead of O(groups × fragments), fixing the multi-minute compaction commits seen at Silk Node scale (~4.5M fragments × 384 rewrite groups; debug benchmark ~775s → ~4.6s).Algorithm: build a one-time
HashMap<fragment_id, position>overfinal_fragments, then for eachRewriteGroup:replacements[start] = new_fragments(inserted in place)new_fragmentsat the endFinal list is assembled in a single pass. Semantics match the previous implementation (in-place replacement for contiguous groups, append for non-contiguous,
CommitConflicton missing first fragment), verified by existingdataset::transactionanddataset::optimizetests.Hardening found during review: the old code silently tolerated two rewrite groups replacing the same fragment (the O(n) version would have silently overwritten a
replacementsentry, losing new fragments). Now any group replacing an already-removed position returnsCommitConflict("attempts to replace fragment id=N more than once"), with a regression test. A single compaction plan can't produce overlapping groups (bins come from one linear scan) and concurrent rewrites touching the same fragments are already rejected by the conflict resolver, so this only guards against bugs/replayed transactions.Known CI failures inherited from main (not caused by this PR, intentionally left out of scope): clippy 1.91 lints in unchanged
cleanup.rs, notebook CI missingpandas/tqdm, andcargo-denyRustSec advisories (crossbeam-epoch, h2, aws-lc-sys, …).Link to Devin session: https://app.devin.ai/sessions/1bf2b920440f4a1db74a8e2809a6d8e8
Requested by: @tanish-exa