Perf: improve delete merges via pre-reserve before bulk append; add large Instruments/benchmark helpers; document no-feature-flag policy in agent.md

Author: KentBeck

agent.md

@@ -0,0 +1,19 @@
+# Engineering Conventions for BPlusTree3
+
+- No feature flags for internal experiments. We have no external users, so avoid `#[cfg(feature = ...)]` branches. Implement improvements directly (or in short‑lived local branches) and remove experimental code before merging.
+
+- Performance work
+  - Validate with existing Criterion benches and the large delete runner (`rust/src/bin/large_delete_benchmark.rs`).
+  - For line‑level CPU hotspots, use the Instruments workload (`rust/src/bin/instruments_delete_target.rs`) and store traces under `rust/delete_profile.trace` (not committed).
+  - Prefer targeted, localized changes that don’t regress insert/get/range performance.
+
+- Coding style
+  - Keep changes minimal and focused on the stated goal.
+  - Reduce repeated arena lookups and redundant separator/key reads in hot paths.
+  - Favor bulk moves and pre‑allocation over per‑element operations.
+
+- Benchmarks to run for delete changes
+  - `cd rust && cargo bench --bench comparison deletion`
+  - `cd rust && cargo run --release --bin large_delete_benchmark`
+  - Optional: record Instruments trace for confirmation of hotspot reductions.
+

rust/src/bin/instruments_delete_target.rs

@@ -0,0 +1,64 @@
+use bplustree::BPlusTreeMap;
+use std::time::{Duration, Instant};
+
+// A long-running delete-focused workload for Instruments Time Profiler.
+// It builds a large tree at a specified capacity, then repeatedly deletes a
+// pseudo-random batch of keys and reinserts them to keep the workload steady.
+// Configure via env vars: CAPACITY, TREE_SIZE, BATCH_SIZE, DURATION_SEC.
+fn main() {
+    let capacity: usize = std::env::var("CAPACITY")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(256);
+    let tree_size: usize = std::env::var("TREE_SIZE")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(2_000_000);
+    let batch_size: usize = std::env::var("BATCH_SIZE")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(500_000);
+    let duration_sec: u64 = std::env::var("DURATION_SEC")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(15);
+
+    eprintln!(
+        "instruments_delete_target: cap={}, size={}, batch={}, duration={}s",
+        capacity, tree_size, batch_size, duration_sec
+    );
+
+    // Build initial tree
+    let mut tree = BPlusTreeMap::new(capacity).expect("init B+tree");
+    for i in 0..tree_size {
+        // small values to reduce memory
+        tree.insert(i as i32, i as i32);
+    }
+
+    // Prepare a pseudo-random but deterministic batch of keys
+    let mut keys: Vec<i32> = Vec::with_capacity(batch_size);
+    let mut seed = 42_u64;
+    for _ in 0..batch_size {
+        seed = seed.wrapping_mul(1103515245).wrapping_add(12345);
+        let k = (seed as usize) % tree_size;
+        keys.push(k as i32);
+    }
+
+    // Run mixed cycles of deletes and reinserts until duration elapses
+    let deadline = Instant::now() + Duration::from_secs(duration_sec);
+    let mut cycles: u64 = 0;
+    while Instant::now() < deadline {
+        // Delete phase
+        for &k in &keys {
+            let _ = tree.remove(&k);
+        }
+        // Reinsert phase to keep tree size stable
+        for &k in &keys {
+            tree.insert(k, k);
+        }
+        cycles += 1;
+    }
+
+    eprintln!("completed cycles: {} (cap={}, size={})", cycles, capacity, tree_size);
+}
+

rust/src/bin/large_delete_benchmark.rs

