dedupe: streaming pipeline with a persistent pool (Stage 2) - #116
Merged
Conversation
Replace the per-batch, per-pass thread pool (created and drained inside every dedupe_results() call) with one pool for the whole dedupe phase and a bounded streaming producer. The main thread loads generation batch i+1 while batch i dedupes, so the pool never drains between generation batches or between the whole-file and extent passes - removing the barriers that left workers idle at every 1024-file boundary and while the next batch loaded. - run_dedupe.c: phase-global pool + batch/work-item model. Each batch owns its two results trees (whole-file + extent) and the producer admits at most DEDUPE_MAX_INFLIGHT (2) batches (RAM double buffer). Batches are reaped in generation (FIFO) order; dedupe_seq only advances to a batch's seq_hi once it and all earlier batches complete (dedupe_advance_seq), preserving the Ctrl+C invariant. New API: dedupe_phase_begin/end, dedupe_await_slot, dedupe_begin_batch, dedupe_push, dedupe_seal_batch. - Filerec lifetime via batch-held refs (filerec_get/put): each batch holds one ref per filerec it loaded and drops them at completion. All get/put/new/find run on the single producer thread, so the filerec registry needs no lock and workers never touch it - they only read filerec fields and mutate their own batch's results tree (under the existing global mutex). No free_all_filerecs() between batches. This is a deliberate, safer alternative to per-extent refs released on workers (same lifetime guarantee, far less lock surface). - oans.c: process_duplicates splits into the streaming producer (stream_duplicates) for -d and the unchanged sequential report path (report_duplicates) for reporting. The producer loads on a separate read connection (WAL readers don't block the writer); the in-memory shared-cache db reuses the global handle and serializes loads with dbfile_lock(). Builds on Stage 2.1 (static whole-file exclusion in GET_DUPLICATE_EXTENTS), which makes the extent load order-independent so both passes can stream without a barrier. No schema change. Tests: test_streaming_dedupe.py (many groups across generations, cross-window convergence, whole-file+extent streaming, in-memory path). make check (111) and make integration-valgrind both clean. Isolated multi-batch dedupe A/B vs master: ~3-4% faster, peak RSS unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The streaming phase frees filerecs per batch in load order (not head-first), so the singly-linked filerec_head made each filerec_free()'s SLIST_REMOVE an O(n) search -> O(n^2) teardown. On the larger-than-RAM Linux-tree benchmark (189k filerecs) that was ~11 s of extra user CPU, profiling as 37% self in filerec_free, and it wiped out the streaming win (median 22 s vs master 13 s). Switch filerec_head to the kernel doubly-linked list_head (O(1) list_del), so teardown is O(n log n) (rb_erase-bound). Warm dedupe user CPU drops from ~11 s back to ~2.9 s, matching master. make check (111) + valgrind clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- push_results: fold the ref-holding walk and the group-collection walk into one rb-tree traversal (both read the same stable tree). - stream_load_batch: replace the six repeated 'if (inmem) dbfile_lock()' lines with load_lock()/load_unlock() helpers that name the shared-handle policy. - dedupe_begin_batch: drop redundant re-zeroing of calloc'd fields. - push_results: abort_on(!sorted) instead of if (!sorted) abort_on(1). No behavior change. make check (111) + integration-valgrind clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pdedupe_add_pushed_work(dext_work(sorted[i])) ran AFTER g_thread_pool_push(): the moment the item is queued a worker owns the group and can free it - an already-shared group is cleaned and freed in microseconds, so on a rerun-heavy tree the producer raced it reliably. Reading the freed dext fed len * (0 - 1) into the pushed-work total, inflating the progress denominator to ~2^59: the bar froze at its monotone high-water mark and the ETA read tens of millions of hours (field report: 8% / ETA ~38449277h28m). Latent on master since the byte-weighted bar (same ordering in push_extents); the streaming pipeline's tighter overlap made it fire. Valgrind's serialization never caught it - a fast worker almost never wins under memcheck. Capture the group's work before the push, assert it is sane (a single group cannot verify an exbibyte), and stop rendering an ETA beyond a year - by then the inputs are wrong, and the string is long enough to wrap the bar line and desync the live block redraw (the doubled idle slot lists in the same report). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A failed g_thread_pool_new() left dedupe_pool NULL but the phase kept going: every push would hit the NULL pool, no item would ever run, and dedupe_phase_end() would wait forever on batches that cannot complete. Abort instead - there is nothing to salvage without a pool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
find_additional_dedupe() walks the GLOBAL filerec list. Pre-streaming, free_all_filerecs() between batches kept that list scoped to the current window; with batches in flight it also held the previous window's filerecs, so --dedupe-options=partial would rescan the prior window's files every batch (stale block trees, duplicated discovery racing the previous batch's workers). Reap all earlier batches (dedupe_drain) before the search - partial mode gives up cross-batch pipelining, the default mode keeps the full overlap. Pinned by a new multi-window partial-mode streaming test. Also stop dropping the batch loaders' return codes on the floor (a failed load now says which window it lost instead of silently deduping a partial tree), and record the new producer/worker lifetime rules in CLAUDE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The corrupt value the guard exists to catch is len * (0 - 1) from a freed dext: 128 MiB * (2^32 - 1) ~= 2^59, which slipped under the 2^60 cap. One group cannot verify a pebibyte; use 2^50. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 24, 2026
martinus
added a commit
that referenced
this pull request
Jul 26, 2026
…gure (#151) The README was last substantively updated in #118, two releases ago, so it was missing everything shipped since and carried one number that no longer matched the benchmark doc. Fix a wrong figure: the path-hash row claimed "41 vs 73 MiB on the benchmark tree". Neither number appears in docs/benchmarks.md, which measures 39.7 vs 70.9 MiB -- and that figure comes from the larger-than-RAM tree, not the 2.07M-file tree the surrounding table describes. Correct both the numbers and the attribution, and reword the table intro, which named only one of the two benchmarks the rows actually draw from. Add the missing user-facing work: - paths beyond PATH_MAX are hashed and deduped (#117/#124/#128) - the streaming dedupe pipeline (#116), which had no bullet at all - the O(extents^2) fragmented-file scan fix (#134) - the two dedupe-phase races (#123, #129) - clang ASAN/UBSAN/TSAN CI legs, warnings-as-errors, make check-all - --cpu-threads, absent from both CLI lists despite being in --help - progress polish: scan-phase throughput (#120), idle workers (#143) Tighten for readability: drop the standalone larger-than-RAM NOTE, which stated the same ~13x claim a third time; its unique content (RSS, hashfile size) moves into the speedups table where the reader is already comparing figures. Mark upstream issue references as "upstream #NNN" throughout -- bare markfasheh#331/markfasheh#374/markfasheh#376/markfasheh#387 now read as oans issues, since oans has its own numbers in that range. Docs-only; no code touched. Co-authored-by: Claude <noreply@anthropic.com>
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.
Replaces #113, which GitHub closed when #112's branch was deleted on merge (a closed PR can't change base). Rebased onto master; the tree is byte-identical to the reviewed and tested #113 tip (
git diff e73b2b7 HEADis empty).See #113 for the full description, review discussion, benchmarks and the field-regression fixes (pushed-dext UAF, NULL-pool path, partial-mode drain, sanity guards). Gates on this exact tree:
make check112/112,make integration-valgrindclean, coldbench.py realistic/manyno regression, field-tested on the 3.5M-file hashfile.🤖 Generated with Claude Code