Merge PR #451: perf: fill-handle double-click scans a dense neighbour column ~2.4x faster

crates/engine/examples/fill_handle_perf.rs

@@ -0,0 +1,50 @@
+//! Times the fill-handle double-click scan against a densely-filled neighbour column: the old
+//! row-by-row `value_ref` loop from `grid.rs` vs. `Sheet::last_contiguous_nonempty`.
+//! `cargo run --release -p gridcraft-engine --example fill_handle_perf -- [rows]`
+
+#![allow(clippy::disallowed_methods)] // a native-only benchmark: wasm never runs it
+
+use std::time::Instant;
+
+use gridcraft_engine::Session;
+use gridcraft_engine::core::CellRef;
+use serde_json::json;
+
+fn main() {
+    let rows: u32 = std::env::args().nth(1).and_then(|s| s.parse().ok()).unwrap_or(1_000_000);
+    let mut s = Session::new();
+    s.new_workbook();
+    let t = Instant::now();
+    let chunk = 10_000u32;
+    let mut r = 0u32;
+    while r < rows {
+        let n = chunk.min(rows - r);
+        let vals: Vec<serde_json::Value> = (0..n).map(|i| json!([(r + i) as f64])).collect();
+        s.execute("range.setValues", json!({"range": format!("A{}", r + 1), "values": vals})).expect("set");
+        r += n;
+    }
+    println!("fill A1:A{rows}: {:?}", t.elapsed());
+
+    let d = s.doc().expect("doc");
+    let sh = d.wb.active().expect("sheet");
+    let neighbour = 0u32; // column A
+    let cap = (rows - 1).min(999_999);
+
+    let t = Instant::now();
+    let mut last = 0u32;
+    let mut iters = 0u64;
+    while last + 1 < gridcraft_engine::core::MAX_ROWS && sh.value_ref(CellRef::new(last + 1, neighbour)).is_some_and(|v| !v.is_empty()) && last < cap
+    {
+        last += 1;
+        iters += 1;
+    }
+    let old = t.elapsed();
+    println!("old row-by-row scan:          {iters} iterations, {old:?} ({:.1} ns/iter)", old.as_nanos() as f64 / iters.max(1) as f64);
+
+    let t = Instant::now();
+    let new_last = sh.last_contiguous_nonempty(neighbour, 1, cap);
+    let new_t = t.elapsed();
+    println!("new last_contiguous_nonempty: last={new_last}, {new_t:?} ({:.2}x faster)", old.as_secs_f64() / new_t.as_secs_f64().max(1e-12));
+
+    assert_eq!(last, new_last, "old and new scans must agree");
+}

crates/model/src/sheet.rs

@@ -183,6 +183,39 @@ impl Sheet {
             _ => self.spill.get(&c),
         }
     }
