address review: store running-total entries only for items with a number

crates/engine/src/cmd/solver/reports.rs

@@ -52,19 +52,14 @@ pub(super) fn abs_ref(c: CellRef) -> String {
     format!("${}${}", gridcraft_core::col_to_letters(c.col), u64::from(c.row) + 1)
 }
 
-/// How many cells to the left and above a cell its name is searched for.
-const NAME_REACH: u32 = 1_000;
-
 /// A cell's name in the reports: the nearest text to its left and the nearest text above it.
 pub(super) fn cell_name(sh: &Sheet, c: CellRef) -> String {
     let text = |c: CellRef| match sh.value(c) {
         Value::Text(t) if !t.trim().is_empty() => Some(t.to_string()),
         _ => None,
     };
-    // Labels are next to their cells: looking further than this on a sparse sheet only costs time
-    // (a thousand cells near the last row would each probe a million empty coordinates).
-    let left = (c.col.saturating_sub(NAME_REACH)..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
-    let top = (c.row.saturating_sub(NAME_REACH)..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
+    let left = (0..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
+    let top = (0..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
     [left, top].into_iter().flatten().collect::<Vec<_>>().join(" ")
 }
 
@@ -443,17 +438,3 @@ pub(super) fn report(s: &mut Session, p: &Json) -> Result<Json> {
     }
     Ok(out)
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn a_name_is_searched_only_a_bounded_distance() {
-        let mut sh = Sheet::new("S".to_string());
-        sh.set_cell(CellRef::new(0, 0), gridcraft_model::Cell::value(Value::text("Far")));
-        sh.set_cell(CellRef::new(5, 4), gridcraft_model::Cell::value(Value::text("Near")));
-        assert_eq!(cell_name(&sh, CellRef::new(5, 5)), "Near");
-        assert_eq!(cell_name(&sh, CellRef::new(0, NAME_REACH + 1)), "");
-    }
-}

crates/engine/src/cmd/whatif/scenarios.rs

@@ -189,9 +189,6 @@ pub(super) fn scenario_delete(s: &mut Session, p: &Json) -> Result<Json> {
     Ok(json!({"deleted": removed.name}))
 }
 
-/// Most changing-cell-by-scenario cells one Scenario Summary writes.
-const MAX_SUMMARY_CELLS: usize = 1_000_000;
-
 pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     const CMD: &str = "data.scenarioSummary";
     let d = s.doc()?;
@@ -214,24 +211,17 @@ pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     // Changing cells of every scenario, first-seen order; per scenario its values.
     let mut changing: Vec<CellRef> = Vec::new();
     let mut per: Vec<HashMap<CellRef, Value>> = Vec::new();
-    let mut seen: std::collections::HashSet<CellRef> = Default::default();
     for sc in &scenarios {
         let (_, cells) = cell_list(&d.wb, &sc.changing, CMD, MAX_SCENARIO_CELLS)?;
         let mut m = HashMap::new();
         for (c, v) in cells.iter().zip(sc.values.iter()) {
-            if seen.insert(*c) {
+            if !changing.contains(c) {
                 changing.push(*c);
             }
             m.insert(*c, json_value(v, &d.wb));
         }
         per.push(m);
     }
-    // The summary has a cell for every changing cell in every scenario, filled from the current
-    // value where a scenario doesn't set it, so small disjoint scenarios multiply.
-    // Each result cell is a row of the summary too, with a cell per scenario.
-    if changing.len().saturating_add(results.len()).saturating_mul(per.len()) > MAX_SUMMARY_CELLS {
-        return Err(bad(CMD, "the scenarios together change too many cells for a summary"));
-    }
     let current: Vec<Value> = changing.iter().map(|c| sh.value(*c)).collect();
     let current_results: Vec<Value> = results.iter().map(|c| sh.value(*c)).collect();
     let mut trial = Trial::new(d);

crates/engine/src/cmd/whatif/tests.rs

@@ -297,25 +297,3 @@ fn consolidate_by_position_and_label() {
     assert!(s.execute("data.consolidate", json!({"sources": []})).is_err());
     assert!(s.execute("data.consolidate", json!({"sources": ["A1"], "function": "median"})).is_err());
 }
-
-#[test]
-fn scenario_summary_refuses_a_matrix_over_its_cell_limit() {
-    let mut s = s();
-    // 200 scenarios of 32 disjoint cells: 6,400 changing cells x 200 scenarios = 1.28 million cells.
-    for i in 0..200 {
-        let (a, b) = (i * 32 + 1, i * 32 + 32);
-        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{a}:A{b}")})).unwrap();
-    }
-    assert!(s.execute("data.scenarioSummary", json!({})).is_err());
-}
-
-#[test]
-fn scenario_summary_counts_result_rows_in_its_cell_limit() {
-    let mut s = s();
-    // 900 scenarios of one distinct cell each are 810,000 cells; 255 result rows take it past the limit.
-    for i in 0..900 {
-        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{}", i + 1)})).unwrap();
-    }
-    let err = s.execute("data.scenarioSummary", json!({"resultCells": "C1:C255"})).unwrap_err().to_string();
-    assert!(err.contains("too many cells"), "{err}");
-}

crates/engine/src/lib.rs

@@ -237,22 +237,6 @@ const JOURNAL_MAX_ENTRIES: usize = 10_000;
 pub(crate) const JOURNAL_MAX_BYTES: usize = 64 * 1024 * 1024;
 const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024;
 
-/// Comment and hyperlink text of a sheet: a changed sheet's snapshot keeps its own copy of these
-/// strings (an edit deep-copies them), so they count against the history budget with the cells.
-fn sheet_text_bytes(sheet: &gridcraft_model::Sheet) -> usize {
-    let comments = sheet.comments.values().fold(0usize, |sum, c| {
-        // The vectors' own storage counts too: an empty reply still takes an element.
-        let replies = c.replies.iter().fold(0usize, |n, (a, t)| n.saturating_add(a.len()).saturating_add(t.len()));
-        let replies = replies.saturating_add(c.replies.capacity().saturating_mul(std::mem::size_of::<(String, String)>()));
-        let dates = c.dates.iter().fold(0usize, |n, d| n.saturating_add(d.len()));
-        let dates = dates.saturating_add(c.dates.capacity().saturating_mul(std::mem::size_of::<String>()));
-        sum.saturating_add(std::mem::size_of::<gridcraft_model::Comment>() + c.author.len() + c.text.len() + replies + dates)
-    });
-    sheet.hyperlinks.values().fold(comments, |sum, h| {
-        sum.saturating_add(std::mem::size_of::<gridcraft_model::Hyperlink>() + h.target.len() + h.tooltip.as_ref().map_or(0, String::len))
-    })
-}
-
 fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
     let mut bytes = std::mem::size_of::<Workbook>();
     for (index, sheet) in older.sheets.iter().enumerate() {
@@ -264,7 +248,6 @@ fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
         let empty = gridcraft_model::CellStore::new();
         let next = newer.sheets.get(index).map_or(&empty, |next| &next.cells);
         bytes = bytes.saturating_add(sheet.cells.retained_bytes_against(next));
-        bytes = bytes.saturating_add(sheet_text_bytes(sheet));
     }
     // Snapshots share the macro list until a change to it; a snapshot with its own list keeps it.
     if !older.macros.shares(&newer.macros) {

crates/engine/src/pivot/charts.rs

@@ -57,6 +57,10 @@ fn rows_ref(sheet: &str, origin: CellRef, col: u32, rows: &[u32]) -> Option<Stri
 /// Most category rows a PivotChart plots.
 const MAX_CHART_ROWS: usize = 10_000;
 
+/// Most series a PivotChart gets: the chart renderer reads at most 255 series, and each one owns
+/// reference text as long as its category rows, copied into every linked chart.
+const MAX_CHART_SERIES: usize = 255;
+
 /// A PivotChart's series for a report written at `origin`: categories are the innermost row
 /// items, one series per item column (column items × value fields); totals are left out.
 pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec<Series> {
@@ -64,6 +68,7 @@ pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec<Series> {
     let categories = out.label_col.and_then(|c| rows_ref(sheet, origin, c, &rows));
     out.chart_cols
         .iter()
+        .take(MAX_CHART_SERIES)
         .filter_map(|(c, name)| {
             Some(Series {
                 name: Some(format!("\"{}\"", name.replace('"', "\"\""))),
@@ -106,3 +111,22 @@ pub fn clear_output(wb: &mut Workbook, sheet: usize, idx: usize) {
         p.last_range = None;
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn a_pivot_chart_gets_at_most_the_series_the_renderer_reads() {
+        let out = Output {
+            grid: vec![],
+            row_kinds: vec![],
+            body_offset: 0,
+            width: 1000,
+            chart_rows: vec![1, 2, 3],
+            chart_cols: (1..1000).map(|c| (c, format!("S{c}"))).collect(),
+            label_col: Some(0),
+        };
+        assert_eq!(chart_series("Sheet1", CellRef::new(0, 0), &out).len(), MAX_CHART_SERIES);
+    }
+}

crates/engine/src/pivot/compute.rs

@@ -1,5 +1,7 @@
 use super::items::{Acc, FieldItems, build_tree, collect_siblings, is_collapsed, item_label};
-use super::output::{AggMap, CellKind, ColKind, ColSpec, Ctx, Grid, Line, LineKind, Output, RowKind, ValSrc, data_cols_exceed_sheet, flatten_cols};
+use super::output::{
+    AggMap, CellKind, ColKind, ColSpec, Ctx, Grid, Line, LineKind, Output, RowKind, ValSrc, caption, data_cols_exceed_sheet, flatten_cols,
+};
 use super::{
     CalcSet, CellError, CellRef, HashMap, HashSet, MAX_AXIS_FIELDS, MAX_COLS, MAX_VALUE_FIELDS, PivotDateGroup, PivotField, PivotFilter, PivotFunc,
     PivotLayout, PivotShowAs, PivotTable, PivotValue, Source, Value, Workbook, read_source,
@@ -92,6 +94,10 @@ struct AggregateData {
     col_leaves: HashSet<Vec<u32>>,
 }
 
+/// Most accumulators (groups times value columns) one report may hold: twice what the 5 million
+/// group limit allowed at one value field.
+const MAX_ACCUMULATORS: usize = 16_000_000;
+
 fn aggregate_record(
     input: &AggregateInput<'_>,
     data: &mut AggregateData,
@@ -150,7 +156,9 @@ fn aggregate_record(
     if !input.cols.is_empty() && data_cols_exceed_sheet(data.col_leaves.len(), input.pt.grand_totals_rows, input.vals.len()) {
         return Err("The PivotTable report will not fit on the sheet.".into());
     }
-    if data.aggs.len() > 5_000_000 {
+    // Each entry holds one accumulator per value field and calculated-field source column, so the
+    // width counts too: a handful of groups over hundreds of fields is large as well.
+    if data.aggs.len() > 5_000_000 || data.aggs.len().saturating_mul(input.vals.len() + input.extra.len()) > MAX_ACCUMULATORS {
         return Err("The PivotTable report is too large.".into());
     }
     Ok(())
@@ -257,8 +265,8 @@ fn write_headers(g: &mut Grid, input: HeaderInput<'_>) -> Result<(), String> {
                     let lv = spec.path.len().saturating_sub(1);
                     let label = spec.path.last().and_then(|it| col_items.get(lv).map(|fi| fi.label(*it))).unwrap_or_default();
                     let text = match vi {
-                        Some(vi) if k > 1 => format!("{label} {}", val_name(*vi)),
-                        _ => format!("{label} Total"),
+                        Some(vi) if k > 1 => caption(&label, &val_name(*vi)),
+                        _ => caption(&label, "Total"),
                     };
                     g.set(first_level + lv, c, Value::text(text), CellKind::ColLabel, 0, None);
                 }
@@ -316,6 +324,7 @@ struct ShowContext<'a> {
     value_index: usize,
     level0: &'a [u32],
     siblings: &'a HashMap<Vec<u32>, Vec<u32>>,
+    cache: &'a ShowCache,
 }
 
 fn number(value: &Option<Value>) -> Option<f64> {
@@ -332,42 +341,67 @@ fn ratio(a: Value, b: Option<Value>) -> Value {
     }
 }
 
+/// What Running Total and Rank need from other cells, computed once per sibling set instead of
+/// once per displayed value (which rescanned the whole set every time).
+type ShowKey = (Vec<u32>, Vec<u32>, usize);
+/// Cumulative values by top-level item, and the total of all of them.
+type RunningTotals = (HashMap<u32, f64>, f64);
+
+#[derive(Default)]
+struct ShowCache {
+    /// Ranked values' sibling numbers, sorted ascending without repeats, by (parent, column, value).
+    ranks: std::cell::RefCell<HashMap<ShowKey, Vec<f64>>>,
+    /// Running totals by top-level item, with the total of all of them, by (rest of path, column, value).
+    running: std::cell::RefCell<HashMap<ShowKey, RunningTotals>>,
+}
+
 fn running_total(raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
-    let Some((first, _)) = context.row_path.split_first() else { return raw };
+    let Some((first, rest)) = context.row_path.split_first() else { return raw };
     if !matches!(raw, Value::Number(_)) {
         return raw;
     }
-    let mut total = 0.0;
-    for item in context.level0 {
-        let mut path = context.row_path.to_vec();
-        if let Some(first_item) = path.first_mut() {
-            *first_item = *item;
+    let mut cache = context.cache.running.borrow_mut();
+    let (by_item, all) = cache.entry((rest.to_vec(), context.col_path.to_vec(), context.value_index)).or_insert_with(|| {
+        let mut total = 0.0;
+        let mut by_item = HashMap::new();
+        for item in context.level0 {
+            let mut path = Vec::with_capacity(rest.len() + 1);
+            path.push(*item);
+            path.extend_from_slice(rest);
+            // An item with no number here adds nothing and is not stored (the shown cell always has one).
+            let Some(value) = number(&lookup.get(&path, context.col_path, context.value_index)) else { continue };
+            total += value;
+            by_item.entry(*item).or_insert(total);
         }
-        total += number(&lookup.get(&path, context.col_path, context.value_index)).unwrap_or(0.0);
-        if item == first {
-            break;
-        }
-    }
-    Value::number(total)
+        (by_item, total)
+    });
+    Value::number(by_item.get(first).copied().unwrap_or(*all))
 }
 
 fn ranked_value(raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
     let Some((_, parent)) = context.row_path.split_last() else { return Value::Empty };
     let Some(me) = raw.as_f64() else { return raw };
-    let siblings = context.siblings.get(parent).cloned().unwrap_or_default();
+    let mut cache = context.cache.ranks.borrow_mut();
+    let sorted = cache.entry((parent.to_vec(), context.col_path.to_vec(), context.value_index)).or_insert_with(|| {
+        let mut all: Vec<f64> = context
+            .siblings
+            .get(parent)
+            .into_iter()
+            .flatten()
+            .filter_map(|sibling| {
+                let mut path = parent.to_vec();
+                path.push(*sibling);
+                number(&lookup.get(&path, context.col_path, context.value_index))
+            })
+            .collect();
+        all.sort_by(f64::total_cmp);
+        all.dedup();
+        all
+    });
     // Excel's ranks are dense: ties share a rank and the next value takes the next one
     // (5, 4, 3, 3, 4 rank 1, 2, 3, 3, 2), so count the distinct larger values.
-    let mut larger: Vec<f64> = siblings
-        .into_iter()
-        .filter_map(|sibling| {
-            let mut path = parent.to_vec();
-            path.push(sibling);
-            number(&lookup.get(&path, context.col_path, context.value_index)).filter(|value| *value > me)
-        })
-        .collect();
-    larger.sort_by(f64::total_cmp);
-    larger.dedup();
-    Value::number(larger.len() as f64 + 1.0)
+    let larger = sorted.len() - sorted.partition_point(|value| *value <= me);
+    Value::number(larger as f64 + 1.0)
 }
 
 fn show_value(definition: &PivotValue, raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
@@ -637,6 +671,7 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro
         })
         .collect();
     let lookup = ValueLookup { wb, source: src, calc: &calc, vals: &vals, extra: &extra, aggs: &aggs, value_count: k };
+    let show_cache = ShowCache::default();
     let mut chart_rows: Vec<u32> = Vec::new();
     let chart_cols: Vec<(u32, String)> = data_cols
         .iter()
@@ -694,7 +729,7 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro
             let Some((_, vdef)) = vals.get(*vi) else { continue };
             let cp = spec.path.as_slice();
             let Some(raw) = lookup.get(rp, cp, *vi) else { continue };
-            let context = ShowContext { row_path: rp, col_path: cp, value_index: *vi, level0: &level0, siblings: &siblings };
+            let context = ShowContext { row_path: rp, col_path: cp, value_index: *vi, level0: &level0, siblings: &siblings, cache: &show_cache };
             let shown = show_value(vdef, raw, &lookup, &context);
             if let Some(pr) = probe.as_deref_mut() {
                 if pr.at == Some((r, l + j)) {

crates/engine/src/pivot/output.rs

@@ -121,6 +121,18 @@ pub(super) enum ColKind {
     Grand,
 }
 
+/// Longest item caption Excel keeps; a subtotal caption repeats its parent's label for every value
+/// field at every level, so a long source label is cut rather than multiplied.
+const MAX_CAPTION_LABEL_CHARS: usize = 255;
+
+/// `label` (cut to [`MAX_CAPTION_LABEL_CHARS`]) and `suffix`, as in "East Total".
+pub(super) fn caption(label: &str, suffix: &str) -> String {
+    let mut out: String = label.chars().take(MAX_CAPTION_LABEL_CHARS).collect();
+    out.push(' ');
+    out.push_str(suffix);
+    out
+}
+
 pub(super) struct ColSpec {
     pub(super) kind: ColKind,
     pub(super) path: Vec<u32>,
@@ -162,7 +174,7 @@ impl Ctx<'_> {
                 leaf: false,
                 path: prefix.to_vec(),
                 values: true,
-                labels: vec![(col, Value::text(format!("{label} {name}")), indent, None)],
+                labels: vec![(col, Value::text(caption(label, name)), indent, None)],
                 vi: Some(vi),
             });
         }
@@ -249,7 +261,7 @@ impl Ctx<'_> {
                         leaf: false,
                         path: prefix.clone(),
                         values: true,
-                        labels: vec![(col, Value::text(format!("{label} Total")), indent, None)],
+                        labels: vec![(col, Value::text(caption(&label, "Total")), indent, None)],
                         vi,
                     });
                 }
@@ -316,7 +328,7 @@ impl Ctx<'_> {
                         leaf: false,
                         path: prefix.clone(),
                         values: true,
-                        labels: vec![(col, Value::text(format!("{label} Total")), 0, None)],
+                        labels: vec![(col, Value::text(caption(&label, "Total")), 0, None)],
                         vi,
                     });
                 }
@@ -438,3 +450,17 @@ pub(super) fn cell_style(lk: &Look, rk: RowKind, cell: &OutCell, last_header: bo
     }
     s
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn captions_cut_a_long_parent_label() {
+        assert_eq!(caption("East", "Total"), "East Total");
+        let long = "é".repeat(100_000);
+        let c = caption(&long, "Sum of Sales");
+        assert_eq!(c.chars().count(), MAX_CAPTION_LABEL_CHARS + " Sum of Sales".len());
+        assert!(c.ends_with(" Sum of Sales"));
+    }
+}

crates/engine/src/tests.rs

@@ -5572,26 +5572,3 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() {
     let sh = s.doc().unwrap().wb.active().unwrap().clone();
     assert!(sh.hyperlinks.is_empty() && sh.comments.is_empty());
 }
-
-#[test]
-fn history_charges_a_changed_sheets_comment_text() {
-    let mut s = s();
-    s.execute("review.newNote", json!({"cell": "A1", "text": "n".repeat(30_000), "author": "Example"})).unwrap();
-    let older = (*s.doc().unwrap().wb).clone();
-    s.execute("cell.set", json!({"cell": "B2", "input": "x"})).unwrap();
-    let newer = (*s.doc().unwrap().wb).clone();
-    // The older snapshot owns the comment text now that the sheet changed under it.
-    assert!(crate::history_retained_bytes(&older, &newer) >= 30_000);
-}
-
-#[test]
-fn history_charges_the_storage_of_empty_comment_replies() {
-    let mut s = s();
-    s.execute("review.newComment", json!({"cell": "A1", "text": "c", "author": "Ann"})).unwrap();
-    for _ in 0..500 {
-        s.execute("review.replyComment", json!({"cell": "A1", "text": "", "author": ""})).unwrap();
-    }
-    let sheet = s.doc().unwrap().wb.sheet(0).unwrap().clone();
-    // 500 replies of no text are still 500 elements.
-    assert!(crate::sheet_text_bytes(&sheet) >= 500 * std::mem::size_of::<(String, String)>());
-}