Merge pull request #247 from fiddlerwoaroof/collapse-fu/h6z1

improvise-h6z1: one fold memo per grid build (stacked on #239)

benches/grid_projection.rs

@@ -54,6 +54,57 @@ fn dense_app(regions: usize, products: usize, measures: usize) -> App {
     app
 }
 
+/// Collapsed groups on the row axis and a formula chain on the column axis, so
+/// every fold cell and every total re-derives the chain (improvise-h6z1).
+fn chain_app(regions: usize, products: usize) -> App {
+    let mut wb = Workbook::new("Chain");
+    wb.add_category("Region").unwrap();
+    wb.add_category("Product").unwrap();
+    for r in 0..regions {
+        wb.add_item_in_group("Region", format!("Region{r}"), format!("G{}", r / 10))
+            .unwrap();
+    }
+    for p in 0..products {
+        wb.add_item("Product", format!("Product{p}")).unwrap();
+    }
+    wb.add_item("_Measure", "Revenue").unwrap();
+    let chain = [
+        "F1 = Revenue * 2",
+        "F2 = F1 + Revenue",
+        "F3 = F2 + F1",
+        "F4 = F3 + F2",
+        "F5 = F4 / F1",
+    ];
+    for text in chain {
+        let formula = improvise::formula::parse_formula(text, "_Measure").unwrap();
+        wb.model.add_formula(formula);
+    }
+    {
+        let v = wb.active_view_mut();
+        v.set_axis("Region", Axis::Row);
+        v.set_axis("Product", Axis::Row);
+        v.set_axis("_Measure", Axis::Column);
+        for g in 0..regions.div_ceil(10) {
+            v.toggle_group_collapse("Region", &format!("G{g}"));
+        }
+    }
+    let mut n = 0.0f64;
+    for r in 0..regions {
+        for p in 0..products {
+            n += 1.0;
+            let key = CellKey::new(vec![
+                ("Region".to_string(), format!("Region{r}")),
+                ("Product".to_string(), format!("Product{p}")),
+                ("_Measure".to_string(), "Revenue".to_string()),
+            ]);
+            wb.model.set_cell(key, CellValue::Number(n));
+        }
+    }
+    let mut app = App::new(wb, None);
+    app.rebuild_layout();
+    app
+}
+
 /// Days in collapsed groups of `per_group` on Row, with Product: every row is a
 /// fold over a few days, never the whole category (improvise-jq0m).
 fn grouped_app(days: usize, per_group: usize, products: usize, measures: usize) -> App {
@@ -187,6 +238,9 @@ fn main() {
     case("large", 60, 20, 8, 50);
     case("18k cells (gotcha 41)", 150, 20, 6, 20);
     case("pathological", 300, 30, 10, 5);
+    println!("\nfolded rows and a five-formula chain (improvise-h6z1)\n");
+    report("folded chain ~18k", chain_app(1000, 30), 5);
+    report("folded chain ~90k", chain_app(5000, 30), 3);
     println!("\ncollapsed small groups on the row axis (improvise-jq0m)\n");
     report("364 days in weeks", grouped_app(364, 7, 20, 6), 5);
     report("364 days in pairs", grouped_app(364, 2, 20, 6), 5);

context/code/command.md

@@ -1903,6 +1903,13 @@ while the spanning fold reads the store. Sharing one total between
 two, so sharing needs an invalidation story. Dropping the totals from the widths
 would make the two width computations disagree about `visible_cols`.
 
+**A grid-wide fold memo (improvise-h6z1)** is the other half, and it answers a
+different cost. It removes the repeated **formula** chains that a fold row and a
+total re-derive, about 2x on a folded five-formula chain. It does **not** move
+the table below. Those grids hold only stored measures, a stored fold is never
+memoised, and each total reads its column's records once. That one read per
+column is what the scan above made cheap.
+
 **What it changes besides speed:** a stored fold's values now arrive in the
 store's insertion order, not member order. That is the order gotcha 18 names as
 the store's answer. Min, Max, Count and text unanimity are order-free. A Sum or

context/code/core.md

@@ -929,8 +929,18 @@ every member. Five decisions:
     without the memo, about 60 ms with it, and about 10 ms on gh/main. gh/main
     was cheap because it read the members out of the formula cache, which is
     what gave the wrong ratios. The remaining ~6× is each column's cell
-    re-deriving the chain below it (45 evaluations per row where 9 would do). A
-    memo scoped to the whole grid would remove it; it is not built.
+    re-deriving the chain below it (45 evaluations per row where 9 would do).
+    **The memo is now shared across one grid build** (improvise-h6z1).
+    `viewport::grid_body` makes one `FoldMemo` and passes it through
+    `GridLayout::evaluate_in` and `column_total` for every cell and total, so a
+    fold row's chain is evaluated 9 times, not 45. A memo is valid only for the
+    model state it read. That is why it is made per build and never stored on
+    `GridLayout`, which outlives cell writes.
+    The memo is keyed by the fold key **and** the span's categories. Within one
+    grid build the span half cannot yet differ for one key, because a cell's key
+    always names the row categories and a total's never does. It is kept because
+    `evaluate_spanning_in` is public, and a caller sharing one memo across two
+    spans would otherwise read the wrong fold. No test can see it today.
 - **A fold covers the members the grid draws** (improvise-f2oo). Hidden members
   are not in it. So a fold is exactly the rows it replaces, the total of a
   column's total does not move on `z`, and a group whose every member is hidden

context/test-evidence/core.md

@@ -1512,6 +1512,20 @@ are each compared with the same document with Month on `Axis::None`. At
 cannot pass. Rival: `read_coordinate` ignoring the reach (`if false && …`).
 This test fails.
 
+### a_fold_row_evaluates_each_formula_once_across_the_row
+
+improvise-h6z1. `F0 = Revenue`, `F{i} = F{i-1} + F{i-1}` up to F8, as columns,
+with Q1 collapsed so the Q1 row is a fold. The test evaluates every cell of that
+row twice and compares the two runs: once with a fresh memo per cell (`evaluate`),
+where each column re-derives the chain below it (1 + 2 + … + 9 = **45**), and
+once with one memo shared across the row (`evaluate_in`), which gives **9**. It
+also asserts the two runs produce identical values, so a memo that returned a
+wrong entry cannot pass. The 45 is asserted as a setup, so a fixture whose chain
+had collapsed into one column could not pass the 9 by accident. Rival:
+`evaluate_covering_in` ignoring the memo it is given and making a fresh one.
+Injected into a full workspace run, it fails this test (45 against 9) and
+nothing else.
+
 ### a_spanned_scan_finds_what_a_lookup_per_member_finds
 
 improvise-yb4b. `fold_stored` used to enumerate one `CellKey` per member of a

crates/improvise-command/src/ui/viewport.rs

@@ -9,6 +9,7 @@ use crate::format::{FormatPlan, NumberFormat};
 use crate::model::Model;
 use crate::model::category::MEASURE_CATEGORY;
 use crate::model::cell::{CellKey, CellValue};
+use crate::model::types::FoldMemo;
 use crate::view::{AxisEntry, GridLayout};
 
 /// Minimum column width — enough for short numbers/labels + 1 char gap.
@@ -46,11 +47,16 @@ pub fn grid_body(
     let n = layout.col_count();
     let records = layout.is_records_mode();
     let mut widths = header_level_seed(layout, n);
+    // One memo for the whole build: a fold every cell and total reaches is
+    // evaluated once (improvise-h6z1). The model does not change during it.
+    let memo = FoldMemo::default();
     // A column mixing measures has no total to show: `context/code/command.md`.
     let measures_on_rows = layout.row_cats.iter().any(|c| c == MEASURE_CATEGORY);
     let totalled = !records && !measures_on_rows && layout.row_count() > 0 && n > 0;
     let totals: Vec<Option<CellValue>> = match totalled {
-        true => (0..n).map(|ci| layout.column_total(model, ci)).collect(),
+        true => (0..n)
+            .map(|ci| layout.column_total(model, ci, &memo))
+            .collect(),
         false => Vec::new(),
     };
     let mut window: Vec<Vec<Option<CellValue>>> = Vec::with_capacity(rows.len());
@@ -67,7 +73,7 @@ pub fn grid_body(
             false => Vec::new(),
         };
         for ci in 0..n {
-            let (key, value) = cell_value(model, layout, records, ri, ci);
+            let (key, value) = cell_value(model, layout, records, ri, ci, &memo);
             let fmt = plan.at(key.as_ref());
             let text = match records {
                 true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(),
@@ -105,12 +111,15 @@ fn cell_value(
     records: bool,
     row: usize,
     col: usize,
+    memo: &FoldMemo,
 ) -> (Option<CellKey>, Option<CellValue>) {
     match records {
         true => (None, layout.records_value(row, col)),
         false => {
             let key = layout.cell_key(row, col);
-            let value = key.as_ref().and_then(|k| layout.evaluate(model, k));
+            let value = key
+                .as_ref()
+                .and_then(|k| layout.evaluate_in(model, k, memo));
             (key, value)
         }
     }

crates/improvise-core/src/model/types.rs

@@ -140,12 +140,28 @@ fn agg_values(
 // How many formulas a fold has evaluated: what improvise-o1y0's regression counts.
 #[cfg(test)]
 thread_local! {
-    static FOLD_FORMULA_EVALUATIONS: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
+    pub(crate) static FOLD_FORMULA_EVALUATIONS: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
 }
 
-/// One fold evaluation's memory: each formula key is evaluated once, and a key met
-/// while still in progress (`None`) is a cycle (improvise-o1y0).
-type FoldMemo = std::cell::RefCell<HashMap<CellKey, Option<Option<CellValue>>>>;
+/// Each (fold key, span) evaluated once across one grid build (improvise-o1y0,
+/// improvise-h6z1). Valid for one model state only: `context/code/core.md` §14.
+#[derive(Default)]
+pub struct FoldMemo(std::cell::RefCell<HashMap<(CellKey, Vec<String>), MemoEntry>>);
+
+/// `None` while in progress, which is how a cycle is seen (improvise-o1y0).
+type MemoEntry = Option<Option<CellValue>>;
+
+impl FoldMemo {
+    fn get(&self, key: &CellKey, span: Span) -> Option<MemoEntry> {
+        let at = (key.clone(), span.over.to_vec());
+        self.0.borrow().get(&at).cloned()
+    }
+
+    fn put(&self, key: &CellKey, span: Span, entry: MemoEntry) {
+        let at = (key.clone(), span.over.to_vec());
+        self.0.borrow_mut().insert(at, entry);
+    }
+}
 
 /// Which members of a collapsed group its fold covers. The model knows no
 /// view, so the grid passes the members it draws (core.md §14).
@@ -685,6 +701,19 @@ impl Model {
         self.evaluate_fold(key, agg, span, &FoldMemo::default())
     }
 
+    /// [`Self::evaluate_covering`] sharing `memo` with every other evaluation of one
+    /// grid build, which must not outlive the model state it read (improvise-h6z1).
+    pub fn evaluate_covering_in(
+        &self,
+        key: &CellKey,
+        agg: Aggregation,
+        covers: Covers,
+        memo: &FoldMemo,
+    ) -> Option<CellValue> {
+        let span = Span { covers, over: &[] };
+        self.evaluate_fold(key, agg, span, memo)
+    }
+
     /// The value `key` has as ONE fold over every covered item of the `over`
     /// categories it omits — a column's total (improvise-wm20, core.md §14).
     pub fn evaluate_spanning(
@@ -693,9 +722,21 @@ impl Model {
         over: &[String],
         agg: Aggregation,
         covers: Covers,
+    ) -> Option<CellValue> {
+        self.evaluate_spanning_in(key, over, agg, covers, &FoldMemo::default())
+    }
+
+    /// [`Self::evaluate_spanning`] sharing one grid build's `memo` (improvise-h6z1).
+    pub fn evaluate_spanning_in(
+        &self,
+        key: &CellKey,
+        over: &[String],
+        agg: Aggregation,
+        covers: Covers,
+        memo: &FoldMemo,
     ) -> Option<CellValue> {
         let span = Span { covers, over };
-        self.evaluate_fold(key, agg, span, &FoldMemo::default())
+        self.evaluate_fold(key, agg, span, memo)
     }
 
     /// A key naming a group folds; any other key reads as before. A formula is
@@ -726,17 +767,17 @@ impl Model {
         expansion: &Expansion,
         (agg, span, memo): (Aggregation, Span, &FoldMemo),
     ) -> Option<CellValue> {
-        let seen = memo.borrow().get(key).cloned();
+        let seen = memo.get(key, span);
         match seen {
             Some(Some(value)) => value,
             Some(None) => Some(CellValue::Error("circular".into())),
             None => {
-                memo.borrow_mut().insert(key.clone(), None);
+                memo.put(key, span, None);
                 #[cfg(test)]
                 FOLD_FORMULA_EVALUATIONS.with(|n| n.set(n.get() + 1));
                 let reach = Reach::Fold { agg, span, memo };
                 let value = self.eval_formula_folded(formula, key, expansion, agg, reach);
-                memo.borrow_mut().insert(key.clone(), Some(value.clone()));
+                memo.put(key, span, Some(value.clone()));
                 value
             }
         }

crates/improvise-core/src/view/layout.rs

@@ -3,6 +3,7 @@ use std::rc::Rc;
 
 use crate::model::category::{DIM_CATEGORY, INDEX_CATEGORY, MEASURE_CATEGORY};
 use crate::model::cell::{CellKey, CellValue};
+use crate::model::types::FoldMemo;
 use crate::model::{Aggregation, Model};
 use crate::view::{Axis, View};
 
@@ -657,17 +658,22 @@ impl GridLayout {
     /// The value of a pivot key as THIS grid reads it: aggregated over its None
     /// axis, and a fold over the members it draws (core.md §14). One evaluation.
     pub fn evaluate(&self, model: &Model, key: &CellKey) -> Option<CellValue> {
+        self.evaluate_in(model, key, &FoldMemo::default())
+    }
+
+    /// [`Self::evaluate`] sharing one grid build's `memo` (improvise-h6z1).
+    pub fn evaluate_in(&self, model: &Model, key: &CellKey, memo: &FoldMemo) -> Option<CellValue> {
         let drawn = |cat: &str, item: &str| !self.hidden.get(cat).is_some_and(|h| h.contains(item));
-        model.evaluate_covering(key, Aggregation::of(&self.none_cats), &drawn)
+        model.evaluate_covering_in(key, Aggregation::of(&self.none_cats), &drawn, memo)
     }
 
     /// A column's total: its cell as if every drawn row were ONE fold, so a
     /// formula is evaluated over the pooled rows (improvise-wm20, core.md §14).
-    pub fn column_total(&self, model: &Model, col: usize) -> Option<CellValue> {
+    pub fn column_total(&self, model: &Model, col: usize, memo: &FoldMemo) -> Option<CellValue> {
         let key = self.column_key(col)?;
         let drawn = |cat: &str, item: &str| !self.hidden.get(cat).is_some_and(|h| h.contains(item));
         let agg = Aggregation::of(&self.none_cats);
-        model.evaluate_spanning(&key, &self.row_cats, agg, &drawn)
+        model.evaluate_spanning_in(&key, &self.row_cats, agg, &drawn, memo)
     }
 
     /// Display text for (row, col) in either mode.
@@ -904,6 +910,7 @@ mod tests {
         prune_axis_items, synthetic_record_info,
     };
     use crate::model::cell::{CellKey, CellValue};
+    use crate::model::types::{FOLD_FORMULA_EVALUATIONS, FoldMemo};
     use crate::view::Axis;
     use crate::workbook::Workbook;
 
@@ -1841,6 +1848,46 @@ mod tests {
             .and_then(|v| v.as_f64())
     }
 
+    /// improvise-h6z1: one grid build evaluates each formula a fold row reaches
+    /// once, not once per column. context/test-evidence/core.md.
+    #[test]
+    fn a_fold_row_evaluates_each_formula_once_across_the_row() {
+        let chain: Vec<String> = std::iter::once("F0 = Revenue".to_string())
+            .chain((1..=8).map(|i| format!("F{i} = F{p} + F{p}", p = i - 1)))
+            .collect();
+        let chain: Vec<&str> = chain.iter().map(String::as_str).collect();
+        let mut wb = margin_book(&chain);
+        wb.active_view_mut().toggle_group_collapse("Month", "Q1");
+        wb.model.recompute_formulas(&wb.active_view().none_cats());
+        let layout = GridLayout::new(&wb.model, wb.active_view());
+        let q1 = (0..layout.row_count())
+            .find(|&r| layout.row_label(r) == "Q1")
+            .expect("a Q1 fold row");
+        let row_keys: Vec<CellKey> = (0..layout.col_count())
+            .filter_map(|c| layout.cell_key(q1, c))
+            .collect();
+        let evaluations = |shared: bool| {
+            let memo = FoldMemo::default();
+            FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0));
+            let values: Vec<_> = row_keys
+                .iter()
+                .map(|k| match shared {
+                    true => layout.evaluate_in(&wb.model, k, &memo),
+                    false => layout.evaluate(&wb.model, k),
+                })
+                .collect();
+            (FOLD_FORMULA_EVALUATIONS.with(|n| n.get()), values)
+        };
+        let (apart, separately) = evaluations(false);
+        let (together, shared) = evaluations(true);
+        assert_eq!(shared, separately, "sharing the memo changed a value");
+        assert_eq!(
+            apart, 45,
+            "setup: each column re-derives the chain below it"
+        );
+        assert_eq!(together, 9, "F0..F8 once each across the whole row");
+    }
+
     /// improvise-eccl — see context/test-evidence/core.md.
     #[test]
     fn a_folds_ratio_formula_is_the_ratio_of_its_folded_inputs() {