@@ -0,0 +1,89 @@
+use bplustree::BPlusTreeMap;
+use std::collections::BTreeMap;
+use std::time::Instant;
+
+// Large-scale delete benchmark comparing BPlusTreeMap vs BTreeMap
+// Focus: delete performance with large trees (1M+) and capacity 256
+// Note: Run in release mode for meaningful results.
+fn main() {
+    // Configurable via env vars if needed
+    let tree_size: usize = std::env::var("TREE_SIZE")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(1_000_000);
+    let capacity: usize = std::env::var("CAPACITY")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(256);
+    let delete_sample: usize = std::env::var("DELETE_SAMPLE")
+        .ok()
+        .and_then(|v| v.parse().ok())
+        .unwrap_or(100_000);
+
+    println!("=== Large Delete Benchmark ===");
+    println!("Size: {} elements, Capacity: {} keys/node", tree_size, capacity);
+    println!("Delete sample: {} keys (pseudo-random)", delete_sample);
+
+    // Prepare delete keys (pseudo-random deterministic sequence across range [0, tree_size))
+    let delete_keys: Vec<usize> = (0..delete_sample)
+        .scan(42_u64, |seed, _| {
+            *seed = seed.wrapping_mul(1103515245).wrapping_add(12345);
+            Some((*seed as usize) % tree_size)
+        })
+        .collect();
+
+    // Build maps
+    println!("\nBuilding maps...");
+    let mut bplus = BPlusTreeMap::new(capacity).expect("init bplus");
+    let mut btree = BTreeMap::new();
+
+    let start = Instant::now();
+    for i in 0..tree_size {
+        bplus.insert(i, i);
+    }
+    let bplus_build = start.elapsed();
+
+    let start = Instant::now();
+    for i in 0..tree_size {
+        btree.insert(i, i);
+    }
+    let btree_build = start.elapsed();
+
+    println!(
+        "Build times: BPlusTreeMap={:?}, BTreeMap={:?}",
+        bplus_build, btree_build
+    );
+
+    // Clone maps to avoid interaction between runs
+    println!("\nDeleting ({} keys)...", delete_sample);
+
+    // BPlusTreeMap delete timing
+    let mut bplus_copy = bplus; // move
+    let start = Instant::now();
+    for &k in &delete_keys {
+        let _ = bplus_copy.remove(&k);
+    }
+    let bplus_delete = start.elapsed();
+
+    // BTreeMap delete timing
+    let mut btree_copy = btree; // move
+    let start = Instant::now();
+    for &k in &delete_keys {
+        let _ = btree_copy.remove(&k);
+    }
+    let btree_delete = start.elapsed();
+
+    let bplus_per_op = (bplus_delete.as_nanos() as f64) / (delete_sample as f64);
+    let btree_per_op = (btree_delete.as_nanos() as f64) / (delete_sample as f64);
+    let ratio = btree_per_op / bplus_per_op;
+
+    println!("\nDelete times:");
+    println!("  BPlusTreeMap: {:?} total ({:.1} ns/op)", bplus_delete, bplus_per_op);
+    println!("  BTreeMap:     {:?} total ({:.1} ns/op)", btree_delete, btree_per_op);
+    println!(
+        "  Ratio:        {:.2}x {}",
+        ratio,
+        if ratio > 1.0 { "(BPlusTreeMap faster)" } else { "(BTreeMap faster)" }
+    );
+}
+

rust/src/delete_operations.rs

@@ -176,10 +176,8 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
 
     /// Rebalance an underfull child in an arena branch
     #[inline]
