From 684c6d95dec8a72d2d943dbb39ea7f8b63c45295 Mon Sep 17 00:00:00 2001 From: retoor Date: Mon, 14 Sep 2026 09:55:47 +0000 Subject: [PATCH] Fix pack_write's O(n^2) dedup + correctness bug; document mount table O(n^2) A audit for other instances of the file-index O(n^2) shape (fixed previously via the persistent treap) found two more real issues: 1. pack_write's Section 9.2 exact-duplicate elimination was a linear scan of every previously-seen (hash, size) pair per entry -- O(n^2) total, invisible in the existing benchmark because its identical-content test files made every scan match on the first comparison. Measured with unique content instead: 80,000 entries took 1.74s, with a 20,000->80,000 step showing 15.6x for a 4x-N step, matching O(n^2)'s 16x prediction. The same scan also trusted a (hash, size) match without ever comparing actual bytes -- a latent correctness bug, since FNV-1a64 is explicitly not collision-resistant. Fixed both at once with an open-addressing hash table (load factor 1/2, linear probing) plus a memcmp verification before ever reusing a data_off. Post-fix: 80,000 entries in 0.044s (39.6x faster), ratio drops to 3.35x (consistent with O(n)). Covered permanently by two new/extended tests: a white-box assertion in test_pack_overlay.c that duplicate-content entries share one data_off and distinct-content entries do not, and a new tests/test_pack_write_perf.c regression tripwire against 10,000 unique entries. 2. vfs.c's mount table uses the same full-array-copy-per-write pattern the file index used to, confirmed O(n^2) via a new bench/bench.c category (500/2,000/8,000 mounts, both 4x-N steps showing 15-20x). Deliberately NOT rewritten: mount points are created by a program's own source code, not workload-driven, so realistic mount counts never reach the scale that made the file index's O(n^2) a real problem. Documented with full reasoning in BENCH.md and CLAUDE.md rather than silently left as an undocumented gap. Also fixes a real CI gap the new tests exposed: ci.yml's sanitizer-build steps never passed -D_GNU_SOURCE when compiling test files (only the library .o's got it), which was harmless while no test included internal.h and became a link failure once two did (internal.h needs _GNU_SOURCE for pthread_rwlock_t). And documents, in CONTRIBUTING.md and CLAUDE.md, a sandbox flake observed directly during this work's own sanitizer runs: ASan/UBSan test binaries occasionally fail to start with AddressSanitizer:DEADLYSIGNAL (sometimes looping rather than exiting), non-deterministically hitting different unrelated binaries across runs -- a startup race, not a memory-safety bug, confirmed by clean passes on retry; sanitizer runs in such an environment should be timeout-wrapped. BENCH.md's "After" table and Appendix B are replaced with the current, complete 54-measurement bench/bench.c run (the original 45 plus the new mount-scaling category); the pre-fix 45-measurement "Before" table is kept as the historical record, per this project's documentation standard. Verified: make test (all 6 binaries, including the 2 new/changed), a clean make all, and repeated ASan+UBSan runs (0 real findings; the DEADLYSIGNAL flake above was observed and correctly distinguished from a real finding by re-running until a clean pass). TSan could not be run in this sandbox (pre-existing, documented environment limitation). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB --- .github/workflows/ci.yml | 4 +- BENCH.md | 548 ++++++++++++++++++++++++----------- CLAUDE.md | 20 +- CONTRIBUTING.md | 42 +++ README.md | 27 +- bench/bench.c | 66 +++++ src/pack.c | 63 +++- tests/test_pack_overlay.c | 23 ++ tests/test_pack_write_perf.c | 105 +++++++ 9 files changed, 693 insertions(+), 205 deletions(-) create mode 100644 tests/test_pack_write_perf.c diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 27ad72e..976bddb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,7 +25,7 @@ jobs: done for t in tests/test_*.c; do name=$(basename "${t%.c}") - cc -std=c11 -O1 -g -Iinclude -Isrc -fsanitize=address,undefined \ + cc -std=c11 -O1 -g -Iinclude -Isrc -D_GNU_SOURCE -fsanitize=address,undefined \ "$t" build/san/*.o -lpthread -o "build/san/$name" "./build/san/$name" done @@ -39,7 +39,7 @@ jobs: done for t in tests/test_*.c; do name=$(basename "${t%.c}") - cc -std=c11 -O1 -g -Iinclude -Isrc -fsanitize=thread \ + cc -std=c11 -O1 -g -Iinclude -Isrc -D_GNU_SOURCE -fsanitize=thread \ "$t" build/tsan/*.o -lpthread -o "build/tsan/$name" "./build/tsan/$name" done diff --git a/BENCH.md b/BENCH.md index a476519..acf35d3 100644 --- a/BENCH.md +++ b/BENCH.md @@ -1,15 +1,24 @@ # PackFS vs. the host filesystem: benchmark results -**Status: the O(n²) bulk create/unlink finding this document originally -reported has been fixed.** The index (`UpperSnapshot`) is now a persistent -treap instead of a flat sorted array (`src/upper.c`; see `CLAUDE.md`'s -"Known performance characteristics"). This document keeps the original -run's numbers as the historical record of the problem — per this project's -own documentation standard, a correction extends the record rather than -erasing it — and adds the post-fix run's numbers alongside them, in full, -not a curated subset of either. **Read "Resolution" below before drawing -conclusions from the "Before" tables**, which describe a version of the -index this codebase no longer has. +**Status: two O(n²) findings this document originally reported have been +fixed; one further O(n²) finding is documented below and deliberately left +unfixed, with justification.** The file index (`UpperSnapshot`) is now a +persistent treap instead of a flat sorted array (`src/upper.c`; see +`CLAUDE.md`'s "Known performance characteristics") — see "Resolution" +below. `pack_write`'s compaction-time duplicate-content elimination +(Section 9.2) was also a linear scan with the same O(n²) shape, plus a +latent correctness bug (a hash match was trusted without a `memcmp` +verification); both are fixed — see "Resolution #2" below. The mount +table (`vfs.c`'s `MountSnapshot`) uses the same full-array-copy pattern the +file index used to, is confirmed O(n²) in mount count, and is *not* +rewritten — see "Finding: mount table scaling" below for why that is the +correct call, not an oversight. This document keeps every prior run's +numbers as the historical record of each problem — per this project's own +documentation standard, a correction extends the record rather than erasing +it — and adds each fix's numbers alongside them, in full, not a curated +subset of either. **Read "Resolution," "Resolution #2," and "Finding: mount +table scaling" below before drawing conclusions from the "Before" tables**, +which describe a version of the index this codebase no longer has. This document reports the results of `bench/bench.c` (`make bench`), each run once on the environment described below. These are single runs, not a @@ -20,11 +29,17 @@ exactly what each backend label measures, is documented in `bench/bench.c`'s file header; read it before interpreting the numbers below, since "raw fs," "raw+fsync," and "dir" are not interchangeable baselines. -Every table below reproduces every one of the 45 measurements each run's -`=== summary (45 measurements) ===` block reported — none omitted, none -combined — and the complete verbatim program output for both runs, exactly -as captured, is preserved in the two appendices at the end as the primary -source record. +The "Before" table below reproduces every one of the 45 measurements that +run's `=== summary (45 measurements) ===` block reported; the "After" table +reproduces every one of the 54 measurements the current `bench/bench.c` +reports (the original 45, unchanged in category, plus 9 new mount-table- +scaling measurements added after the original run — see "Finding: mount +table scaling"). None omitted, none combined in either table, and the +complete verbatim program output for both runs, exactly as captured, is +preserved in the two appendices at the end as the primary source record. +The `pack_write` dedup fix ("Resolution #2") is measured separately, by +calling `pack_write` directly rather than through `bench/bench.c` — see +that section for why. ## Environment @@ -35,12 +50,19 @@ source record. disk or tmpfs. This matters most for the `fsync` numbers (see below). - `N_SMALL` = 20,000 files × 128 bytes for the metadata-heavy suite; `N_DIRS` = 4,000; concurrency = 8 threads × 4,000 ops; large files up to - 64 MB. Full parameters are in `bench/bench.c`. -- Wall-clock time: 5m49s (before), 4m4s (after), almost entirely spent in - the single `raw+fsync` 20,000-file create test (~116s in both runs — this - cost is on the host's storage, not PackFS, and is within measurement noise - of identical before and after the fix, as expected, since that test never - touches PackFS's index at all). + 64 MB; `N_MOUNTS` = 2,000 (run at N/4, N, N×4 — 500/2,000/8,000 — for the + mount-table-scaling category added after the original run). Full + parameters are in `bench/bench.c`. +- Wall-clock time: 5m49s (before), 4m4s (after the treap fix, before the + mount-scaling category existed), 5m24s (current, with the mount-scaling + category added). The dominant cost throughout is the single `raw+fsync` + 20,000-file create test (~116s in every run — this cost is on the host's + storage, not PackFS, and is within measurement noise across all three + runs, as expected, since that test never touches PackFS's index at all); + the added mount-scaling category itself accounts for only ~3.7s of the + increase from 4m4s to 5m24s, so the remainder is run-to-run variance in + untimed setup/teardown, consistent with this document's stated single-run, + illustrative-of-shape methodology, not a regression. ## Resolution @@ -105,6 +127,141 @@ renames, nested directories, thousands of entries, cross-checked against an independent reference model — not just "the benchmark ran fast without crashing") is `tests/test_index_stress.c`, part of `make test`. +## Resolution #2: pack_write compaction dedup + +A second, independent O(n²) finding, found while auditing the codebase for +other instances of the same shape of bug after "Resolution" above: `src/ +pack.c`'s `pack_write` implements Section 9.2's exact-duplicate elimination +(two entries with byte-identical content share one `data_off` in the +compacted pack) as a linear scan, for each entry, of every previously-seen +`(hash, size)` pair. That is O(n) per entry, O(n²) total across n distinct +blobs — the same complexity-class bug "Resolution" fixed in the file index, +here in the compaction writer instead. It went unnoticed by the original +benchmark for a specific, identifiable reason: `bench/bench.c`'s compaction +test (category 8, "compact N entries to pack") writes byte-identical content +into every file, so every entry's linear scan matched on the very first +comparison (against entry 0) and never actually scanned — the benchmark's +own test data happened to be the linear scan's *best* case, not a +representative one. Measuring with unique per-file content instead (each +entry's actual worst case) surfaced it: + +| N (unique entries) | Time | +|---|---| +| 5,000 | 0.0138s | +| 20,000 | 0.1114s | +| 80,000 | 1.7389s | + +5,000 → 20,000 is a 4x step in N; time increased 8.07x. 20,000 → 80,000 is +also a 4x step; time increased 15.61x, converging toward O(n²)'s 16x +prediction as N grows (the smaller first ratio reflects fixed per-call +overhead not yet dominated by the quadratic term) — the same convergence +pattern "Resolution" observed for the file index, and the same standard used +there to distinguish a real complexity-class finding from noise. + +This scan had a second, latent problem beyond speed: it compared only +`(hash, size)`, never the actual bytes, before deciding two entries were +duplicates and sharing their `data_off`. FNV-1a64 (`src/hash.c`) is +explicitly not collision-resistant — it is a fast checksum, not a +cryptographic hash — so two different-content entries that happened to +collide on `(hash, size)` would have been silently merged into one blob, +corrupting one of them. This was never observed in practice (no test's data +happened to collide), but was true of the code as written, not a +hypothetical: the fix required for the O(n²) issue also closes it. + +**Fix:** the linear scan was replaced with an open-addressing hash table +(load factor 1/2, linear probing) over `(hash, size)`, plus — critically — +an actual `memcmp` against the candidate's stored data pointer before ever +reusing a `data_off`, so a hash match is only ever a candidate, never +accepted as proof of equality. Comment and full listing: +`src/pack.c`'s `DedupSlot` struct and the rewritten first pass of +`pack_write`. + +**After the fix, same methodology:** + +| N (unique entries) | Time | vs. before | +|---|---|---| +| 5,000 | 0.0079s | 1.75x faster | +| 20,000 | 0.0131s | 8.50x faster | +| 80,000 | 0.0439s | 39.6x faster | + +5,000 → 20,000 now shows 1.66x (O(n) predicts 4x, but a hash table's +dominant cost at these sizes is close to constant per insert, so sub-linear +growth here is expected, not suspicious); 20,000 → 80,000 shows 3.35x, +close to O(n)'s 4x prediction and nowhere near O(n²)'s 16x. The complexity +class changed, exactly as with the file index. + +This fix is not reflected in the "After" table/Appendix B below, because +that `make bench` run predates it — but it does not need to be: category +8's "compact N entries to pack" measurement uses identical content (as +explained above), which was already the linear scan's O(1)-per-entry best +case before this fix and is unaffected by the fix (a hash table lookup that +matches on the first probe is also O(1) per entry); the number in the +"After" table (0.0267s, 20,000 identical-content entries) remains accurate +and does not need to be re-measured. The fix matters for compacting many +files with *distinct* content, which `bench/bench.c`'s existing compaction +test does not exercise — hence the separate, direct `pack_write` +measurement above rather than a `bench/bench.c` change. Correctness is +covered permanently by `tests/test_pack_overlay.c` (asserts `/a.txt` and +`/dup.txt`, identical content, share one `data_off`, while `/b.txt`, +different content, shares neither's — catching both "dedup stopped +happening" and "dedup over-matched"); the performance fix is covered +permanently by `tests/test_pack_write_perf.c` (10,000 unique entries under +a generous time bound, a regression tripwire rather than a tight +assertion, run as part of `make test`). + +## Finding: mount table scaling (O(n²), documented, deliberately not fixed) + +`src/vfs.c`'s mount table (`MountSnapshot`) uses the exact same pattern the +file index used before "Resolution" above: every `vfs_mount`/`vfs_unmount` +copies the entire array of mounted backends before publishing the next +snapshot (Section 5.3's single-writer model applies to the mount table too, +per `concept.md`'s explicit statement that "the mount table is not separate +global state exempt from this model"). This is confirmed O(n²) in mount +count by the `bench/bench.c` category 10 measurements (full numbers in the +"After" table below and Appendix B): + +| N mounts | mount | resolve (stat through N mounts) | unmount | +|---|---|---|---| +| 500 | 0.0054s | 0.0020s | 0.0046s | +| 2,000 | 0.0831s | 0.0296s | 0.0758s | +| 8,000 | 1.4739s | 0.4938s | 1.5172s | + +500 → 2,000 is a 4x step in N: mount increased 15.4x, resolve 14.8x, +unmount 16.5x. 2,000 → 8,000 is also a 4x step: mount increased 17.7x, +resolve 16.7x, unmount 20.0x. All six ratios cluster around O(n²)'s 16x +prediction for a 4x-N step, and nowhere near O(n)'s 4x — the same +complexity-class-confirmation methodology used for both findings above, +applied here and yielding the same verdict. Notably, `resolve` (a single +`vfs_stat` through an already-built N-mount table) is *also* O(n²) across N +resolves, not just O(n) per resolve as might be assumed — consistent with +longest-prefix-match dispatch being a linear scan of the mount table per +call (Section 3's dispatch design), which is itself O(n) per call and O(n²) +across N calls, independent of the snapshot-copy cost on the write side. + +**This is deliberately not fixed, and the reasoning is the same trigger +condition `concept.md` Section 5.3 names for the file index — inverted:** +"a persistent (structurally shared) tree structure is not required until +this assumption is empirically violated." The file index's assumption was +violated because file counts are workload-driven and can plausibly reach +tens of thousands (or more) at runtime, chosen by whatever program embeds +PackFS and whatever data it manages. Mount counts are different in kind, +not just degree: a mount point is created by a call to `vfs_mount` written +into a program's own source code, at a location the program's author +chose — it is bounded by how many lines of "mount this backend at this +path" a person or a build script is willing to write, not by user data, +input size, or any other externally-driven quantity. A program with 8,000 +mount points, structured that way on purpose, is not a realistic workload +this system needs to serve well; a program with 8,000 *files* is an +ordinary one. Converting the mount table to a persistent treap would fix +this at the cost of real, permanent complexity (an additional generic +persistent-tree instantiation, or a second copy of the treap logic +specialized to prefix matching) for a case that has not been and is not +expected to be hit. Should a future use case genuinely need thousands of +mount points at runtime — for instance, one mount per user in a +multi-tenant embedding — this finding, and the numbers above, are the +starting point for that decision; until then, this is recorded as a known, +measured, and consciously accepted limitation, not an unexamined one. + ## Before: complete results (pre-fix; kept as the historical record — see "Resolution" above) All 45 measurements from that run's summary block, none omitted or combined. @@ -157,65 +314,83 @@ All 45 measurements from that run's summary block, none omitted or combined. | concurrent create+read+unlink (8×4,000×3) | mem | 36.7261s | 2,614 ops/s | — | | concurrent create+read+unlink (8×4,000×3) | raw fs | 6.1202s | 15,686 ops/s | — | -## After: complete results (post-fix) +## After: complete results (post-fix, current) -All 45 measurements from that run's summary block, none omitted or combined. +All 54 measurements from the current `bench/bench.c`'s summary block, none +omitted or combined: the original 45 (re-run, post-treap-fix) plus 9 new +mount-table-scaling measurements (category 10, added after the original +run — see "Finding: mount table scaling" above). The `pack_write` dedup fix +("Resolution #2" above) postdates this run but does not change any number +in it — see that section for why the compaction row below is unaffected. | Category | Backend | Time | Throughput | MB/s | |---|---|---|---|---| -| create 20,000 files | mem | 0.0372s | 537,103 ops/s | 65.6 | -| create 20,000 files | raw fs | 1.0098s | 19,805 ops/s | 2.4 | -| create 20,000 files | raw+fsync | 116.0980s | 172 ops/s | 0.0 | -| read 20,000 files | mem | 0.0085s | 2,364,410 ops/s | 288.6 | -| read 20,000 files | raw fs | 0.1594s | 125,459 ops/s | 15.3 | -| stat 20,000 files | mem | 0.0073s | 2,723,001 ops/s | — | -| stat 20,000 files | raw fs | 0.0697s | 287,063 ops/s | — | -| readdir (20,000 entries) | mem | 0.0042s | 4,816,418 /s | — | -| readdir (20,000 entries) | raw fs | 0.0070s | 2,840,890 /s | — | -| create 20,000 files | dir | 1.1531s | 17,345 ops/s | 2.1 | -| read 20,000 files | dir | 0.1601s | 124,933 ops/s | 15.3 | -| stat 20,000 files | dir | 0.1509s | 132,517 ops/s | — | -| readdir (20,000 entries) | dir | 0.0039s | 5,128,581 /s | — | -| unlink 20,000 files | mem | 0.0320s | 625,437 ops/s | — | -| unlink 20,000 files | dir | 0.5146s | 38,864 ops/s | — | -| unlink 20,000 files | raw fs | 0.4923s | 40,628 ops/s | — | -| mkdir 4,000 dirs | mem | 0.0052s | 771,085 ops/s | — | -| rmdir 4,000 dirs | mem | 0.0045s | 898,169 ops/s | — | -| mkdir 4,000 dirs | dir | 0.1774s | 22,551 ops/s | — | -| rmdir 4,000 dirs | dir | 0.1118s | 35,778 ops/s | — | -| mkdir 4,000 dirs | raw fs | 0.1367s | 29,251 ops/s | — | -| rmdir 4,000 dirs | raw fs | 0.0901s | 44,388 ops/s | — | -| write 1MB | mem | 0.0003s | 2,937 ops/s | 2,937.3 | -| read 1MB | mem | 0.0000s | 49,752 ops/s | 49,751.7 | -| write 16MB | mem | 0.0108s | 93 ops/s | 1,483.0 | -| read 16MB | mem | 0.0010s | 1,050 ops/s | 16,796.7 | -| write 64MB | mem | 0.0589s | 17 ops/s | 1,086.8 | -| read 64MB | mem | 0.0033s | 304 ops/s | 19,475.8 | -| write 1MB | raw fs | 0.0004s | 2,463 ops/s | 2,462.6 | -| read 1MB | raw fs | 0.0001s | 15,738 ops/s | 15,738.2 | -| write 16MB | raw fs | 0.0046s | 217 ops/s | 3,477.6 | -| read 16MB | raw fs | 0.0011s | 945 ops/s | 15,121.6 | -| write 64MB | raw fs | 0.0183s | 55 ops/s | 3,495.7 | -| read 64MB | raw fs | 0.0050s | 198 ops/s | 12,702.5 | -| write 1MB | raw+fsync | 0.0235s | 43 ops/s | 42.5 | -| read 1MB | raw+fsync | 0.0001s | 9,816 ops/s | 9,816.4 | -| write 16MB | raw+fsync | 0.0228s | 44 ops/s | 701.5 | -| read 16MB | raw+fsync | 0.0012s | 832 ops/s | 13,316.1 | -| write 64MB | raw+fsync | 0.0825s | 12 ops/s | 775.9 | -| read 64MB | raw+fsync | 0.0057s | 176 ops/s | 11,254.5 | -| compact 20,000 entries to pack | pack | 0.0245s | 814,864 ops/s | 99.5 | -| random-read 20,000 entries | pack (mmap'd) | 0.0082s | 2,445,144 ops/s | 298.5 | -| random-read 20,000 entries | raw fs | 0.1657s | 120,709 ops/s | 14.7 | -| concurrent create+read+unlink (8×4,000×3) | mem | 0.2807s | 341,972 ops/s | — | -| concurrent create+read+unlink (8×4,000×3) | raw fs | 6.2980s | 15,243 ops/s | — | +| create 20,000 files | mem | 0.0400s | 499,809 ops/s | 61.0 | +| create 20,000 files | raw fs | 0.9943s | 20,114 ops/s | 2.5 | +| create 20,000 files | raw+fsync | 116.2147s | 172 ops/s | 0.0 | +| read 20,000 files | mem | 0.0099s | 2,011,901 ops/s | 245.6 | +| read 20,000 files | raw fs | 0.1606s | 124,543 ops/s | 15.2 | +| stat 20,000 files | mem | 0.0077s | 2,598,241 ops/s | — | +| stat 20,000 files | raw fs | 0.0699s | 286,222 ops/s | — | +| readdir (20,000 entries) | mem | 0.0041s | 4,845,826 /s | — | +| readdir (20,000 entries) | raw fs | 0.0069s | 2,894,815 /s | — | +| create 20,000 files | dir | 1.1418s | 17,517 ops/s | 2.1 | +| read 20,000 files | dir | 0.1530s | 130,705 ops/s | 16.0 | +| stat 20,000 files | dir | 0.1379s | 145,056 ops/s | — | +| readdir (20,000 entries) | dir | 0.0037s | 5,465,162 /s | — | +| unlink 20,000 files | mem | 0.0179s | 1,117,742 ops/s | — | +| unlink 20,000 files | dir | 0.4911s | 40,724 ops/s | — | +| unlink 20,000 files | raw fs | 0.4746s | 42,141 ops/s | — | +| mkdir 4,000 dirs | mem | 0.0056s | 719,696 ops/s | — | +| rmdir 4,000 dirs | mem | 0.0040s | 993,724 ops/s | — | +| mkdir 4,000 dirs | dir | 0.1710s | 23,386 ops/s | — | +| rmdir 4,000 dirs | dir | 0.1257s | 31,827 ops/s | — | +| mkdir 4,000 dirs | raw fs | 0.1374s | 29,122 ops/s | — | +| rmdir 4,000 dirs | raw fs | 0.0951s | 42,043 ops/s | — | +| write 1MB | mem | 0.0001s | 7,058 ops/s | 7,058.1 | +| read 1MB | mem | 0.0000s | 50,051 ops/s | 50,050.9 | +| write 16MB | mem | 0.0110s | 91 ops/s | 1,458.2 | +| read 16MB | mem | 0.0009s | 1,058 ops/s | 16,920.1 | +| write 64MB | mem | 0.0586s | 17 ops/s | 1,091.3 | +| read 64MB | mem | 0.0032s | 313 ops/s | 20,056.5 | +| write 1MB | raw fs | 0.0004s | 2,759 ops/s | 2,759.4 | +| read 1MB | raw fs | 0.0001s | 16,812 ops/s | 16,812.4 | +| write 16MB | raw fs | 0.0044s | 229 ops/s | 3,656.4 | +| read 16MB | raw fs | 0.0010s | 989 ops/s | 15,827.9 | +| write 64MB | raw fs | 0.0186s | 54 ops/s | 3,449.7 | +| read 64MB | raw fs | 0.0047s | 213 ops/s | 13,633.1 | +| write 1MB | raw+fsync | 0.0180s | 56 ops/s | 55.7 | +| read 1MB | raw+fsync | 0.0001s | 9,059 ops/s | 9,058.8 | +| write 16MB | raw+fsync | 0.0231s | 43 ops/s | 691.7 | +| read 16MB | raw+fsync | 0.0012s | 855 ops/s | 13,672.2 | +| write 64MB | raw+fsync | 0.0803s | 12 ops/s | 797.2 | +| read 64MB | raw+fsync | 0.0048s | 209 ops/s | 13,393.5 | +| compact 20,000 entries to pack | pack | 0.0267s | 747,839 ops/s | 91.3 | +| random-read 20,000 entries | pack (mmap'd) | 0.0082s | 2,435,930 ops/s | 297.4 | +| random-read 20,000 entries | raw fs | 0.1629s | 122,749 ops/s | 15.0 | +| concurrent create+read+unlink (8×4,000×3) | mem | 0.2750s | 349,127 ops/s | — | +| concurrent create+read+unlink (8×4,000×3) | raw fs | 5.6498s | 16,992 ops/s | — | +| mount 500 backends | vfs | 0.0054s | 92,833 ops/s | — | +| resolve, 500 mounts | vfs | 0.0020s | 246,064 ops/s | — | +| unmount 500 backends | vfs | 0.0046s | 109,207 ops/s | — | +| mount 2,000 backends | vfs | 0.0831s | 24,081 ops/s | — | +| resolve, 2,000 mounts | vfs | 0.0296s | 67,458 ops/s | — | +| unmount 2,000 backends | vfs | 0.0758s | 26,397 ops/s | — | +| mount 8,000 backends | vfs | 1.4739s | 5,428 ops/s | — | +| resolve, 8,000 mounts | vfs | 0.4938s | 16,200 ops/s | — | +| unmount 8,000 backends | vfs | 1.5172s | 5,273 ops/s | — | Everything not involving bulk `mem`/`dir` structural writes (reads, `stat`, `readdir`, large sequential I/O, pack random-access, compaction) is unchanged within normal run-to-run noise, as the row-by-row comparison -above shows — the treap rewrite only touches the structural-write path -(Section 5.3), not content reads, content writes, or the pack format. The -`raw+fsync` create row is identical to three decimal places (116.1030s vs -116.0980s) precisely because it never touches PackFS at all. +against the "Before" table shows — the treap rewrite only touches the +structural-write path (Section 5.3), not content reads, content writes, or +the pack format. The `raw+fsync` create row is unchanged within noise +(116.1030s before, 116.2147s here) precisely because it never touches +PackFS at all. The nine new mount-table rows have no "Before" counterpart — +the mount table was never rewritten, so there is no pre/post comparison to +make for them; they are current-state measurements only, discussed in +"Finding: mount table scaling" above. ## Analysis @@ -251,6 +426,12 @@ now marked where "Resolution" above supersedes it. among its clearest wins. - **Concurrent mixed create+read+unlink: RESOLVED, now 22x faster than raw fs** (was 6x *slower* before the fix) — see "Resolution." +- **`pack_write` compaction dedup on distinct-content files: RESOLVED, 1.75– + 39.6x faster depending on N, plus a latent hash-collision correctness bug + closed alongside it** — see "Resolution #2." Not visible in this + benchmark's own compaction test (category 8), which uses identical + content and so never exercised either problem; measured separately by + calling `pack_write` directly. ### Where raw fs still wins, and why that's expected, not a bug @@ -430,147 +611,170 @@ user 2m9.600s sys 0m20.941s ``` -## Appendix B: complete verbatim output, after the fix +## Appendix B: complete verbatim output, after the fix (current, 54 measurements) -Captured exactly as produced by `make bench` against the post-fix -(persistent treap) index; reformatted into the tables above, but reproduced -here unedited as the primary source record. +Captured exactly as produced by the current `make bench` (post-treap-fix +index, with the mount-table-scaling category added); reformatted into the +tables above, but reproduced here unedited as the primary source record. +This run predates the `pack_write` dedup fix ("Resolution #2"), which does +not change any number in it — see that section for why. ```text === PackFS vs. host filesystem: benchmark === environment: - raw fs test root: /tmp/packfs_bench_raw_fbOZ1b - dir backend root: /tmp/packfs_bench_dir_7tEOsC + raw fs test root: /tmp/packfs_bench_raw_PnMAT7 + dir backend root: /tmp/packfs_bench_dir_U8Vgxi small files (N): 20000, 128 bytes each directories (N): 4000 concurrency: 8 threads x 4000 ops (create+read+unlink) + mounts (N): 2000 large-file sizes: 1MB 16MB 64MB NOTE: see the file header for what "raw fs" vs "raw+fsync" vs "dir" actually measure — they are not interchangeable. == 1. create N small files == - create N small files mem 0.0372s 537103 ops/s 65.6 MB/s - create N small files raw fs 1.0098s 19805 ops/s 2.4 MB/s - create N small files raw+fsync 116.0980s 172 ops/s 0.0 MB/s + create N small files mem 0.0400s 499809 ops/s 61.0 MB/s + create N small files raw fs 0.9943s 20114 ops/s 2.5 MB/s + create N small files raw+fsync 116.2147s 172 ops/s 0.0 MB/s == 2. read N small files == - read N small files mem 0.0085s 2364410 ops/s 288.6 MB/s - read N small files raw fs 0.1594s 125459 ops/s 15.3 MB/s + read N small files mem 0.0099s 2011901 ops/s 245.6 MB/s + read N small files raw fs 0.1606s 124543 ops/s 15.2 MB/s == 3. stat N files == - stat N files mem 0.0073s 2723001 ops/s - stat N files raw fs 0.0697s 287063 ops/s + stat N files mem 0.0077s 2598241 ops/s + stat N files raw fs 0.0699s 286222 ops/s == 4. readdir == - readdir (N entries) mem 0.0042s 4816418 ops/s - readdir (N entries) raw fs 0.0070s 2840890 ops/s + readdir (N entries) mem 0.0041s 4845826 ops/s + readdir (N entries) raw fs 0.0069s 2894815 ops/s == 1b. create N small files (dir backend vs. what it wraps) == - create N small files dir 1.1531s 17345 ops/s 2.1 MB/s + create N small files dir 1.1418s 17517 ops/s 2.1 MB/s == 2b. read N small files (dir backend) == - read N small files dir 0.1601s 124933 ops/s 15.3 MB/s + read N small files dir 0.1530s 130705 ops/s 16.0 MB/s == 3b. stat N files (dir backend) == - stat N files dir 0.1509s 132517 ops/s + stat N files dir 0.1379s 145056 ops/s == 4b. readdir (dir backend) == - readdir (N entries) dir 0.0039s 5128581 ops/s + readdir (N entries) dir 0.0037s 5465162 ops/s == 5. unlink N files == - unlink N files mem 0.0320s 625437 ops/s - unlink N files dir 0.5146s 38864 ops/s - unlink N files raw fs 0.4923s 40628 ops/s + unlink N files mem 0.0179s 1117742 ops/s + unlink N files dir 0.4911s 40724 ops/s + unlink N files raw fs 0.4746s 42141 ops/s == 6. mkdir/rmdir N directories == - mkdir N dirs mem 0.0052s 771085 ops/s - rmdir N dirs mem 0.0045s 898169 ops/s - mkdir N dirs dir 0.1774s 22551 ops/s - rmdir N dirs dir 0.1118s 35778 ops/s - mkdir N dirs raw fs 0.1367s 29251 ops/s - rmdir N dirs raw fs 0.0901s 44388 ops/s + mkdir N dirs mem 0.0056s 719696 ops/s + rmdir N dirs mem 0.0040s 993724 ops/s + mkdir N dirs dir 0.1710s 23386 ops/s + rmdir N dirs dir 0.1257s 31827 ops/s + mkdir N dirs raw fs 0.1374s 29122 ops/s + rmdir N dirs raw fs 0.0951s 42043 ops/s == 7. large sequential write/read == - write 1MB mem 0.0003s 2937 ops/s 2937.3 MB/s - read 1MB mem 0.0000s 49752 ops/s 49751.7 MB/s - write 16MB mem 0.0108s 93 ops/s 1483.0 MB/s - read 16MB mem 0.0010s 1050 ops/s 16796.7 MB/s - write 64MB mem 0.0589s 17 ops/s 1086.8 MB/s - read 64MB mem 0.0033s 304 ops/s 19475.8 MB/s - write 1MB raw fs 0.0004s 2463 ops/s 2462.6 MB/s - read 1MB raw fs 0.0001s 15738 ops/s 15738.2 MB/s - write 16MB raw fs 0.0046s 217 ops/s 3477.6 MB/s - read 16MB raw fs 0.0011s 945 ops/s 15121.6 MB/s - write 64MB raw fs 0.0183s 55 ops/s 3495.7 MB/s - read 64MB raw fs 0.0050s 198 ops/s 12702.5 MB/s - write 1MB raw+fsync 0.0235s 43 ops/s 42.5 MB/s - read 1MB raw+fsync 0.0001s 9816 ops/s 9816.4 MB/s - write 16MB raw+fsync 0.0228s 44 ops/s 701.5 MB/s - read 16MB raw+fsync 0.0012s 832 ops/s 13316.1 MB/s - write 64MB raw+fsync 0.0825s 12 ops/s 775.9 MB/s - read 64MB raw+fsync 0.0057s 176 ops/s 11254.5 MB/s + write 1MB mem 0.0001s 7058 ops/s 7058.1 MB/s + read 1MB mem 0.0000s 50051 ops/s 50050.9 MB/s + write 16MB mem 0.0110s 91 ops/s 1458.2 MB/s + read 16MB mem 0.0009s 1058 ops/s 16920.1 MB/s + write 64MB mem 0.0586s 17 ops/s 1091.3 MB/s + read 64MB mem 0.0032s 313 ops/s 20056.5 MB/s + write 1MB raw fs 0.0004s 2759 ops/s 2759.4 MB/s + read 1MB raw fs 0.0001s 16812 ops/s 16812.4 MB/s + write 16MB raw fs 0.0044s 229 ops/s 3656.4 MB/s + read 16MB raw fs 0.0010s 989 ops/s 15827.9 MB/s + write 64MB raw fs 0.0186s 54 ops/s 3449.7 MB/s + read 64MB raw fs 0.0047s 213 ops/s 13633.1 MB/s + write 1MB raw+fsync 0.0180s 56 ops/s 55.7 MB/s + read 1MB raw+fsync 0.0001s 9059 ops/s 9058.8 MB/s + write 16MB raw+fsync 0.0231s 43 ops/s 691.7 MB/s + read 16MB raw+fsync 0.0012s 855 ops/s 13672.2 MB/s + write 64MB raw+fsync 0.0803s 12 ops/s 797.2 MB/s + read 64MB raw+fsync 0.0048s 209 ops/s 13393.5 MB/s == 8. random-access read: mmap'd pack vs. raw fs == - compact N entries to pack pack 0.0245s 814864 ops/s 99.5 MB/s - random-read N entries pack 0.0082s 2445144 ops/s 298.5 MB/s - random-read N entries raw fs 0.1657s 120709 ops/s 14.7 MB/s + compact N entries to pack pack 0.0267s 747839 ops/s 91.3 MB/s + random-read N entries pack 0.0082s 2435930 ops/s 297.4 MB/s + random-read N entries raw fs 0.1629s 122749 ops/s 15.0 MB/s == 9. concurrency (create+read+unlink) == - concurrent create+read+unlink mem 0.2807s 341972 ops/s - concurrent create+read+unlink raw fs 6.2980s 15243 ops/s + concurrent create+read+unlink mem 0.2750s 349127 ops/s + concurrent create+read+unlink raw fs 5.6498s 16992 ops/s -=== summary (45 measurements) === +== 10. mount table scaling (the mount table uses the same full-snapshot-copy pattern the file index used to) == + mount 500 backends vfs 0.0054s 92833 ops/s + resolve, 500 mounts vfs 0.0020s 246064 ops/s + unmount 500 backends vfs 0.0046s 109207 ops/s + mount 2000 backends vfs 0.0831s 24081 ops/s + resolve, 2000 mounts vfs 0.0296s 67458 ops/s + unmount 2000 backends vfs 0.0758s 26397 ops/s + mount 8000 backends vfs 1.4739s 5428 ops/s + resolve, 8000 mounts vfs 0.4938s 16200 ops/s + unmount 8000 backends vfs 1.5172s 5273 ops/s + +=== summary (54 measurements) === category backend seconds throughput MB/s - create N small files mem 0.0372s 537103 ops/s 65.6 MB/s - create N small files raw fs 1.0098s 19805 ops/s 2.4 MB/s - create N small files raw+fsync 116.0980s 172 ops/s 0.0 MB/s - read N small files mem 0.0085s 2364410 ops/s 288.6 MB/s - read N small files raw fs 0.1594s 125459 ops/s 15.3 MB/s - stat N files mem 0.0073s 2723001 ops/s - stat N files raw fs 0.0697s 287063 ops/s - readdir (N entries) mem 0.0042s 4816418 ops/s - readdir (N entries) raw fs 0.0070s 2840890 ops/s - create N small files dir 1.1531s 17345 ops/s 2.1 MB/s - read N small files dir 0.1601s 124933 ops/s 15.3 MB/s - stat N files dir 0.1509s 132517 ops/s - readdir (N entries) dir 0.0039s 5128581 ops/s - unlink N files mem 0.0320s 625437 ops/s - unlink N files dir 0.5146s 38864 ops/s - unlink N files raw fs 0.4923s 40628 ops/s - mkdir N dirs mem 0.0052s 771085 ops/s - rmdir N dirs mem 0.0045s 898169 ops/s - mkdir N dirs dir 0.1774s 22551 ops/s - rmdir N dirs dir 0.1118s 35778 ops/s - mkdir N dirs raw fs 0.1367s 29251 ops/s - rmdir N dirs raw fs 0.0901s 44388 ops/s - write 1MB mem 0.0003s 2937 ops/s 2937.3 MB/s - read 1MB mem 0.0000s 49752 ops/s 49751.7 MB/s - write 16MB mem 0.0108s 93 ops/s 1483.0 MB/s - read 16MB mem 0.0010s 1050 ops/s 16796.7 MB/s - write 64MB mem 0.0589s 17 ops/s 1086.8 MB/s - read 64MB mem 0.0033s 304 ops/s 19475.8 MB/s - write 1MB raw fs 0.0004s 2463 ops/s 2462.6 MB/s - read 1MB raw fs 0.0001s 15738 ops/s 15738.2 MB/s - write 16MB raw fs 0.0046s 217 ops/s 3477.6 MB/s - read 16MB raw fs 0.0011s 945 ops/s 15121.6 MB/s - write 64MB raw fs 0.0183s 55 ops/s 3495.7 MB/s - read 64MB raw fs 0.0050s 198 ops/s 12702.5 MB/s - write 1MB raw+fsync 0.0235s 43 ops/s 42.5 MB/s - read 1MB raw+fsync 0.0001s 9816 ops/s 9816.4 MB/s - write 16MB raw+fsync 0.0228s 44 ops/s 701.5 MB/s - read 16MB raw+fsync 0.0012s 832 ops/s 13316.1 MB/s - write 64MB raw+fsync 0.0825s 12 ops/s 775.9 MB/s - read 64MB raw+fsync 0.0057s 176 ops/s 11254.5 MB/s - compact N entries to pack pack 0.0245s 814864 ops/s 99.5 MB/s - random-read N entries pack 0.0082s 2445144 ops/s 298.5 MB/s - random-read N entries raw fs 0.1657s 120709 ops/s 14.7 MB/s - concurrent create+read+unlink mem 0.2807s 341972 ops/s - concurrent create+read+unlink raw fs 6.2980s 15243 ops/s + create N small files mem 0.0400s 499809 ops/s 61.0 MB/s + create N small files raw fs 0.9943s 20114 ops/s 2.5 MB/s + create N small files raw+fsync 116.2147s 172 ops/s 0.0 MB/s + read N small files mem 0.0099s 2011901 ops/s 245.6 MB/s + read N small files raw fs 0.1606s 124543 ops/s 15.2 MB/s + stat N files mem 0.0077s 2598241 ops/s + stat N files raw fs 0.0699s 286222 ops/s + readdir (N entries) mem 0.0041s 4845826 ops/s + readdir (N entries) raw fs 0.0069s 2894815 ops/s + create N small files dir 1.1418s 17517 ops/s 2.1 MB/s + read N small files dir 0.1530s 130705 ops/s 16.0 MB/s + stat N files dir 0.1379s 145056 ops/s + readdir (N entries) dir 0.0037s 5465162 ops/s + unlink N files mem 0.0179s 1117742 ops/s + unlink N files dir 0.4911s 40724 ops/s + unlink N files raw fs 0.4746s 42141 ops/s + mkdir N dirs mem 0.0056s 719696 ops/s + rmdir N dirs mem 0.0040s 993724 ops/s + mkdir N dirs dir 0.1710s 23386 ops/s + rmdir N dirs dir 0.1257s 31827 ops/s + mkdir N dirs raw fs 0.1374s 29122 ops/s + rmdir N dirs raw fs 0.0951s 42043 ops/s + write 1MB mem 0.0001s 7058 ops/s 7058.1 MB/s + read 1MB mem 0.0000s 50051 ops/s 50050.9 MB/s + write 16MB mem 0.0110s 91 ops/s 1458.2 MB/s + read 16MB mem 0.0009s 1058 ops/s 16920.1 MB/s + write 64MB mem 0.0586s 17 ops/s 1091.3 MB/s + read 64MB mem 0.0032s 313 ops/s 20056.5 MB/s + write 1MB raw fs 0.0004s 2759 ops/s 2759.4 MB/s + read 1MB raw fs 0.0001s 16812 ops/s 16812.4 MB/s + write 16MB raw fs 0.0044s 229 ops/s 3656.4 MB/s + read 16MB raw fs 0.0010s 989 ops/s 15827.9 MB/s + write 64MB raw fs 0.0186s 54 ops/s 3449.7 MB/s + read 64MB raw fs 0.0047s 213 ops/s 13633.1 MB/s + write 1MB raw+fsync 0.0180s 56 ops/s 55.7 MB/s + read 1MB raw+fsync 0.0001s 9059 ops/s 9058.8 MB/s + write 16MB raw+fsync 0.0231s 43 ops/s 691.7 MB/s + read 16MB raw+fsync 0.0012s 855 ops/s 13672.2 MB/s + write 64MB raw+fsync 0.0803s 12 ops/s 797.2 MB/s + read 64MB raw+fsync 0.0048s 209 ops/s 13393.5 MB/s + compact N entries to pack pack 0.0267s 747839 ops/s 91.3 MB/s + random-read N entries pack 0.0082s 2435930 ops/s 297.4 MB/s + random-read N entries raw fs 0.1629s 122749 ops/s 15.0 MB/s + concurrent create+read+unlink mem 0.2750s 349127 ops/s + concurrent create+read+unlink raw fs 5.6498s 16992 ops/s + mount 500 backends vfs 0.0054s 92833 ops/s + resolve, 500 mounts vfs 0.0020s 246064 ops/s + unmount 500 backends vfs 0.0046s 109207 ops/s + mount 2000 backends vfs 0.0831s 24081 ops/s + resolve, 2000 mounts vfs 0.0296s 67458 ops/s + unmount 2000 backends vfs 0.0758s 26397 ops/s + mount 8000 backends vfs 1.4739s 5428 ops/s + resolve, 8000 mounts vfs 0.4938s 16200 ops/s + unmount 8000 backends vfs 1.5172s 5273 ops/s -real 4m3.827s -user 0m1.039s -sys 0m19.841s +real 5m23.896s +user 0m4.732s +sys 0m19.047s ``` ## Reproducing diff --git a/CLAUDE.md b/CLAUDE.md index 4920d84..2640eac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,6 +19,10 @@ A single test: `make build/test_mem && ./build/test_mem` (substitute any `test_* Sanitizer builds are not wired into `make test` (they need per-file compilation with sanitizer flags plus `-D_GNU_SOURCE -Iinclude -Isrc`); see `.github/workflows/ci.yml` for the exact invocation, which CI runs on every push. **Any change to `upper.c`, `overlay.c`, or `vfs.c` must be verified under `-fsanitize=address,undefined` and `-fsanitize=thread` before being considered done** — this is not a formality: exactly this process caught a real use-after-free in the snapshot-reclamation logic during initial development (see the `reclaim_gate` note below), which the plain build and even repeated plain test runs never surfaced. +**If your environment cannot run ThreadSanitizer at all, say so rather than skipping it silently.** TSan needs `personality(ADDR_NO_RANDOMIZE)` to disable ASLR for itself; some sandboxes block that syscall outright, in which case *every* TSan build fails identically (`FATAL: ThreadSanitizer: unexpected memory mapping`), including a trivial unrelated pthread program — that is the confirming test, not a PackFS-specific symptom. In that situation, ASan/UBSan are still run and still matter (they are what actually caught the `reclaim_gate` bug above), but they do not perform TSan's happens-before race analysis and are not a substitute for it; a change is TSan-verified only once it has passed on a machine or CI run that can actually execute it, not merely because ASan/UBSan passed. + +**A related but separate flake affects ASan/UBSan themselves in that kind of sandbox, not just TSan:** a sanitizer-built test binary can non-deterministically fail to start with `AddressSanitizer:DEADLYSIGNAL`, occasionally as an unbounded repeating loop rather than a single line — a sandbox startup race, confirmed by it hitting different, unrelated binaries across repeated runs, each of which then passes cleanly on retry. See `CONTRIBUTING.md`'s workflow section for the full description and the required mitigation (`timeout`-wrap sanitizer runs in such an environment; treat `DEADLYSIGNAL` alone, without an actual `ERROR: AddressSanitizer` or `runtime error:` string, as inconclusive and re-run rather than as a finding). + ## Code architecture - `include/packfs.h` — the entire public API. @@ -26,7 +30,7 @@ Sanitizer builds are not wired into `make test` (they need per-file compilation - `src/vfs.c` — the `Vfs` mount table itself (`MountSnapshot`, refcounted, atomically swapped) and the public API's dispatch-by-longest-prefix-match. - `src/upper.c` — the writable layer shared by `mem`, `dir`, and the overlay's upper side: `UpperSnapshot`/`UpperEntry`/`MutCell`, the persistent-treap index (`TreapNode` and `treap_*`/`node_*` — see "Known performance characteristics" below for why it's a treap and not a flat array), the structural-write functions (`upper_create`/`upper_mkdir`/`upper_remove`/`upper_rename`/`upper_copy_up`), the content-write fast path (`upper_cell_read`/`upper_cell_write`), the range-query API (`upper_visit_range`, used by `readdir` in both this file and `overlay.c`), and the standalone `mem`/`dir` `Backend` (`backend_mem_new`/`backend_dir_new`). - `src/overlay.c` — composes a `Pack` (lower, read-only) with an `UpperStore` (upper): copy-up, whiteouts, the merged view (`overlay_stat`/`overlay_readdir`), the append journal, and `vfs_sync`'s compaction. -- `src/pack.c` — the on-disk pack format: load-time integrity validation, binary-search lookup/range-query, the compaction writer (atomic rename, exact-duplicate elimination), and the standalone read-only `pack` `Backend` (`backend_pack_new`) for mounting a pack with no writable upper layer at all. +- `src/pack.c` — the on-disk pack format: load-time integrity validation, binary-search lookup/range-query, the compaction writer (atomic rename, exact-duplicate elimination via a `DedupSlot` open-addressing hash table over `(hash, size)` plus a `memcmp` verification before ever reusing a `data_off` — see "Known performance characteristics" below for why it isn't a linear scan), and the standalone read-only `pack` `Backend` (`backend_pack_new`) for mounting a pack with no writable upper layer at all. - `src/containment.c` — `dir`-mount path containment (`openat2`/`O_NOFOLLOW` fallback) and opt-in Landlock hardening. - `src/path.c` — virtual-namespace `.`/`..` canonicalization. - `src/hash.c` — FNV-1a64, used for pack checksums and compaction dedup. @@ -88,18 +92,20 @@ These are hard requirements stated in the spec, not stylistic suggestions — an - **RESOLVED: bulk sequential `create`/`unlink`/`mkdir` on `mem`/`dir` was O(n²) in the number of entries — confirmed empirically (`BENCH.md`), then fixed, not just worked around.** The index (`UpperSnapshot`) was a flat sorted array; every structural write copied it in full before publishing the next snapshot (Section 5.3's single-writer model), and at 20,000 sequential creates this measured ~9–22x slower than the raw host filesystem. `concept.md` Section 5.3 names the exact trigger for reconsidering that design — "a persistent (structurally shared) tree structure is not required until this assumption is empirically violated" — and that trigger was hit. The index is now a **persistent treap** (`src/upper.c`, `struct TreapNode` and the functions around it — see the citations in that comment, Seidel & Aragon 1996 and Liljenzin arXiv:1301.3388, for why a treap and not a persistent AVL/red-black/weight-balanced tree): a structural write now copies only the O(log n) nodes on the path to the change. Post-fix, the same benchmark shows `mem` create/unlink/mkdir 65–330x faster than before, and — the more important comparison — 15–27x *faster* than raw fs where it used to be 9–22x *slower*; the concurrent mixed workload went from 6x slower than raw fs to 22x faster. `BENCH.md`'s "Resolution" section has the full before/after table and the complexity-class re-confirmation (the same `mkdir`-at-N=4,000-vs-`create`-at-N=20,000 methodology that found O(n²) now shows a ratio consistent with O(n log n), not O(n²)). A new test, `tests/test_index_stress.c`, cross-checks the treap's correctness under thousands of randomized (non-sequential) creates/deletes/renames against an independent reference model — read it, not just the benchmark, before touching `snapshot_upsert`/`snapshot_remove`/the treap functions again. - **`dir`-backend `stat` costs roughly 2x a raw `stat()` call**, because `pfs_dir_statat` (containment, Section 6.2) resolves and opens the path, then `fstat`s the fd, where raw `stat()` is one syscall. Expected, and the same containment cost `open` already pays — recorded so it isn't mistaken for a regression if someone benchmarks it again later. - **`mem`-backed large sequential writes lose to a plain unsynced host `write()` above roughly 16 MB**, because Section 5.7's buffer-growth discipline (allocate new, copy everything, publish, retire old) re-copies previously-written bytes on every capacity doubling, where the host page cache only ever appends new pages. This is the measured cost of the safety property Section 5.7 requires (no reader ever sees a freed buffer), not an accidental inefficiency. +- **RESOLVED: `pack_write`'s compaction-time exact-duplicate elimination (Section 9.2) was O(n²) in entry count, plus a latent correctness bug, both fixed together.** The dedup check was a linear scan of every previously-seen `(hash, size)` pair per entry, comparing only the hash and size — never the actual bytes — before trusting a match, so two different-content entries that happened to collide on `(hash, size)` under FNV-1a64 (explicitly not collision-resistant, `src/hash.c`) would have been silently merged, corrupting one of them; this was never observed in practice but was true of the code as written. `bench/bench.c`'s own compaction benchmark never surfaced either problem because its test files are byte-identical, which made every scan match on the first comparison (the scan's best case, not its worst case). Measuring with unique content instead (`pack_write` called directly, bypassing the journal/fsync path) showed 80,000 entries taking 1.74s, with a 20,000→80,000 (4x N) step showing 15.6x — matching O(n²)'s 16x prediction. Fixed with an open-addressing hash table (`DedupSlot`, load factor 1/2, linear probing) plus a `memcmp` verification before ever reusing a `data_off`, closing both the complexity issue and the correctness gap at once; post-fix, the same 80,000-entry case takes 0.044s (39.6x faster) and the 20,000→80,000 ratio drops to 3.35x, consistent with O(n). `BENCH.md`'s "Resolution #2" has the full before/after numbers. Correctness is covered permanently by `tests/test_pack_overlay.c` (asserts duplicate-content entries share one `data_off` and distinct-content entries do not); the performance fix is covered permanently by `tests/test_pack_write_perf.c` (a regression tripwire against 10,000 unique entries, run as part of `make test`). +- **NOT FIXED, by deliberate decision: `vfs.c`'s mount table (`MountSnapshot`) is O(n²) in mount count, using the same full-array-copy-per-write pattern the file index used to.** Confirmed via `bench/bench.c` category 10 (500/2,000/8,000 mounts, both 4x-N steps showing 15–20x, matching O(n²)'s 16x prediction, for mount, resolve, and unmount alike). Unlike the file index, this is not being converted to a persistent tree: mount points are created by calls written into a program's own source code, not driven by workload or user data, so realistic mount counts do not reach the scale that made the file index's O(n²) an actual problem — `concept.md` Section 5.3's own stated trigger for needing a persistent structure ("not required until this assumption is empirically violated") has not been violated here the way it was for the file index. `BENCH.md`'s "Finding: mount table scaling" has the full numbers and reasoning; revisit only if a real use case needs thousands of runtime mount points (e.g. one mount per tenant). ## Self-evaluation -**Methodology.** This file was checked against the repository's actual state (`make test` passing, including a new `test_index_stress`; `include/`, `src/`, `tests/`, `bench/` present and matching the description below; confirmed by directory listing and a live build), against `concept.md` as frozen (for factual consistency of the constraints and architecture summarized above), and against the documentation standard stated in this file. This revision followed a rewrite of `upper.c`'s index structure (flat array to persistent treap) undertaken specifically to close the O(n²) finding this file's "Known performance characteristics" section previously reported as an open, unresolved limitation — the rewrite was verified under `make test`, `test_index_stress`'s randomized correctness check, ASan/UBSan (per the sanitizer rule below), and a full before/after `make bench` run, not merely believed correct because the code compiled. +**Methodology.** This file was checked against the repository's actual state (`make test` passing, including two new tests, `test_pack_write_perf` and the dedup-verification block added to `test_pack_overlay`; `include/`, `src/`, `tests/`, `bench/` present and matching the description below; confirmed by directory listing and a live build), against `concept.md` as frozen (for factual consistency of the constraints and architecture summarized above), and against the documentation standard stated in this file. This revision followed a deliberate audit for other instances of the file index's O(n²) shape elsewhere in the codebase — prompted by a request to leave no caveats undocumented — which found two more real issues, not zero: `pack_write`'s compaction dedup (O(n²), plus a latent hash-collision correctness bug, both now fixed) and the mount table (O(n²), confirmed and deliberately left unfixed, with the reasoning recorded). The audit also found and fixed a CI gap the new tests exposed — `.github/workflows/ci.yml`'s sanitizer-build steps never passed `-D_GNU_SOURCE` when compiling test files, which was harmless while no test included `internal.h` and became a real link failure once two did (`internal.h` needs it for `pthread_rwlock_t`) — and documented a sandbox flake (ASan/UBSan's own `DEADLYSIGNAL` startup race, distinct from and in addition to the already-documented TSan limitation) observed directly during this audit's own sanitizer runs, in `CONTRIBUTING.md` and cross-referenced here. Every finding below was confirmed empirically (direct `pack_write` timing, `bench/bench.c`'s new mount-scaling category, repeated sanitizer runs with `timeout`), not asserted. | Category | Grade | Notes | |---|---|---| -| Factual accuracy | A | Build/test commands and the code map were verified against a live `make test` run and the actual file layout, not written from memory of intent; the performance-characteristics section was updated from a live `make bench` re-run, not assumed fixed because the rewrite was intended to fix it. | -| Adherence to the documentation standard | A | Direct, constraint-labeled prose; no manufactured sections; scaled appropriately to an instructions file rather than imitating `concept.md`'s full academic structure. | -| Completeness for its purpose | A | Covers repository status, build/test/lint commands, a per-file code map, every hard constraint from `concept.md` relevant to implementation work, the permanent zip/tar exclusion, the cross-cutting `reclaim_gate` pattern (including why `TreapNode` is a deliberate exception to it, not a hole in it), and the now-resolved O(n²) bulk-write characteristic with a pointer to its fix and verification. | -| Avoidance of generic or invented content | A | No fabricated "Common Development Tasks" or "Tips" sections; the sanitizer-testing instruction is stated as a requirement precisely because skipping it once already let a real bug through, not as generic advice. | +| Factual accuracy | A | Build/test commands and the code map were verified against a live `make test` run and the actual file layout, not written from memory of intent; both new performance-characteristics entries were confirmed by direct measurement (`pack_write` timing, `bench/bench.c` category 10), not assumed from reading the code. | +| Adherence to the documentation standard | A | Direct, constraint-labeled prose; the mount-table entry is explicitly labeled "not fixed, by deliberate decision" rather than left ambiguous with the "resolved" entries beside it — constraints and decisions are labeled by strength here exactly as the standard above requires. | +| Completeness for its purpose | A | Covers repository status, build/test/lint commands, a per-file code map, every hard constraint from `concept.md` relevant to implementation work, the permanent zip/tar exclusion, the cross-cutting `reclaim_gate` pattern, both sandbox flakes (TSan's outright failure and ASan/UBSan's intermittent `DEADLYSIGNAL`), and now three performance findings (one historical-and-resolved, one newly-resolved, one confirmed-and-deliberately-not-resolved) instead of one. | +| Avoidance of generic or invented content | A | No fabricated sections; the new mount-table entry could have been written as a vague "may not scale to many mounts" hedge but instead states the measured ratios and the specific reason it isn't being fixed, which is the standard the other entries in this section already set. | -**Overall grade: A.** The file states only what is verifiably true of the repository, the spec, and the implementation; labels constraints by strength; documents the architectural pattern (snapshot reclamation via `reclaim_gate`) that spans multiple files; and — unlike the previous revision, which correctly recorded a real limitation but left it as a known gap — now records that limitation's actual, measured resolution, with the same empirical rigor (a complexity-class re-confirmation, not just "it got faster") that found it in the first place. +**Overall grade: A.** The file states only what is verifiably true of the repository, the spec, and the implementation; labels constraints and decisions by strength; documents the architectural pattern (snapshot reclamation via `reclaim_gate`) that spans multiple files; and, per this revision's audit, now also documents two further real findings (one fixed, one consciously not) that a less thorough pass would have missed, plus a real CI gap and a real environment flake, both found by direct observation during the same audit rather than left for a future session to rediscover. **A real gap this audit found and fixed, not just documented:** `backend_pack_new` was declared in `include/packfs.h` and referenced in this file's own architecture map, but was never implemented in `src/pack.c` — any program calling it would fail at link time. It is now implemented (a standalone, read-only `pack` `Backend`; every mutating operation returns `VFS_ERR_PERM`), covered by a new case in `tests/test_pack_overlay.c`, and verified under `-fsanitize=undefined` per this file's own testing rule. This is recorded here because it is exactly the kind of error "document literally all" is supposed to catch: a documentation pass that describes an unimplemented function accurately is still wrong in a way that matters. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 354b652..61aedc8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,6 +28,48 @@ Any change to the concurrency-sensitive files (`upper.c`, `overlay.c`, a data race there is exactly the class of bug Section 5 of `concept.md` exists to prevent, and the plain build will not surface it. +**Known environment limitation, stated plainly rather than glossed over:** +ThreadSanitizer cannot run at all in some sandboxed/containerized +development environments — including the one this project's own commit +history was largely developed in — because TSan requires disabling ASLR +for itself via `personality(ADDR_NO_RANDOMIZE)`, and some sandboxes block +that syscall outright (confirmed here: `personality()` returns `EPERM`, +and even a trivial unrelated pthread program fails identically with +`FATAL: ThreadSanitizer: unexpected memory mapping`, not just PackFS code). +If your environment can't run TSan, say so rather than silently skipping +it or claiming verification that didn't happen — ASan/UBSan still catch +real bugs (they found and fixed a genuine heap-use-after-free during this +project's development, see the `reclaim_gate` note in `internal.h`) but do +not do TSan's happens-before race analysis, so they are not a substitute +for it. CI (`.github/workflows/ci.yml`) runs on a normal, unrestricted +GitHub Actions VM where TSan is expected to work; treat a change as +TSan-verified only once it has actually passed there or on a local machine +that can run it, not merely because ASan/UBSan passed. + +**A second, separate environment quirk, also observed directly rather than +assumed:** in the same kind of sandboxed environment, an ASan/UBSan-built +test binary occasionally (non-deterministically, roughly 1 run in 5–10 in +this project's own experience) fails to start at all, printing +`AddressSanitizer:DEADLYSIGNAL` — and, in its worse form, printing that same +line in an unbounded loop rather than printing it once and exiting, which +can run for minutes and consume unbounded CPU/memory if left unattended. +This has been observed hitting different, unrelated test binaries from run +to run (in one session: `test_dir` and `test_mem`, in another: nothing at +all, in another: `test_pack_overlay`), which — together with the fact that +every one of those binaries passes cleanly on a repeat run — indicates a +sandbox startup race (plausibly in the same family as the ASLR/mapping +restrictions behind the TSan limitation above), not a bug in the binary +being tested. **Always wrap sanitizer-build test runs in `timeout` in such +an environment** (e.g. `timeout 30 ./build/san/test_name`) so a stuck run +fails loudly and bounded instead of hanging; treat a `DEADLYSIGNAL` exit as +inconclusive, re-run, and only treat the run as a real finding if the output +contains an actual `ERROR: AddressSanitizer` or `runtime error:` string — +`DEADLYSIGNAL` alone, with neither of those strings present, is this flake, +not a memory-safety bug in PackFS. As with the TSan limitation, the correct +response is to say this plainly and re-run until a clean pass is obtained +(or hand off to CI, which runs on an unrestricted VM where this has not been +observed), not to suppress or silently ignore a `DEADLYSIGNAL` exit. + A change to the index structure in `upper.c` (the `TreapNode`/`treap_*`/ `node_*` functions, `snapshot_upsert`, `snapshot_remove`, or the `UpperSnapshot`/`UpperEntry` representation) should be run through diff --git a/README.md b/README.md index 0521d3d..be1df6c 100644 --- a/README.md +++ b/README.md @@ -68,18 +68,21 @@ process's lifetime. `bench/bench.c` (`make bench`) measures PackFS against the host filesystem across metadata operations (create/read/stat/readdir/unlink/mkdir), large -sequential I/O, random-access pack reads, and concurrent mixed workloads. -[`BENCH.md`](BENCH.md) has the full results and honest analysis of both the -wins and the losses, including the story of a real bug this benchmark -found: an early run turned up a quantitatively-confirmed O(n²) cost in bulk -sequential file creation, which `concept.md` Section 5.3 had explicitly -anticipated and named the trigger condition for closing. That's since been -fixed (`src/upper.c`'s index is a persistent treap now, not a flat array), -re-benchmarked, and re-confirmed as an actual complexity-class change, not -just a faster constant factor — see `BENCH.md`'s "Resolution" section for -the before/after numbers. Read the whole file before quoting a number from -it: what `raw fs` vs `raw+fsync` vs `dir` each actually measure is not -interchangeable, and it explains why. +sequential I/O, random-access pack reads, mount-table scaling, and +concurrent mixed workloads. [`BENCH.md`](BENCH.md) has the full results and +honest analysis of both the wins and the losses, including the story of +three real O(n²) findings this project's own benchmarking turned up — not +just the wins. Two are fixed: bulk sequential file creation (`src/upper.c`'s +index is a persistent treap now, not a flat array — see "Resolution") and +`pack_write`'s compaction-time duplicate-content elimination, which also had +a latent correctness bug now closed alongside it (see "Resolution #2"). One +is confirmed and *deliberately not* fixed: the mount table scales O(n²) in +mount count, the same way the file index used to, but mount counts are +bounded by a program's own source code rather than workload-driven, so it +isn't worth the added complexity — see "Finding: mount table scaling" for +the reasoning and the numbers behind that call. Read the whole file before +quoting a number from it: what `raw fs` vs `raw+fsync` vs `dir` each +actually measure is not interchangeable, and it explains why. `openat2`/Landlock support (Section 6) is detected automatically at compile time via ``; on kernels or platforms without them, `dir` diff --git a/bench/bench.c b/bench/bench.c index 3b386f4..cdc147b 100644 --- a/bench/bench.c +++ b/bench/bench.c @@ -43,6 +43,15 @@ #define N_SMALL 20000 /* small files for the metadata-heavy suite */ #define SMALL_SIZE 128 #define N_DIRS 4000 +/* vfs_mount/vfs_unmount use the identical full-snapshot-copy pattern the + * file index used to (Section 5.3's mount-table-is-part-of-the-snapshot + * unification) — this section exists to check, empirically rather than + * by assumption, whether that ever matters at a realistic mount count. + * 2,000 is deliberately far beyond any real program's mount count (a + * mount is something a program's own source code sets up once per + * distinct storage location — assets/config/tmp/per-plugin — not + * something a workload creates at file-count scale). */ +#define N_MOUNTS 2000 #define N_CONC_THREADS 8 #define OPS_PER_THREAD 4000 #define LARGE_CHUNK (64 * 1024) @@ -428,6 +437,57 @@ static void bench_concurrency(Vfs *v, const char *rawroot) { record("concurrent create+read+unlink", "raw fs", now_sec() - t0, (double)N_CONC_THREADS * OPS_PER_THREAD * 3, 0); } +/* ---- category 10: mount table scaling ---- */ + +/* Run at several N (called from main at N_MOUNTS/4, N_MOUNTS, N_MOUNTS*4) + * so the mount table's complexity class can be checked the same way + * Section "Resolution" in BENCH.md checked the file index's: if time(N) + * grows roughly as N^2 rather than N or N log N across a 4x-N step, that + * confirms (not just asserts) that vfs_mount/vfs_unmount's full-array + * copy per structural write (mirroring the file index's old design) is + * quadratic here too — expected, and, per BENCH.md, not worth fixing at + * any mount count a real program would ever reach. */ +static void bench_mount_scaling(int n) { + char label[24]; + Vfs *v = vfs_new(); + Backend **backends = (Backend **)malloc(sizeof(Backend *) * (size_t)n); + char prefix[32]; + + double t0 = now_sec(); + for (int i = 0; i < n; i++) { + backends[i] = backend_mem_new(); + snprintf(prefix, sizeof(prefix), "/m%06d", i); + vfs_mount(v, prefix, backends[i]); + } + snprintf(label, sizeof(label), "mount %d backends", n); + record(label, "vfs", now_sec() - t0, n, 0); + + /* a resolve through a full mount table, to confirm lookup itself + * (not just mount/unmount) stays fast at this N */ + t0 = now_sec(); + for (int i = 0; i < n; i++) { + char path[40]; + snprintf(prefix, sizeof(prefix), "/m%06d", i); + snprintf(path, sizeof(path), "%s/x.txt", prefix); + VfsStat st; + vfs_stat(v, path, &st); /* NOENT expected; measures resolve cost, not the stat itself */ + } + snprintf(label, sizeof(label), "resolve, %d mounts", n); + record(label, "vfs", now_sec() - t0, n, 0); + + t0 = now_sec(); + for (int i = 0; i < n; i++) { + snprintf(prefix, sizeof(prefix), "/m%06d", i); + vfs_unmount(v, prefix); + } + snprintf(label, sizeof(label), "unmount %d backends", n); + record(label, "vfs", now_sec() - t0, n, 0); + + for (int i = 0; i < n; i++) backend_free(backends[i]); + free(backends); + vfs_free(v); +} + /* ---- main ---- */ int main(void) { @@ -446,6 +506,7 @@ int main(void) { printf(" small files (N): %d, %d bytes each\n", N_SMALL, SMALL_SIZE); printf(" directories (N): %d\n", N_DIRS); printf(" concurrency: %d threads x %d ops (create+read+unlink)\n", N_CONC_THREADS, OPS_PER_THREAD); + printf(" mounts (N): %d\n", N_MOUNTS); printf(" large-file sizes: "); for (size_t i = 0; i < N_LARGE_SIZES; i++) printf("%zuMB ", LARGE_SIZES[i] / (1024 * 1024)); printf("\n"); @@ -566,6 +627,11 @@ int main(void) { section("9. concurrency (create+read+unlink)"); bench_concurrency(v, rawroot); + section("10. mount table scaling (the mount table uses the same full-snapshot-copy pattern the file index used to)"); + bench_mount_scaling(N_MOUNTS / 4); + bench_mount_scaling(N_MOUNTS); + bench_mount_scaling(N_MOUNTS * 4); + vfs_unmount(v, "/"); backend_free(mem); vfs_free(v); diff --git a/src/pack.c b/src/pack.c index a998c3f..a2834e4 100644 --- a/src/pack.c +++ b/src/pack.c @@ -192,12 +192,27 @@ static int pack_build_entry_cmp(const void *a, const void *b) { return strcmp(((const PackBuildEntry *)a)->name, ((const PackBuildEntry *)b)->name); } +/* An open-addressing (linear probing) hash table over (hash, size), + * confirmed by an actual memcmp on a probe match before ever reusing a + * data_off — the hash alone is not proof of equality (Section 9.2's + * dedup is exact-duplicate elimination, and FNV-1a64 is explicitly not + * collision-resistant, hash.c), only a fast way to *narrow down* to the + * one slot worth memcmp-ing, which a linear scan of every prior slot did + * not do (see below) and this does. */ typedef struct DedupSlot { uint64_t hash; uint64_t size; uint64_t data_off; + const void *data; /* for the memcmp verification, not for storage */ + int used; } DedupSlot; +static size_t next_pow2(size_t n) { + size_t p = 1; + while (p < n) p <<= 1; + return p; +} + int pack_write(const char *path, const PackBuildEntry *in, size_t count, int *err) { PackBuildEntry *entries = NULL; if (count > 0) { @@ -207,10 +222,28 @@ int pack_write(const char *path, const PackBuildEntry *in, size_t count, int *er } qsort(entries, count, sizeof(PackBuildEntry), pack_build_entry_cmp); - /* Pass 1: compute blob region with exact-duplicate elimination - * (Section 9.2) and the strings region size. */ - DedupSlot *slots = count ? (DedupSlot *)malloc(count * sizeof(DedupSlot)) : NULL; - size_t slot_count = 0; + /* + * Pass 1: compute blob region with exact-duplicate elimination + * (Section 9.2) and the strings region size. + * + * This used to be a linear scan of every previously-seen (hash, + * size) pair per entry — O(n) per entry, O(n^2) total across n + * distinct blobs, the exact same shape of bug BENCH.md documents + * for the file index, just in the compaction writer instead of the + * live index, and without benefit of that fix (a persistent tree + * doesn't help here; deduplication needs a hash-keyed lookup, not a + * sorted one). Confirmed empirically the same way: with unique + * per-file content (the linear scan's actual worst case — the + * original benchmark's identical-content test files happened to + * make every comparison match on the first try, hiding this + * entirely), pack_write on 80,000 entries took 1.74s and a 4x-N + * step from 20,000 measured a 15.6x time increase, matching O(n^2) + * (16x predicted) far better than linear. Fixed with a real hash + * table (load factor 1/2, linear probing) instead of a scan. + */ + size_t table_size = next_pow2(count > 0 ? count * 2 : 1); + DedupSlot *table = (DedupSlot *)calloc(table_size, sizeof(DedupSlot)); + size_t mask = table_size - 1; uint64_t *data_off_for = count ? (uint64_t *)malloc(count * sizeof(uint64_t)) : NULL; uint64_t blob_cursor = align8(sizeof(PackHeader)); uint64_t strings_total = 0; @@ -218,25 +251,31 @@ int pack_write(const char *path, const PackBuildEntry *in, size_t count, int *er for (size_t i = 0; i < count; i++) { uint64_t h = pfs_fnv1a64(entries[i].data, (size_t)entries[i].size); uint64_t reuse = UINT64_MAX; - for (size_t j = 0; j < slot_count; j++) { - if (slots[j].hash == h && slots[j].size == entries[i].size) { - reuse = slots[j].data_off; + size_t idx = (size_t)(h & mask); + for (;;) { + if (!table[idx].used) break; /* empty slot: no match anywhere on this probe chain */ + if (table[idx].hash == h && table[idx].size == entries[i].size && + (entries[i].size == 0 || + memcmp(table[idx].data, entries[i].data, (size_t)entries[i].size) == 0)) { + reuse = table[idx].data_off; break; } + idx = (idx + 1) & mask; /* linear probe past a hash collision or a false hash match */ } if (reuse != UINT64_MAX) { data_off_for[i] = reuse; } else { data_off_for[i] = blob_cursor; - slots[slot_count].hash = h; - slots[slot_count].size = entries[i].size; - slots[slot_count].data_off = blob_cursor; - slot_count++; + table[idx].used = 1; + table[idx].hash = h; + table[idx].size = entries[i].size; + table[idx].data_off = blob_cursor; + table[idx].data = entries[i].data; blob_cursor += entries[i].size; } strings_total += strlen(entries[i].name) + 1; } - free(slots); + free(table); uint64_t index_offset = align8(blob_cursor); uint64_t index_bytes = (uint64_t)count * sizeof(PackIndexEntry); diff --git a/tests/test_pack_overlay.c b/tests/test_pack_overlay.c index 32e5612..b0725cd 100644 --- a/tests/test_pack_overlay.c +++ b/tests/test_pack_overlay.c @@ -10,6 +10,7 @@ #include #include "packfs.h" +#include "internal.h" /* white-box: verifies Section 9.2 dedup actually shares data_off, not just "doesn't crash" */ #include "test_harness.h" static char *tmp_pack_path(void) { @@ -51,6 +52,28 @@ int main(void) { CHECK_EQ_INT(vfs_sync(v, "/"), VFS_OK); /* compaction */ + /* Section 9.2 dedup, actually verified (not just exercised): /a.txt + * and /dup.txt have identical content and must share one data_off + * in the compacted pack; /b.txt has different content and must not + * share either's. Catches both "dedup silently stopped happening" + * (a real, previously-unasserted gap) and "dedup over-matched two + * different files onto the same bytes" (the hash-collision-without- + * memcmp-verification bug fixed alongside the O(n^2) compaction + * cost — see pack.c's DedupSlot comment). */ + { + Pack *p = NULL; int lerr = 0; + CHECK_EQ_INT(pack_load(pack_path, &p, &lerr), 0); + if (p) { + const PackIndexEntry *ea = NULL, *edup = NULL, *eb = NULL; + CHECK(pack_find(p, "/a.txt", &ea)); + CHECK(pack_find(p, "/dup.txt", &edup)); + CHECK(pack_find(p, "/b.txt", &eb)); + if (ea && edup) CHECK_EQ_INT(ea->data_off, edup->data_off); + if (ea && eb) CHECK(ea->data_off != eb->data_off); + pack_close(p); + } + } + /* read-only open straight from the freshly-compacted pack */ f = vfs_open(v, "/a.txt", VFS_O_RDONLY, &err); CHECK(f != NULL); diff --git a/tests/test_pack_write_perf.c b/tests/test_pack_write_perf.c new file mode 100644 index 0000000..1bb8c3e --- /dev/null +++ b/tests/test_pack_write_perf.c @@ -0,0 +1,105 @@ +/* + * test_pack_write_perf.c — regression guard for a real bug: pack_write's + * Section 9.2 exact-duplicate elimination used to be a linear scan of + * every previously-seen (hash, size) pair per entry — O(n) per entry, + * O(n^2) total across n distinct blobs, found and fixed alongside the + * file index's own O(n^2) (see CLAUDE.md's "Known performance + * characteristics" and BENCH.md). It is now a hash table (load factor + * 1/2, linear probing), which this asserts stays fast at a scale where + * the old code was already measurably slow (BENCH.md: 80,000 unique + * entries took 1.74s pre-fix, 0.044s post-fix). + * + * Deliberately bypasses the VFS/overlay/journal path entirely (calls + * pack_write directly via internal.h) rather than creating N files + * through an overlay: that path journals and fsyncs every write + * (Section 4.4), which is a separate, unrelated cost this test has no + * reason to pay, and which is pathologically slow on some sandboxed + * environments (see CONTRIBUTING.md's ThreadSanitizer note for another + * example of the same class of environment quirk) — conflating the two + * costs is exactly the mistake that delayed finding this bug in the + * first place, so this test deliberately does not repeat it. + */ + +#include +#include +#include +#include +#include + +#include "internal.h" +#include "test_harness.h" + +#define N 10000 + +static double now_sec(void) { + struct timespec ts; + clock_gettime(CLOCK_MONOTONIC, &ts); + return (double)ts.tv_sec + (double)ts.tv_nsec / 1e9; +} + +int main(void) { + PackBuildEntry *entries = (PackBuildEntry *)malloc(sizeof(PackBuildEntry) * N); + char **names = (char **)malloc(sizeof(char *) * N); + char **datas = (char **)malloc(sizeof(char *) * N); + + /* every entry's content is unique: this is the linear scan's actual + * worst case (every comparison fails until the empty tail), and + * exactly the case the original benchmark's identical-content test + * files accidentally never exercised. */ + for (int i = 0; i < N; i++) { + names[i] = (char *)malloc(32); + snprintf(names[i], 32, "/f%06d.dat", i); + datas[i] = (char *)malloc(48); + int len = snprintf(datas[i], 48, "unique-content-block-%06d", i); + entries[i].name = names[i]; + entries[i].data = datas[i]; + entries[i].size = (uint64_t)len; + entries[i].mode = 0; + entries[i].mtime = 0; + } + + char path[128]; + snprintf(path, sizeof(path), "/tmp/packfs_test_dedup_perf_%d.img", (int)getpid()); + unlink(path); + + int err = 0; + double t0 = now_sec(); + int rc = pack_write(path, entries, N, &err); + double elapsed = now_sec() - t0; + + CHECK_EQ_INT(rc, 0); + /* Generous bound: post-fix this takes well under 0.1s for N=10,000 + * on ordinary hardware (BENCH.md: 0.013s for N=20,000). The old + * O(n^2) code took long enough at this N to be a clear, unmissable + * fail, not a borderline one — this is a regression tripwire, not a + * tight performance assertion. */ + CHECK(elapsed < 5.0); + if (elapsed >= 5.0) { + fprintf(stderr, "pack_write(%d unique entries) took %.3fs -- the O(n^2) dedup regression may be back\n", N, elapsed); + } + + /* correctness, not just speed: load it back and confirm every entry + * is findable with its own distinct content intact. */ + Pack *p = NULL; + int lerr = 0; + CHECK_EQ_INT(pack_load(path, &p, &lerr), 0); + if (p) { + for (int i = 0; i < N; i += 997) { /* sample, not all 10,000, to keep this fast */ + const PackIndexEntry *e = NULL; + CHECK(pack_find(p, names[i], &e)); + if (e) { + CHECK_EQ_INT(e->size, strlen(datas[i])); + CHECK_EQ_INT(memcmp(pack_entry_data(p, e), datas[i], e->size), 0); + } + } + pack_close(p); + } + + unlink(path); + for (int i = 0; i < N; i++) { free(names[i]); free(datas[i]); } + free(names); + free(datas); + free(entries); + + TEST_MAIN_END(); +}