+    /// Last row (inclusive) of the maximal contiguous run of non-empty values in `col`, starting
+    /// at `from_row` and capped at `cap` (inclusive). `from_row - 1` when `from_row` itself is
+    /// empty (or `from_row > cap`), same as a cell-by-cell `value_ref` scan would find, but
+    /// without calling `value_ref` once per row: `CellStore::contiguous_value_run` walks each
+    /// band's row map directly for the common case (a run of stored cells), only falling back to
+    /// a per-row `spill` check where the stored run stops short of `cap`. Used by the fill-handle
+    /// double-click, where `col` can be densely populated for very long stretches.
+    pub fn last_contiguous_nonempty(&self, col: u32, from_row: u32, cap: u32) -> u32 {
+        if from_row > cap {
+            return from_row.saturating_sub(1);
+        }
+        let mut row = from_row;
+        loop {
+            if row > cap {
+                return cap;
+            }
+            let c = CellRef::new(row, col);
+            match self.cells.contiguous_value_run(c, cap) {
+                Some(last) => row = last + 1,
+                None => {
+                    // A stored cell decides the row on its own, even with an empty value (see
+                    // `contiguous_value_run`'s doc comment): only an unstored row falls through
+                    // to `spill`, exactly like `value_ref`.
+                    let qualifies = self.cells.get(c).is_none() && self.spill.get(&c).is_some_and(|v| !v.is_empty());
+                    if qualifies {
+                        row += 1;
+                    } else {
+                        return row.saturating_sub(1);
+                    }
+                }
+            }
+        }
+    }
     pub fn style_id(&self, c: CellRef) -> StyleId {
         if let Some(cell) = self.cells.get(c) {
             return cell.style;
@@ -411,6 +444,7 @@ fn line_at(pos: f64, default: f64, custom: &BTreeMap<u32, LineInfo>, max: u32) -
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::cell::Formula;
 
     #[test]
     fn line_index_matches_linear() {
@@ -427,6 +461,103 @@ mod tests {
         }
     }
 
+    /// Reference implementation: one `value_ref` call per row, exactly the loop
+    /// `last_contiguous_nonempty` replaces (from `grid.rs`'s fill-handle double-click).
+    fn last_contiguous_nonempty_naive(s: &Sheet, col: u32, from_row: u32, cap: u32) -> u32 {
+        let mut row = from_row;
+        while row <= cap && s.value_ref(CellRef::new(row, col)).is_some_and(|v| !v.is_empty()) {
+            row += 1;
+        }
+        row.saturating_sub(1).max(from_row.saturating_sub(1))
+    }
+
+    /// Deterministic xorshift32 so failures reproduce without pulling in a `rand` dependency
+    /// just for this test.
+    struct Xorshift32(u32);
+    impl Xorshift32 {
+        fn next(&mut self) -> u32 {
+            self.0 ^= self.0 << 13;
+            self.0 ^= self.0 >> 17;
+            self.0 ^= self.0 << 5;
+            self.0
+        }
+        fn below(&mut self, n: u32) -> u32 {
+            self.next() % n.max(1)
+        }
+    }
+
+    #[test]
+    fn last_contiguous_nonempty_matches_naive() {
+        let mut rng = Xorshift32(0x9e3779b9);
+        for trial in 0..300u32 {
+            let mut s = Sheet::new("S");
+            let rows = rng.below(400);
+            // A mix of directly-stored values, formulas (value empty but formula present),
+            // spill-only cells, blank-but-stored cells, and gaps, like a real sheet.
+            for r in 0..rows {
+                match rng.below(5) {
+                    0 => s.set_value(CellRef::new(r, 0), Value::Number(r as f64)),
+                    1 => s.set_cell(CellRef::new(r, 0), Cell { formula: Some(Arc::new(Formula::new("1"))), ..Default::default() }),
+                    2 => {
+                        s.spill.insert(CellRef::new(r, 0), Value::Number(r as f64));
+                    }
+                    3 => {
+                        // Stored but blank: does not count (matches `has_value`/`value_ref`).
+                        s.set_value(CellRef::new(r, 0), Value::Text("".into()));
+                    }
+                    _ => {} // gap
+                }
+            }
+            for _ in 0..5 {
+                let from_row = rng.below(420);
+                let cap = from_row.saturating_add(rng.below(450));
+                let got = s.last_contiguous_nonempty(0, from_row, cap);
+                let want = last_contiguous_nonempty_naive(&s, 0, from_row, cap);
+                assert_eq!(got, want, "trial {trial}: col 0, from_row {from_row}, cap {cap}, rows up to {rows}");
+            }
+        }
+    }
+
+    #[test]
+    fn last_contiguous_nonempty_dense_run() {
+        let mut s = Sheet::new("S");
+        for r in 0..2000u32 {
+            s.set_value(CellRef::new(r, 2), Value::Number(r as f64));
+        }
+        assert_eq!(s.last_contiguous_nonempty(2, 0, 1_999), 1999);
+        assert_eq!(s.last_contiguous_nonempty(2, 0, 1_000), 1000); // capped
+        assert_eq!(s.last_contiguous_nonempty(2, 500, 1_999), 1999);
+        assert_eq!(s.last_contiguous_nonempty(2, 2000, 3000), 1999); // nothing at from_row
+        assert_eq!(s.last_contiguous_nonempty(2, 2001, 3000), 2000); // from_row > cap: from_row - 1
+    }
+
+    #[test]
+    fn last_contiguous_nonempty_formula_with_empty_value_ends_the_run() {
+        let mut s = Sheet::new("S");
+        s.set_value(CellRef::new(0, 1), Value::Number(1.0));
+        s.set_value(CellRef::new(1, 1), Value::Number(2.0));
+        // row 2: a formula (e.g. `=""`) whose cached value is empty. A stored cell always
+        // decides the row on its own, exactly like `value_ref`: this does NOT count, same as
+        // `value_ref(...).is_some_and(|v| !v.is_empty())` would say for it, even though it has
+        // a formula (unlike `CellStore::has`, used for Ctrl+arrow navigation, which would).
+        s.set_cell(CellRef::new(2, 1), Cell { formula: Some(Arc::new(Formula::new("\"\""))), ..Default::default() });
+        s.set_value(CellRef::new(3, 1), Value::Number(4.0)); // would extend the run if row 2 didn't stop it
+        assert_eq!(s.last_contiguous_nonempty(1, 0, 100), 1);
+        assert_eq!(s.last_contiguous_nonempty(1, 2, 100), 1); // from_row itself is the stop
+    }
+
+    #[test]
+    fn last_contiguous_nonempty_spill_extends_an_unstored_row() {
+        let mut s = Sheet::new("S");
+        s.set_value(CellRef::new(0, 1), Value::Number(1.0));
+        // row 1: nothing stored, but a spill value from an array formula anchored elsewhere.
+        s.spill.insert(CellRef::new(1, 1), Value::Number(2.0));
+        s.set_value(CellRef::new(2, 1), Value::Number(3.0));
+        // row 3: a genuine gap (nothing stored, no spill).
+        assert_eq!(s.last_contiguous_nonempty(1, 0, 100), 2);
+        assert_eq!(s.last_contiguous_nonempty(1, 3, 100), 2); // from_row itself is the gap
+    }
+
     #[test]
     fn geometry() {
         let mut s = Sheet::new("S");

crates/model/src/store.rs

@@ -13,6 +13,10 @@ use crate::cell::Cell;
 
 const BAND: u32 = 64;
 
+fn has_value(cell: &Cell) -> bool {
+    !cell.value.is_empty() || cell.formula.is_some()
+}
+
 #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
 struct Band {
     /// row → (col → cell)
@@ -118,7 +122,45 @@ impl CellStore {
     }
     /// Last stored row in column `col` at or above `below_or_at` (for Ctrl+arrow navigation).
     pub fn has(&self, c: CellRef) -> bool {
-        self.get(c).is_some_and(|cell| !cell.value.is_empty() || cell.formula.is_some())
+        self.get(c).is_some_and(has_value)
+    }
+    /// Last row (inclusive, capped at `cap`) of the maximal run of contiguous rows at or after
+    /// `from.row` that store a cell in `from.col` with a non-empty *value* in `from.col`. `None`
+    /// when `from` itself doesn't qualify.
+    ///
+    /// Deliberately narrower than `has`/`get` (which also count a formula with no cached value
+    /// yet, for Ctrl+arrow navigation): this matches `Sheet::value_ref`, whose callers test the
+    /// `Value` it returns with `!v.is_empty()` regardless of whether a formula produced it — a
+    /// formula cell with an empty cached value (`=""`, or not yet recalculated) is "empty" to
+    /// them. A stored cell always decides the row on its own either way (`value_ref` never falls
+    /// through to `spill` once a cell is stored there, whether or not its value is empty), so
+    /// `last_contiguous_nonempty` only needs to check `spill` where this returns `None`, i.e.
+    /// no cell is stored at `from.col` for that row at all.
+    ///
+    /// Walks each band's row map with `BTreeMap::range` (one in-order pass per band) instead of
+    /// one `get` per row, so a long, densely-populated run costs roughly one tree-traversal step
+    /// per row rather than one binary search per row.
+    pub fn contiguous_value_run(&self, from: CellRef, cap: u32) -> Option<u32> {
+        let mut last = None;
+        let mut expect = from.row;
+        while expect <= cap {
+            let band_key = expect / BAND;
+            let Some(band) = self.bands.get(&band_key) else { break };
+            let band_hi = ((band_key + 1) * BAND - 1).min(cap);
+            let mut advanced = false;
+            for (r, cols) in band.rows.range(expect..=band_hi) {
+                if *r != expect || cols.get(&from.col).is_none_or(|c| c.value.is_empty()) {
+                    return last;
+                }
+                last = Some(expect);
+                expect += 1;
+                advanced = true;
+            }
+            if !advanced {
+                break;
+            }
+        }
+        last
     }
     /// Positions whose cells differ between `self` and `other` (stored on one side only, or not
     /// equal). Bands both stores share are skipped, so comparing an edited copy with the store

crates/ui-egui/src/grid.rs

@@ -2005,11 +2005,8 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: &
         // Fill down as far as the neighbouring column goes.
         let src = sel.current();
         let neighbour = if src.start.col > 0 { src.start.col - 1 } else { src.end.col + 1 };
-        let mut last = src.end.row;
-        while last + 1 < MAX_ROWS && sh.value_ref(CellRef::new(last + 1, neighbour)).is_some_and(|v| !v.is_empty()) && last - src.end.row < 1_000_000
-        {
-            last += 1;
-        }
+        let cap = src.end.row.saturating_add(1_000_000).min(MAX_ROWS - 1);
+        let last = if src.end.row < cap { sh.last_contiguous_nonempty(neighbour, src.end.row + 1, cap) } else { src.end.row };
         if last > src.end.row {
             let target = RangeRef::new(src.start, CellRef::new(last, src.end.col));
             let _ = app.run("edit.autoFill", json!({"source": src.a1(), "target": target.a1()}));