feat(hasher): add mmap path for Tier 3 hashing on large files (#7)#42
Closed
itsmessc wants to merge 3 commits into
Closed
feat(hasher): add mmap path for Tier 3 hashing on large files (#7)#42itsmessc wants to merge 3 commits into
itsmessc wants to merge 3 commits into
Conversation
…t28#7) For files >= 1 MiB on Unix, full_hash now memory-maps the file and passes the mapped slice directly to blake3::hash(), eliminating the 128 KB userspace copy loop. Smaller files and non-Unix targets keep the existing BufReader path unchanged. Also removes the dead sparse_hash() call in the scan worker whose return value was discarded, causing every file to be read twice. - Add memmap2 = 0.9 dependency - Add src/lib.rs to expose hasher for Criterion benchmarks - Add benches/hash_bench.rs: mmap vs buffered at 512 KiB/1 MiB/4 MiB/16 MiB - Add three unit tests in hasher.rs covering empty file, mmap/buffered agreement, and small-file correctness Closes Rakshat28#7 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e Cargo.lock Items used by the binary (via its own mod hasher) and the benchmark appear unused from the lib crate perspective. Suppress with targeted #[allow(dead_code)] rather than a crate-wide gate.
There was a problem hiding this comment.
Pull request overview
This PR adds a Unix-only mmap fast path for large-file hashing, exposes the hashing module through a new library target for Criterion benchmarks, and removes an unnecessary extra hash read in the scan pipeline. It fits into the codebase by optimizing the Tier 3/full-hash path used during dedupe/scan while adding benchmark and unit-test coverage around the new hasher behavior.
Changes:
- Add an
mmap-based implementation path tohasher::full_hashfor files at or above 1 MiB on Unix. - Remove the discarded
sparse_hash()call from the scan worker so candidate files are no longer read twice. - Add a library target, benchmarking harness, and hasher unit tests to compare and validate buffered vs. mmap hashing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Simplifies scan worker hashing by removing the unused sparse_hash() call. |
| src/lib.rs | Adds a library target surface so benches can import hashing code. |
| src/hasher.rs | Implements the mmap threshold path, adds buffered benchmark helper, and adds unit tests. |
| benches/hash_bench.rs | Adds Criterion benchmarks comparing mmap and buffered hashing across file sizes. |
| Cargo.toml | Adds memmap2, registers the benchmark target, and adds Criterion dev dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// Always uses the 128 KB BufReader loop, regardless of file size. | ||
| /// Exposed for benchmarking so the mmap and buffered paths can be compared directly. | ||
| pub(crate) fn full_hash_buffered(path: &Path) -> Result<Hash> { |
Comment on lines
+48
to
+59
| #[cfg(unix)] | ||
| if len >= MMAP_THRESHOLD { | ||
| // SAFETY: The file may be modified by another process while it is mapped. | ||
| // bdstorage operates on user-owned files not expected to be concurrently | ||
| // written. In the worst case a concurrent write produces a stale hash that | ||
| // the next scan will correct. The mapped region is used as a read-only | ||
| // &[u8] slice by BLAKE3, so no Rust memory-safety invariant is violated. | ||
| let mmap = unsafe { memmap2::Mmap::map(&file) } | ||
| .with_context(|| format!("mmap file {:?}", path))?; | ||
| return Ok(blake3::hash(&mmap[..]).into()); | ||
| } | ||
|
|
Comment on lines
+72
to
+73
| /// Always uses the 128 KB BufReader loop, regardless of file size. | ||
| /// Exposed for benchmarking so the mmap and buffered paths can be compared directly. |
Comment on lines
+55
to
+57
| let mmap = unsafe { memmap2::Mmap::map(&file) } | ||
| .with_context(|| format!("mmap file {:?}", path))?; | ||
| return Ok(blake3::hash(&mmap[..]).into()); |
- pub(crate) -> pub on full_hash_buffered so Criterion bench (external crate) can import it through bdstorage::hasher - fall through to chunked-read if Mmap::map fails instead of returning an error, preventing silent file drops on unsupported filesystems - update SAFETY comment to mention SIGBUS risk from concurrent truncation - fix doc comment: 'BufReader loop' -> '128 KB chunked read loop'
Owner
|
@itsmessc The PR is failing CI test, also include updated benchmark numbers (and steps to replicate them). |
Owner
|
Hey @itsmessc @saicharan-superagi , just checking in on this! It looks like the integration tests are failing. Let us know if you need any help debugging this, otherwise, we might need to close this PR in a few days to keep the queue tidy. |
Author
|
I will check with that |
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
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.
For files >= 1 MiB on Unix, full_hash now memory-maps the file and passes the mapped slice directly to blake3::hash(), eliminating the 128 KB userspace copy loop. Smaller files and non-Unix targets keep the existing BufReader path unchanged.
Also removes the dead sparse_hash() call in the scan worker whose return value was discarded, causing every file to be read twice.
Closes #7