Remove RandomMultiPair and make (key, value) the only entry identity - #13
Merged
Merged
Conversation
RandomMultiPair ordered entries by (key, random discriminator) so that a V which was only PartialEq could still be stored in a multimap. Both of the things that follow from that were inherent to the design rather than bugs sitting on top of it. Its Ord consulted value equality before the discriminator, which is not transitive, so binary search over nodes was unreliable, same-key churn accumulated duplicate logical pairs, and once same-key nodes split the skip map held entry keys comparing Equal, which corrupted routing and livelocked the split retry loop. Making the order lawful, as 0.0.8 did, meant the value stopped participating in it, so a stored entry could no longer be found by its value and insert had to scan every entry sharing the key. That is O(n) per insert and quadratic to fill one key. Measured, one key, values inserted one at a time: values 0.0.7 0.0.8 this 1,000 259 ns 21.95 us 877 ns 4,000 320 ns 87.13 us 746 ns 16,000 416 ns 356.31 us 333 ns 356 us per insert at 16,000 values, against 333 ns here. The distinct-key case recovers too, 5.57 us to 285 ns. OrdMultiPair was never slower at any size, including two values per key, so there was no size at which the random representation paid for itself. OrdMultiPair, whose identity is the (key, value) pair lexicographically ordered, is now the only representation. It is a lawful total order, a stored entry is located by binary search, and duplicate replacement falls out of the order rather than needing a scan. The niche the random representation served, a V that is PartialEq but not Ord, does not justify a second representation: wrap such a value in a newtype with a total order. The type is gone rather than deprecated, and the reasoning is recorded on MultiPair so it is not reintroduced. One test asserted that replacing a logically equal pair preserved its discriminator; that property no longer exists and the test goes with the type. Two tests built nodes in discriminator order and now build them in (key, value) order.
added 2 commits
September 1, 2026 14:38
…d it remove_where, remove_where_inner and remove_where_cdc walked a node looking for the first element satisfying a predicate. Only RandomMultiPair used them: its Ord did not include the value, so removing a logical (key, value) pair meant finding it by scanning. Ord identity removes by binary search on the pair itself, so nothing reaches them now. That is one more unbounded scan gone from the set alongside the one in insert, and the three tests that exercised it go with it.
The tests that exercised the random representation are gone, and with them the only uses of ChangeEvent and OrdMultiPair inside the set test module.
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
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.
Closes the quadratic-insert regression in 0.0.8 by removing the representation that caused it.
The regression
RandomMultiPair::insert_cdc_intoscans every entry already stored under the key on each insert, because the value does not participate in the order and a stored entry cannot be located by it. O(n) per insert, quadratic to fill one key.Reproducer, no WorkTable involved:
Distinct keys recover as well, 5.57 us to 285 ns.
This reaches consumers hard. AgentCode indexes a generation under one
snapshot_id, so 14,400 rows land on one key: a one-file incremental update went from 698 ms on beta.13 to 15.1 s on beta.14, and marginal cost per row from 9.04 us to 330 us. The microbenchmark above predicts 318 us/row at 14,400, which accounts for the production figure to within 4%.Why removal rather than repair
Both problems were inherent to ordering by
(key, random discriminator):Ordconsulted value equality before the discriminator, which is not transitive. Two prior audits already flagged this as P1.OrdMultiPairhas neither problem by construction: identity is the(key, value)pair, lexicographically ordered, so the order is lawful and a stored entry is found by binary search. Duplicate replacement falls out of it.It is also never slower. Measured across sizes from two values per key upward, Ord won everywhere, by 1.83x at n=2 and 68x at n=4096. There is no size at which the random representation paid for itself.
Nothing consumes
RandomMultiPair: not WorkTable, not any consumer in this organisation. Only this crate's own tests referenced it.What changed
RandomMultiPairdeleted;MultiPairis nowOrdMultiPair.MultiPairso the type is not reintroduced.(key, value)order.Tests
106 pass with
concurrent,cdc,multimap, 34 with default features. The doc-test failures on this crate are pre-existing and identical before and after.Note for WorkTable
WorkTable needs a matching change, which is small:
MultiPairRecreateno longer takes a discriminator, and the reconstruction path stops synthesising one. There is one open decision there, which is that the persisted index format currently encodes insertion order within a key while(key, value)order is required. Detail in the companion issue.