44 week 7 m1 finalize packfiles storage performance baseline

docs/week8-performance.md

@@ -0,0 +1,87 @@
+# Week 8 storage performance
+
+## Reproduction
+
+The shared moderately large fixture is generated by
+[`scripts/benchmark_packfiles.py`](../scripts/benchmark_packfiles.py): 512
+deterministic blob objects, each with a 2,048-byte repeated payload and a
+unique prefix. The benchmark uses a fresh temporary repository, reads through
+a newly constructed `ObjectStore`, and removes loose objects before the
+packed-only read. It reports retained loose bytes separately from pack and
+index bytes.
+
+On Windows from the repository root:
+
+```powershell
+.\.venv\Scripts\python.exe scripts\benchmark_packfiles.py --profile
+.\.venv\Scripts\python.exe scripts\benchmark_packfiles.py --no-cache --profile
+```
+
+`--no-cache` is the pre-Week-8 comparison: it reparses and validates the
+published index and complete pack for every lookup. The benchmark does not
+change the object format, enable pruning, or use delta compression.
+
+## M4 measurement
+
+Environment for the recorded run:
+
+- Python 3.11.9
+- Windows 10, build 26200
+- 512 objects, 2,048-byte payload parameter
+- One fresh-store read of every object in each storage mode
+
+Cached run (`--profile`):
+
+| Measurement | Result |
+| --- | ---: |
+| Loose object write time | 1,972.382 ms |
+| Loose stored bytes | 27,534 |
+| Loose fresh-store read time | 7,278.564 ms |
+| Pack time | 128.798 ms |
+| Retained loose bytes after packing | 27,534 |
+| Pack bytes | 33,179 |
+| Index bytes | 18,444 |
+| Packed-only retained loose bytes | 0 |
+| Packed-only fresh-store read time | 190.982 ms |
+
+The loose and packed byte totals are not directly interchangeable: packing
+does not prune the loose originals, and the current format has no delta
+compression. The packed-only footprint in this run is 51,623 bytes
+(33,179-byte pack plus 18,444-byte index), while the loose-only footprint is
+27,534 bytes.
+
+## Measured fix and profile
+
+The pre-fix comparison (`--no-cache`) reported 5,017.308 ms for the
+packed-only fresh-store read. Its repeated-read profile (three passes) spent
+28.802 seconds total, including 25.959 seconds in complete pack validation and
+21.207 seconds in repeated record decoding.
+
+The fix caches metadata only after a published index and its matching pack
+have passed the existing validation. Each lookup still discovers the current
+`pack-*.idx` directory entries, and the cache signature includes index and
+pack size plus modification time. A newly published pack is therefore found,
+and a changed published file invalidates its cached validation. Selected
+records continue to be decoded and hash-checked on every read.
+
+The cached repeated-read profile took 0.688 seconds for the same three passes.
+The profile's remaining hot path is filesystem discovery and metadata checks
+(`Path.glob`/`Path.stat`), not full-pack validation.
+
+The normal object test suite also covers a small correctness fixture with empty,
+binary, and Unicode blobs; nested trees; two-parent commits; overlapping
+objects across multiple packs; packed-only and mixed reads; duplicate requests;
+fresh store instances; and newly published packs. Existing corruption and
+publication tests cover invalid indexes and records, truncation and zlib
+failures, wrong hashes, and failed publication. These tests assert behavior,
+not wall-clock thresholds.
+
+## Remaining findings and scope
+
+- Loose reads remain sensitive to Windows filesystem latency; this change does
+  not add a loose-object cache.
+- Packed storage is larger when loose originals are retained. Pruning and
+  delta compression remain outside this issue.
+- Pack/index format redesign remains outside this issue.
+- Run `scripts/quality-check.sh` before linking the PR and requesting teammate
+  review. This worktree intentionally has no commit, push, or pull.

minigit/objects.py

@@ -49,6 +49,14 @@ def __init__(self, repo_path=".") -> None:
         self.root = Path(repo_path)
         self.objects_dir = self.root / ".minigit" / "objects"
         self.pack_dir = self.objects_dir / "pack"
