diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md new file mode 100644 index 00000000..0674ccf5 --- /dev/null +++ b/notes/unit_value_perf.md @@ -0,0 +1,678 @@ +# Unit-value optimizations: measured results + +Two optimizations aimed at `PathMap<()>` and other trivially-valued tries, from a +survey of places the trie pays for a value that carries no information. + +| commit | change | +| --- | --- | +| `e2755ed` | Skip value-drop work in `LineListNode` when `V` has none to do | +| `12edc03` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | +| `f480236` | Memory attribution by node type under the `counters` feature | +| `67004f8` | Don't clone a dense node until a subtraction actually changes it (**A1**) | +| `2398bf7` | Join a dense node in place when the other side brings no new bytes (**A5**) | + +A fourth change -- packing the CoFree value flag into its child pointer -- was +built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). + +**A note on the survey these came from.** Its estimates were made by reading +struct definitions, and three of them did not survive measurement: S3 was not +implementable *and* worthless, S4 turned out to cost nothing to begin with, and +S1's headline ("2x node memory") was true per-node but meant 5% on +MORK-shaped data, because dense nodes are only a fifth of those bytes. Measure +first; the profiler in `f480236` exists for that. + +## Results + +Min of 3 runs per side, each run itself the min of 7 timed repetitions, every +operation in its own process. 1M paths of 8 random bytes. + +| operation | before (ms) | after (ms) | change | verdict | +| --- | ---: | ---: | ---: | --- | +| `join_into` — operands share a root | 108.1 | **0.000** | **eliminated** | real gain | +| `join` — operands share a root | 0.000 | 0.000 | — | already short-circuited | +| `subtract` — operands share a root | 0.000 | 0.000 | — | already short-circuited | +| `join_into` — disjoint operands | 102.4 | 104.6 | +2.1% | within noise | +| `join` — disjoint operands | 85.8 | 87.0 | +1.5% | within noise | +| `meet` — full overlap, distinct nodes | 95.0 | 95.0 | +0.0% | within noise | +| `subtract` — full overlap, distinct nodes | 123.3 | 114.3 | −7.3% | within noise | +| drop `PathMap<()>` | 193.1 | 193.7 | +0.3% | within noise | +| drop `PathMap` | 191.7 | 192.0 | +0.1% | within noise | +| drop `PathMap>` | 365.8 | 367.1 | +0.4% | within noise | + +**Noise floor**, measured by running the same tree against itself three times: +7–11% on the algebra operations, 2–3% on the drops. Every row above except the +first sits inside it. Treat the −7.3% on `subtract` as noise, not a gain — the +change touches nothing on that path beyond a compile-time-constant guard. + +## What each optimization actually bought + +### `join_into` on a shared subtrie: 108 ms → free + +The only unambiguous win, and it came from a shortcut that was *missing* rather +than one that was slow. `TrieNodeODRc::join_into` had no pointer-equality check +and went straight to `make_mut()`, which deep-copies the node when it is shared — +precisely the case where both sides are likely to be the same pointer. Three call +sites that hand-rolled `make_mut().join_into_dyn()` now route through it and +inherit the check: + +- `PathMap::join_into` +- `CoFree::join_into` (the recursive branch) +- `WriteZipper::join_into_take` + +The size of the win scales with the trie, because the whole descent disappears. +It is worth the most in workloads that join overlapping tries built by grafting +or cloning, where pointer sharing is common — which is the regime `()` values plus +merkleization create. + +### Drop elision: no measurable throughput change + +`LineListNode`'s drop paths tested whether each payload slot held a child or a +value, then called `ManuallyDrop::drop` on the value. For a value type with no +drop glue that call does nothing, but the branches around it still ran. + +LLVM was already folding the no-op `LocalOrHeap<(), _>::drop` at `-O2`, so what +the change removes on top of that is a well-predicted branch — invisible against +the allocator traffic and cache misses of walking a million-node trie. What it +does buy: + +- The elision is explicit rather than optimizer-dependent, so it holds in debug too. +- The drop path got simpler: `is_used_child_N()` replaces four separate bit tests. +- It generalizes past `()` to any value type with no drop glue that fits inline. + +One trap worth recording. The obvious guard, `needs_drop::()`, is **wrong** +here. The slot is a `LocalOrHeap`, which carries an unconditional `Drop` +impl and boxes any `V` too large to store inline — so a `[u8; 64]` has no drop +glue of its own but does own a heap allocation. The predicate has to cover both: + +```rust +pub(crate) const fn val_slot_needs_drop() -> bool { + core::mem::needs_drop::() || core::mem::size_of::() > core::mem::size_of::() +} +``` + +`ValSlotStorage` is now the type alias the union itself uses, so the size +threshold cannot drift away from the layout. `cargo miri test` covers the +`[u8; 64]` case, since only miri can observe the leak a wrong guard would cause. + +### `IDEMPOTENT`: a correctness fix, not a speedup + +The gating half of `12edc03` costs and saves nothing at `()`, because +`IDEMPOTENT` is `true` there and folds away. Its value is that the constant now +means something. Six sites take the shared-subtrie shortcut — not the three that +grepping for `ptr_eq` finds, but also the `TaggedNodeRef` dispatchers underneath, +which is where it fires at *every* level of a descent, in two copies (one per +`slim_dispatch` setting). Before this change a value type whose join actually +combines its operands — a multiset adding multiplicities — would have had its +structurally shared branches silently skipped instead of combined. + +No in-tree value type declares `IDEMPOTENT = false`, so no existing behavior +changed; all 710 lib tests pass unchanged. + +## Method + +- rustc 1.95.0-nightly, `--release`, `-C target-cpu=native` (from `.cargo/config.toml`) +- Default features (`graft_root_vals`, `slim_ptrs`, `serialization`); jemalloc **off** +- x86_64 Linux, 64 cores +- Baseline is `master` at `8b8802a` in a separate worktree with its own target dir +- Each operation runs in its own process. This matters: an earlier pass that timed + everything in one process showed a spurious 6–9% regression on three operations, + because at baseline the self-shared join does real allocator work that warms the + heap for whatever is measured next. Process isolation removed it. +- Inputs are built outside the timed region; results are dropped outside it. + +The timing harness was temporary and is not in the tree; reconstructing it means +building two `PathMap<()>`s from xorshift-generated 8-byte keys, timing the +operation with `Instant`, and taking the min across repetitions. The *memory* +harness is in the tree -- `cargo test --release --features counters,serialization +--test memory_profile -- --nocapture` reproduces every memory table below. + +## Where the memory actually is + +`memory_profile` (`f480236`) walks physical nodes and attributes bytes by node type. +Three tries, all `PathMap<()>`: + +| dataset | values | list nodes | dense nodes | bytes/value | +| --- | ---: | ---: | ---: | ---: | +| `big_logic.metta` (MORK) | 91,692 | 7.63 MB (**79.7%**) | 1.95 MB (20.3%) | 104.5 | +| 1M random 8-byte keys | 1,000,000 | 62.2 MB (69.2%) | 27.7 MB (30.8%) | 89.9 | +| shakespeare words | 67,505 | 2.16 MB (52.8%) | 1.93 MB (47.2%) | 60.7 | + +The split swings from 80/20 to 53/47 with key shape, so "shrink a dense slot" and +"shrink a list node" mean very different things depending on the workload. On the +MORK-shaped data, list nodes are four fifths of the trie. + +### Dense slot arrays run 16-41% over-allocated + +| dataset | slots used | slots allocated | slack | +| --- | ---: | ---: | ---: | +| MORK | 52,474 | 60,714 | 15.7% | +| random 8-byte | 1,038,499 | 1,466,715 | **41.2%** | +| shakespeare | 55,916 | 72,132 | 29.0% | + +`Vec`'s amortized doubling, on arrays that top out at 256 slots and are usually +built one byte at a time. + +## Shelved: packing the value-presence flag into the child pointer + +**Implemented, measured, and not merged.** It lives on branch +`experiment/cofree-pointer-packing` (`2fd48be`), rebuilt on top of this branch's +history. It works, it is miri-clean, and it buys real memory -- but it costs +roughly 9% on iteration over MORK-shaped data, which is the workload that +matters here, so the trade goes the wrong way. Kept as a branch to show the +approach is viable and to record what it costs. + + +A dense-node slot was `{ Option, Option }`: one word for the +pointer, and a whole word more for `Option` once alignment rounds it up. Node +allocations are 8-byte aligned, so bits 0..3 of a node address are always zero. +`OrdinaryCoFree` now stores an absent child as the empty-node sentinel instead of +`None` -- so the word is never null -- and borrows bit 0 to say whether its +`MaybeUninit` is initialized. + +| slot | before | after | +| --- | ---: | ---: | +| `OrdinaryCoFree<()>` | 16 B | **8 B** | +| `OrdinaryCoFree` | 16 B | 16 B | +| `OrdinaryCoFree` | 24 B | **16 B** | + +Pinned by static assertions, so the layout cannot regress silently. + +### Result + +| dataset | memory | build | iterate | lookup | +| --- | ---: | ---: | ---: | ---: | +| `big_logic.metta` (MORK) | 9.58 -> 9.09 MB (**-5.1%**) | +3.3% | +9.3% | +0.7% | +| 1M random 8-byte keys | 89.94 -> 78.21 MB (**-13.0%**) | -7.8% | -3.4% | -9.2% | +| shakespeare words | 4.10 -> 3.52 MB (**-14.1%**) | -11.6% | -11.5% | -28.3% | + +Memory is a clean win and lands within a tenth of a percent of what the profiler +predicted. **The timing is why this is shelved.** Shakespeare gets faster on every +axis and random keys improve modestly, but MORK iteration measures ~9% slower. +That is at the edge of what these benchmarks resolve, so it may be less than it +looks -- but 5% memory is not worth risking 9% iteration on the target workload, +and the burden of proof is on the change. + +The likely cause is that `has_rec()` went from an `Option` null check to +`is_empty()`, which extracts and compares the pointer tag. That is a few +instructions on a very hot path, and MORK's tries are list-node-heavy, so they +pay it without getting much of the dense-node memory win in return. Anyone +picking this up should start there. The algebra +micro-benchmarks (`join`, `meet`, `subtract`, both disjoint and overlapping) all +moved less than 5%. + +### Why the flag went in the pointer rather than a mask on the node + +The alternative was a second `ByteMask` on `ByteNode`. It would have been safer +per-line but much larger: a `CoFree` would stop being self-describing, and the +algebra clones and drops them *outside* the node that owns them -- +`pjoin`, `pmeet`, `psubtract` and `prestrict` each accumulate `CoFree`s into a +fresh vector before any node exists to own it. Keeping the flag inside the slot +left all ~108 call sites untouched. + +### The invariant, and what it cost + +The bit belongs to the **slot**, not to either node, so every in-place +replacement of a `TrieNodeODRc` must preserve it. `replace_node` and `swap_node` +do; plain assignment does not, and the failure is silent -- the slot's value just +disappears. Converting the assignment sites broke six existing tests, and one +more case (`ByteNode::node_replace_child`) that no existing test covered and that +a regex over deref-assignments missed because of the method-call chain +(`*cf.rec_mut().unwrap() = new_node`). + +Every failure was a slot holding a value *and* a child -- i.e. a path that is a +proper prefix of another path. `tests/cofree_val_flag.rs` pins that down: +copy-on-write of a shared trie, node upgrades under a write zipper, grafting over +a slot that holds a value, algebra over prefix-heavy tries, and removal from a +slot that holds both. All pass under miri, along with the differential algebra +test and 118 filtered lib tests. The suite is kept on the main line as +`tests/prefix_value_preservation.rs`, since the property is worth pinning down +whatever the representation. + +Two notes for anyone touching this again: + +- The empty sentinel moved from `0xBAADF00D` to `0xBAADF00C`, because it has to + be able to carry the flag like any other node pointer. A static assertion + enforces that bit 0 is clear. +- `ptr_eq` and `shared_node_id` mask the bit off. Leaving it in would have + silently defeated the shared-subtrie shortcuts from `12edc03` and weakened the + catamorphism cache, without failing a single test. + +### S2 -- the CellCoFree box: not worth doing + +`CellCoFree` keeps the old `Option` layout under the name `PinnedCoFree`, because +`CellByteNode::prepare_cf` hands a `WriteZipper` a `&mut Option` that has to +point at a real `Option`. That costs nothing measurable: cell nodes appear only +under a `ZipperHead` and account for **zero bytes** in all three tries profiled +here, so halving their per-slot box would have saved nothing. + +## Negative results + +These eight were measured and rejected. Recording them so they are not +re-attempted from first principles. + +### F6 -- shrinking the payload enums: one is already optimal, the other is not where the time goes + +The survey wanted `ValOrChild` and `PayloadRef` collapsed to "a nullable pointer +plus a bool -- one word". Measured: + +| `V` | `ValOrChild` | `PayloadRef` | +| --- | ---: | ---: | +| `()` | **8** | 16 | +| `u64` | 16 | 16 | +| `String` | 24 | 16 | + +**`ValOrChild<()>` is already one word.** `Val(())` carries nothing, so rustc +encodes it in the child pointer's null niche -- no discriminant, no padding. +There was nothing to do. + +**`PayloadRef` cannot use that niche**, because it has two pointer-carrying +variants plus `None`, so one word goes to the discriminant. But it is 16 bytes +for *every* `V`, so it is not a unit-value issue, and it is a transient -- built +on the stack during meets, never stored in a trie -- so its width costs memory +nowhere. `payload_layout_tests` pins both facts, since a third variant on +`ValOrChild` would silently double it. + +The path *is* hot, which is the one place the survey was right. On a +list-node-heavy set-algebra workload (long shared prefixes, meet/subtract/restrict): + + node_get_payloads (LineListNode) 8.09% + pmeet_generic 5.75% + pmeet_generic_internal 3.54% + ------ + 17.7% + +But the cycles inside it are not the enum: + +| line | what it is | cycles | +| --- | --- | ---: | +| `line_list_node.rs:248/249` | `is_used::` header bit tests | 4.38% | +| `:391`, `:396` | key-length extraction from the header | 3.03% | +| `:406`, `:551`, `:556` | building key slices, `get_both_keys` | 3.29% | +| `trie_node.rs:620` | `pmeet_generic_internal` prologue, i.e. array spills | 1.97% | + +Only the last line is attributable to payload width at all, and shrinking a +two-element `(usize, PayloadRef)` array from 48 bytes to 32 would reclaim a +fraction of it. The rest is header decoding and key slicing -- inherent work for +a node that packs two variable-length keys into a 16-bit header, and the thing to +look at if this path ever needs to be faster. + +### F4 -- "the result-merging machinery is what actually costs": off by an order of magnitude + +The survey claimed the generic algebra's result plumbing -- `AlgebraicResult`, +`merge`, `combine_algebraic_results`, the `Hetero*` traits -- is where a +`PathMap<()>` spends its time, because the identity masks keep it from folding +away. Profiled with `perf` on 400k-path joins, meets and subtracts, attributing +cycles by source line so inlined code lands on its own line: + +| source | cycles | +| --- | ---: | +| `malloc.c` | **29.72%** | +| `dense_byte_node.rs` | 17.71% | +| `trie_node.rs` | 13.31% | +| `option.rs` | 3.82% | +| `atomic.rs` (refcounts) | 2.71% | +| `line_list_node.rs` | 2.48% | +| **`ring.rs`** -- all of `AlgebraicResult` and the `Lattice` impls | **1.06%** | + +`combine_algebraic_results` shows up separately at 2.86%. So the merging +machinery is roughly **4%**, against **30% in the allocator**. Rewriting it as +bit arithmetic -- which is what the survey proposed, and which depended on the +shelved S1 anyway -- would have chased about a twenty-fifth of the runtime. + +## A1 -- landed, as the fix the profile pointed at rather than the one proposed + +A1 proposed rewriting the four dense-node operations as mask arithmetic. Checked +against the code, two thirds of it was either impossible or already done: + +- **The narrowed descent already exists.** `pmeet` and `prestrict` iterate + `self.mask & other.mask`; `pjoin` iterates the union but only *recurses* on the + intersection; `psubtract` only recurses where `other` has the bit. +- **The value dimension cannot become mask ops** without the per-node `val_mask` + from S1, which is shelved. + +What was left was A1's aside about `psubtract` opening with `self.clone()` before +knowing the answer -- and that turned out to be the whole prize, and needs no mask +split. Instrumented on 400k-path subtractions: + +| operands | calls returning `Identity` | slot clones wasted | +| --- | ---: | ---: | +| disjoint | **100.0%** | 431,714 of 431,714 (**100%**) | +| 5% overlap | 71.2% | 251,481 of 432,028 (58.2%) | +| a 25% subset | 0.0% | nothing to waste | + +`btn` is now an `Option`, left `None` while the result is still identical to +`self` and cloned at the first real change. Min of 3 runs, 400k paths: + +| subtract | before | after | change | +| --- | ---: | ---: | ---: | +| disjoint operands | 30.04 ms | 17.18 ms | **-42.8%** | +| 5% overlap | 34.87 ms | 25.18 ms | **-27.8%** | +| a 25% subset | 40.43 ms | 39.20 ms | -3.0% | +| a 95% subset | 69.96 ms | 69.44 ms | -0.7% | + +The last two have nothing to reclaim and sit in the noise floor. `meet`, whose +code is untouched, moved -8.9% to +0.1% across the same operands -- that is the +floor. Verified with the differential algebra test and miri. + +### A5 -- `join_into` in place when the other side brings no new bytes + +`ByteNode::join_into` allocated a slot array sized to the union of the two masks +and moved every slot into it, even when the union equals `self.mask`. Then nothing +needs inserting and the overlapping slots can be joined where they sit. A5 +proposed this behind the shelved val-mask; it needs no such thing. + +The case is the norm for an incremental ingest -- a trie joined with a batch of +paths it mostly already covers: + +| operands | calls where other's mask is a subset | slot moves avoidable | +| --- | ---: | ---: | +| disjoint | 0.3% | 5.2% | +| batch, 1% new keys | **95.3%** | **95.3%** | +| already contained (5%) | **100%** | 100% | +| already contained (95%) | **100%** | 451,600 | + +Interleaved, min of 8 rounds, `join` as an untouched control: + +| join_into | before | after | | +| --- | ---: | ---: | --- | +| disjoint | 61.55 ms | 61.81 ms | +0.4% | +| 1% new keys | 2.37 ms | 2.21 ms | -6.8% | +| contained (5%) | 12.81 ms | 12.51 ms | -2.3% | +| contained (95%) | 80.42 ms | 76.54 ms | **-4.8%** | +| *join (control)* | | | *-0.8% to +0.5%* | + +Consistent in direction across all four shapes with the control flat, and no +regression on the disjoint case the branch cannot help. The 1% figure sits on a +2 ms measurement with 22-43% spread -- read it as "no worse"; the 95% case, at 8% +spread, is the one to believe. + +Modest, and the reason is worth keeping: these are slot **moves**, a memcpy of the +`CoFree`s, not the **clones** that made the same idea worth 42.8% in `psubtract` +where each was an atomic refcount bump. What A5 avoids is one allocation and one +memcpy per node. + +### The same fix does not pay for `pjoin` or `prestrict` + +Both were implemented -- deferring the slot vector until the first result that is +neither `SELF_IDENT` nor `COUNTER_IDENT`, back-filling the prefix from whichever +operand was still identity, which the flags make recoverable because a live flag +implies that operand's mask equals the result mask, so the slots line up by index. +Correct, and reverted anyway. + +Interleaved runs, min of 4 per side, with `meet` and `subtract` as untouched +controls: + +| join | before | after | | +| --- | ---: | ---: | --- | +| disjoint | 46.28 ms | 51.19 ms | **+10.6%** | +| 5% overlap | 49.25 ms | 52.25 ms | +6.1% | +| 25% subset | 40.33 ms | 34.91 ms | **-13.4%** | +| 95% subset | 60.90 ms | 57.13 ms | -6.2% | +| identical | 61.43 ms | 59.67 ms | -2.9% | + +Replacing `Vec::push` with an unchecked write into the reserved slot brought the +disjoint regression from +10.6% to +3.7%, but did not grow the wins. `prestrict` +landed inside the noise in both directions across two runs. + +**Why it pays for `psubtract` and not these two.** `psubtract` cloned the *whole +node* up front, and on disjoint operands it barely recurses -- few bytes are in +both masks -- so the clone was nearly the entire cost of the call and removing it +took 42% off. `pjoin` and `prestrict` only skip the *slot vector*; the recursive +descent still happens on every overlapping byte and dominates. F4 measured 99.6% +of `pjoin`'s slot clones as wasted at 95% overlap, and eliminating all of them +bought ~6%, which is where the recursion leaves room. + +A measurement note worth keeping: a first pass showed everything regressing 11-24% +*including the two controls*, which share their code with the baseline exactly. +That was the machine drifting between the two measurement windows. Any comparison +here has to interleave the two sides and carry an untouched control, or it will +measure the room temperature. + +## A correctness bug in `restrict`, found while testing the above -- fixed + +`restrict(a, a)` must be `a` for any `a`: every path of `a` has a value at itself +in `a`, and the rule is to keep everything at or below a value in the other +operand. It did not hold when a path both carried a value and branched: + +| input | `restrict(a, a)` | dropped | +| --- | --- | --- | +| `{ab, abc}` | `{ab, abc}` | -- | +| `{ab, abc, abd}` | `{abc, abd}` | **`ab`** | +| `{a, ab, abc}` | `{ab, abc}` | **`a`** | +| `{ab, abc, abd, abe, abf}` | 4 of 5 | **`ab`** | + +One child was fine; two or more and the value was silently dropped. It predates +this branch -- it reproduced identically on `master` (`8b8802a`). + +**The cause.** `follow_path_to_value` walked the other operand following onward +links, and only looked for a value once the walk ran out of them. A node can hold +a value *and* a child under the same key -- a path that both ends there and +continues -- and in that case the walk took the link and reported "no value, path +continues", so `restrict_slot_contents` discarded the value. The check now happens +at every node before the link is taken, which also covers a value sitting at a +prefix of the key being followed. Fixed on `master` in `b2a0c09`; this branch no longer carries it. + +**Cost:** none measurable. Interleaved against the untouched `meet` as a control, +restrict moves -1.5% to +0.6% across five operand shapes; the control moves -1.4% +to +1.6%. + +**Why it survived.** `restrict` was the only one of the four algebraic operations +without a differential test. It now has a `BTreeSet` oracle -- a path of `self` +survives when some prefix of it, the empty prefix and the path itself included, +carries a value in `other` -- run over prefix-free and prefix-heavy sets and the +minimal shapes above. + +A standalone repro lives on branch `restrict-branching-value-repro`, branched +from `master`: three tests, two of which fail there and all of which pass here. + +## The thing the F4 profile actually found + +Where the allocator time goes is worth its own section, because it is a real and +fixable inefficiency, and it is not unit-value-specific. + +`ByteNode::pjoin` allocates the result slot vector and clones every slot into it +*before* it knows whether the result is a new node or just one of its operands. +When the answer turns out to be `Identity`, all of that is discarded. Instrumented +on 400k-path joins: + +| operands | calls returning `Identity` | slot clones wasted | +| --- | ---: | ---: | +| disjoint | 0.0% | 0 of 789,291 (0.0%) | +| one is a subset of the other | **99.7%** | 159,078 of 167,800 (**94.8%**) | +| 95% overlap | **100.0%** | 449,799 of 451,600 (**99.6%**) | + +Each wasted slot clone is an atomic refcount increment plus a matching decrement +on drop, and each wasted call is a `Vec` allocation and free. The effect is +visible end to end, and it inverts what you would expect: + +| join | time | +| --- | ---: | +| disjoint operands -- builds a genuinely new 800k-path trie | 48.75 ms | +| one operand a subset -- result equals the larger operand | 42.64 ms | +| 95% overlap -- result is essentially the larger operand | **74.96 ms** | + +The join that has almost nothing to do takes **1.5x longer** than the one that +doubles the trie, because it does the full copy and then throws it away. Joining +a trie with something it already mostly contains is a common shape -- it is what +incremental ingestion looks like -- so this is worth fixing. + +The fix is to defer materializing the vector until the first slot result that is +neither `SELF_IDENT` nor `COUNTER_IDENT`. While both node-level identity flags +are alive, the result prefix is exactly one operand's slots, so it can be +back-filled at the point the flags die, using the side that was still identity. +Recursive sub-joins that returned `Identity` allocated nothing themselves, so the +saving is the whole per-node cost. Not attempted here. + +### S5 -- `LocalOrHeap` "pins the payload union at 8 bytes": it does not + +The survey said the `LocalOrHeap` arm is why a payload slot is 8 +bytes wide, and that a set-specialized node would have to replace it. Both halves +are wrong. + +`LocalOrHeap` is a *fixed-size cell* -- it boxes anything too big to fit -- so +the value arm is the same width for every `V`, and the union's width is set by +its **other** arm, the child pointer: + +| `V` | `size_of::()` | union | boxed? | +| --- | ---: | ---: | --- | +| `()` | 0 | 8 | no | +| `u64` | 8 | 8 | no | +| `String` | 24 | 8 | **yes** | +| `[u8; 1024]` | 1024 | 8 | **yes** | + +Deleting the value arm entirely would not shrink the union by a byte, because the +child pointer needs those 8 regardless. And for `()` the arm costs nothing at +runtime either: `size_of::<()>() == 0`, so nothing is boxed. +`val_slot_layout_tests` asserts both facts. + +**What it did surface, which is real but not a unit-value concern.** Values live +overwhelmingly in list-node slots, and those are the ones that box: + +| dataset | values in list slots | in dense slots | +| --- | ---: | ---: | +| `big_logic.metta` (MORK) | **99.93%** | 0.07% | +| 1M random 8-byte keys | **100.00%** | 0.00% | +| shakespeare words | 53.44% | 46.56% | + +So for a `V` over the threshold, MORK-shaped data pays roughly one heap +allocation *per value*. Priced on 91,692 paths: + +| value | size | boxed | build | drop | +| --- | ---: | --- | ---: | ---: | +| `u64` | 8 | no | 23.66 ms | 6.94 ms | +| `[u8; 8]` | 8 | no | 23.76 ms | 6.96 ms | +| `[u8; 9]` | 9 | **yes** | 28.22 ms (**+19%**) | 9.43 ms (**+36%**) | +| `[u8; 16]` | 16 | yes | 26.02 ms | 9.13 ms | + +One byte over the line costs ~19% on insertion and ~36% on drop. Widening the +cell would fix it, but only by making every node bigger for every `V` -- +including `()`, which needs none of it -- and sizing the cell per-`V` needs +`generic_const_exprs`. So the actionable part is documentation: the cliff is now +described on [`TrieValue`](../src/lib.rs), where someone choosing a value type +will see it. + +### F3 -- eliding the value hash in `merkleize`: nothing to elide, and the obvious version is a bug + +The survey said `merkleize` "requires `V: Hash` to hash nothing". The natural +reading -- put a const branch around `val.hash(&mut hasher)` -- is wrong twice. + +Both sites hash `Option<&V>`, not `V`, and the `Option` is what carries the +information: + +| expression | bytes written | hash | +| --- | ---: | --- | +| `().hash(h)` | **0** | -- | +| `Option::<&()>::None` | 8 | `0` | +| `Option::<&()>::Some(&())` | 8 | `27512614111` | + +So the value payload *already* costs nothing at `()` -- `impl Hash for ()` writes +zero bytes and the call folds away. What the 8 bytes carry is the discriminant, +and for a set trie that discriminant is the only information there is: whether +the path is a member. Skipping it would make a path with a value hash identically +to one without, and `merkleize` would merge structurally distinct tries. That is +a correctness bug, not an optimization. + +The `V: Hash` bound cannot come off either -- it is needed for a general `V`, and +`()` implements `Hash`, so it costs nothing to satisfy. + +One adjacent remnant is real but too small to take: the "value, no child" branch +builds a fresh `GxHasher` per leaf value to compute what is, for any zero-sized +`V`, a constant. Priced directly: + + merkleize, big_logic.metta 21.03 ms for 91,692 values (229 ns/value) + merkleize, shakespeare 5.65 ms for 67,505 values ( 84 ns/value) + the per-leaf hasher 3.03 ns x 91,692 leaves = 0.28 ms + +0.28 ms of 21.03 ms is **1.3%** -- below this machine's measured noise floor, in +the one routine where a subtle change silently corrupts structural sharing. +Not worth it. + +### S4 -- replacing the dangling-path sentinel with a bit: nothing to save + +The earlier survey claimed that a path kept alive by `remove_val(prune = false)` +costs "a pointer + node per pruned leaf", because a `LineListNode` records it by +pointing the slot's child link at `TrieNodeODRc::new_empty()`. Measured, it costs +nothing at all: + +- **The sentinel never allocates.** `new_empty()` is a bogus address + (`0xBAADF00D`) carrying `EMPTY_NODE_TAG`. There is no node. +- **The payload word exists regardless.** A `LineListNode` is a fixed-size + struct, so the union the sentinel sits in is there whether the slot uses it or + not. Marking the slot "dangling" in the header instead would free zero bytes. +- **A `DenseByteNode` already does what S4 proposed.** It represents a dangling + path as a CoFree holding neither a child nor a value -- there is no sentinel in + a dense node to begin with. + +The measurement: taking shakespeare and removing half its values with +`prune = false` produces 14,212 dangling slots and leaves the trie **byte for +byte identical**, 4,095,744 before and after. `tests/memory_profile.rs` asserts +this so the claim does not have to be re-derived. + +Also checked, since it was the only caller that really allocates: a `WriteZipper` +opened on a path that does not exist allocates an empty `LineListNode` for its +root, but does **not** leave it behind -- creating 2000 such zippers and dropping +them without writing moved the byte count by exactly 0, and no allocated empty +node was ever observed in any trie profiled here. + +### S3 -- reclaiming value-slot bytes for list-node keys: not possible, and worthless + +The earlier survey claimed a `LineListNode` value slot's 8 bytes could go to key +storage when `V` is a ZST, taking `KEY_BYTES_CNT` from 42 to 58. That was wrong +twice over. + +**It cannot be done.** The slot is a union with a child pointer, and which one it +holds is a runtime property of the node. The space has to be sized for the pointer +whatever `V` is. There is no static reclaim without a separate node type. + +**It would not be worth it anyway.** Only 10.4% of MORK list nodes are at the +42-byte cap, and *zero* on the other two datasets. + +### Sweeping `KEY_BYTES_CNT`: already near-optimal for MORK, no single best value + +| K | node size | MORK | random 8-byte | shakespeare | +| ---: | ---: | ---: | ---: | ---: | +| 10 | 32 B | +18% | **-35%** | **-27%** | +| 14 | 40 B | +16% | -28% | -20% | +| 18 | 40 B | +1.5% | -28% | -20% | +| 26 | 48 B | **-2%** | -19% | -14% | +| 34 | 56 B | -2% | -10% | -7% | +| 42 | 64 B | baseline | baseline | baseline | + +Timings moved less than 5% throughout. MORK's long paths want the current 42; +short-key workloads want roughly 10. No single constant serves both, which is an +argument for a second, smaller list-node type rather than a different constant. + +### Bounded growth for dense slot arrays: a wash + +Replacing `Vec` doubling with growth to the next multiple of `SLOT_GROWTH`: + +| growth | MORK | random 8-byte | shakespeare | random build time | +| --- | ---: | ---: | ---: | ---: | +| exact | -1.4% | **-7.6%** | -5.6% | **+26%** | +| 2 | -1.1% | -7.0% | -4.6% | +8.5% | +| 4 | -0.6% | -5.9% | -2.3% | +8.3% | +| 8 | +1.6% | -3.6% | +2.2% | ~flat | + +The memory it reclaims is paid for in build time, and for granularity 8 and above +the rounding wastes more than doubling did on MORK's small dense nodes (3.4 slots +on average). Rejected. + +## Still on the table + +Both of these are small items from a wider survey. The larger unit-value wins are +untouched: + +- **Dense-node slots are 16 bytes where 8 carry information.** `OrdinaryCoFree` is + `{ Option, Option }`; at `()` the pointer is 8 bytes and + `Option<()>` is 1, which alignment rounds to 16. Hoisting the presence bit into a + second `ByteMask` halves a 256-way dense node from 4128 to 2112 bytes. +- **The algebra becomes mask arithmetic** once that split exists: the value + dimension of join/meet/subtract/restrict is one bitwise operation, and recursion + narrows to the intersection of the two child masks. +- **`LineListNode` spends 16 of its 64 bytes on payload slots a set doesn't need**, + which could go to inline key bytes instead (42 → 58 for a two-leaf node). + +These need either a value policy carrying an associated slot type — the refactor +`ring.rs` and `dense_byte_node.rs` already gesture at, and that +`pathmap-book/src/A.0003_policy_API.md` is a stub for — or parallel set-specialized +node types. Neither is reachable with the kind of local guard used here. diff --git a/src/counters.rs b/src/counters.rs index 78baf39e..fa80e148 100644 --- a/src/counters.rs +++ b/src/counters.rs @@ -250,6 +250,146 @@ pub(crate) fn record_make_unique(cloned: bool) { } } +/// The key-byte capacity of a single [LineListNode](crate::line_list_node), and the resulting node size +pub const LIST_NODE_KEY_BYTES: usize = crate::line_list_node::KEY_BYTES_CNT; +/// The size in bytes of a single `LineListNode`, derived from [LIST_NODE_KEY_BYTES] +pub const LIST_NODE_SIZE: usize = crate::line_list_node::LINE_LIST_NODE_SIZE; + +/// Memory attribution for a trie, broken down by node type +/// +/// Walks physical (deduplicated) nodes, so structurally shared subtries are counted once. Use it to +/// decide where layout work pays: the split between list-node and dense-node bytes varies enormously +/// with key shape, and is what determines whether a given optimization is worth anything. +#[derive(Clone, Default, Debug)] +pub struct MemProfile { + pub list_nodes: usize, + pub list_bytes: usize, + pub dense_nodes: usize, + pub dense_items: usize, + pub dense_cap: usize, + pub dense_bytes: usize, + pub cell_nodes: usize, + pub cell_items: usize, + pub cell_bytes: usize, + pub klen_hist: Vec, + pub val_slots: usize, + pub child_slots: usize, + pub both_slots: usize, + pub key_bytes_used: usize, + pub at_cap: usize, + pub empty_nodes: usize, + pub dangling_slots: usize, + pub dense_val_slots: usize, + pub node_klen_hist: Vec, + pub allval_nodes: usize, + pub allval_klen_hist: Vec, +} +impl MemProfile { + fn merge(mut self, o: Self) -> Self { + self.list_nodes += o.list_nodes; self.list_bytes += o.list_bytes; + self.dense_nodes += o.dense_nodes; self.dense_items += o.dense_items; self.dense_bytes += o.dense_bytes; self.dense_cap += o.dense_cap; + self.cell_nodes += o.cell_nodes; self.cell_items += o.cell_items; self.cell_bytes += o.cell_bytes; + self.val_slots += o.val_slots; self.child_slots += o.child_slots; + self.both_slots += o.both_slots; self.key_bytes_used += o.key_bytes_used; self.at_cap += o.at_cap; + if self.klen_hist.len() < o.klen_hist.len() { self.klen_hist.resize(o.klen_hist.len(), 0); } + for (i, c) in o.klen_hist.iter().enumerate() { self.klen_hist[i] += c; } + if self.node_klen_hist.len() < o.node_klen_hist.len() { self.node_klen_hist.resize(o.node_klen_hist.len(), 0); } + for (i, c) in o.node_klen_hist.iter().enumerate() { self.node_klen_hist[i] += c; } + if self.allval_klen_hist.len() < o.allval_klen_hist.len() { self.allval_klen_hist.resize(o.allval_klen_hist.len(), 0); } + for (i, c) in o.allval_klen_hist.iter().enumerate() { self.allval_klen_hist[i] += c; } + self.allval_nodes += o.allval_nodes; + self.empty_nodes += o.empty_nodes; self.dangling_slots += o.dangling_slots; + self.dense_val_slots += o.dense_val_slots; + self + } + pub fn total_bytes(&self) -> usize { self.list_bytes + self.dense_bytes + self.cell_bytes } + pub fn report_list_slots(&self) { + let tot: usize = self.klen_hist.iter().sum(); + println!(" list slot key-length histogram ({} slots, {} value-slots, {} child-slots):", tot, self.val_slots, self.child_slots); + let mut acc = 0; + for (len, cnt) in self.klen_hist.iter().enumerate() { + if *cnt == 0 { continue } + acc += cnt; + println!(" len {:>2}: {:>9} ({:4.1}%) cum {:4.1}%", len, cnt, *cnt as f64/tot as f64*100.0, acc as f64/tot as f64*100.0); + } + println!(" nodes with both slots used: {} total key bytes used per node avg: {:.1} of {}", + self.both_slots, self.key_bytes_used as f64 / self.list_nodes.max(1) as f64, crate::line_list_node::KEY_BYTES_CNT); + let tn: usize = self.node_klen_hist.iter().sum(); + let mut cum = 0usize; let mut cum_av = 0usize; + println!(" per-NODE total key bytes (cumulative fit):"); + for (len, cnt) in self.node_klen_hist.iter().enumerate() { + cum += cnt; cum_av += self.allval_klen_hist[len]; + if [6usize,10,14,18,22,26,34,42,50,58,84].contains(&len) { + println!(" <= {:>2} bytes: {:5.1}% of all list nodes | {:5.1}% of the {} leaf-only nodes", + len, cum as f64/tn as f64*100.0, cum_av as f64/self.allval_nodes.max(1) as f64*100.0, self.allval_nodes); + } + } + println!(" leaf-only nodes (no child slot): {} of {} ({:4.1}%)", self.allval_nodes, self.list_nodes, self.allval_nodes as f64/self.list_nodes.max(1) as f64*100.0); + println!(" nodes at the key cap (key0+key1 >= {}): {}", crate::line_list_node::KEY_BYTES_CNT, self.at_cap); + } + pub fn report(&self, label: &str, vals: usize) { + let t = self.total_bytes() as f64; + println!("--- {label} ---"); + println!(" values {vals}"); + println!(" list nodes {:>9} bytes {:>11} ({:4.1}% of trie)", self.list_nodes, self.list_bytes, self.list_bytes as f64/t*100.0); + println!(" dense nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {} avg {:.1}/node", + self.dense_nodes, self.dense_bytes, self.dense_bytes as f64/t*100.0, self.dense_items, + self.dense_items as f64 / self.dense_nodes.max(1) as f64); + println!(" dense slots: len {} cap {} ({:.1}% over-allocated)", self.dense_items, self.dense_cap, (self.dense_cap as f64/self.dense_items.max(1) as f64 - 1.0)*100.0); + println!(" cell nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {}", self.cell_nodes, self.cell_bytes, self.cell_bytes as f64/t*100.0, self.cell_items); + println!(" empty (allocated) nodes {} dangling sentinel slots {}", self.empty_nodes, self.dangling_slots); + let vs = self.val_slots + self.dense_val_slots; + println!(" values by home: list-node slots {} ({:.2}%) dense-node slots {} ({:.2}%)", + self.val_slots, self.val_slots as f64 / vs.max(1) as f64 * 100.0, + self.dense_val_slots, self.dense_val_slots as f64 / vs.max(1) as f64 * 100.0); + println!(" TOTAL bytes {:>9} = {:.1} bytes/value", self.total_bytes(), t / vals.max(1) as f64); + } +} + +/// Builds a [MemProfile] for `map`. See the type docs +pub fn memory_profile(map: &PathMap) -> MemProfile { + use crate::trie_node::traverse_physical; + use crate::alloc::GlobalAlloc; + let cf = core::mem::size_of::>(); + let cellcf = core::mem::size_of::>(); + let list_sz = core::mem::size_of::>(); + let dense_sz = core::mem::size_of::>(); + let cell_sz = core::mem::size_of::>(); + let Some(root) = map.root() else { return MemProfile::default() }; + traverse_physical(root, move |node, ctx: MemProfile| { + let mut c = ctx; + if node.item_count() == 0 { c.empty_nodes += 1; } + if let Some(l) = node.as_list() { + c.list_nodes += 1; c.list_bytes += list_sz; + //A slot whose child link is the empty sentinel is a dangling path: the path exists but + //carries no value and leads nowhere + if l.is_used_child_0() && unsafe{ l.child_in_slot::<0>() }.is_empty() { c.dangling_slots += 1 } + if l.is_used_child_1() && unsafe{ l.child_in_slot::<1>() }.is_empty() { c.dangling_slots += 1 } + let (k0, k1) = l.get_both_keys(); + if c.klen_hist.len() < crate::line_list_node::KEY_BYTES_CNT + 1 { c.klen_hist.resize(crate::line_list_node::KEY_BYTES_CNT + 1, 0); } + c.klen_hist[k0.len()] += 1; + if k1.len() > 0 { c.klen_hist[k1.len()] += 1; c.both_slots += 1; } + c.key_bytes_used += k0.len() + k1.len(); + if k0.len() + k1.len() >= crate::line_list_node::KEY_BYTES_CNT { c.at_cap += 1; } + let ktot = k0.len() + k1.len(); + if c.node_klen_hist.len() < 2*crate::line_list_node::KEY_BYTES_CNT + 2 { c.node_klen_hist.resize(2*crate::line_list_node::KEY_BYTES_CNT + 2, 0); c.allval_klen_hist.resize(2*crate::line_list_node::KEY_BYTES_CNT + 2, 0); } + c.node_klen_hist[ktot] += 1; + let has_child = l.is_used_child_0() || l.is_used_child_1(); + if !has_child { c.allval_nodes += 1; c.allval_klen_hist[ktot] += 1; } + if l.is_used_value_0() { c.val_slots += 1 } else if l.is_used_child_0() { c.child_slots += 1 } + if l.is_used_value_1() { c.val_slots += 1 } else if l.is_used_child_1() { c.child_slots += 1 } + } + else if let Some(d) = node.as_dense() { + c.dense_nodes += 1; c.dense_items += d.slot_count(); c.dense_val_slots += d.val_slot_count(); c.dense_cap += d.slot_capacity(); c.dense_bytes += dense_sz + d.slot_capacity()*cf; + } else if node.tag() == crate::trie_node::CELL_BYTE_NODE_TAG { + let n = node.item_count(); + // each CellCoFree additionally owns a boxed OrdinaryCoFree + c.cell_nodes += 1; c.cell_items += n; c.cell_bytes += cell_sz + n*(cellcf + cf); + } + c + }, |a, b| a.merge(b)) +} + #[cfg(test)] mod tests { use super::{cow_counters, reset_cow_counters}; diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index c28c78cd..ab9a4d87 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -15,6 +15,8 @@ use crate::line_list_node::LineListNode; //NOTE: This: `core::array::from_fn(|i| i as u8);` ought to work, but https://github.com/rust-lang/rust/issues/109341 const ALL_BYTES: [u8; 256] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255]; + + /// A ByteNode with insides that **cannot** be shared across threads pub type DenseByteNode = ByteNode, A>; @@ -119,6 +121,16 @@ impl> ByteNode }; word_base + (mask_word & preceding_bits).count_ones() as usize } + /// Number of allocated `CoFree` slots, including unused `Vec` capacity + #[inline] + pub fn slot_capacity(&self) -> usize { self.values.capacity() } + /// Number of slots holding a value + #[inline] + pub fn val_slot_count(&self) -> usize { self.values.iter().filter(|cf| cf.has_val()).count() } + /// Number of occupied `CoFree` slots. Note this differs from [Self::item_count], which counts + /// a slot's child link and value as two separate items + #[inline] + pub fn slot_count(&self) -> usize { self.values.len() } #[inline] pub fn reserve_capacity(&mut self, additional: usize) { self.values.reserve(additional) @@ -1828,14 +1840,9 @@ impl, Other let (other_rec, other_val) = other.into_both(); let rec_status = match self.rec_mut() { Some(self_rec) => match other_rec { - Some(other_rec) => { - let (status, result) = self_rec.make_mut().join_into_dyn(other_rec); - match result { - Ok(()) => {}, - Err(replacement_node) => {*self_rec = replacement_node}, - } - status - }, + //NOTE: `TrieNodeODRc::join_into` is the same operation, and additionally short-circuits + // when both sides are the same shared subtrie + Some(other_rec) => self_rec.join_into(other_rec), None => AlgebraicStatus::Identity, }, None => match other_rec { @@ -2104,6 +2111,46 @@ impl, Other let mut is_identity = self.mask == jm; + //When `other` brings no new bytes the result mask is `self.mask`, so nothing has to be + //inserted and the slots can be joined where they sit. The general path below allocates a + //fresh slot array and moves every slot into it, which for an incremental ingest -- a trie + //joined with a batch of paths it mostly already covers -- is the entire cost of the call. + if jm == self.mask { + let mut is_identity = true; + let mut l = 0; + let mut r = 0; + for i in 0..4 { + let mut lm = self.mask.0[i]; + let om = other.mask.0[i]; + while lm != 0 { + let index = lm.trailing_zeros(); + if (om >> index) & 1 != 0 { + //SAFETY: `other`'s slots are all consumed here, because its mask is a + // subset of `self`'s, and its length is zeroed below so they are not + // dropped a second time + let rv = unsafe { std::ptr::read(other.values.get_unchecked(r)) }; + let lv = unsafe { self.values.get_unchecked_mut(l) }; + match lv.join_into(rv) { + AlgebraicStatus::Identity => { }, + AlgebraicStatus::Element => { is_identity = false; }, + AlgebraicStatus::None => unreachable!(), //Some.join(Some) shouldn't create None + } + r += 1; + } + l += 1; + lm ^= 1u64 << index; + } + } + unsafe { other.values.set_len(0) } + return if self.mask.is_empty_mask() { + AlgebraicStatus::None + } else if is_identity { + AlgebraicStatus::Identity + } else { + AlgebraicStatus::Element + } + } + let jmc = [jm.0[0].count_ones(), jm.0[1].count_ones(), jm.0[2].count_ones(), jm.0[3].count_ones()]; let l = (jmc[0] + jmc[1] + jmc[2] + jmc[3]) as usize; @@ -2289,8 +2336,11 @@ impl, Other // `other` be differently parameterized types impl> ByteNode { fn psubtract>(&self, other: &ByteNode) -> AlgebraicResult where Self: Sized { - let mut is_identity = true; - let mut btn = self.clone(); + //`btn` stays `None` for as long as the result is still identical to `self`, so a subtraction + //that removes nothing from this node -- the common case when the operands barely overlap -- + //never clones it. The eager version cloned every node it visited, including its whole slot + //array with a refcount bump per slot, and then discarded the lot on the `Identity` return. + let mut btn: Option = None; for i in 0..4 { let mut lm = self.mask.0[i]; @@ -2298,19 +2348,21 @@ impl { - is_identity = false; - btn.remove(64*(i as u8) + (index as u8)); + btn.get_or_insert_with(|| self.clone()).remove(byte); }, AlgebraicResult::Identity(mask) => { debug_assert_eq!(mask, SELF_IDENT); //subtract is non-commutative }, AlgebraicResult::Element(jv) => { - is_identity = false; - let dst = unsafe { btn.get_unchecked_mut(64*(i as u8) + (index as u8)) }; + //NOTE: the clone is taken at the first change, so `btn` still matches + //`self` everywhere the loop has already been, and indexing it by byte + //stays correct even after a `remove` has shifted its slots + let dst = unsafe { btn.get_or_insert_with(|| self.clone()).get_unchecked_mut(byte) }; *dst = jv; }, } @@ -2320,14 +2372,19 @@ impl if self.is_empty() { + AlgebraicResult::None + } else { AlgebraicResult::Identity(SELF_IDENT) + }, + Some(btn) => if btn.is_empty() { + AlgebraicResult::None } else { AlgebraicResult::Element(btn) - } + }, } } } diff --git a/src/lib.rs b/src/lib.rs index 8df862be..44df25da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -143,6 +143,20 @@ mod bridge_node; mod old_cursor; /// A supertrait that encapsulates the bounds for a value that can be put in a [PathMap] +/// +/// # Value size, and a cliff at 8 bytes +/// +/// Most values in a trie live in a list node, whose payload slot is a fixed-size cell shared with +/// the child pointer. A `V` that fits in that cell is stored inline; a larger one is boxed +/// individually, one heap allocation per value. The threshold is the size of a node pointer, so +/// **8 bytes** on 64-bit targets with the default `slim_ptrs` feature. +/// +/// The cliff is sharp and does not show up in the type system. On a trie of ~92k paths where 99.9% +/// of values sit in list-node slots, going from an 8-byte value to a 9-byte one costs about 19% on +/// insertion and 36% on drop, entirely in allocator traffic. +/// +/// If your value is close to the limit it is worth keeping it under: pack it, shrink an +/// enum's discriminant, or store an index into a side table rather than the payload itself. pub trait TrieValue: Clone + Send + Sync + Unpin {} impl TrieValue for T where T : Clone + Send + Sync + Unpin {} diff --git a/src/line_list_node.rs b/src/line_list_node.rs index eb907636..0dc1b7db 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -37,7 +37,7 @@ pub struct LineListNode { #[cfg(feature = "slim_ptrs")] pub(crate) const KEY_BYTES_CNT: usize = 42; #[cfg(not(feature = "slim_ptrs"))] -pub(crate) const KEY_BYTES_CNT: usize = 14; +pub(crate) const KEY_BYTES_CNT: usize = 42; // Only the slim_ptrs layout is asserted. The not(slim_ptrs) TrieNodeODRc has no // empty-sentinel representation yet (`new_empty`/`is_empty`/`make_unique`/`==` @@ -47,7 +47,9 @@ pub(crate) const KEY_BYTES_CNT: usize = 14; // asserted until the sentinel design lands. #[cfg(all(feature = "slim_ptrs", target_arch = "x86_64", not(miri)))] const _: [(); core::mem::size_of::>()] = - [(); 64]; + [(); LINE_LIST_NODE_SIZE]; +/// refcnt 4 + header 2 + two 8-byte payload slots + the key area, rounded up to 8-byte alignment +pub(crate) const LINE_LIST_NODE_SIZE: usize = ((4 + 2 + 16 + KEY_BYTES_CNT) + 7) / 8 * 8; const SLOT_0_USED_MASK: u16 = 1 << 15; const SLOT_1_USED_MASK: u16 = 1 << 14; @@ -60,28 +62,24 @@ impl Drop for LineListNode { // pathological paths are almost entirely non-branching. Therefore, we will invoke a recursive // drop function if the node branches, and an iterative drop if it doesn't - let slot0_used = self.is_used::<0>(); - let slot1_used = self.is_used::<1>(); - let slot0_child = self.is_child_ptr::<0>(); - let slot1_child = self.is_child_ptr::<1>(); + //NOTE: when `V` needs no drop work (`PathMap<()>`, and any small `Copy` value type), the + // value arms below fold away, and a node holding two values drops without touching them + let slot0_child = self.is_used_child_0(); + let slot1_child = self.is_used_child_1(); - if (slot0_used && slot0_child) != (slot1_used && slot1_child) { + if slot0_child != slot1_child { //If there is exactly one child, do the non-recursive drop list_node_iterative_drop(self); } else { - if slot0_used { - if slot0_child { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.child) } - } else { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.val) } - } + if slot0_child { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.child) } + } else if val_slot_needs_drop::() && self.is_used::<0>() { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.val) } } - if slot1_used { - if slot1_child { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.child) } - } else { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.val) } - } + if slot1_child { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.child) } + } else if val_slot_needs_drop::() && self.is_used::<1>() { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.val) } } } } @@ -117,7 +115,7 @@ fn list_node_take_child_to_drop(node: &mut let child1 = node.is_used_child_1(); match (child0, child1) { (true, false) => { - if node.is_used::<1>() { + if val_slot_needs_drop::() && node.is_used::<1>() { unsafe{ ManuallyDrop::drop(&mut node.val_or_child1.val) } } node.header = 0; @@ -129,7 +127,7 @@ fn list_node_take_child_to_drop(node: &mut } }, (false, true) => { - if node.is_used::<0>() { + if val_slot_needs_drop::() && node.is_used::<0>() { unsafe{ ManuallyDrop::drop(&mut node.val_or_child0.val) } } node.header = 0; @@ -414,7 +412,7 @@ impl LineListNode { } } #[inline] - unsafe fn child_in_slot(&self) -> &TrieNodeODRc { + pub(crate) unsafe fn child_in_slot(&self) -> &TrieNodeODRc { match SLOT { 0 => unsafe{ &*self.val_or_child0.child }, 1 => unsafe{ &*self.val_or_child1.child }, diff --git a/src/ring.rs b/src/ring.rs index 4b9b1d45..238f39b5 100644 --- a/src/ring.rs +++ b/src/ring.rs @@ -538,9 +538,16 @@ pub trait Lattice { /// `join_into(self) -> AlgebraicStatus::Identity`, /// `pmeet(self) -> AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT)`, /// - /// WARNING! This constant is currently informational only. The node-level and zipper - /// algebra implementations do not yet consult it, so changing it has no effect - /// on their behavior. It is planned for gating implementation shortcuts in the future + /// The node-level algebra consults this constant to decide whether an operation between two + /// references to the *same* shared subtrie can be answered without descending into it. Leaving + /// it at the `true` default is correct for every set-like or "last writer wins" value type, + /// including `()`. Set it to `false` for a value type whose join or meet actually combines the + /// operands, such as a multiset that adds multiplicities, otherwise structurally shared branches + /// will be silently skipped instead of combined. + /// + /// NOTE: this constant covers `pjoin`, `join_into` and `pmeet` together. A type that is + /// idempotent under one but not the other must declare `false`. Subtraction is governed + /// separately, by [DistributiveLattice::IDEMPOTENT]. const IDEMPOTENT: bool = true; /// Implements the union operation between two instances of a type in a partial lattice, resulting in @@ -619,9 +626,12 @@ pub trait DistributiveLattice { /// If `IDEMPOTENT = true` the implementor is asserting that: /// `psubtract(self) -> AlgebraicResult::None`, /// - /// WARNING! This constant is currently informational only. The node-level and zipper - /// algebra implementations do not yet consult it, so changing it has no effect - /// on their behavior. It is planned for gating implementation shortcuts in the future + /// The node-level algebra consults this constant to decide whether subtracting a shared subtrie + /// from itself can be answered as "nothing survives" without descending into it. Set it to + /// `false` for a value type where `x - x` leaves something behind, such as a multiset that + /// removes one occurrence per subtraction. + /// + /// See also [Lattice::IDEMPOTENT], which governs join and meet. const IDEMPOTENT: bool = true; /// Implements the partial subtract operation diff --git a/src/trie_map.rs b/src/trie_map.rs index 3343fa58..95203628 100644 --- a/src/trie_map.rs +++ b/src/trie_map.rs @@ -720,12 +720,7 @@ impl Lattice for PathMap let (other_root_node, other_root_val) = other.into_root(); let root_node_status = if let Some(other_root) = other_root_node { - let (status, result) = self.get_or_init_root_mut().make_mut().join_into_dyn(other_root); - match result { - Ok(()) => {}, - Err(replacement) => { *self.get_or_init_root_mut() = replacement; } - } - status + self.get_or_init_root_mut().join_into(other_root) } else { if self.is_empty() { AlgebraicStatus::None diff --git a/src/trie_node.rs b/src/trie_node.rs index 4cb2076e..92611939 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -484,12 +484,37 @@ impl ValOrChild { } } +/// The inline storage budget for a value held in a [ValOrChildUnion]. A `V` larger than this is +/// boxed by [LocalOrHeap], and therefore owns a heap allocation that must be freed +#[cfg(feature = "slim_ptrs")] +pub(crate) type ValSlotStorage = [u8; 8]; +#[cfg(not(feature = "slim_ptrs"))] +pub(crate) type ValSlotStorage = [u8; 16]; + +/// Returns `true` if the value in a [ValOrChildUnion] needs any drop work at all +/// +/// [LocalOrHeap] carries an unconditional `Drop` impl, so `needs_drop::>()` is +/// always `true` and tells us nothing. The slot only really needs dropping when `V` has drop glue +/// of its own, or when `V` is too big to store inline and therefore owns a heap allocation. +/// +/// This depends only on `V`, so it folds to a constant at monomorphization and the branches it +/// guards vanish. For a unit-valued trie (`PathMap<()>`), and for any other trie whose value is a +/// small `Copy` type, that removes the value arms from the node drop paths entirely. +/// `true` if a `V` is too large to live inline in a [ValOrChildUnion], and so is boxed individually +/// by [LocalOrHeap] in every list-node slot that holds one +#[inline(always)] +pub(crate) const fn val_is_boxed() -> bool { + core::mem::size_of::() > core::mem::size_of::() +} + +#[inline(always)] +pub(crate) const fn val_slot_needs_drop() -> bool { + core::mem::needs_drop::() || val_is_boxed::() +} + pub union ValOrChildUnion { pub child: ManuallyDrop>, - #[cfg(feature = "slim_ptrs")] - pub val: ManuallyDrop>, - #[cfg(not(feature = "slim_ptrs"))] - pub val: ManuallyDrop>, + pub val: ManuallyDrop>, pub _unused: () } @@ -1381,7 +1406,9 @@ mod tagged_node_ref { } pub fn pjoin_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.shared_node_id() == other.shared_node_id() { + //A node joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } match self { @@ -1394,7 +1421,8 @@ mod tagged_node_ref { } pub fn pmeet_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.shared_node_id() == other.shared_node_id() { + //See the note in `pjoin_dyn`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } match self { @@ -1407,7 +1435,9 @@ mod tagged_node_ref { } pub fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { - if self.shared_node_id() == other.shared_node_id() { + //A node subtracted from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::None; } match self { @@ -2016,7 +2046,9 @@ mod tagged_node_ref { } pub fn pjoin_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.ptr == other.ptr { + //A node joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -2031,7 +2063,8 @@ mod tagged_node_ref { } pub fn pmeet_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.ptr == other.ptr { + //See the note in `pjoin_dyn`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -2046,7 +2079,9 @@ mod tagged_node_ref { } pub fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { - if self.ptr == other.ptr { + //A node subtracted from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::None; } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -3098,7 +3133,9 @@ mod opaque_dyn_rc_trie_node { impl TrieNodeODRc { #[inline] pub fn pjoin(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //A shared subtrie joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { self.as_tagged().pjoin_dyn(other.as_tagged()) @@ -3115,6 +3152,12 @@ impl TrieNodeODRc { } #[inline] pub fn join_into(&mut self, node: TrieNodeODRc) -> AlgebraicStatus { + //Joining a shared subtrie into itself is a no-op when the value join is idempotent. This + // is worth checking up front because `make_mut` below deep-copies the node when it's shared, + // which is exactly the situation where `node` is likely to be the same pointer as `self` + if ::IDEMPOTENT && self.ptr_eq(&node) { + return AlgebraicStatus::Identity + } let (status, result) = self.make_mut().join_into_dyn(node); match result { Ok(()) => {}, @@ -3126,7 +3169,8 @@ impl TrieNodeODRc { } #[inline] pub fn pmeet(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //See the note in `pjoin`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { self.as_tagged().pmeet_dyn(other.as_tagged()) @@ -3137,7 +3181,9 @@ impl TrieNodeODRc { //See above, pseudo-impl for [DistributiveLattice] trait impl TrieNodeODRc { pub fn psubtract(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //Subtracting a shared subtrie from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::None } else { self.as_tagged().psubtract_dyn(other.as_tagged()) @@ -3247,6 +3293,93 @@ impl DistributiveLat } } +#[cfg(test)] +mod payload_layout_tests { + use super::*; + use crate::alloc::GlobalAlloc; + + /// [ValOrChild] is already as small as it can be, courtesy of the niche in the child pointer + /// + /// `Val(())` carries nothing, so rustc encodes it in the child pointer's null niche and the + /// whole enum is one word -- no discriminant, no padding. Adding a third variant, or any field + /// that defeats the niche, would silently double it for every unit-valued trie. + #[test] + fn val_or_child_is_niche_packed_for_a_unit_value() { + assert_eq!( + core::mem::size_of::>(), + core::mem::size_of::>(), + "ValOrChild<()> should fit in the child pointer's niche" + ); + } + + /// [PayloadRef] cannot use that niche: it has *two* pointer-carrying variants plus `None`, so + /// one word goes to the discriminant regardless of `V`. It is a transient -- built on the + /// stack during meets and never stored in a trie -- so its width costs memory nowhere. + #[test] + fn payload_ref_costs_a_discriminant_word_for_every_value_type() { + let w = core::mem::size_of::<&'static TrieNodeODRc<(), GlobalAlloc>>(); + assert_eq!(core::mem::size_of::>(), 2 * w); + assert_eq!(core::mem::size_of::>(), 2 * w); + } +} + +#[cfg(test)] +mod val_slot_layout_tests { + use super::*; + use crate::alloc::GlobalAlloc; + + /// The payload union is the width of its *child* arm, not its value arm + /// + /// [LocalOrHeap] is a fixed-size cell -- it boxes any `V` too big to fit -- so the value arm is + /// the same width for every `V`, and the union is 8 bytes whether `V` is `()` or `[u8; 1024]`. + /// Removing the value arm entirely would not shrink it, because the child pointer needs those 8 + /// bytes regardless. + #[test] + fn payload_union_width_is_set_by_the_child_pointer() { + let child = core::mem::size_of::>(); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + } + + /// Values larger than [ValSlotStorage] are boxed one at a time in list-node slots. This is a + /// sharp, silent cliff -- see the note on [`TrieValue`](crate::TrieValue) + #[test] + fn the_inline_value_budget_is_where_it_is_documented() { + assert!(core::mem::size_of::() >= 8); + assert!(!crate::trie_node::val_is_boxed::()); + assert!(crate::trie_node::val_is_boxed::<[u8; 16]>()); + assert!(!crate::trie_node::val_is_boxed::<()>()); + } +} + +#[cfg(test)] +mod val_slot_drop_tests { + use super::{ValSlotStorage, val_slot_needs_drop}; + + /// The whole point of [val_slot_needs_drop] is that it is `false` for the value types whose + /// drop work the node paths can skip. If these ever flip to `true` the elision silently stops + /// happening, so pin them down. + #[test] + fn val_slot_needs_drop_is_false_for_trivial_values() { + assert!(!val_slot_needs_drop::<()>()); + assert!(!val_slot_needs_drop::()); + assert!(!val_slot_needs_drop::()); + assert!(!val_slot_needs_drop::<[u8; 4]>()); + } + + /// A value with drop glue always needs the drop, and so does one too big to live inline, + /// because [local_or_heap::LocalOrHeap] boxes it + #[test] + fn val_slot_needs_drop_is_true_when_there_is_work_to_do() { + assert!(val_slot_needs_drop::>()); //drop glue + assert!(val_slot_needs_drop::()); //drop glue + assert!(val_slot_needs_drop::<[u8; 64]>()); //no drop glue, but heap-allocated + assert!(core::mem::size_of::<[u8; 64]>() > core::mem::size_of::()); + } +} + /// Test to make sure slim_ptrs are good with provenance under miri #[cfg(test)] mod tests { diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 5b233b83..96067714 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1743,11 +1743,8 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC Some(src) => { match self.take_focus(false) { Some(mut self_node) => { - let (status, result) = self_node.make_mut().join_into_dyn(src); - match result { - Ok(()) => self.graft_internal(Some(self_node)), - Err(replacement_node) => self.graft_internal(Some(replacement_node)), - } + let status = self_node.join_into(src); + self.graft_internal(Some(self_node)); status }, None => { diff --git a/tests/idempotent_lattice.rs b/tests/idempotent_lattice.rs new file mode 100644 index 00000000..2957b0fd --- /dev/null +++ b/tests/idempotent_lattice.rs @@ -0,0 +1,138 @@ +//! Tests that [`Lattice::IDEMPOTENT`] and [`DistributiveLattice::IDEMPOTENT`] are honored by the +//! node-level algebra. +//! +//! The trie shares subtries by pointer, so an algebraic operation frequently ends up with the same +//! node on both sides. For a set-like value (including `()`) the answer can be produced without +//! descending, but for a value type that actually *combines* its operands that shortcut would skip +//! the shared branch instead of combining it. These tests use such a value type. + +use pathmap::PathMap; +use pathmap::ring::*; +use pathmap::zipper::*; + +/// A multiplicity in a multiset. Joining adds occurrences, so joining a subtrie with itself is +/// emphatically not the same as leaving it alone. +#[derive(Clone, Debug, PartialEq, Eq)] +struct Count(u64); + +impl Lattice for Count { + const IDEMPOTENT: bool = false; + fn pjoin(&self, other: &Self) -> AlgebraicResult { + AlgebraicResult::Element(Count(self.0 + other.0)) + } + fn pmeet(&self, other: &Self) -> AlgebraicResult { + AlgebraicResult::Element(Count(self.0.min(other.0))) + } +} + +impl DistributiveLattice for Count { + const IDEMPOTENT: bool = false; + /// Removes a single occurrence, whatever `other` holds + fn psubtract(&self, _other: &Self) -> AlgebraicResult { + if self.0 > 1 { + AlgebraicResult::Element(Count(self.0 - 1)) + } else { + AlgebraicResult::None + } + } +} + +const PATHS: &[&[u8]] = &[b"aaa", b"aab", b"abc", b"bbb", b"bbcd"]; + +fn counted(n: u64) -> PathMap { + let mut map = PathMap::new(); + for path in PATHS { + map.set_val_at(path, Count(n)); + } + map +} + +/// Guards the premise of every test below: cloning a `PathMap` shares the root node by pointer, so +/// the operations really do see the same node on both sides. If this ever stops being true the +/// tests below would still pass while testing nothing. +#[track_caller] +fn assert_shares_root(a: &PathMap, b: &PathMap) { + let (za, zb) = (a.read_zipper(), b.read_zipper()); + assert!(za.is_shared() && zb.is_shared(), "test premise broken: root node is not shared"); + assert_eq!(za.shared_node_id(), zb.shared_node_id(), "test premise broken: roots are different nodes"); +} + +#[test] +fn non_idempotent_pjoin_descends_shared_subtries() { + let a = counted(1); + let b = a.clone(); + assert_shares_root(&a, &b); + + let joined = a.join(&b); + for path in PATHS { + assert_eq!(joined.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_join_into_descends_shared_subtries() { + let mut a = counted(1); + let b = a.clone(); + assert_shares_root(&a, &b); + + a.join_into(b); + for path in PATHS { + assert_eq!(a.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_write_zipper_join_into_descends_shared_subtries() { + let mut dst = counted(1); + let src = dst.clone(); + assert_shares_root(&dst, &src); + + let mut wz = dst.write_zipper(); + wz.join_into(&src.read_zipper()); + drop(wz); + + for path in PATHS { + assert_eq!(dst.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_psubtract_descends_shared_subtries() { + let a = counted(3); + let b = a.clone(); + assert_shares_root(&a, &b); + + //One occurrence comes off each path; the paths survive + let once = a.subtract(&b); + for path in PATHS { + assert_eq!(once.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } + + //Repeat until the multiplicities run out, at which point the paths do go away + let twice = once.subtract(&a); + let thrice = twice.subtract(&a); + for path in PATHS { + assert_eq!(twice.get_val_at(path), Some(&Count(1)), "at {path:?}"); + assert_eq!(thrice.get_val_at(path), None, "at {path:?}"); + } + assert!(thrice.is_empty()); +} + +#[test] +fn idempotent_default_still_short_circuits_correctly() { + //`()` and other set-like values keep the default `IDEMPOTENT = true`, and joining or + //subtracting a shared subtrie against itself must still give set semantics + let a: PathMap<()> = PATHS.iter().copied().collect(); + let b = a.clone(); + + let joined = a.join(&b); + assert_eq!(joined.val_count(), PATHS.len()); + for path in PATHS { + assert_eq!(joined.get_val_at(path), Some(&()), "at {path:?}"); + } + + let met = a.meet(&b); + assert_eq!(met.val_count(), PATHS.len()); + + assert!(a.subtract(&b).is_empty()); +} diff --git a/tests/memory_profile.rs b/tests/memory_profile.rs new file mode 100644 index 00000000..ef6af91b --- /dev/null +++ b/tests/memory_profile.rs @@ -0,0 +1,99 @@ +#![cfg(all(feature = "counters", feature = "serialization"))] + +use std::time::Instant; +use pathmap::PathMap; +use pathmap::counters::memory_profile; +use pathmap::zipper::{ZipperMoving, ZipperIteration}; + +fn xorshift_keys(seed: u64, n: u32) -> Vec<[u8; 8]> { + let mut x = seed; + (0..n).map(|_| { x ^= x << 13; x ^= x >> 7; x ^= x << 17; x.to_be_bytes() }).collect() +} +fn timeit(reps: usize, mut f: impl FnMut() -> f64) -> f64 { + let mut b = f64::MAX; for _ in 0..reps { let t = f(); if t < b { b = t } } b +} +fn survey(label: &str, paths: &[Vec]) { + let build_ms = timeit(3, || { + let t = Instant::now(); + let mut m: PathMap<()> = PathMap::new(); + for p in paths { m.set_val_at(&p[..], ()); } + let e = t.elapsed().as_secs_f64()*1e3; drop(m); e + }); + let mut m: PathMap<()> = PathMap::new(); + for p in paths { m.set_val_at(&p[..], ()); } + let iter_ms = timeit(3, || { + let t = Instant::now(); + let mut z = m.read_zipper(); let mut n = 0u64; + while z.to_next_val() { n += 1 } + let e = t.elapsed().as_secs_f64()*1e3; assert!(n > 0); e + }); + let get_ms = timeit(3, || { + let t = Instant::now(); + let mut hits = 0u64; + for p in paths { if m.get_val_at(&p[..]).is_some() { hits += 1 } } + let e = t.elapsed().as_secs_f64()*1e3; assert!(hits > 0); e + }); + let prof = memory_profile(&m); + prof.report(label, m.val_count()); + println!("SWEEP\t{}\tK={}\tnodesz={}\tbytes={}\tlist_nodes={}\tdense_nodes={}\tbuild={:.1}\titer={:.1}\tget={:.1}", + label, pathmap::counters::LIST_NODE_KEY_BYTES, pathmap::counters::LIST_NODE_SIZE, + prof.total_bytes(), prof.list_nodes, prof.dense_nodes, build_ms, iter_ms, get_ms); +} + +/// Dangling paths: paths that exist but carry no value. `remove_val(prune=false)` is how they are +/// made, and a `LineListNode` records one by pointing the slot at the empty-node sentinel. +#[test] +fn dangling_path_survey() { + use pathmap::zipper::ZipperWriting; + let words: Vec> = std::fs::read_to_string("benches/shakespeare.txt").unwrap() + .split_ascii_whitespace().map(|w| w.as_bytes().to_vec()).collect::>() + .into_iter().collect(); + + let mut m: PathMap<()> = PathMap::new(); + for w in &words { m.set_val_at(&w[..], ()); } + let before = memory_profile(&m); + before.report("shakespeare, no dangling paths", m.val_count()); + + // strip every other value without pruning -- the maximal dangling-path case + for (i, w) in words.iter().enumerate() { + if i % 2 == 0 { + let mut wz = m.write_zipper_at_path(&w[..]); + wz.remove_val(false); + } + } + let after = memory_profile(&m); + after.report("shakespeare, half the values removed with prune=false", m.val_count()); + println!("DANGLING bytes {} -> {} empty nodes {} -> {} sentinel slots {} -> {}", + before.total_bytes(), after.total_bytes(), + before.empty_nodes, after.empty_nodes, before.dangling_slots, after.dangling_slots); + + //A dangling path costs nothing over the value it replaced. The empty-node sentinel is a bogus + //address rather than an allocation, and the payload word it sits in is part of a fixed-size + //`LineListNode` whether it is used or not -- so turning 14k values into dangling paths moves + //the byte count not at all. A `DenseByteNode` does not use the sentinel in the first place: it + //represents a dangling path as a CoFree holding neither a child nor a value. + assert!(after.dangling_slots > 10_000, "the fixture should have produced plenty of dangling paths"); + assert_eq!(after.total_bytes(), before.total_bytes(), + "dangling paths must cost nothing over the values they replaced"); + assert_eq!(after.empty_nodes, 0, "the sentinel must never become an allocated node"); + assert_eq!(before.empty_nodes, 0); +} + +#[test] +fn memory_profile_survey() { + // MORK-representative + let mut m: PathMap<()> = PathMap::new(); + let f = std::fs::File::open("benches/big_logic.metta.paths").unwrap(); + pathmap::paths_serialization::deserialize_paths(m.write_zipper(), f, ()).unwrap(); + let mork: Vec> = { let mut v = vec![]; let mut z = m.read_zipper(); + while z.to_next_val() { v.push(z.path().to_vec()) } v }; + survey("mork_big_logic", &mork); + + let rnd: Vec> = xorshift_keys(0x243F6A8885A308D3, 1_000_000).iter().map(|k| k.to_vec()).collect(); + survey("random_8byte", &rnd); + + let text = std::fs::read_to_string("benches/shakespeare.txt").unwrap(); + let words: Vec> = { let mut v: Vec> = text.split_ascii_whitespace().map(|w| w.as_bytes().to_vec()).collect(); + v.sort(); v.dedup(); v }; + survey("shakespeare", &words); +} diff --git a/tests/prefix_value_preservation.rs b/tests/prefix_value_preservation.rs new file mode 100644 index 00000000..c61aaf13 --- /dev/null +++ b/tests/prefix_value_preservation.rs @@ -0,0 +1,137 @@ +//! Tests that a value on a path which is a *proper prefix* of other paths survives structural +//! mutation of the trie. +//! +//! These slots -- the ones holding both a value and a child link -- are where value loss hides. A +//! slot with only one of the two cannot lose anything, so bugs in copy-on-write, node upgrades and +//! grafting show up here and nowhere else. The suite was written while packing the value-presence +//! flag into the child pointer (see the `experiment/cofree-pointer-packing` branch), which broke +//! six existing tests and one case no existing test covered; it is kept on the main line because +//! the property it checks is worth pinning down whatever the representation. + +use pathmap::PathMap; +use pathmap::ring::{Lattice, DistributiveLattice}; +use pathmap::zipper::{ZipperMoving, ZipperWriting, ZipperValues, ZipperIteration}; + +/// Paths chosen so that many are proper prefixes of others, and so the trie is wide enough at the +/// root to force dense nodes +fn prefix_heavy() -> Vec> { + let mut paths = vec![]; + for b in 0u8..64 { + paths.push(vec![b]); + paths.push(vec![b, b]); + paths.push(vec![b, b, b]); + paths.push(vec![b, b, b, 0]); + } + paths +} + +fn build(paths: &[Vec]) -> PathMap { + let mut m = PathMap::new(); + for (i, p) in paths.iter().enumerate() { m.set_val_at(&p[..], i as u64); } + m +} + +#[track_caller] +fn assert_intact(m: &PathMap, paths: &[Vec], extra: usize, what: &str) { + for (i, p) in paths.iter().enumerate() { + assert_eq!(m.get_val_at(&p[..]), Some(&(i as u64)), "{what}: lost the value at {p:?}"); + } + assert_eq!(m.val_count(), paths.len() + extra, "{what}: value count changed"); +} + +#[test] +fn values_survive_copy_on_write_of_a_shared_trie() { + let paths = prefix_heavy(); + let original = build(&paths); + + //Aliasing the trie means every write below has to clone the nodes on its path, replacing + //child links in slots that also hold values + let alias = original.clone(); + let mut copy = original.clone(); + for b in 0u8..64 { + copy.set_val_at(&[b, b, b, 1][..], 9999); + } + + assert_intact(&alias, &paths, 0, "aliased handle"); + assert_intact(&original, &paths, 0, "original"); + for b in 0u8..64 { + assert_eq!(copy.get_val_at(&[b, b, b, 1][..]), Some(&9999)); + } +} + +#[test] +fn values_survive_node_upgrades_under_a_write_zipper() { + let paths = prefix_heavy(); + let mut m = build(&paths); + + //Fanning out under an existing prefix forces list nodes to grow into dense nodes while their + //slots still hold values + { + let mut wz = m.write_zipper_at_path(b"\x00"); + //bytes 64.. are untouched by the fixture, so nothing existing is overwritten + for b in 64u8..=255 { wz.descend_to(&[b][..]); wz.set_val(7); wz.reset(); } + } + for b in 64u8..=255 { + assert_eq!(m.get_val_at(&[0u8, b][..]), Some(&7), "the newly written value at [0,{b}] is there"); + } + assert_intact(&m, &paths, 192, "after node upgrades"); +} + +#[test] +fn values_survive_grafting_over_a_slot_that_has_one() { + let paths = prefix_heavy(); + let mut m = build(&paths); + let donor: PathMap = build(&[b"zz".to_vec(), b"zzz".to_vec()]); + + //Grafting replaces the child link of a slot that already holds a value + { + let mut wz = m.write_zipper_at_path(b"\x01\x01"); + wz.graft(&donor.read_zipper()); + } + //NOTE: with the default `graft_root_vals` feature the value *at* the focus is part of the + //graft, so [1,1] takes the donor's (absent) root value. The value above it must be untouched. + assert_eq!(m.get_val_at(&[1u8][..]), Some(&4), "the value above the graft point survived"); + assert!(m.get_val_at(&[1u8, 1, b'z', b'z'][..]).is_some(), "the grafted subtrie is present"); + //and every other prefix value in the trie is unaffected + for b in 2u8..64 { + assert_eq!(m.get_val_at(&[b][..]), Some(&((b as u64)*4)), "value at [{b}]"); + assert_eq!(m.get_val_at(&[b, b][..]), Some(&((b as u64)*4 + 1)), "value at [{b},{b}]"); + } +} + +#[test] +fn values_survive_algebra_on_prefix_heavy_tries() { + let paths = prefix_heavy(); + let a = build(&paths); + let b = build(&paths); + + let joined = a.join(&b); + assert_intact(&joined, &paths, 0, "join"); + let met = a.meet(&b); + assert_eq!(met.val_count(), paths.len(), "meet"); + assert!(a.subtract(&b).is_empty(), "subtract of equals"); + + //And the values must still be reachable by iteration, not just by lookup + let mut z = joined.read_zipper(); + let mut seen = 0; + while z.to_next_val() { assert!(z.val().is_some()); seen += 1; } + assert_eq!(seen, paths.len()); +} + +#[test] +fn a_slot_with_both_value_and_child_round_trips_through_removal() { + let paths = prefix_heavy(); + let mut m = build(&paths); + + //Removing the value from a slot that also has a child must leave the child alone, and vice versa + for b in 0u8..64 { + let path = [b]; + let mut wz = m.write_zipper_at_path(&path[..]); + assert!(wz.remove_val(false).is_some(), "value at [{b}] was already gone"); + } + for b in 0u8..64 { + assert_eq!(m.get_val_at(&[b][..]), None); + assert_eq!(m.get_val_at(&[b, b][..]), Some(&((b as u64)*4 + 1)), "child subtrie of [{b}] survived"); + assert_eq!(m.get_val_at(&[b, b, b, 0][..]), Some(&((b as u64)*4 + 3))); + } +} diff --git a/tests/value_drop_elision.rs b/tests/value_drop_elision.rs new file mode 100644 index 00000000..bcb9a98e --- /dev/null +++ b/tests/value_drop_elision.rs @@ -0,0 +1,134 @@ +//! Tests that skipping the value-drop arms in the node drop paths (for value types that need no +//! drop work) doesn't skip drops for value types that do. +//! +//! Two cases matter, and they are distinguished by different halves of the predicate: +//! * a value with drop glue of its own +//! * a value with *no* drop glue that is nonetheless too large to live inline in a node slot, and +//! is therefore boxed on the heap. Nothing here can observe that leak directly; run this under +//! `cargo miri test` to check it. + +use std::sync::atomic::{AtomicIsize, Ordering::SeqCst}; + +use pathmap::PathMap; +use pathmap::zipper::{ZipperMoving, ZipperWriting}; + +static LIVE: AtomicIsize = AtomicIsize::new(0); + +/// A value that keeps count of how many instances are alive +#[derive(Debug)] +struct Tracked(#[allow(dead_code)] u64); + +impl Tracked { + fn new(n: u64) -> Self { + LIVE.fetch_add(1, SeqCst); + Self(n) + } +} +impl Clone for Tracked { + fn clone(&self) -> Self { + Self::new(self.0) + } +} +impl Drop for Tracked { + fn drop(&mut self) { + LIVE.fetch_sub(1, SeqCst); + } +} + +/// No drop glue, but 64 bytes is far past what a node slot stores inline, so it is heap-allocated +#[derive(Clone, Copy, PartialEq, Debug)] +struct Big([u8; 64]); + +/// Paths chosen to produce a mix of node shapes: leaves holding one and two values, values sitting +/// above branches, and a fan-out wide enough to force a dense node +fn paths() -> Vec> { + let mut paths: Vec> = vec![ + b"a".to_vec(), + b"ab".to_vec(), + b"abc".to_vec(), + b"abd".to_vec(), + b"az".to_vec(), + b"a-very-long-path-that-will-not-fit-inside-a-single-list-node-key".to_vec(), + ]; + for byte in 0u8..=255 { + paths.push(vec![b'w', byte]); + } + paths +} + +#[test] +fn values_with_drop_glue_are_still_dropped() { + assert_eq!(LIVE.load(SeqCst), 0, "another test leaked into this one"); + + let mut map: PathMap = PathMap::new(); + for (i, path) in paths().iter().enumerate() { + map.set_val_at(path, Tracked::new(i as u64)); + } + assert!(LIVE.load(SeqCst) > 0); + + //Exercise the paths that drop a value without dropping the whole node + let mut wz = map.write_zipper(); + wz.descend_to(b"abc"); + assert!(wz.remove_val(false).is_some()); + wz.reset(); + wz.descend_to(b"abd"); + assert!(wz.remove_val(true).is_some()); + wz.reset(); + wz.descend_to(b"w"); + assert!(wz.remove_branches(true)); + drop(wz); + + //And a clone, so the node drop paths run against a shared trie too + let copy = map.clone(); + drop(map); + drop(copy); + + assert_eq!(LIVE.load(SeqCst), 0, "values were leaked"); +} + +#[test] +fn large_values_without_drop_glue_round_trip() { + let mut map: PathMap = PathMap::new(); + for (i, path) in paths().iter().enumerate() { + map.set_val_at(path, Big([i as u8; 64])); + } + for (i, path) in paths().iter().enumerate() { + assert_eq!(map.get_val_at(path), Some(&Big([i as u8; 64])), "at {path:?}"); + } + + let copy = map.clone(); + drop(map); + for (i, path) in paths().iter().enumerate() { + assert_eq!(copy.get_val_at(path), Some(&Big([i as u8; 64])), "at {path:?}"); + } + //The heap allocations behind these values are freed here; miri checks it + drop(copy); +} + +#[test] +fn trivial_values_round_trip_with_the_drop_arms_elided() { + let all = paths(); + + let mut unit: PathMap<()> = PathMap::new(); + let mut small: PathMap = PathMap::new(); + for (i, path) in all.iter().enumerate() { + unit.set_val_at(path, ()); + small.set_val_at(path, i as u32); + } + + assert_eq!(unit.val_count(), all.len()); + assert_eq!(small.val_count(), all.len()); + for (i, path) in all.iter().enumerate() { + assert_eq!(unit.get_val_at(path), Some(&()), "at {path:?}"); + assert_eq!(small.get_val_at(path), Some(&(i as u32)), "at {path:?}"); + } + + //Removing values leaves the paths dangling when `prune` is false; the node still has to be + //dropped correctly afterwards + let mut wz = small.write_zipper(); + wz.descend_to(b"abc"); + assert_eq!(wz.remove_val(false), Some(2)); + drop(wz); + assert!(small.path_exists_at(b"abc")); + assert_eq!(small.get_val_at(b"abc"), None); +}