perf(hasher): mmap piece hashing#173
Conversation
The automatic worker count doubled the CPU count on non-darwin platforms for large workloads. Measurement on IO-bound storage (HDD arrays, FUSE/mergerfs) showed extra read() workers add no throughput — wall time is disk-bound — while each additional concurrent reader inflates kernel CPU through syscall and page-cache contention. Cap at the CPU count.
Hash pieces by mapping file windows instead of read()-ing into a pooled buffer, avoiding the per-chunk syscall and kernel->user copy. On FUSE/network filesystems (e.g. mergerfs) this cuts sys CPU several-fold on cold reads while wall time stays disk-bound, and output is byte-identical. Windows are unmapped as the worker advances so resident memory stays bounded regardless of file size, and SetPanicOnFault turns a file truncated mid-hash into a clean error instead of a SIGBUS crash. mmap is the default on unix and falls back to the buffered read() path on Windows, for small files, on mmap failure, or when MKBRR_HASH_MMAP=0. Equivalence with the read() path is enforced by a differential test and FuzzChunkReaderEquivalence.
📝 WalkthroughWalkthroughAdds a strategy-selected chunkReader for piece hashing, with Unix mmap and buffered read implementations, per-platform FUSE detection, mmap gating by size and environment, and tests covering strategy selection, equivalence, and fallback behavior. Changesmmap chunk-reader hashing
Sequence Diagram(s)sequenceDiagram
participant NewPieceHasher
participant resolveHashStrategy
participant hashPieceRange
participant hashStrategy
participant mmapChunkReader
participant bufferedChunkReader
NewPieceHasher->>resolveHashStrategy: resolve useMmap and mmapWindow
NewPieceHasher->>hashPieceRange: build pieceHasher.strategy
hashPieceRange->>hashStrategy: newChunkReader(bufferPool)
alt useMmap and mmapSupported
hashStrategy-->>hashPieceRange: mmapChunkReader
loop each file span
hashPieceRange->>mmapChunkReader: feed(fr, start, length, hasher)
end
hashPieceRange->>mmapChunkReader: release()
else buffered path
hashStrategy-->>hashPieceRange: bufferedChunkReader
loop each file span
hashPieceRange->>bufferedChunkReader: feed(fr, start, length, hasher)
end
hashPieceRange->>bufferedChunkReader: release()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Benchmarking on a 12-core mergerfs (FUSE) box showed that dropping from 2x to 1x hashing workers regressed create wall time ~20% on a cold multi-GiB file: hashing is IO-bound there and keeping more reads in flight hides per-read latency. The extra readers cost some sys CPU, but wall time is what users wait on. Restore the previous behavior (darwin stays at 1x, where oversubscription gave no benefit) and document why.
- detect files shorter than their scanned size via fstat and return io.ErrUnexpectedEOF, matching the read() path, instead of silently hashing the kernel's zero-fill of the partial final page (which would bake a wrong piece hash into the torrent when a file shrinks or is written between scan and hashing) - fall back to a buffered read() of the span when mmap fails at runtime (some FUSE/network/overlay mounts) so create succeeds there instead of aborting with no recourse - only recover genuine memory faults in the mmap path (assert the fault's Addr() interface) and restore the prior SetPanicOnFault setting, so a real bug keeps its stack trace instead of being mislabeled as a file change - round the mmap window down to a page multiple and clamp it, avoiding redundant boundary-page remaps and a 32-bit int overflow from an extreme MKBRR_MMAP_WINDOW Add tests for the short-file case, the read() fallback (byte-identical to the read path), and the strategy-selection/env-gate logic that the equivalence and fuzz tests bypass.
mmap reads on FUSE/network filesystems fault page-by-page through the userspace daemon instead of getting read()'s readahead batching, which benchmarked ~10% slower wall time than read() on mergerfs at the default worker count. Since FUSE/mergerfs is a common target for this tool, auto-detect it via statfs (FUSE_SUPER_MAGIC on linux, f_fstypename on darwin) and use the read() path there. mmap stays the default on local filesystems, where it lowers sys CPU and allocations at no wall-time cost. MKBRR_HASH_MMAP=1 still forces mmap on regardless; =0 forces it off everywhere.
s0up4200
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
torrent/mmap_unix_test.go (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: assert the error type, not just non-nil.
The comment promises parity with the read() path's
io.ErrUnexpectedEOF, but the test only checks both errors are non-nil. Asserting the concrete sentinel would lock in the documented contract and catch a regression where the mmap path errors for a different reason.♻️ Strengthen assertions
- if errB == nil { + if !errors.Is(errB, io.ErrUnexpectedEOF) { t.Fatal("read() path unexpectedly succeeded on a short file; test cannot establish the baseline") } - if errM == nil { + if !errors.Is(errM, io.ErrUnexpectedEOF) { t.Fatalf("mmap path silently accepted a short file (would bake a wrong hash); read() path errored with %v", errB) }Add
"errors"and"io"to imports.🤖 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 `@torrent/mmap_unix_test.go` around lines 59 - 64, The mmap short-file test in mmap_unix_test.go only checks that both paths fail, but it should also verify the concrete error returned by the read() and mmap paths matches the documented io.ErrUnexpectedEOF contract. Update the assertions around errB and errM to compare against the sentinel error, and add the needed errors and io imports so the test catches regressions where mmap fails for a different reason. Use the existing short-file checks in the mmap_unix_test.go test case to locate the change.
🤖 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.
Nitpick comments:
In `@torrent/mmap_unix_test.go`:
- Around line 59-64: The mmap short-file test in mmap_unix_test.go only checks
that both paths fail, but it should also verify the concrete error returned by
the read() and mmap paths matches the documented io.ErrUnexpectedEOF contract.
Update the assertions around errB and errM to compare against the sentinel
error, and add the needed errors and io imports so the test catches regressions
where mmap fails for a different reason. Use the existing short-file checks in
the mmap_unix_test.go test case to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 897cdba0-5e91-47ec-ab77-5fb8951f0c5f
📒 Files selected for processing (9)
torrent/chunkreader.gotorrent/chunkreader_test.gotorrent/fuse_darwin.gotorrent/fuse_linux.gotorrent/fuse_other.gotorrent/hasher.gotorrent/mmap_unix.gotorrent/mmap_unix_test.gotorrent/workers.go
✅ Files skipped from review due to trivial changes (2)
- torrent/fuse_linux.go
- torrent/workers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- torrent/chunkreader.go
- torrent/mmap_unix.go
- torrent/hasher.go
|
https://pkg.go.dev/github.com/jaypipes/ghw@v0.24.0/pkg/block#StorageController MAP_POPULATE MADV_WILLNEED |
|
Thanks for the pointers, Kyle — a genuinely useful list, especially MAP_HUGETLB and LockOSThread, which I'd never have gotten around to checking otherwise. I took the lot to a real mergerfs box (12-core Linux LXC, HDD-backed, 8 GiB file, 24 workers) and benchmarked the prefetch levers instead of just reasoning from first principles. All five variants produced the identical infohash.
Caveat up front so I don't overclaim: this box is a 4 GiB-RAM LXC where I can't drop caches, so cold reads are approximated with a file larger than RAM. Run-to-run variance is ~7-15%, so wall deltas within a few percent aren't meaningful — the trustworthy signals are the sys-CPU column (less noisy) and the clear These were exactly the right levers to check — here's where each landed:
Net: nothing here matched The genuinely unexplored slice of your idea is the plain local rotational HDD (non-FUSE) case, which I couldn't bench here (dev box is NVMe, the test box's local fs is ZFS). If a local HDD shows the same pathology, the right fix looks like a ~4-line Generated by Claude Code. |
|
Claude my man you're looking at this too myopically. There is no stable /sys on Windows or Mac - either rip out the small piece to determine the bus that the storage is on (this would only benefit NVMe - unless if you've tested otherwise), or use the library until something more clean and clear becomes available. Yes, the behaviour here is pathological on a hdd, and is why this (mmap) would be ill-advised on media with seek latency, especially with the "stop the world" issues around pagefaulting and missing the transparent epoll usage on Linux that golang benefits from. The behaviour with fuse, shows some of this latency to you already. From the comments it sounds like you didn't create vm.nr_hugepages within your environment, hence the clear error provided. Where did you put the mmap code? Are you mapping the entire file into chunks at the start, and giving those mappings enough time to populate, or are you spinning the goroutine (they're not free), mapping, then seeing if you can do anything with them - which would cause the issues seen. Basically all you're doing right now is bypassing the syscall barrier, which can work when done appropriately. Fighting the golang scheduler, as seen in previous attempts by the author have not gone well. Prepared by b's mom on glm-5.2. |
|
Fair on the scheduler point — sharper framing than mine: an mmap fault blocks the M without the MAP_HUGETLB — it isn't the missing Structure — it's not goroutine-per-map. The existing per-piece-range workers each do synchronous map → hash → unmap on one ≤16 MiB window. "Map ahead, let it populate, then hash" is what POPULATE (sync prefault) and WILLNEED (async readahead) attempted: POPULATE reverted to /sys portability — Linux-only, agreed, but the rotational read would be build-tagged with no-op stubs elsewhere, same as the existing FUSE Generated by Claude Code. |
|
s0up, to be clear up front: not aimed at you. The fact that you actually ran numbers is useful. The part I’m going to be blunt about is the Claude-generated interpretation, because it is doing the classic LLM thing: producing a polished explanation while optimizing the wrong abstraction. This is not “mmap vs read” in the abstract. Looking at the code, this is: worker:
mmap <=16 MiB window
MADV_SEQUENTIAL
hash
munmap
repeatacross a bunch of workers. That is a very specific mmap design, and frankly it is the caveman version for anything with seek latency or weird backing behavior. It does not prove “mmap is handled.” It proves that repeated per-worker The actual issue is not: The issue is: On NVMe/local SSD, sure, mmap can be excellent. On HDD, FUSE, mergerfs, network-ish filesystems, overlay stacks, or anything with seek latency, letting worker goroutines discover needed pages by faulting them in is exactly how you create latency soup. The current gate encodes: That is not the same as: “Not FUSE” is not a storage class. The benchmark also does not establish the missing case. An 8 GiB file in a 4 GiB LXC where caches cannot be dropped is useful as a stress test, but it is not a clean answer for plain local rotational media. A SATA HDD on ext4/XFS/ZFS is still untested, and I would absolutely not assume mmap is safe there just because it is not FUSE. The sys CPU interpretation is also too neat. Lower sys CPU from mmap does not automatically mean better behavior. You can move the cost out of explicit Also, The mmap implementation worth testing is not this: The mmap implementation worth testing is centralized mapping plus centralized prefetch: In other words, workers should not be the first thing to discover that a page is needed by taking a major fault. The coordinator should say “we are about to read this section” before the worker gets there. On Linux that means something like: or, depending on what benchmarks better: The important part is not the exact hint. The important part is bounded lookahead: That is the piece Claude completely missed. It benchmarked the dumb mmap shape and then acted like the mmap design space was exhausted. I would split this into storage-aware modes: Concrete improvements I would suggest:
This: s := hashStrategy{useMmap: mmapSupported, mmapWindow: defaultMmapWindow}is too broad. The default should be based on detected storage latency class, not operating system.
Use If it says rotational, do not mmap by default. Yes,
Instead of repeatedly mapping 16 MiB windows per worker, try: The current per-window map/unmap loop measures VMA churn too. That is not the best mmap can do.
The coordinator should prefetch upcoming sections before workers touch them: This gives the kernel a chance to populate page cache before the worker enters the hash loop. It also lets you bound lookahead instead of creating a page-fault storm.
For HDD and probably FUSE/mergerfs, ranges should be read in increasing offset order as much as possible. Parallel hashing can happen after the read, but the I/O path should not be randomized by worker scheduling.
The buffered reader currently does cursor-based
24 workers on NVMe might be fine. 24 workers on HDD/FUSE is not parallelism, it is vandalism. Defaults should look more like: with explicit overrides.
Wall/sys is not enough. For this workload the interesting metrics are: Otherwise Claude is just going to keep inventing confidence from two columns.
This comment says one So my actual conclusion:
If the next test is worth doing, I would test three things: on: Until then, “mmap default on Unix except FUSE” is not a performance policy. It is just a guess with a FUSE exception. |
2 participants
Hashes pieces by mapping each file in bounded, page-aligned windows instead of
read()into a pooled buffer. On local filesystems this drops the per-read syscall and the kernel→userspace copy, lowering sys CPU and allocating ~2000× less (no read buffers are needed), with no change to wall time — SHA-1 is already hardware-accelerated, so the win is entirely in the read path. mmap is the default on unix for inputs ≥1 MiB, and uses the bufferedread()path on Windows, for small inputs, and — proactively — on FUSE mounts (mergerfs, sshfs, rclone), where page-fault reads get no readahead batching and benchmarked ~10% slower thanread()on mergerfs. If anmmapcall fails at runtime (e.g. a mount that doesn't support it), that span transparently falls back toread()rather than aborting.MKBRR_HASH_MMAP=0forces the read path everywhere;MKBRR_HASH_MMAP=1forces mmap on regardless of filesystem.Output is byte-identical to the read path — enforced by a differential test and
FuzzChunkReaderEquivalence, and verified to produce the same infohash across the old binary and both paths on multi-GiB files on both local NVMe and mergerfs. mmap windows are unmapped as the worker advances so resident memory stays bounded regardless of file size. A file truncated while mapped becomes a clean error instead of a SIGBUS crash (debug.SetPanicOnFault, scoped to memory faults so real bugs keep their stack trace), and a file shorter than its scanned size is detected up front rather than silently hashing the kernel's zero-fill. Thecheck/verify path still usesread()and can follow in a later change.