-    /// Rebalance an underfull child node using optimized sibling information gathering.
-    /// This version minimizes arena access by batching all sibling checks.
     fn rebalance_child(&mut self, parent_id: NodeId, child_index: usize) -> bool {
-        // OPTIMIZATION: Gather all rebalancing information in minimal arena accesses
+        // Gather rebalancing information in minimal arena accesses
         let rebalance_info = {
             let parent_branch = match self.get_branch(parent_id) {
                 Some(branch) => branch,
@@ -188,7 +186,6 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
 
             let child_is_leaf = matches!(parent_branch.children[child_index], NodeRef::Leaf(_, _));
 
-            // OPTIMIZATION: Batch sibling information gathering with direct node access
             let left_sibling_info = if child_index > 0 {
                 let sibling_ref = parent_branch.children[child_index - 1];
                 let can_donate = match &sibling_ref {
@@ -228,7 +225,6 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
 
         let (child_is_leaf, left_sibling_info, right_sibling_info) = rebalance_info;
 
-        // Dispatch to appropriate rebalancing strategy
         if child_is_leaf {
             self.rebalance_leaf(
                 parent_id,
@@ -245,6 +241,8 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
             )
         }
     }
+
+    // (Experimental ID-based helpers removed)
 }
 
 #[cfg(test)]
@@ -460,10 +458,13 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
                 None => return false,
             };
 
-            // Then merge into left
+            // Then merge into left, reserving capacity to avoid reallocations
             let Some(left_branch) = self.get_branch_mut(left_id) else {
                 return false;
             };
+            let add_keys = 1 + child_keys.len(); // separator + child keys
+            let add_children = child_children.len();
+            left_branch.reserve_for_merge(add_keys, add_children);
             left_branch.keys.push(separator_key);
             left_branch.keys.append(&mut child_keys);
             left_branch.children.append(&mut child_children);
@@ -513,10 +514,13 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
                 None => return false,
             };
 
-            // Then merge into child
+            // Then merge into child, reserving capacity to avoid reallocations
             let Some(child_branch) = self.get_branch_mut(child_id) else {
                 return false;
             };
+            let add_keys = 1 + right_keys.len(); // separator + right keys
+            let add_children = right_children.len();
+            child_branch.reserve_for_merge(add_keys, add_children);
             child_branch.keys.push(separator_key);
             child_branch.keys.append(&mut right_keys);
             child_branch.children.append(&mut right_children);
@@ -729,10 +733,10 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
             None => return false,
         };
 
-        // Merge into left leaf and update linked list - use early return for cleaner flow
-        let Some(left_leaf) = self.get_leaf_mut(left_id) else {
-            return false;
-        };
+        // Merge into left leaf and update linked list - reserve to avoid reallocations
+        let Some(left_leaf) = self.get_leaf_mut(left_id) else { return false; };
+        let add = child_keys.len();
+        left_leaf.reserve_for_merge(add);
         left_leaf.append_keys(&mut child_keys);
         left_leaf.append_values(&mut child_values);
         left_leaf.next = child_next;
@@ -782,10 +786,10 @@ impl<K: Ord + Clone, V: Clone> BPlusTreeMap<K, V> {
                 None => return false,
             };
 
-            // Then merge into child
-            let Some(child_leaf) = self.get_leaf_mut(child_id) else {
-                return false;
-            };
+            // Then merge into child, reserving capacity to avoid reallocations
+            let Some(child_leaf) = self.get_leaf_mut(child_id) else { return false; };
+            let add = right_keys.len();
+            child_leaf.reserve_for_merge(add);
             child_leaf.append_keys(&mut right_keys);
             child_leaf.append_values(&mut right_values);
             child_leaf.next = right_next;

rust/src/node.rs

@@ -226,6 +226,14 @@ impl<K: Ord + Clone, V: Clone> LeafNode<K, V> {
         self.values.append(other);
     }
 
