RFC: bypass defrag prevention on mostly-matchable content - #887
Draft
XciD wants to merge 2 commits into
Draft
Conversation
Defrag prevention re-stores dedup ranges shorter than its rolling chunks-per-range average so they merge with neighboring new data into contiguous fresh runs. That reasoning only holds when the surrounding content is mostly new. On mostly-matchable content (a re-upload of a file whose bytes already sit in CAS with a fragmented layout), the throttle degenerates into its worst case: it alternates accepted and rejected ranges at the hysteresis equilibrium, so the file both keeps referencing the fragmented ranges AND re-stores about half the bytes. Observed on production buckets (multi-GB media files re-imported over an interleaved first upload): ~2x stored bytes, +90-160% xorb overhead, CPR pinned at exactly the min_n_chunks_per_range equilibrium (8.07 and 7.8 on two files), terms alternating gap-2 between the old xorbs and the re-stored ones. Track how many processed chunks had a match on offer (accepted or rejected) and bypass the throttle once that density exceeds deduplication.defrag_prevention_matchable_density_bypass (default 0.7, > 1.0 restores the previous behavior), after a 512-chunk warm-up. Re-uploads then dedup fully and inherit the reference layout instead of duplicating storage; low-density files (the case the throttle was built for) are untouched. Validated with the two ignored exploration harnesses added here: - fragmented-reference re-upload: defrag_prevented drops 5.9% -> 0, re-stored bytes drop to only the genuinely new chunks; - sparse scattered matches in mostly-new content: byte-identical behavior with and without the gate.
Review feedback on the density bypass: - The bypass is throttle policy, so its counters, threshold, warm-up constant and decision now live in DefragPrevention next to the rolling CPR estimator, instead of being bolted onto FileDeduper. The gate in process_chunks is back to its original shape. - The denominator piggybacks on the estimator feed methods (every settled chunk already flows through add_range/increment_last_range), removing the hand-maintained per-branch counting in the dedup loop; the accept-path numerator is fed from add_file_data_sequence_entry and the rejected-offer count is recorded at the decision itself. - The density check now runs only when the throttle is actually consulted (short-circuited by the contiguous-continuation fast path) and uses a multiply instead of a division. - Bypassing explicitly leaves the hysteresis state untouched, documented at the decision site. Test cleanups from the same review: hoist the DirectAccessClient import, replace two hand-rolled LCGs with the module's DeterministicRng, factor the repeated start_clean/add_data/finish ceremony into a clean_file_in_session helper, and feed the per-generation trailer as a second add_data call instead of cloning the 256MB buffer.
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.
Stacked on #886 (base branch set accordingly); only the last commit is new. Opening as an RFC: this changes upload dedup behavior and the threshold deserves discussion.
Problem
Defrag prevention re-stores dedup ranges shorter than its rolling chunks-per-range average, so the re-stored chunks merge with neighboring new data into long contiguous fresh runs. That reasoning only holds when the surrounding content is mostly new.
On mostly-matchable content, i.e. a re-upload of a file whose bytes already sit in CAS with a fragmented layout (interrupted-and-retried imports, mounts re-writing files, copy fallbacks), the throttle degenerates into a worst-of-both-worlds fixed point: it alternates accepted and rejected ranges around the hysteresis band, so the file keeps referencing the fragmented old ranges AND re-stores roughly half the bytes as fresh xorbs.
Observed on production Storage Bucket files (multi-GB media re-imported over an interleaved first upload):
min_n_chunks_per_rangehysteresis equilibrium: 8.07 and 7.8 measured on two files, which is the fingerprint of the throttle oscillatingReproduced end to end with the two ignored exploration harnesses included in this PR (local test server, no network). Key findings from the harness:
Change
Track how many processed chunks had a dedup match on offer, whether the throttle accepted or rejected them. Once that matchable density exceeds
deduplication.defrag_prevention_matchable_density_bypass(new config, default 0.7, envHF_XET_DEDUPLICATION_DEFRAG_PREVENTION_MATCHABLE_DENSITY_BYPASS, set > 1.0 for the previous behavior), the throttle is bypassed: skipping dedup on content that is almost entirely matchable buys no contiguity — the file still references the fragmented ranges in between — and only duplicates storage. A 512-chunk warm-up prevents a tiny early sample from flipping the gate.Validation with the harnesses:
defrag_prevented > 0)Full
xet-datasuite passes (284 lib tests + integration suites).Discussion points
HF_XET_DEDUPLICATION_MIN_N_CHUNKS_PER_RANGE=0, but that gives up the throttle entirely; this PR keeps it for the case it was designed for.Related: the test-only
random_data()helper inrange_upload.rshas a 16 MB period (bits 16..24 ofi*K mod 2^24), so >16 MB test buffers dedup against themselves and distort layout measurements — worth a separate small fix; the harnesses here use the full-periodDeterministicRnginstead.