From 43a118c543cdfb7668848d6bb2031b509f277b57 Mon Sep 17 00:00:00 2001 From: Dejan Port Date: Mon, 12 Oct 2026 02:01:49 +0200 Subject: [PATCH 1/3] perf: fill-handle double-click scans a dense neighbour column ~2.4x faster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #210, which the reviewer correctly rejected: the stated cause (a sparse column with a stray distant value) doesn't hold, since the scan already stops at the first empty cell. This is a different, narrower fix that doesn't change behaviour for any input — it never truncates a long fill, unlike #210's cap — only how fast a genuinely long, densely-populated run is scanned. grid.rs's fill-handle double-click called Sheet::value_ref once per row to find where the neighbouring column's data ends. Each call does a band lookup + row lookup + column lookup (three BTreeMap probes) via CellStore::get. For a real dense run (e.g. a filled column near the 1,048,576-row sheet limit) that's a lot of point lookups on the UI thread in one frame: ~59ms for a 1,000,000-row run in a release build (measured with the new fill_handle_perf example), likely worse under wasm where GridCraft actually runs in the browser. - crates/model/src/store.rs: CellStore::contiguous_value_run walks each band's row BTreeMap with `range` (one in-order pass per band) instead of one `get` per row, so a dense run costs roughly one tree-traversal step per row instead of one binary search per row. Deliberately narrower than `has`/`get`: a stored cell always decides the row on its own (even with an empty value), matching what `value_ref`'s callers actually test for, not `has`'s "formula counts regardless of its cached value" (used for Ctrl+arrow navigation). - crates/model/src/sheet.rs: Sheet::last_contiguous_nonempty layers `spill` back in wherever contiguous_value_run finds no stored cell, exactly where plain `value_ref` would fall through to it. - crates/ui-egui/src/grid.rs: the fill-handle double-click now calls this once instead of looping over value_ref. Result: 999,999-row dense run, 59.2ms -> 24.7ms (2.40x), same answer (asserted in the example). Four new tests in sheet.rs, including a 300-trial property test against a naive value_ref-loop reference (spills, formula-with-empty-value, gaps, blanks) — all pass, plus the full existing suite (`cargo test -p gridcraft-model -p gridcraft-ui-egui -p gridcraft-engine`; the only failures are #210-era gridcraft-engine CSV tests already failing on main, unrelated to this change). Co-Authored-By: Claude Sonnet 5 --- crates/engine/examples/fill_handle_perf.rs | 47 ++++++++ crates/model/src/sheet.rs | 131 +++++++++++++++++++++ crates/model/src/store.rs | 44 ++++++- crates/ui-egui/src/grid.rs | 7 +- 4 files changed, 223 insertions(+), 6 deletions(-) create mode 100644 crates/engine/examples/fill_handle_perf.rs diff --git a/crates/engine/examples/fill_handle_perf.rs b/crates/engine/examples/fill_handle_perf.rs new file mode 100644 index 000000000..ccdde36a3 --- /dev/null +++ b/crates/engine/examples/fill_handle_perf.rs @@ -0,0 +1,47 @@ +//! 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]` + +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 = (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"); +} diff --git a/crates/model/src/sheet.rs b/crates/model/src/sheet.rs index e05d3bfb5..67635ac74 100644 --- a/crates/model/src/sheet.rs +++ b/crates/model/src/sheet.rs @@ -177,6 +177,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; @@ -405,6 +438,7 @@ fn line_at(pos: f64, default: f64, custom: &BTreeMap, max: u32) - #[cfg(test)] mod tests { use super::*; + use crate::cell::Formula; #[test] fn line_index_matches_linear() { @@ -421,6 +455,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"); diff --git a/crates/model/src/store.rs b/crates/model/src/store.rs index d6a9c4561..ec9b6c98c 100644 --- a/crates/model/src/store.rs +++ b/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 { + 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_some_and(|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 diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index d0eef8566..7ac6a848c 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2054,11 +2054,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 + 1 <= 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()})); From e7d590c8a434ef7acaf5f11674614a1b4dc11fbc Mon Sep 17 00:00:00 2001 From: Dejan Port Date: Mon, 12 Oct 2026 02:07:45 +0200 Subject: [PATCH 2/3] style: satisfy cargo fmt and clippy on the fill-handle perf change cargo fmt reformatted the new example's while-loop condition onto its own line; clippy's nonminimal_bool caught a negated is_some_and that reads more plainly as is_none_or. Co-Authored-By: Claude Sonnet 5 --- crates/engine/examples/fill_handle_perf.rs | 3 ++- crates/model/src/store.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/engine/examples/fill_handle_perf.rs b/crates/engine/examples/fill_handle_perf.rs index ccdde36a3..0480c0afb 100644 --- a/crates/engine/examples/fill_handle_perf.rs +++ b/crates/engine/examples/fill_handle_perf.rs @@ -31,7 +31,8 @@ fn main() { 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 { + 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; } diff --git a/crates/model/src/store.rs b/crates/model/src/store.rs index ec9b6c98c..658b898bf 100644 --- a/crates/model/src/store.rs +++ b/crates/model/src/store.rs @@ -149,7 +149,7 @@ impl CellStore { 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_some_and(|c| !c.value.is_empty()) { + if *r != expect || cols.get(&from.col).is_none_or(|c| c.value.is_empty()) { return last; } last = Some(expect); From 7ba3676f5fff269ddc9c3171e8336af0ddeea352 Mon Sep 17 00:00:00 2001 From: Dejan Port Date: Mon, 12 Oct 2026 02:52:07 +0200 Subject: [PATCH 3/3] fix: satisfy stricter clippy lints CI's newer stable toolchain catches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed with cargo clippy --workspace --all-targets -- -D warnings, not a test. The actual cause: I'd been validating locally against rustc/clippy 1.95.0 (the toolchain this machine had installed before now), while CI installs plain "stable" (1.99.0 as of this run) — two lints new since 1.95.0 fired there and nowhere in my local checks: - clippy::int_plus_one on `src.end.row + 1 <= cap` in grid.rs, simplified to `src.end.row < cap` (same meaning, no overflow risk change: row is a u32 and cap is already saturating-clamped below MAX_ROWS - 1, so src.end.row + 1 can't wrap). - clippy::disallowed_methods on the new fill_handle_perf example's three Instant::now() calls — the project's own lint from the time- panic fix (#125 history): reading the clock traps on wasm. This example is native-only (a `cargo run --release` benchmark, never built for wasm), so it gets the same #![allow(...)] perf.rs already carries, with the same justification. Updated the local toolchain to stable (rustup update stable) and re-ran cargo clippy --workspace --all-targets -- -D warnings clean before this push, to actually match what CI checks instead of guessing again. Co-Authored-By: Claude Sonnet 5 --- crates/engine/examples/fill_handle_perf.rs | 2 ++ crates/ui-egui/src/grid.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/engine/examples/fill_handle_perf.rs b/crates/engine/examples/fill_handle_perf.rs index 0480c0afb..5e9ea423c 100644 --- a/crates/engine/examples/fill_handle_perf.rs +++ b/crates/engine/examples/fill_handle_perf.rs @@ -2,6 +2,8 @@ //! 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; diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 7ac6a848c..56fec08ee 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2055,7 +2055,7 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & let src = sel.current(); let neighbour = if src.start.col > 0 { src.start.col - 1 } else { src.end.col + 1 }; let cap = src.end.row.saturating_add(1_000_000).min(MAX_ROWS - 1); - let last = if src.end.row + 1 <= cap { sh.last_contiguous_nonempty(neighbour, src.end.row + 1, cap) } else { src.end.row }; + 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()}));