+        self._index_cache: dict[
+            Path,
+            tuple[
+                tuple[int, int, int, int],
+                dict[str, tuple[int, int]],
+                Path,
+            ],
+        ] = {}
 
     def hash_object(self, data: bytes, obj_type: str) -> str:
         """
@@ -218,7 +226,7 @@ def _read_packed_object(self, obj_hash: str) -> tuple[str, bytes] | None:
         if not self.pack_dir.exists():
             return None
         for index_path in sorted(self.pack_dir.glob("pack-*.idx")):
-            entries, pack_path = self._read_index(index_path)
+            entries, pack_path = self._read_cached_index(index_path)
             entry = entries.get(obj_hash)
             if entry is None:
                 continue
@@ -232,6 +240,31 @@ def _read_packed_object(self, obj_hash: str) -> tuple[str, bytes] | None:
             return self._decode_packed_record(obj_hash, record)
         return None
 
+    def _read_cached_index(self, index_path: Path) -> tuple[dict[str, tuple[int, int]], Path]:
+        """
+        Reads a cached index from the file system.
+        """
+        pack_path = index_path.with_suffix(".pack")
+        try:
+            index_stat = index_path.stat()
+            pack_stat = pack_path.stat()
+        except OSError as error:
+            raise ObjectCorruptError(index_path.name) from error
+
+        signature = (
+            index_stat.st_mtime_ns,
+            index_stat.st_size,
+            pack_stat.st_mtime_ns,
+            pack_stat.st_size,
+        )
+        cached = self._index_cache.get(index_path)
+        if cached is not None and cached[0] == signature:
+            return cached[1], cached[2]
+
+        entries, pack_path = self._read_index(index_path, pack_path)
+        self._index_cache[index_path] = (signature, entries, pack_path)
+        return entries, pack_path
+
     def _read_index(
         self, index_path: Path, pack_path: Path | None = None
     ) -> tuple[dict[str, tuple[int, int]], Path]:
@@ -291,6 +324,11 @@ def _validate_pack(
                     or position + record_length > len(raw)
                 ):
                     raise ValueError
+                self._decode_packed_record(
+                    "",
+                    raw[position : position + record_length],
+                    verify_hash=False,
+                )
                 records.append((position, record_length))
                 position += record_length
             if position != len(raw):
@@ -300,10 +338,17 @@ def _validate_pack(
                 indexed_records = list(entries.values())
                 if set(indexed_records) != valid_records or len(indexed_records) != count:
                     raise ValueError
-        except (OSError, struct.error, ValueError) as error:
+                for obj_hash, (offset, record_length) in entries.items():
+                    self._decode_packed_record(
+                        obj_hash,
+                        raw[offset : offset + record_length],
+                    )
+        except (OSError, struct.error, ValueError, ObjectCorruptError) as error:
             raise ObjectCorruptError(pack_path.name) from error
 
-    def _decode_packed_record(self, obj_hash: str, record: bytes) -> tuple[str, bytes]:
+    def _decode_packed_record(
+        self, obj_hash: str, record: bytes, verify_hash: bool = True
+    ) -> tuple[str, bytes]:
         try:
             if len(record) < self._RECORD_HEADER.size:
                 raise ValueError
@@ -321,7 +366,9 @@ def _decode_packed_record(self, obj_hash: str, record: bytes) -> tuple[str, byte
             content = decompressor.decompress(compressed) + decompressor.flush()
             if not decompressor.eof or decompressor.unused_data or decompressor.unconsumed_tail:
                 raise ValueError
-            if len(content) != data_length or self.hash_object(content, obj_type) != obj_hash:
+            if len(content) != data_length or (
+                verify_hash and self.hash_object(content, obj_type) != obj_hash
+            ):
                 raise ValueError
         except (UnicodeDecodeError, ValueError, zlib.error) as error:
             raise ObjectCorruptError(obj_hash) from error

minigit/remote.py

@@ -24,6 +24,7 @@
 from minigit.objects import ObjectStore
 
 MAX_PROTOCOL_LINE_BYTES = 65536
+MAX_OBJECT_BYTES = 64 * 1024 * 1024
 
 
 def send_line(sock, text: str) -> None:
@@ -78,6 +79,27 @@ def recv_exact(sock, buf: bytearray, size: int) -> bytes:
     return data
 
 
+def _parse_object_header(header: str) -> tuple[str, int] | None:
+    """Parse an OBJ header without accepting an unbounded receive size."""
+
+    command, _, rest = header.partition(" ")
+    if command != "OBJ":
+        return None
+    obj_type, separator, length_text = rest.partition(" ")
+    if (
+        not separator
+        or obj_type not in {"blob", "tree", "commit"}
+        or not length_text.isascii()
+        or not length_text.isdigit()
+        or len(length_text) > 20
+    ):
+        return None
+    length = int(length_text)
+    if length > MAX_OBJECT_BYTES:
+        return None
+    return obj_type, length
+
+
 def _valid_hash(value: str) -> bool:
     """Return whether `value` is a full 40-character lowercase hex object hash."""
 
@@ -303,6 +325,8 @@ def _put_object(self, sock, buf: bytearray, obj_hash: str) -> None:
         """Send `PUT <hash>` followed by the object's `OBJ` header and bytes."""
 
         obj_type, content = self.store.read_object(obj_hash)
