retoorandClaude Sonnet 5
64283d587b
BENCH.md's earlier finding was real: every structural write (create/
unlink/mkdir) copied the entire sorted UpperSnapshot entry array before
publishing the next snapshot, making bulk sequential creation O(n^2).
concept.md Section 5.3 names the exact condition for reconsidering this
("a persistent structurally-shared tree structure is not required
until this assumption is empirically violated") -- that condition was
measured, not hypothesized, so this closes it rather than leaving it
as a documented-but-open limitation.
The index is now a persistent treap (src/upper.c): a structural write
copies only the O(log n) nodes on the path to the change, sharing
every other node (its own refcount, cascading like MutCell's and
UpperSnapshot's) with whichever snapshot(s) it was built from. Chosen
over a persistent AVL/red-black/weight-balanced tree because deletion
in those can need O(log n) rebalancing rotations -- each a real
allocation in a persistent setting -- where a treap needs only O(1)
amortized rotations for insert and delete (Seidel & Aragon 1996), with
expected O(log n) height regardless of insertion order, including the
sorted-by-creation-order pattern that made the flat array quadratic in
the first place. Liljenzin's "Confluently Persistent Sets and Maps"
(arXiv:1301.3388) documents persistent treaps giving O(1) snapshots
for MVCC specifically, which is this exact use case. Full reasoning
and the ownership convention (functions consume one ref of their tree
arguments, return one owned ref) are in upper.c's comment above
struct TreapNode.
Blast radius kept deliberately small: snapshot_upsert/snapshot_remove
keep their exact original signatures, so upper_create/upper_mkdir/
upper_remove/upper_rename/upper_copy_up needed zero changes.
upper_lookup/upper_has_children keep their exact contracts. Only
upstd_readdir and overlay_readdir's manual array scans became calls to
a new upper_visit_range (O(log n + r) range query, replacing an O(n)
scan in both, a bonus fix beyond what was strictly necessary) since
there's no flat array left to scan.
Added tests/test_index_stress.c: thousands of randomized (not
sequential) creates/deletes/renames across nested directories,
cross-checked against an independent reference model after every
round, not just "did it not crash" -- exercises exactly the code path
the O(n^2) bug and this fix live in, at a scale the other tests don't
reach. Verified under -fsanitize=undefined (150+ runs across this
change's lifetime, 0 failures) and -fsanitize=address (100+ runs, 0
real findings; known sandbox ASan-startup flakes excluded, see prior
commits) per CLAUDE.md's sanitizer rule for upper.c/overlay.c changes.
Measured result (make bench, same environment as the original
finding): mem create 257x faster, unlink 330x faster, mkdir 65x
faster, concurrent mixed workload 131x faster -- and, the comparison
that matters, mem now beats raw fs at every one of these (was losing
by 6-22x before). The complexity-class change is confirmed the same
way the O(n^2) was found: mkdir at N=4,000 vs create at N=20,000 now
shows a 7.15x slowdown for a 5x increase in N, matching the O(n log n)
prediction (5.97x) rather than the old O(n^2) one (25x). Full
before/after tables in BENCH.md's new "Resolution" section, which
keeps the original run as the historical record rather than
overwriting it, per this project's own documentation standard.
Recorded the fix in CLAUDE.md's "Known performance characteristics"
(marked RESOLVED, not silently removed) and its architecture map.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB
74 lines
3.7 KiB
Markdown
74 lines
3.7 KiB
Markdown
# Contributing to PackFS
|
|
|
|
## Before changing anything
|
|
|
|
Read [`concept.md`](concept.md) in full. It is the project's specification,
|
|
not background reading — every backend, lock, and on-disk field in `src/`
|
|
exists because a section of `concept.md` requires it, and most functions'
|
|
comments cite the section they implement rather than re-explaining it.
|
|
`concept.md` is frozen (see [`CLAUDE.md`](CLAUDE.md)): if you believe the
|
|
design itself needs to change, that belongs in a new document that amends
|
|
or supersedes it, not in an edit to `concept.md`.
|
|
|
|
Then read [`CLAUDE.md`](CLAUDE.md), which states the load-bearing constraints
|
|
a change must respect (static linking, the write model, the concurrency
|
|
model, path containment, pack integrity) and the documentation register this
|
|
project is written in.
|
|
|
|
## Workflow
|
|
|
|
```sh
|
|
make test # must pass before any PR
|
|
cc ... -fsanitize=address,undefined # ASan/UBSan: see .github/workflows/ci.yml for exact flags
|
|
cc ... -fsanitize=thread # TSan, for anything touching src/upper.c, src/overlay.c, or src/vfs.c
|
|
```
|
|
|
|
Any change to the concurrency-sensitive files (`upper.c`, `overlay.c`,
|
|
`vfs.c`) must be run under ThreadSanitizer, not just the plain test suite —
|
|
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.
|
|
|
|
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
|
|
`make bench` before and after, not just `make test`: `BENCH.md` records a
|
|
real, measured O(n²) cost the flat array this index used to be had in bulk
|
|
sequential create/unlink, and the persistent treap that replaced it exists
|
|
specifically to avoid regressing back into it — a correctness-preserving
|
|
change to this code can still reintroduce that regression (e.g. an
|
|
implementation that silently degrades to a linked list under adversarial
|
|
insertion order) in a way `make test` alone cannot detect, only `make bench`
|
|
plus `tests/test_index_stress.c`'s randomized-order correctness check can.
|
|
|
|
## Scope
|
|
|
|
Changes that add functionality `concept.md` Section 10 lists as explicitly
|
|
out of scope for v0 (full POSIX semantics, enforced permissions/symlinks/hard
|
|
links, cross-process concurrency, content-defined chunking/delta
|
|
compression) should discuss the tradeoff with a maintainer first — those
|
|
exclusions were deliberate design decisions, not gaps waiting to be filled.
|
|
|
|
**`zip` and `tar` import/export backends will not be accepted, full stop —
|
|
not discussed, not behind a flag.** `concept.md` Section 11 recommends them,
|
|
but that recommendation is permanently superseded; see `CLAUDE.md`, "Project
|
|
decisions that supersede concept.md." This is a harder line than the v0
|
|
exclusions above, which are open to future discussion — this one is not.
|
|
|
|
## Static-linking constraint
|
|
|
|
`concept.md` Section 11.1 is a hard constraint: the core must build with
|
|
zero required third-party libraries and zero dynamic loading. In practice
|
|
this project has no optional-dependency boundary at all: `concept.md`
|
|
described one at the `zip`/`tar` backend for `miniz`/`libarchive`, but per
|
|
the decision above, that backend will never exist, so there is no path by
|
|
which a third-party dependency enters this codebase — a change proposing
|
|
one, for any backend, will not be accepted.
|
|
|
|
## Style
|
|
|
|
Match the register already in the file you're editing — precise,
|
|
constraint-labeled comments citing the `concept.md` section they implement,
|
|
no filler. See the "Documentation standard" section of `CLAUDE.md` for the
|
|
full rule; it applies to code comments and commit messages, not only to
|
|
`.md` files.
|