WIP: Unit value optimizations - #67
Open
imlvts wants to merge 17 commits into
Open
Conversation
A LineListNode payload slot holds either a child pointer or a value, and the drop paths tested which one it was and 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 run, and the elision was left entirely to the optimizer. The obvious guard, needs_drop::<V>(), would be wrong here. The slot is a LocalOrHeap<V, _>, 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. val_slot_needs_drop::<V>() covers both halves, and ValSlotStorage is now the type alias the union itself uses so the size threshold cannot drift away from the layout. This is a code-size and clarity change, not a throughput one: build-and-drop of a 1M-path PathMap<()> measures ~189ms either way in release, because the cost there is allocator traffic and cache misses, not the folded branches. The value is that the elision is explicit and holds in debug too. Tests cover both halves of the predicate: a drop-counting value asserting no leaks after removes, prunes and a shared-clone drop, and a 64-byte value with no drop glue whose heap allocations only miri can check. Unit tests pin the predicate's value for (), u32, Vec<u8> and [u8; 64] so the elision cannot stop happening silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…_into The trie shares subtries by pointer, so an algebraic operation frequently ends up with the same node on both sides. Join and meet answered that case as Identity and subtract answered it as None, without descending -- which is only sound when the value operation is idempotent. Both IDEMPOTENT constants documented themselves as informational only, so a value type that actually combines its operands, such as a multiset adding multiplicities, would have had its structurally shared branches silently skipped instead of combined. Six sites take the shortcut, not the three that grep for ptr_eq finds: the outer TrieNodeODRc methods, and the TaggedNodeRef dispatchers underneath them, which is where it fires at every level of the descent. There are two copies of the dispatchers, one per slim_dispatch setting. All six now consult the constant. Also adds the shortcut that was missing. TrieNodeODRc::join_into had no pointer check at all and went straight to make_mut(), which deep-copies the node exactly 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. Joining a 1M-path PathMap<()> into a clone of itself goes from ~110ms to effectively free. No in-tree value type declares IDEMPOTENT = false, so no existing behavior changes. The new tests use a multiset Count whose join adds occurrences and whose subtract removes one; all four of them failed before this change. A helper asserts the two maps genuinely share a root node, so the tests cannot quietly stop exercising the shortcut. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Layout work needs to know where the bytes actually are, and node counts alone
do not answer that. `memory_profile` walks physical (deduplicated) nodes and
tallies bytes for list nodes, dense nodes and cell nodes separately, along with
the list-node key-length distribution and the dense-node slot capacity.
Two things it immediately showed, both of which contradict estimates made from
reading the code:
- The split between list-node and dense-node bytes swings from 80/20 to
53/47 depending on key shape, so a per-node saving in either one means
very different things for different workloads.
- `ByteNode::item_count` counts a slot's child link and value as two separate
items, so it is not the slot count. Sizing the slot array from it is wrong
in both directions; `slot_count` and `slot_capacity` are what that needs.
`LINE_LIST_NODE_SIZE` now derives the list node's size from KEY_BYTES_CNT
rather than asserting a hardcoded 64, so the constant can be swept without
hand-editing the assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds to the perf notes: where the bytes actually are by node type, the dense slot-array over-allocation, and three things that were measured and rejected -- the S3 key-byte reclaim (not implementable, and worthless anyway), the KEY_BYTES_CNT sweep (no single constant serves both long-path and short-key workloads), and bounded growth for dense slot arrays (pays for its memory in build time). Also sizes S1 properly against allocated rather than counted slots, and records why its implementation is harder than the struct definition suggests: CoFree values are cloned and dropped outside the node that owns them, so moving the presence bit out costs a manual Drop/Clone and a parallel mask through all four algebraic operations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The packing works and is miri-clean, and the memory it saves is real: -5.1% on MORK-shaped data, -13.0% on random keys, -14.1% on shakespeare, landing within a tenth of a percent of what the profiler predicted. But it measures ~9% slower on iteration over the MORK dataset, and 5% memory is not worth risking 9% iteration on the workload that matters. The likely cause is `has_rec()` going from an `Option` null check to a pointer-tag comparison, on a hot path, in tries that are list-node-heavy and so collect little of the dense-node win. The code moves to `experiment/cofree-pointer-packing`, kept to show the approach is viable and to record what it costs. The notes keep the full result, the invariant it introduces, and the two places it can be silently defeated. The regression tests come to the main line as `tests/prefix_value_preservation.rs`. They cover values on paths that are proper prefixes of other paths -- the slots that hold both a value and a child, which is where value loss hides -- across copy-on-write, node upgrades, grafting, algebra and removal. That property is worth pinning down whatever the representation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The survey claimed a path kept alive by `remove_val(prune = false)` costs a
pointer and a node, because a `LineListNode` records it by pointing the slot's
child link at the empty-node sentinel. Measured, it costs nothing:
- `new_empty()` is a bogus address carrying EMPTY_NODE_TAG. No allocation, no
node.
- The payload word it occupies is part of a fixed-size struct, so marking the
slot "dangling" in the header instead would free zero bytes.
- A `DenseByteNode` never used the sentinel at all -- it already represents a
dangling path as a CoFree holding neither child nor value, which is exactly
what S4 proposed building.
Removing half of shakespeare's values without pruning produces 14,212 dangling
slots and leaves the trie byte for byte identical, 4,095,744 both ways.
`dangling_path_survey` asserts that, so the claim does not have to be re-derived
from the struct definitions that got it wrong the first time.
Also checked the one caller that really does allocate: a WriteZipper opened on
an absent path allocates an empty LineListNode for its root but does not leave
it behind. 2000 such zippers, created and dropped without writing, moved the
byte count by exactly zero, and no allocated empty node appears in any trie
profiled here.
The profiler gains `empty_nodes` and `dangling_slots` counters, which is what
made this decidable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The survey said merkleize "requires V: Hash to hash nothing", implying a const
branch around the value hash. Both hash sites take `Option<&V>`, not `V`, and
that distinction is the whole story:
- `().hash()` writes zero bytes, so the payload already costs nothing and the
call folds away. There is no branch to add.
- `Option<&()>` writes 8 bytes, and None and Some(&()) hash differently. That
is the discriminant, and for a set trie it is the only information present --
whether the path is a member. Eliding it would make a path with a value hash
identically to one without, and merkleize would merge structurally distinct
tries.
The V: Hash bound cannot come off either: it is needed for a general V, and ()
satisfies it for free.
The one adjacent remnant -- the "value, no child" branch building a fresh hasher
per leaf to compute what is a constant for any zero-sized V -- was priced at
3.03ns x 91,692 leaves = 0.28ms against a 21.03ms merkleize, or 1.3%. That is
below the noise floor, in the routine where a subtle change silently corrupts
structural sharing.
No code change; recording the measurements so this is not re-attempted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The survey said `LocalOrHeap<V, [u8; 8]>` is why a payload slot is 8 bytes wide. It is not. `LocalOrHeap` is a fixed-size cell that boxes anything too large 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]` -- because the child pointer arm needs 8 bytes regardless. Deleting the value arm would save nothing, and for `()` it already costs nothing at runtime. `val_slot_layout_tests` asserts both. What the investigation did turn up is real, though it is not a unit-value concern. Values live overwhelmingly in list-node slots -- 99.93% on MORK-shaped data, 100% on fixed-width random keys -- and those are exactly the slots that box. So for a value over the threshold, one heap allocation per value. Measured on 91,692 paths, going from an 8-byte value to a 9-byte one costs +19% on insertion and +36% on drop, entirely allocator traffic. Widening the cell would fix that only by growing every node for every V, including the ones that need none of it, and sizing it per-V needs generic_const_exprs. So the fix here is documentation: TrieValue now describes the cliff, since it is invisible in the type system and lands on whoever picks a value type. `val_is_boxed` gives the threshold one home, which `val_slot_needs_drop` now shares. The profiler gains a `dense_val_slots` counter, which is what made the list-vs-dense split measurable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The survey said the generic result plumbing -- AlgebraicResult, merge, combine_algebraic_results, the Hetero* traits -- is where a PathMap<()> spends its time. Profiled with perf on 400k-path joins, meets and subtracts, with cycles attributed by source line so inlined code lands on its own line, ring.rs accounts for 1.06% and combine_algebraic_results for 2.86%, against 29.72% in malloc. The proposed rewrite would have chased a twenty-fifth of the runtime, and depended on the shelved S1 besides. What the profile did find is worth more than F4 was. ByteNode::pjoin allocates the result slot vector and clones every slot into it before knowing whether the result is a new node or simply one of its operands. When the answer is Identity that work is discarded: on 400k-path joins, 99.7% of calls discard it when one operand is a subset of the other, and 100% at 95% overlap, wasting 94.8% and 99.6% of all slot clones respectively -- each an atomic refcount increment and a matching decrement, plus a Vec allocation and free per call. End to end this inverts the expected ordering: joining two disjoint 400k tries into a new 800k one takes 48.75ms, while joining a trie with a 95% overlapping one -- whose answer is essentially the first operand -- takes 74.96ms. The join with nothing to do is 1.5x slower than the one that doubles the trie. Recorded with the fix sketched (defer materialization until the first slot result that is neither SELF_IDENT nor COUNTER_IDENT, back-filling the prefix from whichever side was still identity). Not attempted; it is general rather than unit-value work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ime goes The survey wanted both payload enums collapsed to a nullable pointer plus a bit. ValOrChild<()> is already one word: `Val(())` carries nothing, so rustc encodes it in the child pointer's null niche. Nothing to do, and a third variant would silently undo it -- hence a test. PayloadRef cannot use that niche, having two pointer-carrying variants plus None, so one word goes to the discriminant. But it is 16 bytes for every V, not just for (), and it is a transient built on the stack during meets and never stored in a trie, so its width costs memory nowhere. The path is hot, which is the part the survey got right: on list-node-heavy set algebra, node_get_payloads plus pmeet_generic and its inner loop are 17.7% of cycles. The cycles are not in the enum, though -- 4.38% is `is_used` header bit tests, 3.03% key-length extraction, 3.29% key-slice construction, and 1.97% the inner function's prologue spilling its arrays. Only that last part scales with payload width, and shrinking a two-element array from 48 to 32 bytes reclaims a fraction of it. Header decoding and key slicing is where that path actually spends its time, and is the thing to look at if it ever needs to be faster. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ByteNode::psubtract` opened with `let mut btn = self.clone()` -- the whole node
including its slot array, with a refcount bump per slot -- before it knew whether
the subtraction would remove anything. When the answer came back `Identity`, all
of that was discarded.
That is the common case, not the corner case. Instrumented on 400k-path
subtractions:
disjoint operands 100.0% of calls return Identity; 431,714 of 431,714
slot clones wasted (100%)
5% overlap 71.2% return Identity; 251,481 of 432,028
wasted (58.2%)
a 25% subset 0.0% return Identity; nothing wasted
`btn` is now an `Option`, left `None` while the result is still identical to
`self` and cloned at the first actual change. Indexing stays correct because the
clone is taken before any modification, so `btn` still matches `self` everywhere
the loop has already been.
Min of 3 runs, 400k paths:
subtract, disjoint 30.04ms -> 17.18ms -42.8%
subtract, 5% overlap 34.87ms -> 25.18ms -27.8%
subtract, 25% subset 40.43ms -> 39.20ms -3.0%
subtract, 95% subset 69.96ms -> 69.44ms -0.7%
The last two are cases with nothing to reclaim, and land inside the noise floor;
`meet`, whose code is untouched, moved -8.9% to +0.1% across the same operands,
which is that floor.
This is the pattern A1 complained about, minus the mask arithmetic it proposed:
the descent is already narrowed to the intersection of the two masks in all four
operations, and the value dimension cannot become mask ops without the shelved
slot split. `pjoin` and `prestrict` waste work the same way but build a fresh
slot vector rather than cloning, so deferring them needs a back-fill; not
attempted here.
Verified with the differential algebra test and miri: 4 + 5 + 5 integration
tests and 76 filtered lib tests across subtract, restrict, meet and join, all
clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two thirds of A1 was already done or blocked: the descent is already narrowed to the mask intersection in all four operations, and the value dimension needs the shelved slot split. Its aside about psubtract's speculative clone was the whole prize -- 42.8% off a disjoint subtract, 27.8% at 5% overlap, neutral where there is nothing to reclaim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…restrict bug
Both were implemented the same way psubtract was -- hold off building the slot
vector until the first result that is neither SELF_IDENT nor COUNTER_IDENT, and
back-fill the prefix from whichever operand was still identity, which the flags
make recoverable since a live flag implies that operand's mask equals the result
mask. Correct, and reverted anyway.
Interleaved, min of 4 rounds per side, with meet and subtract as untouched
controls, join went +10.6% on disjoint operands and -13.4% on a 25% subset.
Swapping Vec::push for an unchecked write into the reserved slot cut the disjoint
regression to +3.7% but did not grow the wins. prestrict landed inside the noise
in both directions across two runs.
psubtract pays because it cloned the whole node up front and barely recurses on
disjoint operands, so the clone was nearly the whole call. These two only skip
the slot vector, and the recursive descent -- which still visits every
overlapping byte -- dominates. F4 measured 99.6% of pjoin's slot clones wasted at
95% overlap, and removing all of them bought about 6%.
Separately: writing a differential oracle for restrict turned up a correctness
bug. restrict(a, a) should be a, and is not when a path both carries a value and
branches -- {ab, abc, abd} loses `ab`, {a, ab, abc} loses `a`. One child is fine,
two or more and the value goes. It reproduces on master, so it predates this
work; the existing differential tests cover join, meet and subtract but not
restrict. The oracle and the minimal repro are committed, #[ignore]d so the suite
stays green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cause, cost, and why the bug survived: restrict was the only one of the four algebraic operations without a differential test. A standalone repro lives on branch restrict-branching-value-repro, off master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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` -- when
`other` contributes no byte `self` does not already have. In that case nothing
needs inserting and the overlapping slots can be joined where they sit.
That case is the norm for an incremental ingest. Instrumented on a 400k-path
trie joined with batches of varying overlap:
disjoint 0.3% of calls, 5.2% of slot moves avoidable
batch, 1% new keys 95.3% of calls, 95.3% of slot moves avoidable
already contained (5%) 100% of calls, 100% avoidable
already contained (95%) 100% of calls, 451,600 slot moves avoidable
The time it buys is modest, because these are slot *moves* -- a memcpy of the
CoFrees -- rather than the clones that made the same fix worth 42.8% in
`psubtract`, where each clone was an atomic refcount bump. What is avoided here
is one allocation and one memcpy per node. Interleaved, min of 8 rounds, with
`join` as an untouched control:
join_into, disjoint 61.55ms -> 61.81ms +0.4%
join_into, 1% new keys 2.37ms -> 2.21ms -6.8%
join_into, contained (5%) 12.81ms -> 12.51ms -2.3%
join_into, contained (95%) 80.42ms -> 76.54ms -4.8%
join (control), all four -0.8% to +0.5%
The direction is consistent across all four shapes with the control flat, and
there is no regression on the disjoint case, which is the one the new branch
cannot help. The 1% figure has a 22-43% run-to-run spread on a 2ms measurement,
so read it as "no worse"; the 95% case, at 8% spread, is the reliable one.
Verified with the differential algebra tests and miri.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
In-place join_into when the other operand brings no new bytes: 95-100% of calls on incremental-ingest shapes, -4.8% on the most reliable case, no regression on disjoint operands. Modest because these are slot moves rather than the clones that made the same idea worth 42.8% in psubtract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch was rebased onto master now that the restrict fix has landed there (b2a0c09), so every commit hash in the notes had moved. The restrict fix drops out of the branch's own table -- it is upstream, and git dropped the duplicate during the rebase as already applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.