diff --git a/ROADMAP.md b/ROADMAP.md index cbf5afd68..5ff15b422 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -204,6 +204,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | 2026-10-12 | Insert › Text › WordArt (`insert.wordArt`, `wordArt.set`): floating decorative text with 10 original styles, outline and shadow, in-place editing, XLSX and print (#784) | | 2026-10-12 | Number formatting: one table of built-in formats (xlsx); numbers, dates and General render with one scratch buffer instead of per-token strings; output unchanged, checked against digests recorded from the old code (#817) | | 2026-10-21 | Macro engine ([macros.md](docs/macros.md)): typed, declarative macros saved in the workbook (a JSON field; hidden names in xlsx); declared inputs checked with messages that name the cells; tokens (`Input.col(2)`, `Row`, `Output`, `Name:Total`, `{{…}}` in formulas); one undo step with full rollback; a footprint dry run with ask, fail, overwrite, shift down or shift right; a policy that keeps macros away from files and the app, with approval for macros in opened files; Repeat, If and Stop steps; a debugger; a relative recorder; `macro.*` commands beside the unchanged Automate scripts | +| 2026-10-13 | Performance: a chart is resolved and rendered once per change of its workbook, zoom or fonts instead of on every frame; idle frames, hovering and scrolling reuse it (#1139, part of #1098) | | 2026-10-13 | Charts: work and memory are bounded by series x points (2,000,000 per chart: every series keeps its first points; 16 trendlines per chart), hairline columns and bars are merged per pixel column, series ranges are clipped before they are evaluated, the moving average is linear and rendering clean data no longer copies it (#1147) | | 2026-10-13 | Charts: one combo plot-kind rule for resolving and drawing; ticks of a scale off the step grid keep their own values (#1146) | | 2026-10-12 | Security: the `--control` server requires an auth token (random, kept in a 0600 file or chosen with `GRIDCRAFT_CONTROL_TOKEN`) on each connection's first line and closes bad connections (#777) | @@ -316,6 +317,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | 2026-10-13 | trivial | Progress log: shared pictures written once on XLSX export (#985) | | 2026-10-13 | minor | Progress log: conditional-format lookups no longer scan the range for rules without statistics (#1098) | | 2026-10-12 | minor | Progress log: hidden-sheet navigation at workbook boundaries | +| 2026-10-13 | trivial | Progress log: charts cached between frames (#1139) | | 2026-10-12 | minor | Progress log: PDF writer cleanup (#816) | | 2026-10-12 | minor | Catalog 276 → 277 / 292: Insert › WordArt (#784) | | 2026-10-12 | minor | Numfmt cleanup: one built-in format table (gridcraft-xlsx), fewer allocations rendering numbers, dates and General, equality test against the previous output (#817); progress log row | diff --git a/crates/chart/src/lib.rs b/crates/chart/src/lib.rs index 4b68b8d17..f4e2895fe 100644 --- a/crates/chart/src/lib.rs +++ b/crates/chart/src/lib.rs @@ -26,7 +26,7 @@ use serde::{Deserialize, Serialize}; pub use axis::{Scale, format_number, nice_scale}; pub use raster::rasterize; pub use render::{Swatch, legend_entries, render, supports_trendline}; -pub use resolve::{default_palette, resolve, series_color, series_plot_kind}; +pub use resolve::{default_palette, has_volatile_series, resolve, series_color, series_plot_kind}; pub use sparkline::render_sparkline; /// Straight (non-premultiplied) RGBA colour. diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index baa0730ad..e7663e0d9 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -155,6 +155,131 @@ fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, limit: usize, depth: } } +/// Whether a series formula of `ch` (chart of `sheet`) calls a function whose result changes +/// between evaluations (`NOW`, `TODAY`, `RAND`, `RANDBETWEEN`, `RANDARRAY`), so a result resolved +/// earlier is stale. Defined names the formulas use (`=FreshClock`, `Sheet1!Rates`, a LAMBDA name) +/// are followed into their own formulas, the way the evaluator looks them up; each name once per +/// chart, so a cycle ends. Names nested deeper than [`MAX_NAME_DEPTH`], or more than +/// [`MAX_NAMES_FOLLOWED`] distinct names, count as volatile: a chart whose stability isn't +/// established is not cached. +pub fn has_volatile_series(wb: &Workbook, sheet: usize, ch: &Chart) -> bool { + let mut seen = Seen::new(); + ch.series.iter().any(|s| { + [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()] + .into_iter() + .flatten() + .any(|f| formula_volatile(wb, sheet, f, 0, &mut seen)) + }) +} + +/// Names followed inside names before a formula counts as volatile. +const MAX_NAME_DEPTH: u32 = 8; +/// Distinct defined names followed per chart before the chart counts as volatile. +const MAX_NAMES_FOLLOWED: usize = 64; + +/// Defined names already followed for one chart, each with the sheet its formula is read on. +type Seen<'a> = Vec<(&'a gridcraft_model::DefinedName, usize)>; + +fn formula_volatile<'a>(wb: &'a Workbook, sheet: usize, f: &str, depth: u32, seen: &mut Seen<'a>) -> bool { + if calls_volatile(f) { + return true; + } + name_tokens(f).into_iter().any(|t| { + let Some((n, at)) = defined_name(wb, sheet, t) else { return false }; + // Followed already, or being followed (a cycle): a volatile call in it is found there. + if seen.iter().any(|&(m, s)| std::ptr::eq(m, n) && s == at) { + return false; + } + if depth >= MAX_NAME_DEPTH || seen.len() >= MAX_NAMES_FOLLOWED { + return true; + } + seen.push((n, at)); + formula_volatile(wb, at, &n.formula, depth + 1, seen) + }) +} + +/// Whether formula text `f` calls one of the volatile functions by name. +fn calls_volatile(f: &str) -> bool { + const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; + // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. + let up = f.to_ascii_uppercase().replace("_XLFN.", ""); + VOLATILE.iter().any(|name| { + up.match_indices(name).any(|(i, _)| { + let before = up[..i].chars().next_back(); + let after = up[i + name.len()..].trim_start(); + before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') + }) + }) +} + +fn is_ident_char(c: char) -> bool { + c.is_alphanumeric() || c == '_' || c == '.' || c == '\\' +} + +/// End of the identifier starting at byte `from` of `f`. +fn ident_end(f: &str, from: usize) -> usize { + f[from..].char_indices().find(|&(_, c)| !is_ident_char(c)).map_or(f.len(), |(j, _)| from + j) +} + +/// End (past the closing quote) of the `q`-quoted text opening at byte `from`; a doubled quote +/// is an escaped one. `None` when it is not closed. +fn quoted_end(f: &str, from: usize, q: char) -> Option { + let mut i = from + q.len_utf8(); + loop { + let j = i + f[i..].find(q)?; + if f[j + q.len_utf8()..].starts_with(q) { + i = j + 2 * q.len_utf8(); + } else { + return Some(j + q.len_utf8()); + } + } +} + +/// The words of formula text `f` that may be defined names: `Name`, `Sheet1!Name`, +/// `'Bob''s'!Name`. String literals and numbers are skipped; function names, column letters and +/// the like come along too and simply aren't found as names. +fn name_tokens(f: &str) -> Vec<&str> { + let mut out = Vec::new(); + let mut i = 0; + while let Some(c) = f[i..].chars().next() { + let start = i; + let end = match c { + '"' => { + i = quoted_end(f, i, '"').unwrap_or(f.len()); + continue; + } + '\'' => quoted_end(f, i, '\'').unwrap_or(f.len()), + c if is_ident_char(c) => ident_end(f, i), + c => { + i += c.len_utf8(); + continue; + } + }; + i = end; + if f[end..].starts_with('!') { + let e = ident_end(f, end + 1); + if e > end + 1 { + out.push(&f[start..e]); + } + i = e; + } else if c != '\'' && !c.is_ascii_digit() { + out.push(&f[start..end]); + } + } + out +} + +/// The defined name `t` (`Name` or `Sheet1!Name`) seen from `sheet`, and the sheet its own +/// formula is read on. +fn defined_name<'a>(wb: &'a Workbook, sheet: usize, t: &str) -> Option<(&'a gridcraft_model::DefinedName, usize)> { + // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). + let (at, bare) = match gridcraft_core::split_sheet(t) { + (Some(s), bare) => (wb.sheet_index(&s)?, bare), + (None, bare) => (sheet, bare), + }; + wb.name(bare, at).map(|n| (n, n.scope.unwrap_or(at))) +} + fn to_num(v: &Value) -> Option { match v { Value::Number(n) if n.is_finite() => Some(*n), @@ -305,11 +430,7 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str, limi /// Whether `t` names a defined name seen from `sheet` (`Name`, or `Sheet1!Name` looked up on /// that sheet), the way the evaluator looks names up. fn is_defined_name(wb: &Workbook, sheet: usize, t: &str) -> bool { - // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). - match gridcraft_core::split_sheet(t) { - (Some(s), bare) => wb.sheet_index(&s).is_some_and(|i| wb.name(bare, i).is_some()), - (None, _) => wb.name(t, sheet).is_some(), - } + defined_name(wb, sheet, t).is_some() } /// A series name: a reference/formula or a defined name (`[0]!Title` in a file) is evaluated, diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 8a9b8640e..a6d845387 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1680,3 +1680,96 @@ fn a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves() { let d = resolve(&wb, 0, &c); assert_eq!(d.series.first().map(|s| s.name.as_str()), Some("Takings")); } + +#[test] +fn volatile_series_formulas_are_detected() { + let wb = Workbook::new(); + let with = |values: &str, name: Option<&str>| { + let mut c = chart(ChartKind::Line); + c.series.push(Series { + name: name.map(Into::into), + categories: None, + values: values.into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + smooth: false, + quartiles: Default::default(), + trendlines: vec![], + }); + c + }; + assert!(has_volatile_series(&wb, 0, &with("=NOW()", None))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(A1:A3)*rand ()", None))); + assert!(has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); + assert!(has_volatile_series(&wb, 0, &with("=RANDARRAY(5)", None))); + assert!(has_volatile_series(&wb, 0, &with("=_xlfn.RANDARRAY(5)", None))); + assert!(!has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); + assert!(!has_volatile_series(&wb, 0, &with("=MYRAND(A1)", None))); + assert!(!has_volatile_series(&wb, 0, &with("=Sheet1!$NOW", None))); + assert!(!has_volatile_series(&wb, 0, &chart(ChartKind::Line))); +} + +#[test] +fn volatile_defined_names_are_followed() { + let with = |values: &str| { + let mut c = chart(ChartKind::Line); + c.series.push(Series { + name: None, + categories: None, + values: values.into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + smooth: false, + quartiles: Default::default(), + trendlines: vec![], + }); + c + }; + let name = |name: &str, scope: Option, formula: &str| gridcraft_model::DefinedName { + name: name.into(), + scope, + formula: formula.into(), + comment: String::new(), + hidden: false, + }; + let mut wb = Workbook::new(); + wb.sheet_mut(0).unwrap().name = "Bob's".into(); + wb.names.push(name("FreshClock", None, "NOW()")); + wb.names.push(name("Later", None, "FreshClock+1")); + wb.names.push(name("Dice", Some(0), "RANDBETWEEN(1,6)")); + wb.names.push(name("Rates", None, "'Bob''s'!$A$1:$A$3")); + wb.names.push(name("Loop", None, "Loop+1")); + wb.names.push(name("Spin", None, "Spin+NOW()")); + wb.names.push(name("Roll", None, "LAMBDA(n,n*RAND())")); + // A workbook-scoped name, a name used inside another, and a sheet-scoped one, bare or qualified. + assert!(has_volatile_series(&wb, 0, &with("=FreshClock"))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(Later, 1)"))); + assert!(has_volatile_series(&wb, 0, &with("=Dice*2"))); + assert!(has_volatile_series(&wb, 0, &with("='Bob''s'!Dice"))); + assert!(has_volatile_series(&wb, 0, &with("=Roll(3)"))); + // A self-referencing name ends (it evaluates to an error, not a changing value); one that + // also calls NOW is volatile. + assert!(!has_volatile_series(&wb, 0, &with("=Loop"))); + assert!(has_volatile_series(&wb, 0, &with("=Spin"))); + // Many series over the same stable name stay cacheable: each name is followed once. + let mut many = with("=Rates"); + let first = many.series[0].clone(); + many.series.extend(std::iter::repeat_n(first, 100)); + assert!(!has_volatile_series(&wb, 0, &many)); + // Names nested deeper than the limit are not known to be stable. + for i in 0..10 { + wb.names.push(name(&format!("Deep{i}"), None, &format!("Deep{}", i + 1))); + } + assert!(has_volatile_series(&wb, 0, &with("=Deep0"))); + // A name that refers to cells, a name inside a string and a name of another scope are stable. + assert!(!has_volatile_series(&wb, 0, &with("=Rates"))); + assert!(!has_volatile_series(&wb, 0, &with("=\"FreshClock\""))); + // Dice belongs to sheet 0: seen from another sheet it isn't defined. + wb.sheets.push(Arc::new(gridcraft_model::Sheet::new("Other"))); + assert!(!has_volatile_series(&wb, 1, &with("=Dice"))); + assert!(has_volatile_series(&wb, 1, &with("=FreshClock"))); +} diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 94b6b01e7..535c91a5c 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -71,6 +71,7 @@ impl SheetApp { capture_geometry: None, capture_geometry_from_raw: false, selected_object: None, + chart_cache: Default::default(), clipboard_text_at_copy: None, cut_text: None, tab_run: None, @@ -114,6 +115,7 @@ impl SheetApp { // Floating pictures' textures go with their workbook, even when no sheet is painted. let open: Vec = self.session.documents().iter().map(|d| d.uid).collect(); floating_pictures::forget_closed(ctx, &open); + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).retain_docs(open.iter().copied()); self.apply_font_state(ctx); if let Some(answer) = self.desktop_theme.take_change() { self.desktop_dark = Some(answer); diff --git a/crates/ui-egui/src/app/fonts.rs b/crates/ui-egui/src/app/fonts.rs index 1f99a9902..76a829a9d 100644 --- a/crates/ui-egui/src/app/fonts.rs +++ b/crates/ui-egui/src/app/fonts.rs @@ -65,6 +65,7 @@ impl SheetApp { } else if self.fonts_rebind { // A font-set change made last frame is active now. self.fonts_rebind = false; + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).clear(); } else if !defer_font_change && !self.prepared_fonts && fonts::scan_done() { self.rebind_workbook_fonts(ctx); } @@ -75,6 +76,8 @@ impl SheetApp { fn start_font_setup(&mut self, ctx: &egui::Context) { if self.fonts_set { self.fonts_ready = true; + // The new set is active now: chart text was measured with the old one. + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).clear(); return; } if self.prepared_fonts { diff --git a/crates/ui-egui/src/chartcache.rs b/crates/ui-egui/src/chartcache.rs new file mode 100644 index 000000000..82e9a96a2 --- /dev/null +++ b/crates/ui-egui/src/chartcache.rs @@ -0,0 +1,495 @@ +//! Rendered charts kept between frames. +//! +//! Resolving a chart's series and laying it out is O(points). Doing it for every visible chart on +//! every frame kept the UI thread busy while scrolling a workbook with a few big charts (#1139, +//! part of #1098). The primitives a chart renders to depend on the workbook, the chart's place in +//! it, the zoom, the screen's pixels-per-point and the fonts text is measured with, so those are +//! what an entry is checked against. +//! +//! **Why "the workbook is unchanged" can be known without a change counter.** A document's +//! workbook is an `Arc` and its content is only reachable for writing through +//! `Arc::make_mut` (or by replacing the `Arc`), because the model has no interior mutability and +//! the code base forbids `unsafe`. While a `Weak` to the allocation exists, `make_mut` never +//! mutates it in place: it clones (other strong owners, e.g. an undo snapshot) or moves the value +//! to a new allocation (the `Weak` is cut loose). So if the document's current `Arc` still points +//! at the allocation the cache holds a `Weak` to, nothing was written since the entries were +//! built, however the write would have been made (command, undo, redo, recalculation, file load, +//! a direct edit from the UI, a future code path). The `Weak` also keeps the allocation from being +//! freed and reused, so a pointer match cannot be a stranger that landed on the same address. + +use std::collections::HashMap; +use std::sync::{Arc, Weak}; + +use gridcraft_chart::Prim; +use gridcraft_engine::model::Workbook; + +/// Most charts kept, over all documents. +const MAX_ENTRIES: usize = 64; +/// Most memory kept (estimated from the primitives), over all documents. +const MAX_BYTES: usize = 48 << 20; + +struct Entry { + /// Zoom and pixels-per-point the primitives were measured for (`f32::to_bits`). + z: u32, + ppp: u32, + prims: Arc<[Prim]>, + bytes: usize, + /// Tick of the last use; the smallest is evicted first. + used: u64, +} + +struct DocCharts { + /// The workbook the entries were built from. + wb: Weak, + /// (sheet index, index in the sheet's `charts`). The workbook is unchanged while the entries + /// live, so the pair names one chart. + charts: HashMap<(usize, usize), Entry>, +} + +#[derive(Default)] +pub(crate) struct ChartCache { + docs: HashMap, + tick: u64, + bytes: usize, + /// Charts built (resolved and rendered) so far; for tests. + #[cfg(test)] + pub(crate) builds: usize, +} + +fn prim_bytes(p: &Prim) -> usize { + std::mem::size_of::() + + match p { + Prim::Line { pts, .. } | Prim::Polygon { pts, .. } => pts.len().saturating_mul(std::mem::size_of::<[f32; 2]>()), + Prim::Text { text, .. } => text.len(), + _ => 0, + } +} + +impl ChartCache { + /// The primitives of chart `index` of `sheet` in document `uid` whose workbook is `wb`, for + /// zoom and pixels per point `(z, ppp)`. `build` renders the chart when there is no valid entry. + pub(crate) fn get( + &mut self, + uid: u64, + wb: &Arc, + sheet: usize, + index: usize, + (z, ppp): (f32, f32), + build: impl FnOnce() -> Vec, + ) -> Arc<[Prim]> { + self.tick = self.tick.wrapping_add(1); + let tick = self.tick; + let doc = self.docs.entry(uid).or_insert_with(|| DocCharts { wb: Arc::downgrade(wb), charts: HashMap::new() }); + if !std::ptr::eq(doc.wb.as_ptr(), Arc::as_ptr(wb)) { + // The document changed (or is another one with this uid): nothing built from the old + // workbook can be trusted. + let dropped: usize = doc.charts.values().map(|e| e.bytes).sum(); + doc.charts.clear(); + doc.wb = Arc::downgrade(wb); + self.bytes = self.bytes.saturating_sub(dropped); + } + let (zb, pb) = (z.to_bits(), ppp.to_bits()); + if let Some(e) = doc.charts.get_mut(&(sheet, index)) + && e.z == zb + && e.ppp == pb + { + e.used = tick; + return e.prims.clone(); + } + #[cfg(test)] + { + self.builds += 1; + } + let prims: Arc<[Prim]> = build().into(); + let bytes = prims.iter().map(prim_bytes).fold(0usize, usize::saturating_add); + let Some(doc) = self.docs.get_mut(&uid) else { return prims }; + if let Some(old) = doc.charts.remove(&(sheet, index)) { + self.bytes = self.bytes.saturating_sub(old.bytes); + } + // A chart that alone would fill a quarter of the budget is not worth keeping. + if bytes <= MAX_BYTES / 4 { + doc.charts.insert((sheet, index), Entry { z: zb, ppp: pb, prims: prims.clone(), bytes, used: tick }); + self.bytes = self.bytes.saturating_add(bytes); + self.evict(); + } + prims + } + + /// Drops least recently used entries until the caps hold. + fn evict(&mut self) { + loop { + let count: usize = self.docs.values().map(|d| d.charts.len()).sum(); + if count <= MAX_ENTRIES && self.bytes <= MAX_BYTES { + return; + } + let oldest = self.docs.iter().flat_map(|(uid, d)| d.charts.iter().map(move |(k, e)| (e.used, *uid, *k))).min(); + let Some((_, uid, key)) = oldest else { return }; + if let Some(e) = self.docs.get_mut(&uid).and_then(|d| d.charts.remove(&key)) { + self.bytes = self.bytes.saturating_sub(e.bytes); + } + } + } + + /// Forgets the documents that are not in `live` (closed workbooks). + pub(crate) fn retain_docs(&mut self, live: impl Iterator) { + let live: Vec = live.collect(); + let bytes = &mut self.bytes; + self.docs.retain(|uid, d| { + let keep = live.contains(uid); + if !keep { + *bytes = bytes.saturating_sub(d.charts.values().map(|e| e.bytes).sum()); + } + keep + }); + } + + /// Forgets everything (the fonts text was measured with changed). + pub(crate) fn clear(&mut self) { + self.docs.clear(); + self.bytes = 0; + } + + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.docs.values().map(|d| d.charts.len()).sum() + } + + #[cfg(test)] + pub(crate) fn docs(&self) -> usize { + self.docs.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::SheetApp; + use crate::chartview::tests::RENDERS; + use egui::{Event, vec2}; + use gridcraft_chart::ApproxMeasure; + use gridcraft_engine::Session; + use serde_json::json; + + type Harness = egui_kittest::Harness<'static, SheetApp>; + + fn wb() -> Arc { + Arc::new(Workbook::new()) + } + + fn rect_prim(w: f32) -> Vec { + vec![Prim::Rect { x: 0.0, y: 0.0, w, h: 1.0, fill: None, stroke: None, radius: 0.0 }] + } + + fn line_prim(points: usize) -> Vec { + vec![Prim::Line { pts: vec![[0.0, 0.0]; points], color: [0, 0, 0, 255], width: 1.0, dash: false }] + } + + #[test] + fn a_workbook_written_after_caching_is_never_taken_for_the_cached_one() { + let mut cache = ChartCache::default(); + let mut w = wb(); + let build = || rect_prim(1.0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 1, "same workbook: a hit"); + // Sole owner of the Arc: without the Weak this write would happen in place, at the same address. + Arc::make_mut(&mut w).active_sheet.set(0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 2, "a write through make_mut moved the workbook to a new allocation"); + // Shared with an undo snapshot: make_mut clones, the pointer changes as well. + let snapshot = w.clone(); + Arc::make_mut(&mut w).active_sheet.set(0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 3); + // Replacing the Arc with an equal workbook (a reload) is a change too. + w = Arc::new((*snapshot).clone()); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 4); + // The old allocation is still held by the cache's Weak while its owner is gone, so a new + // workbook can't be mistaken for it by landing on the same address. + drop(snapshot); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 4); + } + + #[test] + fn zoom_scale_chart_and_document_are_part_of_the_key() { + let mut cache = ChartCache::default(); + let w = wb(); + let build = || rect_prim(1.0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + cache.get(1, &w, 0, 0, (1.25, 1.0), build); + cache.get(1, &w, 0, 0, (1.25, 2.0), build); + cache.get(1, &w, 0, 1, (1.25, 2.0), build); + cache.get(1, &w, 1, 0, (1.25, 2.0), build); + cache.get(2, &w, 0, 0, (1.25, 2.0), build); + assert_eq!(cache.builds, 6); + cache.get(1, &w, 0, 1, (1.25, 2.0), build); + cache.get(2, &w, 0, 0, (1.25, 2.0), build); + assert_eq!(cache.builds, 6, "each of those is cached"); + } + + #[test] + fn the_cache_is_capped_in_entries_and_bytes_and_drops_the_least_recently_used() { + let mut cache = ChartCache::default(); + let w = wb(); + for i in 0..MAX_ENTRIES + 10 { + cache.get(1, &w, 0, i, (1.0, 1.0), || rect_prim(1.0)); + // Chart 0 is used all the time, so it survives. + cache.get(1, &w, 0, 0, (1.0, 1.0), || rect_prim(1.0)); + } + assert_eq!(cache.len(), MAX_ENTRIES); + let before = cache.builds; + cache.get(1, &w, 0, 0, (1.0, 1.0), || rect_prim(1.0)); + assert_eq!(cache.builds, before, "the busy chart is still there"); + cache.get(1, &w, 0, 1, (1.0, 1.0), || rect_prim(1.0)); + assert_eq!(cache.builds, before + 1, "the idle one was evicted"); + + // Bytes: each of these is ~4 MiB of points; only a handful fit. + let mut cache = ChartCache::default(); + let points = (4 << 20) / std::mem::size_of::<[f32; 2]>(); + for i in 0..40 { + cache.get(1, &w, 0, i, (1.0, 1.0), || line_prim(points)); + } + assert!(cache.bytes <= MAX_BYTES && cache.len() < 40, "{} entries, {} bytes", cache.len(), cache.bytes); + // One that would fill a quarter of the budget is not kept, and is still returned. + let mut cache = ChartCache::default(); + let big = cache.get(1, &w, 0, 0, (1.0, 1.0), || line_prim(MAX_BYTES / 4 / std::mem::size_of::<[f32; 2]>() + 1000)); + assert!(!big.is_empty()); + assert_eq!((cache.len(), cache.bytes), (0, 0)); + } + + #[test] + fn closed_documents_are_forgotten() { + let mut cache = ChartCache::default(); + let w = wb(); + cache.get(1, &w, 0, 0, (1.0, 1.0), || line_prim(10)); + cache.get(2, &w, 0, 0, (1.0, 1.0), || line_prim(10)); + cache.retain_docs([2].into_iter()); + assert_eq!((cache.docs(), cache.len()), (1, 1)); + cache.retain_docs(std::iter::empty()); + assert_eq!((cache.docs(), cache.len(), cache.bytes), (0, 0, 0)); + } + + /// A workbook with charts of several kinds over numbers, dates and categories. + fn corpus() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Date", "Sales", "Cost", "Share"]]})).unwrap(); + for i in 0..40u32 { + let date = format!("2024-{:02}-{:02}", 1 + i / 28, 1 + i % 28); + s.execute("cell.set", json!({"cell": format!("A{}", i + 2), "input": date})).unwrap(); + s.execute("cell.set", json!({"cell": format!("B{}", i + 2), "input": format!("{}", (i * 37) % 101)})).unwrap(); + s.execute("cell.set", json!({"cell": format!("C{}", i + 2), "input": format!("=B{}/2+{}", i + 2, i)})).unwrap(); + s.execute("cell.set", json!({"cell": format!("D{}", i + 2), "input": format!("{}", (i % 5) + 1)})).unwrap(); + } + for (i, (kind, range)) in [ + ("column", "A1:C41"), + ("line", "A1:C41"), + ("scatter", "B1:C41"), + ("pie", "A1:B6"), + ("combo", "A1:C41"), + ("area", "A1:B41"), + ("bar", "A1:D41"), + ("doughnut", "A1:B6"), + ] + .into_iter() + .enumerate() + { + s.execute("insert.chart", json!({"range": range, "type": kind, "at": format!("H{}", 2 + i * 16), "width": 320, "height": 220})).unwrap(); + } + s + } + + #[test] + fn cached_output_equals_a_fresh_render_for_a_corpus_of_charts() { + let s = corpus(); + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + assert_eq!(sh.charts.len(), 8); + let mut cache = ChartCache::default(); + for (i, ch) in sh.charts.iter().enumerate() { + let fresh = || { + let data = gridcraft_chart::resolve(&d.wb, d.wb.active_sheet.get(), ch); + gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &ApproxMeasure) + }; + let expect = fresh(); + assert!(expect.len() > 3, "{:?} renders something", ch.kind); + let first = cache.get(d.uid, &d.wb, d.wb.active_sheet.get(), i, (1.0, 1.0), fresh); + let second = cache.get(d.uid, &d.wb, d.wb.active_sheet.get(), i, (1.0, 1.0), || panic!("must be a hit")); + assert_eq!(&*first, &expect[..], "{:?}: built entry", ch.kind); + assert_eq!(&*second, &expect[..], "{:?}: cached entry", ch.kind); + } + assert_eq!(cache.builds, 8); + } + + // --- frames --- + + fn harness(s: Session) -> Harness { + let mut h = egui_kittest::Harness::builder().with_size(vec2(1400.0, 900.0)).build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + SheetApp::new(s, Default::default()), + ); + h.run_steps(8); + h + } + + /// Two charts side by side, both in view, over A1:B12 (B3 is `=B2*2`). + fn two_charts() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Day", "Sales"]]})).unwrap(); + for i in 2..=12u32 { + let input = if i == 3 { "=B2*2".to_string() } else { format!("{}", i * 3 % 17) }; + s.execute("cell.set", json!({"cell": format!("A{i}"), "input": format!("2024-03-{i:02}")})).unwrap(); + s.execute("cell.set", json!({"cell": format!("B{i}"), "input": input})).unwrap(); + } + s.execute("insert.chart", json!({"range": "A1:B12", "type": "column", "at": "D2", "width": 300, "height": 200})).unwrap(); + s.execute("insert.chart", json!({"range": "A1:B12", "type": "scatter", "at": "L2", "width": 300, "height": 200})).unwrap(); + s + } + + fn builds() -> usize { + RENDERS.with(|c| c.get()) + } + + /// Runs `change`, lets a few frames pass and returns how many charts were rendered meanwhile. + fn rendered_after(h: &mut Harness, change: impl FnOnce(&mut Harness)) -> usize { + let before = builds(); + change(h); + h.run_steps(4); + builds() - before + } + + fn exec(h: &mut Harness, id: &str, params: serde_json::Value) { + h.state_mut().session.execute(id, params).unwrap(); + } + + #[test] + fn frames_without_a_change_render_each_chart_once() { + let mut h = harness(two_charts()); + assert!(builds() >= 2, "the charts were drawn"); + let n = rendered_after(&mut h, |h| h.run_steps(30)); + assert_eq!(n, 0, "30 idle frames render nothing again"); + // Pointer movement and wheel scrolling (view state only) change no workbook. + let scroll_before = h.state().view().scroll; + let n = rendered_after(&mut h, |h| { + for i in 0..10 { + h.input_mut().events.push(Event::PointerMoved(egui::pos2(300.0 + i as f32 * 7.0, 400.0))); + h.input_mut().events.push(Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + source: egui::MouseWheelSource::Unknown, + delta: vec2(0.0, -2.0), + phase: egui::TouchPhase::Move, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(1); + } + }); + assert_ne!(h.state().view().scroll, scroll_before, "the sheet did scroll"); + assert_eq!(n, 0, "hover and scrolling reuse the rendered charts"); + assert_eq!(h.state().chart_cache.lock().unwrap().len(), 2); + } + + #[test] + fn every_kind_of_change_renders_the_charts_again_exactly_once() { + let mut h = harness(two_charts()); + type Case<'a> = (&'a str, Box); + let cases: Vec = vec![ + ("cell edit", Box::new(|h| exec(h, "cell.set", json!({"cell": "B5", "input": "99"})))), + ("recalc through a dependency", Box::new(|h| exec(h, "cell.set", json!({"cell": "B2", "input": "7"})))), + ("calculate now", Box::new(|h| exec(h, "formulas.calculateNow", json!({})))), + ("undo", Box::new(|h| exec(h, "edit.undo", json!({})))), + ("redo", Box::new(|h| exec(h, "edit.redo", json!({})))), + ("row hide", Box::new(|h| exec(h, "home.hideRows", json!({"rows": "6:7"})))), + ("chart resize", Box::new(|h| exec(h, "chart.set", json!({"chart": 1, "width": 360, "height": 240})))), + ("format change", Box::new(|h| exec(h, "home.bold", json!({"range": "A1:B1"})))), + ("number format", Box::new(|h| exec(h, "home.numberFormat", json!({"range": "B2:B12", "format": "0.00"})))), + ( + "document theme", + Box::new(|h| { + exec( + h, + "pageLayout.themeColors", + json!({"colors": ["#000000", "#FFFFFF", "#44546A", "#E7E6E6", "#C00000", "#ED7D31", "#A5A5A5", "#FFC000", "#5B9BD5", "#70AD47", "#0563C1", "#954F72"]}), + ) + }), + ), + ]; + for (what, change) in cases { + let n = rendered_after(&mut h, |h| change(h)); + assert_eq!(n, 2, "{what}: both visible charts rendered once"); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0, "{what}: and then nothing more"); + } + // Zoom is part of the key too. + let n = rendered_after(&mut h, |h| exec(h, "view.zoom", json!({"percent": 150}))); + assert_eq!(n, 2, "zoom"); + } + + #[test] + fn switching_sheets_and_loading_files_render_the_new_charts_once() { + let mut h = harness(two_charts()); + exec(&mut h, "home.insertSheet", json!({"before": 1})); + h.run_steps(4); + // The new sheet has no charts; coming back renders this sheet's two once. + let n = rendered_after(&mut h, |h| exec(h, "sheet.activate", json!({"sheet": 0}))); + assert_eq!(n, 2, "sheet switch"); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0); + + // File load: the same workbook opened from bytes is a new document with its own charts. + let bytes = h.state_mut().session.execute("file.saveBytes", json!({})).unwrap(); + let b64 = bytes["base64"].as_str().unwrap().to_string(); + let n = rendered_after(&mut h, |h| exec(h, "file.open", json!({"name": "again.xlsx", "base64": b64}))); + assert_eq!(n, 2, "file load"); + assert_eq!(h.state().session.documents().len(), 2); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 2, "both open documents are cached"); + + // Back in the first document nothing is rendered: its entries were kept. + let n = rendered_after(&mut h, |h| h.state_mut().session.set_active(0)); + assert_eq!(n, 0, "the first document's charts were still cached"); + // Closing a document forgets its entries; the other one is shown (and rendered) now. + let n = rendered_after(&mut h, |h| exec(h, "file.close", json!({"force": true}))); + assert_eq!(h.state().session.documents().len(), 1); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + assert_eq!(n, 0, "the remaining document's charts were cached too"); + } + + #[test] + fn a_chart_with_a_volatile_series_is_rendered_on_every_frame() { + let mut h = harness(two_charts()); + { + let d = h.state_mut().session.active_mut().unwrap(); + let sh = Arc::make_mut(&mut d.wb).sheet_mut(0).unwrap(); + sh.charts[0].series[0].values = "=RAND()".into(); + } + h.run_steps(4); + let n = rendered_after(&mut h, |h| h.run_steps(6)); + assert!(n >= 6, "the volatile chart is resolved again on each frame, got {n}"); + assert!(n < 12, "the other chart still comes from the cache, got {n}"); + } + + #[test] + fn closed_documents_are_forgotten_without_painting_a_chart() { + let mut h = harness(two_charts()); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + exec(&mut h, "file.new", json!({})); + h.run_steps(4); + assert_eq!(h.state().session.documents().len(), 2); + // The charts' document is no longer shown, but still open. + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + h.state_mut().session.set_active(0); + h.run_steps(2); + exec(&mut h, "file.close", json!({"force": true, "document": true})); + h.run_steps(2); + assert_eq!(h.state().session.documents().len(), 1); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 0, "no chart is painted, yet the closed document's entries are gone"); + } +} diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 92d693361..48f0846ae 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -1,6 +1,8 @@ //! Floating objects on the sheet: charts (rendered by `gridcraft-chart` into primitives that //! we paint with egui), pictures, shapes, equations, slicers and form controls (`form_controls.rs`). +use std::sync::Arc; + use egui::epaint::{PathShape, PathStroke}; use egui::{Align2, Color32, FontId, Painter, Pos2, Rect, Stroke, StrokeKind, pos2, vec2}; use gridcraft_chart::{HAlign, Prim, VAlign}; @@ -246,6 +248,31 @@ fn paint_shape_text(p: &Painter, wb: &Workbook, sp: &gridcraft_engine::model::Sh clip.galley(pos, g, color); } +/// Resolves and renders `ch` (chart `ci` of sheet `si`), at zoom `z`. +fn render_chart(p: &Painter, z: f32, wb: &Workbook, si: usize, ch: &gridcraft_engine::model::Chart) -> Vec { + #[cfg(test)] + tests::RENDERS.with(|c| c.set(c.get() + 1)); + let data = gridcraft_chart::resolve(wb, si, ch); + let m = EguiMeasure { painter: p, z }; + gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &m) +} + +/// The primitives of chart `ci` of sheet `si`: from the frame cache when the workbook, zoom and +/// screen scale are those it was rendered for, else rendered now (see [`crate::chartcache`]). +fn chart_prims(app: &SheetApp, p: &Painter, z: f32, wb: &Arc, si: usize, ci: usize, ch: &gridcraft_engine::model::Chart) -> Arc<[Prim]> { + // Only the active document's own workbook is cached: anything else cannot be tied to a uid. + let Some(d) = app.session.active().filter(|d| Arc::ptr_eq(&d.wb, wb)) else { + return render_chart(p, z, wb, si, ch).into(); + }; + // A series that calls NOW or RAND, directly or through a defined name, must be evaluated + // again on every paint. + if gridcraft_chart::has_volatile_series(wb, si, ch) { + return render_chart(p, z, wb, si, ch).into(); + } + let mut cache = app.chart_cache.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + cache.get(d.uid, wb, si, ci, (z, p.ctx().pixels_per_point()), || render_chart(p, z, wb, si, ch)) +} + pub(crate) fn anchor_rect(geo: &Geo, sh: &Sheet, a: &Anchor) -> Rect { let x = geo.x(sh, a.cell.col) + a.dx * geo.z; let y = geo.y(sh, a.cell.row) + a.dy * geo.z; @@ -265,7 +292,7 @@ pub(crate) fn objects_hidden(wb: &Workbook) -> bool { wb.show_objects == gridcraft_engine::model::ShowObjects::None } -pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet) -> Vec { +pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Arc, si: usize, sh: &Sheet) -> Vec { let mut painted = Vec::new(); if objects_hidden(wb) { return painted; @@ -373,7 +400,7 @@ pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workboo } done(start, r); } - for ch in &sh.charts { + for (ci, ch) in sh.charts.iter().enumerate() { let r = anchor_rect(geo, sh, &ch.anchor); if !r.intersects(p.clip_rect()) { continue; @@ -382,9 +409,7 @@ pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workboo // Soft shadow + white card. p.rect_filled(r.translate(vec2(0.0, 1.5)), 2.0, Color32::from_black_alpha(18)); p.rect_filled(r, 0.0, Tokens::get(p.ctx()).grid_bg); - let data = gridcraft_chart::resolve(wb, si, ch); - let m = EguiMeasure { painter: p, z: geo.z }; - let prims = gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &m); + let prims = chart_prims(app, p, geo.z, wb, si, ci, ch); paint_prims(&p.with_clip_rect(r.intersect(p.clip_rect())), r.min, &prims, geo.z); done(start, r); } @@ -591,11 +616,18 @@ pub(crate) fn ink_color(line: Color32, width_pt: f32) -> Color32 { } #[cfg(test)] -mod tests { +pub(crate) mod tests { + use std::cell::Cell; + use gridcraft_chart::Measure; use super::*; + thread_local! { + /// Charts resolved and rendered on this thread (a test runs on its own thread). + pub(crate) static RENDERS: Cell = const { Cell::new(0) }; + } + /// The production measure is not monotone for labels with a line break: egui lays the text out /// line by line, so ending a prefix with `\n…` can be narrower than the shorter `iii…`. The /// chart's label fitting must not assume otherwise (#1152). diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index cea12b880..ce7ce347b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -18,6 +18,7 @@ pub const UPDATE_APP: craft_update::app::App = craft_update::app::App { pub mod backstage; mod border_preview; mod cell_pictures; +mod chartcache; pub mod chartview; pub mod code_editor; pub mod control; @@ -288,6 +289,8 @@ pub struct SheetApp { pub(crate) capture_geometry_from_raw: bool, /// The floating object selected on the sheet (chart, picture, shape, equation or slicer). pub selected_object: Option, + /// Rendered charts kept between frames ([`chartcache`]). + pub(crate) chart_cache: std::sync::Mutex, /// The system clipboard's text when GridCraft last copied without publishing to it (a /// control-channel or MCP `edit.copy`/`edit.cut`). While the system clipboard still holds this /// text, GridCraft's copy is the newer one and Paste uses it. diff --git a/crates/ui-egui/tests/it/ref_edit.rs b/crates/ui-egui/tests/it/ref_edit.rs index fb3bf0cb6..b14a75da0 100644 --- a/crates/ui-egui/tests/it/ref_edit.rs +++ b/crates/ui-egui/tests/it/ref_edit.rs @@ -1064,8 +1064,8 @@ fn the_windows_cancel_with_an_unsavable_draft_closes_the_dialog_and_says_why() { assert_eq!(draft_text(&h).as_deref(), Some("Picked note"), "the draft is kept"); assert_eq!(selection(&h).0, 2, "the selection stays where it is"); let wb = &h.state().session.active().unwrap().wb; - for sheet in 0..3 { - assert!(wb.sheets[sheet].value(CellRef::parse("H1").unwrap()).is_empty(), "nothing ran on sheet {sheet}"); + for (index, sheet) in wb.sheets[..3].iter().enumerate() { + assert!(sheet.value(CellRef::parse("H1").unwrap()).is_empty(), "nothing ran on sheet {index}"); } }