Optimize/ordered index#102
Draft
ryanleecode wants to merge 59 commits into
Draft
Conversation
Contributor
Author
Benchmark Size Comparison (vs main)
OK: All artifacts within 5% regression thresholdRun 28162743739 | deec9e6 | 2026-06-25 10:29 UTC |
Rewrite range as a single-visit in-order traversal with subtree-overlap pruning so children whose key interval cannot overlap [from, to) are not loaded. child_interval_overlaps must use >= / <= for Included bounds (not strict > / <) because duplicate keys straddle node boundaries in the (key, nonce)-ordered B-tree: when entries[i].key == k, both child i and child i+1 can still hold entries with key == k. slot_reads unchanged (28.93 at T=5/N=1M — the prior traversal already visited only the minimal spanning subtree); range p50 latency -15% (86us to 71us at T=5; 312us to 255us at T=2).
…es leaf-only benefit)
Store the common prefix of all entry keys once per node header. Each entry stores only the key suffix (key minus shared prefix), shrinking per-entry size from ~28B to ~21-23B for sorted username corpora. This unlocks T=6 (previously overflowed 416B at 28B/entry), reducing tree depth from log_9(1M)=6 (T=5) to log_11(1M)≈6 (T=6) with wider fanout reducing result-leaf visits. Result at N=1M: slot_reads_per_query = 26.11 (was 28.93 at T=5). This BEATS the Postgres btree bar of 27 page-reads/query. Only encode/decode change; in-memory Node struct and all B-tree algorithms (insert/split/remove/range/select) are unchanged.
The nonce is a globally-unique insert ID. For practical datasets (<4.3B entries), u32 is sufficient. Store 4B on the wire instead of 8B, widen to u64 in memory (API unchanged). Assert nonce <= u32::MAX at encode time. Saves 4B per entry, shrinking per-entry from ~23B to ~19B. This unlocks T=7 (previously overflowed 416B at ~23B/entry with 13 entries). Result at N=1M: slot_reads_per_query = 22.00 at T=7 (was 26.11 at T=6). Depth drops from log_11(1M)=6 to log_13(1M)≈5. Cumulative trajectory: 102.84 → 39.04 → 28.93 → 26.11 → 22.00 (-78.6%).
Replace fixed-width nonce (u32) and value (u64) wire encoding with self-delimiting LEB128 varints. For the username-search workload (nonce 0..1M = 3B, value 0..1M = 3B), this saves ~6B per entry (nonce 4->3, value 8->3), shrinking per-entry from ~19B to ~13B. Extends measurement binary T-sweep from 2..=8 to 2..=12 to cover newly-feasible degrees. Result at N=1M: T=8: 19.89 reads/q (was not reachable before varint) T=9: 17.37 reads/q (new best, -21% vs e7's 22.00 at T=7) Cumulative trajectory: 102.84 -> 39.04 -> 28.93 -> 26.11 -> 22.00 -> 17.37 (-83.1%).
Replace fixed-width child wire fields (4B node_id + 4B subtree_count + 1B own_entry_count = 9B) with self-delimiting LEB128 varints. For the 1M-entry benchmark, node_ids are 0..~85K (3B) and subtree_counts are 7..~50K (3B), so per-child drops from 9B to ~7B. This unlocks T=10 (19 entries per node, fanout 20), depth log_19(1M) ~= 5. Result at N=1M: slot_reads_per_query = 14.31 at T=10. SPEC TARGET REACHED (target was 15.0). Beats Postgres btree bar (27) by 47%. Cumulative: 102.84 -> 14.31 (-86.1%).
Add storage_get_bytes / storage_set_bytes / storage_clear_value to pvm-storage, wrapping pallet-revive's variable-length host API in a single get_storage / set_storage call per key. The EMPTY_VALUE_SENTINEL byte distinguishes a stored-empty value from an absent key. Wire pub mod ordered_index behind #[cfg(feature = "alloc")]. MAX_STORAGE_VALUE_BYTES = 416 (pallet-revive STORAGE_BYTES cap).
Add CountingHost wrapper (counts get_storage/set_storage/clear calls per operation) and the measure-ordered-index binary target, target-gated away from riscv64. Add proptest dev-dep for property-based tests. Include proptest regression seeds (cases that surfaced B-tree bugs during the optimization experiments).
Add the full experiment log (CP-1 baseline through e8b, 8 experiments, 86% slot-read reduction), ce-optimize spec.yaml (target=15.0, reached at 14.31), pg-bar.sh (Postgres benchmark bar script), and port-design.md (SolType port design document).
Compressed header 4B->2B (derive children_len from entries_len + is_leaf; pack prefix_len into flags byte) and added is_self_delimiting() to CompactCodec so LEB128 varint values skip the 1B v_len prefix. T=11 now fits (416B cap) but is WORSE: 15.95 vs T=10's 14.31 reads/q. Same tree depth (5 levels) at N=1M for both T=10 and T=11 — the extra fanout doesn't reduce depth, and wider nodes add traversal overhead. T=10 remains the sweet spot at N=1M. T=11 would only help at N>2.5M where T=10 goes to depth 6 but T=11 stays at depth 5. Reverting to 657162e (optimal T=10 state).
LEB128 varint decode: reject oversized 10th byte that silently wraps bits above u63 instead of returning Err on malformed input. Previously wrapping_shl(63) would discard payload bits 1-6 without error. btree_invariants proptest: add min-degree lower bound check (non-root nodes need >= T-1 entries) via walk_depths, complementing the existing upper bound (<= 2T-1) assertion.
Three corpus profiles matching identity-backend seed data: - sequential (user000000..user999999): experiment baseline - zipf (word-based names with 40-word vocabulary): realistic username distribution, tests prefix compression on diverse keys - uniform (random alphanumeric 5-12 chars): worst-case for prefix compression Validated at T=7/N=1M: all profiles beat PG bar (27 page-reads/q). T=10 overflows for realistic keys (12-14 chars); T=7 is the safe production maximum across all key distributions. Results at T=7/N=1M: sequential=24, zipf=25.46, uniform=13.72.
- cargo fmt: rustfmt all touched files (import ordering, line wrapping, generic param formatting) - clippy --all-targets: add #[allow(dead_code)] on shared counting_host module in validate-realistic binary (only uses reads(), not the full CountingHost API) - add pr-description.md to context
Replace the synthetic slot_reads_per_query proxy (a CountingHost read counter that treated every SLOAD as equal) with the real pallet-revive on-chain weight model transcribed from polkadot-sdk. - pallet_revive_weight.rs (new, immutable): read/write/clear weight closed forms from substrate/frame/revive/src/vm/runtime_costs.rs (cost_storage! routing + full-empty overhead), the Asset Hub Westend generated pallet_revive.rs, and rocksdb_weights.rs. read(n) = 69_340_610 + 1561*n ps; write(new,old) = 170_975_834 + 423*new + 344*old; clear(old) = 173_290_010 + 958*old. - counting_host.rs: capture per-op byte sizes (output len on reads, value + returned old len on writes, all-zero branch on set_storage_or_clear) and accumulate Weight via Cell<Weight>. - measure-ordered-index.rs: primary metric is now weight_ref_time_per_query (ps) + weight_proof_size_per_query (bytes); slot_reads_per_query retained as a diagnostic. Baseline at N=1M, T=10: 995_118_621 ps/q, 148_575 proof bytes/q, 14.31 reads/q (cross-checks the prior proxy).
Archive the reads/q-based ce-optimize run (.context/.../ordered-index -> ordered-index-archive-20260623) and add the new ordered-index-weight spec whose primary metric is weight_ref_time_per_query grounded in pallet-revive. Old experiment-log/port-design/pg-bar preserved under the archive dir for audit.
…erivation OrderedIndex was the lone pvm-storage structure breaking the library's headline Solidity-compatible slot layout invariant (lib.rs:1). Its three administrative cells (root/next_id/next_nonce) used a bespoke keccak256(namespace ++ 0x00 ++ suffix) scheme that no Solidity contract, cast storage, or foundry tool could compute, while its node keys were already Solidity mapping(uint256 => bytes) via storage_derive_key. Route the cells through the Solidity mapping(string => bytes) preimage: keccak256(suffix ++ pad32(root)). pad32 is identity because root is already a 32-byte slot. Computed inline with the in-scope pvm_contract_types::keccak256 (same fn root_key uses); no host needed, new() signature unchanged. Add ordered_index_cell_and_node_keys_are_solidity_layout mirroring the repo's seven existing cast-parity tests: cross-checks each cell key two ways (independent Solidity preimage + the cast-validated storage_derive_key_unpadded helper) and node keys via manual preimage + storage_derive_key. The test failing means the invariant broke. A B-tree node (up to 416 B) has no native Solidity slot form, so 'compatible' here means key-derivation parity (tool-computable keys), not 32-byte slot packing. 186 unit tests + 3 weight tests pass.
End-to-end validation bar for the OrderedIndex weight metric. Builds a minimal PVM contract (examples/ordered-index-bench) that wraps OrderedIndex<String, u64, 10> with insert + range_query methods, deploys it on revive-dev-node, inserts 100 records via real extrinsics, and dry-runs a range query to read weight_required.ref_time(). Contract uses the pico allocator (16384-byte heap; default 1024 traps after ~2 inserts because OrderedIndex::insert's node encode + key clone exhaust the bump allocator). OrderedIndex is constructed per-method via self.host().clone() because it does not impl StorageComponent and so cannot be a #[slot] field. Validation result (N=100, T=10, range over user005): - Predicted storage-ops weight: 208,938,526 ps (~209 us) - Measured on-chain ref_time: 20,001,848,489 ps (~20 ms) - Gap: 95.7x The gap is expected: the committed metric is deliberately storage-ops only (sum of pallet-revive read/write/clear DbWeight). On-chain cost adds dispatch/frame setup, ABI decode, PolkaVM compute, the full seal host-call path per access, and return encode. So the metric is valid as a RELATIVE optimization proxy (fixed overhead is constant across T and codec variants, so T=10 still minimizes the storage-ops component) but NOT an absolute gas predictor. Test passes in ~11s. Requires revive-dev-node at $HOME/.local/bin/revive-dev-node (set via REVIVE_DEV_NODE env) and the cargo-pvm-contract CLI pre-built (cargo build -p cargo-pvm-contract).
ryanleecode
force-pushed
the
optimize/ordered-index
branch
from
f804750 to
fbf0f14
Compare
Split the node into two structurally-distinct variants so illegal shapes cannot be constructed: leaves carry the (key, nonce, value) payload plus a next-leaf link, internal nodes carry separator keys and child references only. Routing reads no longer drag values they do not need, which lifts internal fanout, and a range scan descends once to the first in-range leaf then walks the next-links across leaves with no re-descent. Decode now returns a typed NodeDecodeError with one variant per failure mode in place of an Option that collapsed every failure into None, and the byte parser uses checked arithmetic throughout rather than truncating casts. Domain counts and the entry nonce are branded newtypes. The per-child mirrors are bundled into a single ChildRef so the parallel-vector desync the old flat node permitted is unrepresentable. The Solidity cell and node key derivation is byte-for-byte unchanged; only the value body under each key changes shape, so the mapping-layout parity test passes verbatim. Measured against the pallet-revive weight harness the result is correct and slightly cheaper per query; the read count remains at the depth-three floor that the 416-byte per-value payload limit imposes at ten thousand keys.
…er query Every query reached the root through an indirection: read a u64 root-cell to learn the root node id, then read the node. Since the pallet-revive weight model charges a fixed per-read base of roughly sixty-nine million picoseconds regardless of the value size, that pointer read cost as much as a full node read and was paid on every range, point, select and rank query. The root has no parent, so it needs no id. Store the encoded root node body directly at the b"root" key that previously held the pointer; a query now loads the root in one read and an empty value means an empty tree. Root growth moves the old root content into freshly allocated children and writes the new internal root back to the fixed key; root collapse pulls the sole remaining child up into it. The key derivation is byte-for-byte unchanged, so the Solidity mapping-layout parity holds and that test passes unaltered. Measured against the weight harness at ten thousand keys the read count falls from 4.23 to 3.23 per query and ref_time per query from about 294 to 225 million picoseconds, a 1.31x reduction that reaches the depth-three read floor the 416-byte payload limit imposes; proof_size per query falls by a similar share. Correctness is unchanged.
The decode path returns a distinct error variant per failure mode, but only the encode-decode roundtrip exercised it, so the variants' reachability and distinguishability went unproven. Drive a real internal node and a real leaf through decode under truncation (every prefix length, and the empty buffer) and trailing-byte corruption, asserting the specific Truncated and TrailingBytes variants surface rather than a silent or collapsed failure.
The node's u64 fields — insertion nonce, node id, subtree and entry counts — are encoded with parity-scale-codec's Compact<u64>, the SCALE varint that is the native wire format of the pallet-revive backend and is continuously fuzzed as the encoding layer of every Substrate runtime. This removes a hand-rolled LEB128 codec, including a hand-written overflow guard doing exactly the bit-manipulation an audited standard should own, and satisfies the rule against reinventing audited primitives. parity-scale-codec is already resolved in the lockfile, so no new transitive dependency is introduced, and it is taken with default-features disabled to stay no_std on the contract target. The codec applies only to length-delimited numeric fields inside a node body; routing and separator keys keep the byte-order-preserving key codec, since SCALE Compact is little-endian and length-prefixed and must never back a byte-wise ordered comparison. Observable index behaviour, the property tests and the pallet-revive weight-harness result are unchanged at 3.23 reads per query and about 225 million picoseconds.
The leaf-linked B+ tree panicked with OrderedIndexNodeTooLarge when a remove-path borrow rotated a longer sibling key into a parent separator slot, or a merge concatenated two near-full leaves, pushing the node past the 416-byte pallet-revive cap. Latent for the uniform short keys the suite used, fatal for variable-length keys. Internal-node separators are now the minimal distinguishing prefix between adjacent leaf boundaries (separator_between), re-derived on every borrow/merge via leaf_boundary_separator, so a rebalanced parent separator is always the shortest prefix that routes correctly, never a rotated full key; borrow cannot grow the parent regardless of sibling key size. A store_node_or_split backstop splits any residual over-cap node (e.g. a leaf merge) and propagates the new separator up through the remove path. The long-key property deep_tree_matches_oracle that exposed the bug now passes; select_rank_match_oracle and remove_by_value_matches_oracle guard routing and remove semantics. Measured at 10000 entries / 1000 queries: correctness true, T=5 224.9M / 3.22 reads, T=10 226.7M / 3.25 reads, no regression vs the 225M / 3.23 and 228M baselines (suffix truncation shrinks internal nodes, so fanout is equal-or-higher).
The E0596 borrow diagnostic on nightly-2026-02-01 shortened one clause from "so the data it refers to cannot be borrowed as mutable" to "so it cannot be borrowed as mutable". The snapshot was blessed under 1.92 stable (the longer wording); under the repo's pinned nightly (nightly-2026-02-01) it drifts by this single word. Re-bless under the pinned nightly so the trybuild compile_fail suite is green (7/7). No behavior change to the rejection under test.
Two proptest properties pin the invariants the suffix-truncated routing scheme rests on, independent of the tree: - separator_composite_preserves_total_order: lexicographic byte order over the composite encoding equals the (K, Nonce) total order. Constructive generation over the low byte range [0x00..=0x05] reaches the 0x00-escape branch that the [a-z] tree tests never exercise. - separator_between_yields_valid_separator: composite(left) < sep <= composite(right) and sep is a byte-prefix of composite(right). The composite property was verified to kill the escape-condition mutant (replace == with != at the 0x00 check): it fails on a minimal shrunk input of "" vs "\0\0".
deep_tree_matches_oracle validated only the multiset after removals, never the tree structure, so the entire remove-path rebalance code was never structurally checked. assert_bplus_structure walks every node (size cap, separators strictly increasing, internal children == separators + 1, non-root min-degree, height balance, child-mirror consistency) and is run mid-removal and post-removal in a new removals_preserve_bplus_structure proptest. Verified to kill the refresh_child_mirrors no-op mutant: a stale mirror makes descend_prepared skip a needed rebalance, the child underflows, and the tree panics with OrderedIndexBorrowLeftEmpty.
internal_cut previously started at (m/2).clamp(lo, hi) and searched both directions (cut -= 1 when the left half overflowed, cut += 1 when the right did). For separator-only internal nodes the right half is tiny, so the maximal-left cut always leaves a fitting right half: the bidirectional search and the mid-point start are redundant. Replace with a decrement-only search from hi, matching leaf_cut's pack-left strategy. This deletes the equivalent-mutant-prone mid-point/clamp/bidirectional arithmetic (those mutants produced a different but still-valid split point, so no correctness or structural test could distinguish them) and unifies the two split functions on one strategy.
Check the BTreeMap oracle in full rather than sampling select and rank, so a routing or rank/select mutation can no longer slip between sampled indices. Add a mixed-size-key property (1..90 byte keys) that forces the byte-budgeted split loop to adjust its cut, a unit test pinning leaf prefix compression, an is_empty population test, an unsorted-node shape-guard rejection test, and a range property spanning every Included/Excluded/Unbounded bound combination. Drop the slot-count-exceeds-255 guard in assert_node_shape: the 416-byte value cap admits far fewer than 256 entries per node, and the encode path's u8 length field already rejects an over-count, so the guard was unreachable dead code whose mutants no test could reach.
Document the mutation-testing gate and command in AGENTS.md so future work re-tests only surviving mutants via --iterate instead of re-running the whole set each cycle, scopes the test command to the library with --lib to keep the toolchain-pinned trybuild snapshots out of the baseline, and ignores the mutants.out report directory the iterate mode depends on.
Record an equivalence exclusion for the split cut-point selectors and the routing-separator builder. Their surviving mutants cannot change observable correctness: a B+ tree is valid for any cut that keeps both halves within the byte cap and above the min-degree floor, so content, order, and every query result are identical no matter which valid cut is picked; the only effect is node fill, which the pallet-revive weight harness gates. The separator builder walks a prefix-free composite, so its loop bound can never be reached. Mutants in these functions that do break correctness panic or time out and are caught. The proof for each entry lives beside it in .cargo/mutants.toml.
Add a deep-churn property that builds a multi-level tree and asserts the B+ structural invariants after every single removal, exercising internal-level split, borrow, and merge that shallow trees never reach. Add a deep remove-by-nonce and remove-by-value property that drives the recursive leaf-match and the duplicate-scan through interior nodes, a duplicate-heavy property over a tiny key space, a free-node test that confirms a merged-away node is cleared from storage, and a decode test pinning the exact NodeDecodeError variant for hand-crafted truncations.
Removing by a nonce that falls in a gap between two occurrences of the same key must remove nothing: the routing lands on a present key whose nonce differs, so the leaf match has to test key AND nonce, not key alone. Building the tree past one level first drives this through the recursive descent rather than only the root.
…core directly Replace the exclusion config with direct exact-output tests on the pure decision functions, so the mutants survive nothing rather than being skipped: leaf_cut and internal_cut are asserted to return the maximal pack-left index; the split threshold is extracted into over_cap and pinned at a node sized exactly to the byte cap (which must not split); the separator-search functions are checked at an exact-equal-separator boundary; and separator_between is asserted byte-for-byte and its dead manual loop bound rewritten as a zip/take_while so there is no bound left to mutate.
Rewrite leaf_cut and internal_cut from a decrementing search loop into (lo..=hi).rev().find(fits).unwrap_or(lo). The loop masked bound mutations by self-healing back to a valid cut and carried a no-split panic on an unreachable-in-practice path; the rev-find form is the same largest-fitting cut on the split path, surfaces a genuinely unsplittable node at the storage size assert instead of a local panic, and makes the lo/hi min-degree bounds load-bearing. Pin them with direct tests: a cut forced to the floor, a cut forced to the ceiling on an overflow-by-one node, and the existing maximal-pack case in between.
Add non-overflowing-input tests so a raised min-degree ceiling (which would return a cut that leaves the right half empty) is caught, alongside the existing floor, ceiling, and maximal-pack cases. Together they pin both the lo and hi bounds of leaf_cut and internal_cut in every direction.
Pull the leaf and internal split slicing into split_leaf_parts and split_internal_parts, used by both the cut byte-checks and the real split functions, so the single copy of each index is load-bearing in the actual split rather than duplicated three ways. Pin them with direct tests asserting the exact halves, the pushed-up median, and the boundary separator, which catches every cut+1, ..=cut, and cut-1 index mutation.
Rewrite the decoder's bounds from an explicit cursor + n > bytes.len() comparison plus an indexed slice into bytes.get(range), and drop the unreachable cursor > len guard in read_codec_u64. The get form rejects the same out-of-bounds inputs with the same Truncated variant, leaves no comparison to mutate, and is ordinary safe Rust.
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.