db/state: dedup openDirtyFiles and fold deduplicate.go into the merge paths#22178
Draft
yperbasis wants to merge 3 commits into
Draft
db/state: dedup openDirtyFiles and fold deduplicate.go into the merge paths#22178yperbasis wants to merge 3 commits into
yperbasis wants to merge 3 commits into
Conversation
Extract openDirtyDataFile and openDirtyAccessor helpers in dirty_files.go and rewrite the three openDirtyFiles methods as thin per-entity call lists. Behavior-preserving refactor; per-entity differences (name masks, directories, version checks, log levels, invalid-vs-log handling) are passed as parameters. Two cosmetic deltas: the MatchVersionedFile-error Debug message suffix is now the same for all three entities (previously "FileExist err" / "FileExist" / "MatchVersionedFile error" wording drift), and History's commented-out TODO block about corrupted-file removal is dropped. SnapshotRepo.openDirtyFiles in snap_repo.go is deliberately not converted: it uses NewDecompressorWithMetadata, Error-level logs, version.SearchVersion for accessor paths and no MustSupport check — folding it into the helpers would weaken those differences.
Pins the behavior of the snapshot-rebuild deduplication before folding deduplicateFiles into the merge paths: per-file collapsing of equal-value runs to the run's last txNum, no cross-file dedup, HistorySeek results unchanged, and in-place replacement of the input files (newer current file versions than on disk, as when the rebuild tool runs a newer binary). Passes against the current deduplicate.go implementation.
deduplicate.go was a drifted fork of merge.go: the heap setup and tails were token-identical, and InvertedIndexRoTx.deduplicateFiles still used the old pairwise dedupNumSeqs re-encode that iit.mergeFiles had since replaced with the single-pass builder path. Replace the fork with an optional dedup mode on the merge paths: HistoryRoTx.mergeFiles and InvertedIndexRoTx.mergeFiles take a *valuesDeduper (nil = plain merge, hot paths unchanged). In dedup mode the history scan collapses per-file runs of equal values to the run's last txNum (writeDeduped), records the dropped txNums, and the index merge - which in this mode runs after the value scan - filters them out of the merged sequences (mergeSortedSkipping). CompactRange drives the mode and keeps the in-place replacement semantics: input .ef/.efi files are always removed, the input .v only when its step range differs from the output. Preserved: per-file dedup scope (no cross-file dedup), run-collapse keeping the last txNum, output file naming/versions, ascending write order, unfiltered-max EF sizing (byte-identical .ef output for the single-file rebuild flow). Pinned by TestHistoryCompactRange, which passes unchanged against the old and the folded implementation. Deliberate deviations from the old fork, all on the rebuild-tool path: inputs are now removed only after a successful rebuild instead of via defer even on error; output file handles are closed instead of leaked; history dedup now honors noFsync and reports progress like a normal merge; the compressor log label is "merge hist" instead of "dedup hist"; the per-pair dedup-count println moved after the pair completes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicated state-file lifecycle code by (1) refactoring openDirtyFiles logic across Domain/History/InvertedIndex into shared helpers, and (2) folding the snapshot-rebuild deduplication path into the existing merge paths (deleting deduplicate.go) by introducing an optional deduplication mode.
Changes:
- Introduces
openDirtyDataFile/openDirtyAccessorto deduplicate dirty-file opening logic indirty_files.go. - Adds
valuesDeduperand adedup *valuesDeduperparameter to history/inverted-index merge paths to support rebuild-time value deduplication; removesdeduplicate.go. - Adds/updates tests and benchmarks, including
TestHistoryCompactRange, and updates merge call sites to pass the newdedupparameter.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| db/state/merge.go | Adds valuesDeduper and threads optional dedup mode through history + inverted-index merges. |
| db/state/history.go | Reworks CompactRange to use merge-based dedup mode and in-place replacement semantics. |
| db/state/dirty_files.go | Refactors duplicated “open dirty data/accessor” logic into shared helper functions. |
| db/state/history_test.go | Adds TestHistoryCompactRange and updates merge calls for new signature. |
| db/state/merge_test.go | Updates inverted-index merge test for new signature. |
| db/state/merge_bench_test.go | Updates merge benchmark for new signature. |
| db/state/inverted_index_test.go | Updates helper merge call for new signature. |
| db/state/aggregator.go | Updates inverted-index merge call for new signature. |
| db/state/deduplicate.go | Deleted; functionality folded into merge paths via optional dedup mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+657
to
+660
| skipTxNums := dd.skipTxNums[string(key)] | ||
| dd.keptTxNums = dd.keptTxNums[:0] | ||
| var maxTxNum uint64 | ||
| var it multiencseq.SequenceIterator |
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Println("Values to deduplicate:", dedup.dropped) |
Comment on lines
+1539
to
+1542
| efFiles[i].closeFilesAndRemove() | ||
| if vFiles[i].StartStep(ht.stepSize) != indexIn.StartStep(ht.stepSize) || vFiles[i].EndStep(ht.stepSize) != indexIn.EndStep(ht.stepSize) { | ||
| vFiles[i].closeFilesAndRemove() | ||
| } |
Comment on lines
+387
to
+390
| fPath, fileVer, found, err := version.MatchVersionedFile(mask, dirEntries, dirPath) | ||
| if err != nil { | ||
| logger.Warn("[agg] "+tag, "err", err, "f", filepath.Base(fPath)) | ||
| } |
Collaborator
|
removed |
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.
Two consolidations in the state-files lifecycle:
openDirtyFiles×3 (Domain/History/InvertedIndex indirty_files.go, ~230 lines of copies): the data-file open block and the accessor-open block (repeated 6× — Domain has three accessor kinds) collapse ontoopenDirtyDataFile/openDirtyAccessor. Per-type deltas (name mask, directory, supported-version set, log tag) are parameters; control flow — which failures invalidate the item vs merely log, corrupted→Debug vs else→Warn — is identical and now stated once.SnapshotRepo.openDirtyFilesis deliberately left unconverted (metadata decompressor, noMustSupport, Error-level logging,SearchVersionaccessors — folding would weaken its differences).deduplicate.gowas a drifted fork ofmerge.go(~200 of its 445 lines token-identical with the merge paths, including a stale commented-out debug line) — and it had missed theMergeSortedsingle-pass upgrade the real merge path later got. The fold givesht.mergeFiles/iit.mergeFilesadedup *valuesDeduperparameter: nil (every existing caller) takes byte-for-byte the original code path; dedup mode collapses per-file equal-value runs and filters the recorded txNums via amergeSortedSkippingcounterpart — so the rebuild tooling inherits the single-pass upgrade.deduplicate.gois deleted;CompactRangedrives the mode with in-place replacement semantics preserved.Characterization test first:
TestHistoryCompactRange(built on the existing history test harness — duplicated runs within and across step files, version-bumped current versions to mirror the rebuild tool) was committed and passed against the olddeduplicateFiles, then passed unchanged against the fold with identical dedup counts. It also exercises the part-1 refactored open path end-to-end on reopen.Deviations, all confined to the rebuild-tool path and listed in the commit message: inputs now removed only on success (the old code defer-deleted them even on mid-merge failure), output handles closed rather than leaked,
noFsynchonored, dedup gets progress reporting.Net −308 lines. Verified: full non-short
db/statesuite green twice (~2 min each), scopedgolangci-lintclean (×2),make erigon,go build ./....Part of a dedup series; siblings #22165–#22177.