perf(fullmap): zlib-rs decompression + redb cursor bulk inserts - #73
Merged
Conversation
Advance the fullmap build's storage stack with three measured wins: - flate2 backend switched from miniz_oxide to zlib-rs (pure Rust, fastest api-compatible DEFLATE decompressor). The build's producer threads decompress ~30-40 GB of BABEL .gz; on a real 46 MB BABEL synonym stream zlib-rs decompressed at ~4.6 GB/s vs miniz_oxide's ~3.0 GB/s (~1.5x). - redb pinned to cberner/redb master (the 4.2-to-be, rev a35e7cc) for the automatic ascending-key insert optimization. Measured effect: the 16 RECORDS shard files shrink ~50% on identical input (a 539 MB shard set rebuilds to 270 MB), which also speeds cold lookups. File format is unchanged (still redb v3) — a drop-in engine swap with no forced rebuild and bidirectional cross-version read compatibility (verified). - Phase-4 RECORDS write now appends through redb's experimental cursor API (upper_bound_mut + insert_before). An internally-controlled microbenchmark (2M ascending records, both paths in one process) measured the cursor at ~4x the insert throughput of plain insert() (0.22s vs 0.89s) at identical file size. The cursor requires strictly ascending keys and never overwrites, so a duplicate xxh64 hash (two distinct terms colliding) falls back to a plain insert() for that one record — preserving the historical overwrite semantics — and reopens the cursor; covered by a dedicated test. Swap back to redb = "4.2" once 4.2.0 publishes on crates.io. make check green: ruff, ruff-format, cargo-fmt, pyright, pytest (full suite), cargo test (68 + 10), cargo clippy -D warnings.
…on fallback The cursor collision fallback (close → plain insert → reopen) only caught duplicates WITHIN a single batch because `last` was local to each `flush_shard_batch` call. A duplicate xxh64 hash straddling a BATCH BOUNDARY (group N in batch K, group N+1 — same hash, different term — in batch K+1) would hit `insert_before` with a non-ascending key → `UnorderedKey` → build failure. The old `insert()` never had this problem. Move `last: Option<u64>` to `write_shard_records` (owned across all flushes) and thread `&mut last` into `flush_shard_batch`. Extended the regression test to cover both intra-batch and cross-batch collisions. make check green.
📝 WalkthroughWalkthroughThe Rust crate now uses pinned redb cursor support and zlib-rs. Shard RECORDS writes use cursor-based batch insertion with fallback handling for duplicate hashes. Documentation, changelog entries, and regression tests reflect these changes. ChangesShard write optimization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant write_shard_records
participant flush_shard_batch
participant redb_RECORDS
write_shard_records->>flush_shard_batch: flush sorted batch and last hash
flush_shard_batch->>redb_RECORDS: insert ascending hashes through cursor
flush_shard_batch->>redb_RECORDS: regular insert for duplicate or non-increasing hash
redb_RECORDS-->>flush_shard_batch: reopen cursor and retain last hash
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/fullmap.rs`:
- Line 2067: Replace the unstable ordering in the batch-processing flow with a
stable sort so records sharing the same hash retain their original insertion
order. Update the sort around batch.sort_unstable_by_key, preserving hash
ordering while ensuring Table::insert processes later equal-hash records after
earlier ones.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5806f1de-be7e-4bea-8fcc-b44bec430afe
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mddocs/fullmap.mdrust/Cargo.tomlrust/src/fullmap.rs
| if batch.is_empty() { | ||
| return Ok(0); | ||
| } | ||
| batch.sort_unstable_by_key(|(hash, _)| *hash); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve equal-hash insertion order.
sort_unstable_by_key can reorder records that have the same hash. The fallback then applies Table::insert in that unspecified order. This can store the earlier term instead of the later term for an intra-batch xxh64 collision.
Use a stable sort, or add the original sequence as a tie-breaker, so the documented later-value-wins behavior remains deterministic.
Proposed fix
- batch.sort_unstable_by_key(|(hash, _)| *hash);
+ batch.sort_by_key(|(hash, _)| *hash);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| batch.sort_unstable_by_key(|(hash, _)| *hash); | |
| batch.sort_by_key(|(hash, _)| *hash); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/fullmap.rs` at line 2067, Replace the unstable ordering in the
batch-processing flow with a stable sort so records sharing the same hash retain
their original insertion order. Update the sort around
batch.sort_unstable_by_key, preserving hash ordering while ensuring
Table::insert processes later equal-hash records after earlier ones.
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.
Summary
Three fullmap build/storage-stack improvements, each measured on this repo's build path. No behavior change, no schema bump, no forced rebuild — the file format stays redb v3 (bidirectional cross-version reads verified).
1. flate2 → zlib-rs decompression backend
The build's producer threads decompress ~30–40 GB of BABEL
.gz. Switched flate2 from the defaultminiz_oxidebackend tozlib-rs(pure Rust, runtime SIMD multiversioning — no C toolchain needed for wheels).Measured on a real 46 MB BABEL synonym stream (
synonyms/GeneProteinConflated.txt.gz):→ ~1.5× faster decompression.
2. redb engine: 4.1 → 4.2-to-be (pinned master)
redb = "4"(crates.io 4.1.0) → git-pinnedcberner/redbmaster reva35e7ccfor the ascending-key insert optimization (not yet published; latest stable is still 4.1.0).Measured effect: the 16 RECORDS shard files shrink ~50% on identical input:
This also speeds cold lookups (half the bytes to page in) and halves the DB's disk footprint. The file format is unchanged (still redb v3) — a drop-in engine swap: existing v5 DBs keep opening, no rebuild required. Cross-version read compatibility verified both directions.
Swap back to
redb = "4.2"once 4.2.0 publishes on crates.io.3. Cursor bulk inserts
flush_shard_batchnow appends through redb's experimental cursor API (upper_bound_mut(Unbounded)+insert_before) instead of per-keytable.insert().Measured (internally-controlled microbenchmark: 2 M ascending fixed-size records, both paths in one process, 2 rounds):
insert()insert_before()→ ~4× faster insert throughput at identical file size.
The cursor requires strictly ascending keys and never overwrites (
UnorderedKey), so the rare duplicate xxh64 hash (two distinct terms colliding; ~6% odds per full build) falls back to a plaininsert()for that one record — preserving the historical overwrite semantics — and reopens the cursor.lastis tracked across batch flushes (owned bywrite_shard_records) so a collision straddling a batch boundary is caught too. Covered by a dedicated regression test (intra-batch + cross-batch).Parser benchmark (no change)
The plan asked to test whether swapping
serde_jsonforsonic-rsorsimd-jsonhelps. Benchmarked now on real BABEL rows — decided no:sonic-rs's best case is ~5% on larger rows — noise-level, not worth a dependency swap. The current borrowed-serde design is already the fastest choice. No change.
make checkGreen: ruff, ruff-format, cargo-fmt, pyright, pytest (full suite), cargo test (68 + 10), cargo clippy
-D warnings.Review
The CODE_REVIEWER subagent returned empty; I reviewed the diff myself and found a critical bug: the cursor collision fallback only caught duplicates within a single batch (
lastwas local to eachflush_shard_batchcall), so a duplicate hash straddling a batch boundary would raiseUnorderedKey→ build failure. Fixed by threadinglastacross batches; extended the regression test to cover the cross-batch case (7b7f9af).Summary by CodeRabbit
Performance
Compatibility
Documentation