compact: build the chunk index once, not three times - #10359
Open
ThomasWaldmann wants to merge 1 commit into
Open
compact: build the chunk index once, not three times#10359ThomasWaldmann wants to merge 1 commit into
ThomasWaldmann wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10359 +/- ##
==========================================
+ Coverage 88.02% 88.05% +0.02%
==========================================
Files 103 103
Lines 18913 18917 +4
Branches 2919 2919
==========================================
+ Hits 16649 16658 +9
+ Misses 1572 1567 -5
Partials 692 692 ☔ View full report in Codecov by Harness. |
ThomasWaldmann
force-pushed
the
compact-single-chunkindex
branch
2 times, most recently
from
September 12, 2026 14:13
97d13c8 to
6e1cc1c
Compare
Member
Author
|
@mr-raj12 please check. |
"borg compact" built the chunk index up to three times per run and kept two copies of it in memory at the same time: 1. get_repository_chunks() builds compact's own index (it needs the usage flags). 2. analyze_archives() reads the archive metadata objects, and resolving their pack locations lazily builds a second, identical index inside the Repository. 3. compact_packs() calls delete_chunkindex_from_repo(), which drops the repository's in-memory index, so the archive listing in cleanup_files_cache() built a third one. The chunk index is the biggest structure borg keeps in memory (about 80 bytes per chunk plus hash table overhead), so a large repository paid for that twice over. Share compact's index with the repository while the repository reads through it: get_repository_chunks() installs it as repository.chunks, so analyze_archives() resolves pack locations through that same index. The repository only reads pack locations and F_PENDING from an index, never the F_USED flags or the sizes compact tracks for --stats, and stored index fragments zero the flags either way - so the index the repository would have built is the very same one. The sharing ends in compact_packs(): delete_chunkindex_from_repo() (borgbackup#9748) drops the repository's reference before the first store change, and compact does not hand it back. Nothing needs it there - compact_pack() and merge_packs() get chunks=self.chunks, --stats and save_chunk_index() use self.chunks directly. So while the persisted index is gone, the repository holds no in-memory index that Repository.close() could persist on an aborted run, and save_chunk_index() can empty the index in place with clear=True. cleanup_files_cache() used to list the archives itself, after save_chunk_index() had cleared the index, which is what built the third index. It now reuses the names analyze_archives() already collected, so it needs no repository access at all - and every archive's metadata is read once per compact run instead of twice. Verified on the same repository: master and this produce byte-identical repositories, identical persisted index/* fragments (completed, dry-run, no-op, interrupted and slow-rebuild runs alike) and identical output, with 3 index builds before and 1 after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThomasWaldmann
force-pushed
the
compact-single-chunkindex
branch
from
September 12, 2026 20:52
8cfe9a9 to
99ce853
Compare
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.
Problem
borg compactbuilds the chunk index up to three times per run, and keeps two copies of it in memory at the same time:get_repository_chunks()builds compact's own index — it needs the usage flags (init_flags=F_NONE, thenanalyze_archives()marksF_USED).analyze_archives()reads the archive metadata objects. Resolving their pack locations goes throughRepository.chunks, which lazily builds a second, identical index.compact_packs()callsdelete_chunkindex_from_repo(), which drops the repository's in-memory index (borg2: Interruptedborg compactcan poison the cached chunk index and cause silent data loss #9748 crash-safety). The archive listing incleanup_files_cache()afterwards then builds a third one.The chunk index is the biggest structure borg keeps in memory — a
ChunkIndexentry is 32 bytes of key plus a 48-byte value, plus hash table overhead — so on a large repository compact paid for that twice over in RAM and three times over in build time.Fix
Share compact's index with the repository while the repository reads through it.
get_repository_chunks()installs the index it builds asrepository.chunks, soanalyze_archives()resolves pack locations through that same index instead of lazily building a second one.This is sound because the two indexes are the same thing:
pack_id,obj_offset,obj_size) andF_PENDINGout of an index. It never looks at theF_USEDflags or thesizefield that compact maintains for compaction and--stats.write_chunkindex_to_repo), so both call sites load byte-identical entries anyway;init_flagsonly differs on the slow rebuild-from-packs path.The sharing ends before the first store change, and is not restored. In
compact_packs(),delete_chunkindex_from_repo()(crash-safety, #9748) drops the repository's reference together with the persisted fragments, and compact deliberately does not hand the index back. Nothing needs it there: from that point untilsave_chunk_index(), nothing resolves pack locations through the repository —compact_pack()andmerge_packs()getchunks=self.chunks, and--statsandsave_chunk_index()useself.chunksdirectly. So while the persisted index is gone, the repository holds no in-memory index either, andRepository.close()on an aborted run has nothing to persist. (Handing it back would only have been harmless because compact's index never carriesF_NEWentries — an invariant nothing states or checks.) For the same reasonsave_chunk_index()can empty the index in place withclear=True: the repository no longer references it.cleanup_files_cache()no longer lists the archives. It was the third build: it re-listed the archives aftersave_chunk_index()had cleared the index. It now reuses the namesanalyze_archives()already collected, so it needs no repository access at all. That set is still valid there — compaction only nukes soft-deleted archives, which were never in the non-deleted listing. As a side effect every archive's metadata is now read once per compact run instead of twice.Note what this does not change: the call order in
garbage_collect()is untouched, socleanup_files_cache()still runs after thesig_intre-raise (skipped on Ctrl-C, as before), and it stays outside the window betweendelete_chunkindex_from_repo()andsave_chunk_index()in which the repo has no persisted index.Verification
Measured on the current head against its merge base with master. Built one repository, copied it, and ran compact on each copy with master and with this branch:
diff -r), including the content-addressed pack files and index fragmentsborg check --verify-datapasses on the compacted repositoryThe persisted
index/*namespace was compared separately across the runs that drive different index paths — completed compaction,--dry-run, no-op, noindex/*present (slow rebuild), noindex/*+ dry run, and compact-twice. Identical fragment sets in every case (fragment names are the sha256 of their content), and in each case the persisted entries exactly match what the packs hold, checked against a freshslow_rebuildwalk of the pack headers.An interrupted compaction was checked too. It first appeared to differ, but that is pre-existing nondeterminism, not this change:
drop_packsis aset, so which pack is dropped before the interrupt depends on hash randomization — master differs from itself between runs. WithPYTHONHASHSEED=0master is reproducible and master == branch, whole repo and files caches included. (Completed runs match without pinning becausewrite_chunkindex_to_reposorts keys, so a finished index is order-independent.)test_compact_interrupted_does_not_poison_chunk_indexcovers the hard-abort case: no fragments are left behind.Files-cache cleanup, the behaviour whose data source changed, was checked end to end on a run that really compacts: three archive series, one deleted entirely — exactly that series' cache file is removed and the two survivors are kept, identically on both sides.
New regression test
test_compact_builds_the_chunk_index_only_oncecountsbuild_chunkindex_from_repocalls across a run that really compacts (it assertsstore_changed, so it cannot silently degrade into a no-op run). It fails on master withassert 3 == 1and passes here.Full test suite: 3060 passed, 1011 skipped.
🤖 Generated with Claude Code