From 99ce8535673d1f94f8df87fef386b935514f6da9 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sat, 12 Sep 2026 22:47:16 +0200 Subject: [PATCH] compact: build the chunk index once, not three times "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() (#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 Co-Authored-By: Claude Opus 5 --- src/borg/archiver/compact_cmd.py | 25 +++++++++- .../testsuite/archiver/compact_cmd_test.py | 48 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/borg/archiver/compact_cmd.py b/src/borg/archiver/compact_cmd.py index cc6e880088..a6e942a91d 100644 --- a/src/borg/archiver/compact_cmd.py +++ b/src/borg/archiver/compact_cmd.py @@ -33,6 +33,7 @@ def __init__(self, repository, manifest, *, stats, threshold, dry_run=False): self.total_files = None # overall number of source files written to all archives in this repo self.total_size = None # overall size of source file content data written to all archives self.archives_count = None # number of archives + self.archive_series_names = None # names of the existing archives, set by analyze_archives() self.stats = stats # compute repo space usage before/after - lists all repo objects, can be slow. self.threshold = threshold # rewrite a mixed pack only when its wasted-bytes fraction reaches this percent self.dry_run = dry_run @@ -62,6 +63,12 @@ def get_repository_chunks(self) -> ChunkIndex: chunks = build_chunkindex_from_repo( self.repository, write_immediately=not self.dry_run, init_flags=ChunkIndex.F_NONE ) + # Hand this index to the repository as well, so reading the archives below does not lazily + # build a second, identical copy of the biggest structure borg keeps in memory (see the + # .chunks property). The repository only reads pack locations and F_PENDING from it, never + # the F_USED flags or sizes this index tracks for compaction and --stats. It stays shared + # until compact_packs() invalidates the chunk index before its first store change. + self.repository.chunks = chunks return chunks def save_chunk_index(self): @@ -69,6 +76,8 @@ def save_chunk_index(self): # and also remove all older chunk indexes. # write_chunkindex_to_repo now removes all flags and size infos. # we need this, as we put the wrong size in there to support --stats computations. + # clear=True empties the index in place: safe, the repository dropped its reference to it in + # compact_packs() before the first store change, so it cannot see an empty index here. write_chunkindex_to_repo( self.repository, self.chunks, incremental=False, clear=True, force_write=True, delete_other=True ) @@ -78,9 +87,14 @@ def cleanup_files_cache(self): """ Clean up files cache files for archive series names that no longer exist in the repository. + Works from the archive names analyze_archives() collected, so this needs no repository access: + it runs after save_chunk_index() has cleared the chunk index, and the archive set does not + change in between (compaction only removes soft-deleted archives, which were never in it). + Note: this only works perfectly if the files cache filename suffixes are automatically generated and the user does not manually control them via more than one BORG_FILES_CACHE_SUFFIX env var value. """ + assert self.archive_series_names is not None, "analyze_archives() must run first" logger.info("Cleaning up files cache...") cache_dir = Path(get_cache_dir(self.repository.id_str, create=False)) @@ -89,7 +103,7 @@ def cleanup_files_cache(self): return # Get all existing archive series names - existing_series = set(self.manifest.archives.names()) + existing_series = self.archive_series_names logger.debug(f"Found {len(existing_series)} existing archive series.") # Get the set of all existing files cache file names. @@ -141,6 +155,9 @@ def analyze_archives(self) -> tuple[set, int, int, int]: missing_chunks: set[bytes] = set() archive_infos = self.manifest.archives.list(sort_by=["ts"]) num_archives = len(archive_infos) + # an archive's name is its series name; cleanup_files_cache() needs these later, and reading + # every archive's metadata a second time just to get them again would be wasteful. + self.archive_series_names = {info.name for info in archive_infos} cached_hex_ids = list_archive_reference_caches(self.repository) if not self.dry_run: # drop the reference caches of archives that do not exist anymore. @@ -365,7 +382,11 @@ def compact_packs(self): logger.info("Deleting 0 unused objects...") return repo_size_before, repo_size_before # nothing worth doing; chunk indexes stay valid - # crash-safety (#9748): invalidate chunk indexes before the first store change + # crash-safety (#9748): invalidate chunk indexes before the first store change. This also + # drops the repository's reference to self.chunks (shared since get_repository_chunks()). + # Do not hand it back: until save_chunk_index() has written the updated index, the repo + # must hold no in-memory index that close() could persist on an aborted run. Nothing below + # needs one, compact_pack() and merge_packs() work on chunks=self.chunks. delete_chunkindex_from_repo(self.repository) self.store_changed = True diff --git a/src/borg/testsuite/archiver/compact_cmd_test.py b/src/borg/testsuite/archiver/compact_cmd_test.py index 39482a8718..f6dc4bf281 100644 --- a/src/borg/testsuite/archiver/compact_cmd_test.py +++ b/src/borg/testsuite/archiver/compact_cmd_test.py @@ -14,7 +14,9 @@ from ...cache import delete_chunkindex_from_repo, write_chunkindex_to_repo from ...manifest import Manifest from ...archive import Archive +from ...archiver import compact_cmd from ...archiver.compact_cmd import ArchiveGarbageCollector +from ... import cache from . import cmd, create_regular_file, create_src_archive, generate_archiver_tests, open_repository, RK_ENCRYPTION from . import changedir from ..repository_test import H, fchunk, pdchunk @@ -763,3 +765,49 @@ def test_compact_files_cache_cleanup(archivers, request): # Get expected cache files for remaining archives expected_cache_files = {files_cache_name(name) for name in ["archive1", "archive3"]} assert expected_cache_files == remaining_cache_files, "Unexpected cache files found" + + +def test_compact_builds_the_chunk_index_only_once(archivers, request, monkeypatch): + """The chunk index is the biggest structure borg keeps in memory, so compact must build one, not + several copies of it. + + Compact builds its own index (it needs the usage flags) and hands it to the repository, so reading + the archives resolves pack locations through that same index instead of lazily building a second, + identical one. + """ + archiver = request.getfixturevalue(archivers) + + # file_a only ever belongs to archive1, file_b to both: deleting archive1 leaves file_a's chunks + # unused next to still-used objects in the same pack, so compaction rewrites that pack. + create_regular_file(archiver.input_path, "file_a", contents=os.urandom(1024 * 1024)) + create_regular_file(archiver.input_path, "file_b", contents=os.urandom(1024 * 1024)) + cmd(archiver, "repo-create", RK_ENCRYPTION) + cmd(archiver, "create", "archive1", "input") + os.remove(os.path.join(archiver.input_path, "file_a")) + cmd(archiver, "create", "archive2", "input") + cmd(archiver, "delete", "-a", "archive1") + + builds = 0 + original_build = cache.build_chunkindex_from_repo + + def counting_build(repository, **kwargs): + nonlocal builds + builds += 1 + return original_build(repository, **kwargs) + + # the repository imports the function inside its .chunks property, compact_cmd at module level: + monkeypatch.setattr(cache, "build_chunkindex_from_repo", counting_build) + monkeypatch.setattr(compact_cmd, "build_chunkindex_from_repo", counting_build) + + repository = open_repository(archiver) + with repository: + manifest = Manifest.load(repository, (Manifest.Operation.DELETE,)) + gc = ArchiveGarbageCollector(repository, manifest, stats=True, threshold=0.0) + gc.garbage_collect() + assert gc.store_changed, "this repo must really get compacted, or the test proves nothing" + + assert builds == 1 + + # the repository is still intact and the surviving archive still reads back + cmd(archiver, "check") + cmd(archiver, "list", "archive2")