+        if len(content) > MAX_OBJECT_BYTES:
+            raise NetworkProtocolError(f"object {obj_hash} exceeds transfer size limit")
         send_line(sock, f"PUT {obj_hash}")
         send_line(sock, f"OBJ {obj_type} {len(content)}")
         sock.sendall(content)
@@ -451,16 +475,12 @@ def _receive_object(
         if command != "OBJ":
             raise NetworkProtocolError(f"expected OBJ, got {header!r}")
 
-        obj_type, _, length_text = rest.partition(" ")
-        if obj_type not in {"blob", "tree", "commit"} or not (
-            length_text.isascii()
-            and length_text.isdigit()
-            and len(length_text) <= 20
-            and int(length_text) <= sys.maxsize
-        ):
+        parsed_header = _parse_object_header(header)
+        if parsed_header is None:
             raise NetworkProtocolError(f"malformed OBJ header: {header!r}")
+        obj_type, length = parsed_header
 
-        content = recv_exact(sock, buf, int(length_text))
+        content = recv_exact(sock, buf, length)
         if self.store.hash_object(content, obj_type) != expected_hash:
             raise NetworkProtocolError(f"object {expected_hash} failed hash verification")
         if expected_type is not None and obj_type != expected_type:
@@ -735,22 +755,16 @@ def _receive_pushed_object(self, conn, buf: bytearray, expected_hash: str) -> bo
             return False
 
         header = receive_line(conn, buf)
-        command, _, rest = header.partition(" ")
-        if command != "OBJ":
-            send_line(conn, "ERR expected OBJ")
-            return False
-
-        obj_type, _, length_text = rest.partition(" ")
-        if obj_type not in {"blob", "tree", "commit"} or not (
-            length_text.isascii()
-            and length_text.isdigit()
-            and len(length_text) <= 20
-            and int(length_text) <= sys.maxsize
-        ):
-            send_line(conn, "ERR malformed OBJ header")
+        parsed_header = _parse_object_header(header)
+        if parsed_header is None:
+            if not header.startswith("OBJ "):
+                send_line(conn, "ERR expected OBJ")
+            else:
+                send_line(conn, "ERR malformed OBJ header")
             return False
+        obj_type, length = parsed_header
 
-        content = recv_exact(conn, buf, int(length_text))
+        content = recv_exact(conn, buf, length)
         if self.store.hash_object(content, obj_type) != expected_hash:
             send_line(conn, f"ERR hash mismatch for {expected_hash}")
             return False
@@ -800,6 +814,9 @@ def _send_object(self, conn, obj_hash: str) -> None:
             send_line(conn, f"ERR corrupt object {obj_hash}")
             return
 
+        if len(content) > MAX_OBJECT_BYTES:
+            send_line(conn, f"ERR object {obj_hash} exceeds transfer size limit")
+            return
         send_line(conn, f"OBJ {obj_type} {len(content)}")
         conn.sendall(content)