From 3523e46595506e31ca193744fc6f8efea09ab86b Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:40:57 +0200 Subject: [PATCH] fix: open each archive of a folder once Each ZIP member read reopened its archive and parsed its directory again: 4000 members of one size made 4000 parses. A folder index keeps the archives it reads open, 16 at most, read under a lock from the copy threads, and every index is closed at the end of the run. Archives the list installs are still opened and closed at once. --- install.ps1 | 2 +- install.py | 80 +++++++++++++++++++++++++++++++++++--- install.sh | 2 +- tests/test_install_from.py | 49 +++++++++++++++++++++++ 4 files changed, 126 insertions(+), 7 deletions(-) diff --git a/install.ps1 b/install.ps1 index c0e93dca..561ddaed 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "c361b511d9550b2115d918a0233115cd9f42920bce881f41395c3d7614be534d" + $defaultInstallSha256 = "5a7dc2fe2aa2abed511a94b3711f1846461a306ea185cd34407efac6c526cab0" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 4568f8e5..43d1dca0 100644 --- a/install.py +++ b/install.py @@ -45,6 +45,7 @@ import unicodedata import urllib.error import urllib.parse import urllib.request +import weakref import xml.etree.ElementTree as ET import zipfile import zlib @@ -6766,6 +6767,8 @@ class LocalSources: self.pack: PackSource | None = None # The folders named with --from, read once for every platform. self.folders: list[FolderIndex] = [] + # Every folder index of the run still in use, closed at its end. + self.indexes: weakref.WeakSet[FolderIndex] = weakref.WeakSet() @property def offline(self) -> bool: @@ -6775,10 +6778,16 @@ class LocalSources: def reset(self) -> None: if self.pack is not None: self.pack.close() + self.close_indexes() self.manifest = None self.pack = None self.folders = [] + def close_indexes(self) -> None: + for index in list(self.indexes): + index.close() + self.indexes = weakref.WeakSet() + LOCAL = LocalSources() NOT_IN_PACK = "not in the pack" @@ -6860,7 +6869,7 @@ def _open_copy(copy: LocalCopy) -> Iterator[BinaryIO]: with open(copy.path, "rb") as handle: yield handle return - with zipfile.ZipFile(copy.path) as archive, archive.open(copy.member) as handle: + with copy.index.open_member(copy.path, copy.member) as handle: yield handle @@ -7014,6 +7023,10 @@ class LocalCopy(NamedTuple): return f"{self.member} inside {self.path}" if self.member else str(self.path) +# The archives a FolderIndex keeps open at once. +OPEN_ARCHIVES = 16 + + class FolderIndex: """The files under a folder, by name and by size, hashed on demand. @@ -7032,6 +7045,7 @@ class FolderIndex: self.archives: list[Path] = [] # Name a ._ file stands beside (casefolded) -> the ._ files. self.apple_double: dict[str, list[Path]] = {} + LOCAL.indexes.add(self) # The list's destinations, casefolded (an archive among them is not # unpacked for its members) and as the list writes them. self.sealed: set[str] = set() @@ -7050,6 +7064,10 @@ class FolderIndex: self._member_named: dict[int, dict[str, list[tuple[Path, str]]]] = {} self._scans: dict[tuple[str, int, str], _HashScan] = {} self._member_size: dict[tuple[Path, str], int] = {} + # The archives read so far, open, the last read at the end: a ZIP + # was opened again, its directory parsed again, for each member. + self._zips: dict[Path, zipfile.ZipFile] = {} + self._zip_lock = threading.Lock() # (path, member or "") -> (its size or None when unknown, why) of each # file, member or folder below root that could not be read. self.unreadable: dict[tuple[Path, str], tuple[int | None, str]] = {} @@ -7121,6 +7139,50 @@ class FolderIndex: return False return True + def _archive(self, path: Path) -> zipfile.ZipFile: + """The archive at path, opened once. Called with _zip_lock held. + + A few stay open, the least recently read closed first: a card of + MAME sets would otherwise hold a descriptor per archive. Closing + one leaves a member being read open until it is closed itself. + Only archives the list does not install are read this way. + """ + archive = self._zips.pop(path, None) + if archive is None: + archive = zipfile.ZipFile(path) + while len(self._zips) >= OPEN_ARCHIVES: + self._zips.pop(next(iter(self._zips))).close() + self._zips[path] = archive + return archive + + def infolist(self, path: Path) -> list[zipfile.ZipInfo]: + with self._zip_lock: + return self._archive(path).infolist() + + @contextlib.contextmanager + def open_member(self, path: Path, member: str) -> Iterator[BinaryIO]: + """A member of an archive under the folder, read from any thread.""" + with self._zip_lock: + handle = self._archive(path).open(member) + try: + yield handle + finally: + # The archive counts its open members: the count moves under + # the lock, as when the member was opened. + with self._zip_lock: + handle.close() + + def open_archives(self) -> int: + with self._zip_lock: + return len(self._zips) + + def close(self) -> None: + """Close the archives the index holds open.""" + with self._zip_lock: + for archive in self._zips.values(): + archive.close() + self._zips.clear() + def seed(self, path: Path, sha1: str) -> None: """A SHA-1 already known, so the file is not read again.""" self._sha1[path] = sha1 @@ -7147,7 +7209,7 @@ class FolderIndex: if key not in self._digests: try: if member: - with zipfile.ZipFile(path) as archive, archive.open(member) as handle: + with self.open_member(path, member) as handle: value = _digest_handle(handle, (algorithm,))[algorithm] else: value = _digest_file(path, (algorithm,))[algorithm] @@ -7192,6 +7254,8 @@ class FolderIndex: def zip_members(self, path: Path) -> list[str]: if path not in self._members: try: + # Read once and closed: the archive may be a file of the + # list, which Windows will not replace while it is open. with zipfile.ZipFile(path) as archive: self._members[path] = [n.rsplit("/", 1)[-1].casefold() for n in archive.namelist()] @@ -7209,8 +7273,7 @@ class FolderIndex: or self.relative(path).casefold() in self.sealed): continue try: - with zipfile.ZipFile(path) as archive: - infos = archive.infolist() + infos = self.infolist(path) except OSError as exc: # A ZIP that will not open may hold anything: named. A # damaged one is a download cut short, not a copy. @@ -8332,7 +8395,14 @@ def main() -> None: def _run(args: argparse.Namespace) -> None: - """The run, its text quiet or not.""" + """The run, its text quiet or not, the archives it read closed after.""" + try: + _run_text(args) + finally: + LOCAL.close_indexes() + + +def _run_text(args: argparse.Namespace) -> None: if not args.quiet: _install(args, sys.stdout) return diff --git a/install.sh b/install.sh index 34c27f9d..a5588df7 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="c361b511d9550b2115d918a0233115cd9f42920bce881f41395c3d7614be534d" +DEFAULT_INSTALL_SHA256="5a7dc2fe2aa2abed511a94b3711f1846461a306ea185cd34407efac6c526cab0" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install_from.py b/tests/test_install_from.py index fafc353e..ccc91de6 100644 --- a/tests/test_install_from.py +++ b/tests/test_install_from.py @@ -18,6 +18,7 @@ import io import json import os import subprocess +import threading import sys import time import unittest @@ -843,6 +844,54 @@ class ManySameSizedFiles(FromFolder): self.assertEqual(len(copies), 3) self.assertLess(elapsed, 30, f"{elapsed:.1f} s") + def test_one_archive_of_thousands_of_members_is_opened_once(self): + """Each member read reopened its ZIP and parsed its directory again: + 4000 members of one size made 4000 parses of 4000 entries.""" + count = 4000 + _zip(self.old / "romset.zip", {f"m{n:04}.bin": b"m%015d" % n for n in range(count)}) + wanted = [_entry(f"z{n}.bin", b"m%015d" % n, ["x"]) for n in (count - 1, 3, 1234)] + index = install.FolderIndex(self.old) + opened = [] + real = zipfile.ZipFile + + def counting(*args, **kwargs): + opened.append(args[0]) + return real(*args, **kwargs) + + start = time.monotonic() + with mock.patch.object(install.zipfile, "ZipFile", side_effect=counting): + copies = install.plan_local_copies(wanted, self.dest, None, [index]) + self.assertEqual(len(copies), 3) + # The copy step reads members of the archive the plan opened, + # from threads. + results = [] + threads = [threading.Thread(target=lambda e=e: results.append( + install._copy_local(e, self.dest, self.dest / e["dest"], copies[id(e)], None))) + for e in wanted] + self.dest.mkdir(parents=True, exist_ok=True) + for thread in threads: + thread.start() + for thread in threads: + thread.join() + elapsed = time.monotonic() - start + self.assertEqual(sorted(ok for _d, ok, _r in results), [True] * 3, results) + self.assertEqual(len(opened), 1) + self.assertLess(elapsed, 20, f"{elapsed:.1f} s") + index.close() + self.assertEqual(index.open_archives(), 0) + + def test_a_card_of_many_archives_holds_few_open(self): + count = install.OPEN_ARCHIVES * 3 + for n in range(count): + _zip(self.old / f"sets/s{n:03}.zip", {f"m{n}.bin": b"n%015d" % n}) + wanted = [_entry(f"z{n}.bin", b"n%015d" % n, ["x"]) for n in range(count)] + index = install.FolderIndex(self.old) + copies = install.plan_local_copies(wanted, self.dest, None, [index]) + self.assertEqual(len(copies), count) + self.assertLessEqual(index.open_archives(), install.OPEN_ARCHIVES) + install.LOCAL.close_indexes() + self.assertEqual(index.open_archives(), 0) + def test_a_file_of_the_same_name_comes_first(self): self.keep("a/other.bin", US) self.keep("b/scph5501.bin", US)