perf(grid): keep the whole-grid summary per layout and read a window over it; exact-key probe in DataStore (improvise-felo.13)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
committed by
Edward Langley
parent
b42aae2e08
commit
3b168a2ec8
@@ -24,7 +24,7 @@ use crate::persistence;
|
||||
use crate::ui::effect::{EffectKind, ModelChange, ViewTarget};
|
||||
use crate::ui::project::Dirty;
|
||||
use crate::ui::viewport::{
|
||||
compute_col_widths, compute_row_header_width, compute_visible_cols, grid_width,
|
||||
GridSummary, compute_row_header_width, compute_visible_cols, grid_summary, grid_width,
|
||||
};
|
||||
use crate::view::{GridLayout, View};
|
||||
use crate::workbook::Workbook;
|
||||
@@ -444,7 +444,7 @@ struct LayoutBasis {
|
||||
|
||||
/// What the keypress path measures of a built layout.
|
||||
pub(crate) struct GridGeometry {
|
||||
pub col_widths: Vec<u16>,
|
||||
pub summary: GridSummary,
|
||||
pub row_header_width: u16,
|
||||
}
|
||||
|
||||
@@ -468,6 +468,7 @@ pub struct App {
|
||||
pub term_height: u16,
|
||||
/// **Derived cache**: a pure function of the slices, rebuilt by
|
||||
/// `rebuild_layout()`. Owned by `App` because the rebuild is the host's job.
|
||||
/// A writer that bypasses effects must call it: a keypress no longer does.
|
||||
pub layout: GridLayout,
|
||||
/// How many times `rebuild_layout` has run: what a keypress that changes
|
||||
/// neither the model nor the view must leave alone (improvise-felo.13).
|
||||
@@ -584,7 +585,7 @@ impl App {
|
||||
width_passes: std::cell::Cell::new(0),
|
||||
geometry: std::cell::OnceCell::new(),
|
||||
layout_basis: LayoutBasis::default(),
|
||||
layout_stale: true,
|
||||
layout_stale: false,
|
||||
abort_effects: false,
|
||||
batch_aborted: false,
|
||||
batch_refused: false,
|
||||
@@ -598,6 +599,7 @@ impl App {
|
||||
keymap_set: KeymapSet::default_keymaps(),
|
||||
storage,
|
||||
};
|
||||
app.layout_basis = app.current_layout_basis();
|
||||
app.settle_normal_mode();
|
||||
app
|
||||
}
|
||||
@@ -963,7 +965,7 @@ impl App {
|
||||
let model = &self.model_state.workbook.model;
|
||||
let plan = FormatPlan::of(model, self.view());
|
||||
GridGeometry {
|
||||
col_widths: compute_col_widths(model, &self.layout, &plan),
|
||||
summary: grid_summary(model, &self.layout, &plan),
|
||||
row_header_width: compute_row_header_width(&self.layout),
|
||||
}
|
||||
})
|
||||
@@ -1049,7 +1051,7 @@ impl App {
|
||||
visible_cols: {
|
||||
let geometry = self.grid_geometry();
|
||||
compute_visible_cols(
|
||||
&geometry.col_widths,
|
||||
&geometry.summary.col_widths,
|
||||
geometry.row_header_width,
|
||||
self.grid_width(),
|
||||
cursor.col_offset,
|
||||
@@ -1986,7 +1988,7 @@ mod tests {
|
||||
let mut app = two_col_model();
|
||||
let down = || KeyEvent::new(KeyCode::Down, KeyModifiers::NONE);
|
||||
app.dispatch_key(down());
|
||||
let narrow = app.grid_geometry().col_widths.clone();
|
||||
let narrow = app.grid_geometry().summary.col_widths.clone();
|
||||
let (builds, widths) = (app.layout_builds, app.width_passes.get());
|
||||
let key = CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]);
|
||||
let long = CellValue::Text("a value far wider than a column".into());
|
||||
@@ -1995,8 +1997,8 @@ mod tests {
|
||||
app.dispatch_key(down());
|
||||
assert!(app.width_passes.get() > widths, "an edit must remeasure");
|
||||
let model = &app.model_state.workbook.model;
|
||||
let fresh = compute_col_widths(model, &app.layout, &FormatPlan::of(model, app.view()));
|
||||
assert_eq!(app.grid_geometry().col_widths, fresh, "stale widths");
|
||||
let fresh = crate::ui::viewport::compute_col_widths(model, &app.layout, &FormatPlan::of(model, app.view()));
|
||||
assert_eq!(app.grid_geometry().summary.col_widths, fresh, "stale widths");
|
||||
assert_ne!(fresh, narrow, "the edit widened a column, or this proves nothing");
|
||||
}
|
||||
|
||||
@@ -2249,6 +2251,7 @@ mod tests {
|
||||
.unwrap()
|
||||
.add_item(format!("R{i}"));
|
||||
}
|
||||
app.rebuild_layout();
|
||||
app.term_height = 28; // ~20 visible rows → delta = 15
|
||||
app.cursor_mut().selected = (0, 0);
|
||||
app.handle_key(KeyEvent::new(KeyCode::PageDown, KeyModifiers::NONE))
|
||||
@@ -2273,6 +2276,7 @@ mod tests {
|
||||
.unwrap()
|
||||
.add_item(format!("R{i}"));
|
||||
}
|
||||
app.rebuild_layout();
|
||||
app.term_height = 28;
|
||||
app.cursor_mut().selected = (20, 0);
|
||||
app.handle_key(KeyEvent::new(KeyCode::PageUp, KeyModifiers::NONE))
|
||||
@@ -2292,6 +2296,7 @@ mod tests {
|
||||
.unwrap()
|
||||
.add_item(format!("R{i}"));
|
||||
}
|
||||
app.rebuild_layout();
|
||||
app.term_height = 13; // ~5 visible rows
|
||||
app.cursor_mut().selected = (0, 0);
|
||||
// G jumps to last row (row 12)
|
||||
@@ -2319,6 +2324,7 @@ mod tests {
|
||||
.unwrap()
|
||||
.add_item(format!("R{i}"));
|
||||
}
|
||||
app.rebuild_layout();
|
||||
app.term_height = 13; // ~5 visible rows
|
||||
app.cursor_mut().selected = (0, 0);
|
||||
// Ctrl+d scrolls by 5 rows
|
||||
|
||||
@@ -25,7 +25,7 @@ use improvise_protocol::{
|
||||
use crate::ui::app::App;
|
||||
use crate::ui::effect::{EffectKind, ModelChange};
|
||||
use crate::ui::reduce::{Checked, apply_command};
|
||||
use crate::ui::viewport::{GridBody, grid_body};
|
||||
use crate::ui::viewport::{GridBody, grid_body, grid_window};
|
||||
|
||||
/// Which widgets a batch of effects could have changed. A gate, not a description:
|
||||
/// over-reporting costs work, under-reporting is a correctness bug (gotcha 35).
|
||||
@@ -144,7 +144,13 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache {
|
||||
let cols = viewport.cols_in(layout.col_count());
|
||||
// ONE walk: the widths, the totals and this window's values all come off the
|
||||
// same evaluation — `context/code/command.md` (improvise-jmjz).
|
||||
let body = grid_body(model, layout, &plan, rows.clone(), cols.clone());
|
||||
// The host's own layout keeps its whole-grid measurements until it is rebuilt, so
|
||||
// a window costs its own cells. Another view's grid has no such cache.
|
||||
let hosted = elsewhere.is_none() && derived.is_none();
|
||||
let body = match hosted {
|
||||
true => grid_window(model, layout, &app.grid_geometry().summary, rows.clone(), cols.clone()),
|
||||
false => grid_body(model, layout, &plan, rows.clone(), cols.clone()),
|
||||
};
|
||||
|
||||
GridCache {
|
||||
row_frame: axis_frame(&layout.row_cats, &layout.row_items, rows.clone()),
|
||||
|
||||
@@ -101,6 +101,50 @@ pub fn grid_body(
|
||||
}
|
||||
}
|
||||
|
||||
/// What of a grid build no window changes: the widths, each column's total
|
||||
/// format and the totals. Measured over every cell, so a host keeps one per layout.
|
||||
pub struct GridSummary {
|
||||
pub col_widths: Vec<u16>,
|
||||
pub col_formats: Vec<NumberFormat>,
|
||||
pub totals: Vec<Option<CellValue>>,
|
||||
}
|
||||
|
||||
/// The whole-grid walk, answering only what a window cannot.
|
||||
pub fn grid_summary(model: &Model, layout: &GridLayout, plan: &FormatPlan) -> GridSummary {
|
||||
let body = grid_body(model, layout, plan, 0..0, 0..0);
|
||||
GridSummary {
|
||||
col_widths: body.col_widths,
|
||||
col_formats: body.col_formats,
|
||||
totals: body.totals,
|
||||
}
|
||||
}
|
||||
|
||||
/// [`grid_body`] for a window, given the `summary` of the same layout: evaluates
|
||||
/// only the window's cells. Equal to `grid_body` where `summary` is current.
|
||||
pub fn grid_window(
|
||||
model: &Model,
|
||||
layout: &GridLayout,
|
||||
summary: &GridSummary,
|
||||
rows: std::ops::Range<usize>,
|
||||
cols: std::ops::Range<usize>,
|
||||
) -> GridBody {
|
||||
let eval = layout.evaluator(model);
|
||||
let records = layout.is_records_mode();
|
||||
let window = rows
|
||||
.map(|ri| {
|
||||
cols.clone()
|
||||
.map(|ci| cell_value(layout, &eval, records, ri, ci).1)
|
||||
.collect()
|
||||
})
|
||||
.collect();
|
||||
GridBody {
|
||||
col_widths: summary.col_widths.clone(),
|
||||
col_formats: summary.col_formats.clone(),
|
||||
totals: summary.totals.clone(),
|
||||
window,
|
||||
}
|
||||
}
|
||||
|
||||
/// The one evaluation. Records mode reads its wide row; pivot mode aggregates.
|
||||
fn cell_value(
|
||||
layout: &GridLayout,
|
||||
@@ -146,7 +190,7 @@ fn widen(widths: &mut [u16], col: usize, width: usize) {
|
||||
/// Adaptive column widths alone, for the keypress path, which needs no cells.
|
||||
/// It is [`grid_body`] with an empty window so there is one width rule.
|
||||
pub fn compute_col_widths(model: &Model, layout: &GridLayout, plan: &FormatPlan) -> Vec<u16> {
|
||||
grid_body(model, layout, plan, 0..0, 0..0).col_widths
|
||||
grid_summary(model, layout, plan).col_widths
|
||||
}
|
||||
|
||||
/// The total row-header width for a layout. The rule is the protocol's; this is the
|
||||
@@ -272,4 +316,26 @@ mod tests {
|
||||
"F0..F8 once for the Q1 row, once for the totals"
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-felo.13: a window read off a held summary is the window `grid_body`
|
||||
/// builds. Evidence: `context/test-evidence/layout-cost.md`.
|
||||
#[test]
|
||||
fn a_window_over_a_held_summary_equals_the_one_walk_grid_body() {
|
||||
let wb = folded_chain();
|
||||
let view = wb.active_view();
|
||||
let layout = GridLayout::new(&wb.model, view);
|
||||
let plan = FormatPlan::of(&wb.model, view);
|
||||
let summary = grid_summary(&wb.model, &layout, &plan);
|
||||
let (rows, cols) = (layout.row_count(), layout.col_count());
|
||||
assert!(rows >= 2 && cols >= 3, "a window strictly inside is possible");
|
||||
for (r, c) in [(0..rows, 0..cols), (1..2, 1..3), (rows - 1..rows, 0..1), (0..0, 0..0)] {
|
||||
let walked = grid_body(&wb.model, &layout, &plan, r.clone(), c.clone());
|
||||
let held = grid_window(&wb.model, &layout, &summary, r.clone(), c.clone());
|
||||
assert_eq!(held.window, walked.window, "window {r:?} x {c:?}");
|
||||
assert_eq!(held.col_widths, walked.col_widths);
|
||||
assert_eq!(held.totals, walked.totals);
|
||||
assert_eq!(held.col_formats, walked.col_formats);
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -194,6 +194,12 @@ impl RecordEntry {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
thread_local! {
|
||||
/// How many lookups fell through to set narrowing, for the exact-path test.
|
||||
static NARROWED: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
|
||||
}
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
pub struct DataStore {
|
||||
/// Primary storage: interned keys, insertion-ordered so records mode can
|
||||
@@ -206,6 +212,9 @@ pub struct DataStore {
|
||||
/// Reverse index for the client-facing surrogate (ADR 002), plus each
|
||||
/// record's write counter. Why it lives here: core.md §8.
|
||||
records: HashMap<RecordHash, RecordEntry>,
|
||||
/// How many stored cells have each number of coordinates. Its largest key
|
||||
/// says when a full-width partial can only match one exact cell (§8).
|
||||
arities: std::collections::BTreeMap<usize, usize>,
|
||||
}
|
||||
|
||||
/// Resolved through the symbol table and sorted wherever the storage is a
|
||||
@@ -217,6 +226,7 @@ impl std::fmt::Debug for DataStore {
|
||||
symbols,
|
||||
index,
|
||||
records,
|
||||
arities: _,
|
||||
} = self;
|
||||
let cells: Vec<_> = cells
|
||||
.iter()
|
||||
@@ -291,7 +301,11 @@ impl DataStore {
|
||||
for pair in &ikey.0 {
|
||||
self.index.entry(*pair).or_default().insert(ikey.clone());
|
||||
}
|
||||
let width = ikey.0.len();
|
||||
let is_new_cell = self.cells.insert(ikey, value).is_none();
|
||||
if is_new_cell {
|
||||
*self.arities.entry(width).or_default() += 1;
|
||||
}
|
||||
let entry = self.records.entry(hash).or_insert(RecordEntry {
|
||||
record,
|
||||
cells: 0,
|
||||
@@ -327,6 +341,12 @@ impl DataStore {
|
||||
return;
|
||||
};
|
||||
if self.cells.shift_remove(&ikey).is_some() {
|
||||
if let Some(n) = self.arities.get_mut(&ikey.0.len()) {
|
||||
*n -= 1;
|
||||
if *n == 0 {
|
||||
self.arities.remove(&ikey.0.len());
|
||||
}
|
||||
}
|
||||
for pair in &ikey.0 {
|
||||
if let Some(set) = self.index.get_mut(pair) {
|
||||
set.remove(&ikey);
|
||||
@@ -378,11 +398,29 @@ impl DataStore {
|
||||
/// Positions in `cells` of every cell matching `partial`, **in insertion
|
||||
/// order**. The index narrows; the primary map orders (§8).
|
||||
fn matching_positions(&self, partial: &[(String, String)]) -> Vec<usize> {
|
||||
if let Some(exact) = self.exact_positions(partial) {
|
||||
return exact;
|
||||
}
|
||||
self.intern_partial(partial)
|
||||
.map(|pairs| self.positions_of(&pairs))
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// The answer without narrowing, when `partial` names as many coordinates as
|
||||
/// the widest stored cell has: only a cell equal to it can match (core.md §8).
|
||||
/// `None` where that does not hold, and the narrowing path answers.
|
||||
fn exact_positions(&self, partial: &[(String, String)]) -> Option<Vec<usize>> {
|
||||
let widest = *self.arities.keys().next_back()?;
|
||||
// Strictly sorted by category, the order a stored key is in, so one probe is the lookup.
|
||||
let canonical = partial.windows(2).all(|w| w[0].0 < w[1].0);
|
||||
(!partial.is_empty() && widest <= partial.len() && canonical).then(|| {
|
||||
let found = self.intern_partial(partial).and_then(|pairs| {
|
||||
self.cells.get_index_of(&InternedKey(pairs))
|
||||
});
|
||||
found.into_iter().collect()
|
||||
})
|
||||
}
|
||||
|
||||
/// `partial` as symbols, or `None` when a name was never interned, which
|
||||
/// no stored cell can then match.
|
||||
fn intern_partial(&self, partial: &[(String, String)]) -> Option<Vec<(Symbol, Symbol)>> {
|
||||
@@ -412,6 +450,8 @@ impl DataStore {
|
||||
}
|
||||
|
||||
sets.sort_by_key(|s| s.len());
|
||||
#[cfg(test)]
|
||||
NARROWED.with(|n| n.set(n.get() + 1));
|
||||
|
||||
let mut positions: Vec<usize> = match sets.split_first() {
|
||||
Some((first, rest)) => first
|
||||
@@ -1356,3 +1396,76 @@ mod record_index {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod exact_path_tests {
|
||||
use super::{CellKey, CellValue, DataStore, NARROWED};
|
||||
|
||||
fn key(a: &str, b: &str, c: &str) -> CellKey {
|
||||
CellKey::new(vec![("A".into(), a.into()), ("B".into(), b.into()), ("C".into(), c.into())])
|
||||
}
|
||||
|
||||
fn store() -> DataStore {
|
||||
let mut d = DataStore::new();
|
||||
for i in 0..6 {
|
||||
for j in 0..6 {
|
||||
d.set(key(&format!("a{i}"), &format!("b{j}"), "c"), CellValue::Number((i * 6 + j) as f64));
|
||||
}
|
||||
}
|
||||
d
|
||||
}
|
||||
|
||||
fn narrowings(f: impl FnOnce()) -> usize {
|
||||
NARROWED.with(|n| n.set(0));
|
||||
f();
|
||||
NARROWED.with(|n| n.get())
|
||||
}
|
||||
|
||||
/// improvise-felo.13: context/test-evidence/layout-cost.md.
|
||||
#[test]
|
||||
fn a_full_width_partial_probes_one_cell_instead_of_narrowing() {
|
||||
let d = store();
|
||||
let full = key("a2", "b3", "c").0;
|
||||
let mut got = Vec::new();
|
||||
assert_eq!(narrowings(|| got = d.matching_values(&full).into_iter().cloned().collect()), 0);
|
||||
assert_eq!(got, vec![CellValue::Number(15.0)]);
|
||||
let missing = key("a2", "b9", "c").0;
|
||||
assert!(d.matching_values(&missing).is_empty(), "an absent cell matches nothing");
|
||||
let unknown = key("a2", "b3", "zzz").0;
|
||||
assert!(d.matching_values(&unknown).is_empty(), "an unseen item matches nothing");
|
||||
}
|
||||
|
||||
/// The control: a narrower partial still sums its supersets, so the shortcut
|
||||
/// is not a blanket exact lookup.
|
||||
#[test]
|
||||
fn a_narrower_partial_still_narrows_and_matches_every_superset() {
|
||||
let d = store();
|
||||
let narrow = vec![("A".to_string(), "a2".to_string()), ("C".to_string(), "c".to_string())];
|
||||
let mut n = 0;
|
||||
assert_eq!(narrowings(|| n = d.matching_values(&narrow).len()), 1);
|
||||
assert_eq!(n, 6);
|
||||
}
|
||||
|
||||
/// A wider cell appearing, or leaving, moves the line the shortcut is allowed at.
|
||||
#[test]
|
||||
fn a_wider_cell_turns_the_shortcut_off_until_it_is_removed() {
|
||||
let mut d = store();
|
||||
let wide = CellKey::new(vec![
|
||||
("A".into(), "a2".into()), ("B".into(), "b3".into()), ("C".into(), "c".into()), ("D".into(), "d".into()),
|
||||
]);
|
||||
d.set(wide.clone(), CellValue::Number(100.0));
|
||||
let full = key("a2", "b3", "c").0;
|
||||
assert_eq!(d.matching_values(&full).len(), 2, "the exact cell and the wider cell under it");
|
||||
d.remove(&wide);
|
||||
assert_eq!(narrowings(|| assert_eq!(d.matching_values(&full).len(), 1)), 0);
|
||||
}
|
||||
|
||||
/// An unsorted partial is not a stored key's order, so it must not be probed.
|
||||
#[test]
|
||||
fn an_unsorted_full_width_partial_is_still_answered() {
|
||||
let d = store();
|
||||
let mut full = key("a2", "b3", "c").0;
|
||||
full.reverse();
|
||||
assert_eq!(d.matching_values(&full).len(), 1);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user