+    /// Reserve capacity for an incoming merge of `additional` items.
+    #[inline]
+    pub fn reserve_for_merge(&mut self, additional: usize) {
+        // Reserve for both keys and values (kept in lockstep)
+        self.keys.reserve(additional);
+        self.values.reserve(additional);
+    }
+
     /// Take all keys, leaving an empty vector.
     #[inline]
     pub fn take_keys(&mut self) -> Vec<K> {
@@ -474,6 +482,7 @@ impl<K: Ord + Clone, V: Clone> LeafNode<K, V> {
         Some((self.keys.remove(0), self.values.remove(0)))
     }
 
+
     /// Accept a borrowed key-value pair at the beginning (from left sibling)
     pub fn accept_from_left(&mut self, key: K, value: V) {
         self.keys.insert(0, key);
@@ -713,4 +722,11 @@ impl<K: Ord + Clone, V: Clone> BranchNode<K, V> {
         self.keys.append(&mut other.keys);
         self.children.append(&mut other.children);
     }
+
+    /// Reserve capacity for an incoming merge.
+    #[inline]
+    pub fn reserve_for_merge(&mut self, additional_keys: usize, additional_children: usize) {
+        self.keys.reserve(additional_keys);
+        self.children.reserve(additional_children);
+    }
 }

rust/tools/parse_time_profile.py

@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+import sys
+import xml.etree.ElementTree as ET
+from collections import Counter
+
+"""
+Best-effort parser for Instruments xctrace XML exports to list top functions/frames.
+Usage:
+  python3 rust/tools/parse_time_profile.py rust/delete_export/time_profile.xml
+
+Notes:
+- XML schema varies across Xcode versions; this script attempts to be robust.
+- If time_profile.xml is empty or missing, try time_sample.xml instead:
+  python3 rust/tools/parse_time_profile.py rust/delete_export/time_sample.xml
+"""
+
+def main(path: str) -> int:
+    try:
+        tree = ET.parse(path)
+    except Exception as e:
+        print(f"Failed to parse {path}: {e}")
+        return 1
+
+    root = tree.getroot()
+    # Find all leaf text that look like function symbols; Instruments usually
+    # includes stacks as text content or attributes in nested elements. We will
+    # count any text nodes that look like code symbols (contain '::' or '['file:line']').
+    counter = Counter()
+    for elem in root.iter():
+        text = (elem.text or '').strip()
+        if not text:
+            continue
+        if '::' in text or ' - [' in text or ' + ' in text:
+            # Normalize long frames by splitting on ' + ' (address offsets)
+            frame = text.split(' + ')[0]
+            counter[frame] += 1
+
+    print("Top frames by sample count (heuristic):")
+    for frame, count in counter.most_common(50):
+        print(f"{count:>8}  {frame}")
+
+    return 0
+
+if __name__ == '__main__':
+    if len(sys.argv) != 2:
+        print("Usage: parse_time_profile.py <exported_xml>")
+        sys.exit(2)
+    sys.exit(main(sys.argv[1]))
+

scripts/instruments_export.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+TRACE_PATH=${1:-rust/delete_profile.trace}
+OUT_DIR=${2:-rust/delete_export}
+
+mkdir -p "$OUT_DIR"
+
+echo "Exporting TOC to $OUT_DIR/toc.xml"
+xcrun xctrace export --input "$TRACE_PATH" --toc > "$OUT_DIR/toc.xml"
+
+echo "Exporting time-profile table to $OUT_DIR/time_profile.xml (if available)"
+if ! xcrun xctrace export --input "$TRACE_PATH" --xpath '/trace-toc/run[@number="1"]/data/table[@schema="time-profile"]' > "$OUT_DIR/time_profile.xml"; then
+  echo "time-profile export failed; continuing"
+fi
+
+echo "Exporting time-sample table to $OUT_DIR/time_sample.xml (if available)"
+if ! xcrun xctrace export --input "$TRACE_PATH" --xpath '/trace-toc/run[@number="1"]/data/table[@schema="time-sample"]' > "$OUT_DIR/time_sample.xml"; then
+  echo "time-sample export failed; continuing"
+fi
+
+echo "Exporting thread-info to $OUT_DIR/thread_info.xml"
+xcrun xctrace export --input "$TRACE_PATH" --xpath '/trace-toc/run[@number="1"]/data/table[@schema="thread-info"]' > "$OUT_DIR/thread_info.xml"
+
+echo "Exporting process-info to $OUT_DIR/process_info.xml"
+xcrun xctrace export --input "$TRACE_PATH" --xpath '/trace-toc/run[@number="1"]/data/table[@schema="process-info"]' > "$OUT_DIR/process_info.xml"
+
+echo "Exporting dyld-library-load to $OUT_DIR/dyld_library_load.xml"
+xcrun xctrace export --input "$TRACE_PATH" --xpath '/trace-toc/run[@number="1"]/data/table[@schema="dyld-library-load"]' > "$OUT_DIR/dyld_library_load.xml"
+
+echo "Done. Inspect XML files under $OUT_DIR"
+