address review: cache the chart-sheet usability verdict per relationship id instead of every reference

chart_sheet_usable walked a chart sheet's relationship graph and parsed
its charts for every workbook entry, so many entries naming one shared
graph multiplied the work. The referenced sheet names are now cached by
relationship id.

crates/engine/src/display.rs

@@ -8,13 +8,8 @@ use gridcraft_model::{Sheet, Workbook};
 use gridcraft_numfmt::{Formatted, NumberFormat, format_value};
 
 /// Parsed number formats are cached by code.
-pub(crate) fn parsed(code: &str) -> NumberFormat {
+fn parsed(code: &str) -> NumberFormat {
     static CACHE: std::sync::LazyLock<Mutex<HashMap<String, NumberFormat>>> = std::sync::LazyLock::new(|| Mutex::new(HashMap::new()));
-    // A code of more bytes than 4 × the character limit can't be within it: it shows as General
-    // without being hashed or scanned again for every cell that shares it.
-    if code.len() > 4 * gridcraft_numfmt::MAX_FORMAT_CODE_CHARS {
-        return NumberFormat::parse("General");
-    }
     let lock = || CACHE.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
     if let Some(f) = lock().get(code) {
         return f.clone();
@@ -29,12 +24,7 @@ pub(crate) fn parsed(code: &str) -> NumberFormat {
     f
 }
 
-/// The parsed format of `code`; one longer than the character limit is a General format that keeps
-/// the code it was made from. The engine itself formats through `parsed`, which doesn't copy it.
 pub fn number_format(code: &str) -> NumberFormat {
-    if code.len() > 4 * gridcraft_numfmt::MAX_FORMAT_CODE_CHARS {
-        return NumberFormat::parse(code);
-    }
     parsed(code)
 }
 
@@ -236,31 +226,3 @@ pub fn stats(sheet: &Sheet, ranges: &[RangeRef]) -> Stats {
     }
     s
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    /// A format code far past Excel's 255 characters shows its values as General, for every cell
-    /// that shares it, without being hashed into the cache each time.
-    #[test]
-    fn an_overlong_code_formats_as_general() {
-        let wb = Workbook::new();
-        let code = format!("0.00{}", "\"x\"".repeat(400_000));
-        for _ in 0..1_000 {
-            assert_eq!(format(&Value::Number(1.5), &code, &wb).text, "1.5");
-        }
-        assert_eq!(format(&Value::Number(1.5), "0.00", &wb).text, "1.50");
-    }
-
-    /// The public `number_format` keeps the code of an overlong format; it is General, but not the
-    /// same format as a real `General`.
-    #[test]
-    fn number_format_keeps_an_overlong_code() {
-        let code = format!("0.00{}", "\"x\"".repeat(2_000));
-        let f = number_format(&code);
-        assert!(f.is_general());
-        assert_eq!(f.code(), code);
-        assert_ne!(f, number_format("General"));
-    }
-}

crates/engine/src/io.rs

@@ -443,7 +443,7 @@ pub fn csv_values_bytes(wb: &Workbook) -> Result<Vec<u8>> {
     let Some(sh) = wb.sheet(wb.active_sheet()) else { return Ok(Vec::new()) };
     gridcraft_xlsx::write_csv(sh, wb.csv_delimiter.unwrap_or(b','), |c| match sh.value(c) {
         gridcraft_core::Value::Number(n) if n.is_finite() => {
-            let shape = crate::display::parsed(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).date_shape(n);
+            let shape = crate::display::number_format(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).date_shape(n);
             match shape.zip(gridcraft_core::date::datetime_from_serial(wb.date_system, n)) {
                 Some(((date, time), d)) => iso(&d, date, time),
                 None => n.to_string(),

crates/engine/src/macros/recorder.rs

@@ -440,7 +440,7 @@ pub fn infer_input(wb: &Workbook, sheet: usize, r: RangeRef) -> NamedInput {
         Some(match v {
             Value::Empty => return None,
             Value::Text(t) if t.is_empty() => return None,
-            Value::Number(_) if crate::display::parsed(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).is_date() => ColType::Date,
+            Value::Number(_) if crate::display::number_format(wb.styles.get(sh.style_id(c)).num_fmt.as_str()).is_date() => ColType::Date,
             Value::Number(_) => ColType::Number,
             Value::Text(_) => ColType::Text,
             Value::Bool(_) => ColType::Bool,

crates/engine/src/macros/run.rs

@@ -335,7 +335,7 @@ fn check_input(wb: &Workbook, input: &gridcraft_model::NamedInput, place: Place)
                 gridcraft_model::ColType::Number => v.is_number(),
                 gridcraft_model::ColType::Text => v.is_text(),
                 gridcraft_model::ColType::Bool => matches!(v, Value::Bool(_)),
-                gridcraft_model::ColType::Date => v.is_number() && crate::display::parsed(&date()).is_date(),
+                gridcraft_model::ColType::Date => v.is_number() && crate::display::number_format(&date()).is_date(),
             };
             if !ok {
                 wrong.push((c, holds(&v, ty)));

crates/engine/src/pivot.rs

@@ -182,7 +182,7 @@ pub fn read_source(wb: &Workbook, text: &str, default_sheet: usize) -> Result<So
     for (n, a) in numeric.iter_mut().zip(any.iter()) {
         *n = *n && *a;
     }
-    let dates = numeric.iter().zip(formats.iter()).map(|(n, f)| *n && crate::display::parsed(f).is_date()).collect();
+    let dates = numeric.iter().zip(formats.iter()).map(|(n, f)| *n && crate::display::number_format(f).is_date()).collect();
     Ok(Source { sheet: si, range: r, headers, formats, numeric, dates, rows })
 }
 

crates/xlsx/src/passthrough/write.rs

@@ -254,27 +254,36 @@ pub(crate) fn sheet_names_in(formula: &str) -> Vec<String> {
 /// Whether a kept chart sheet can be written back: its chart's series must still refer to sheets
 /// that exist under the same names (the chart is kept as it was, so a series on a deleted or
 /// renamed sheet would be left pointing at nothing), and no worksheet may have taken its name.
-fn chart_sheet_usable(wb: &Workbook, p: &Preserved, s: &PreservedSheet) -> bool {
+///
+/// `usable` caches, by relationship id, whether that chart sheet's graph refers only to sheets
+/// that exist (`false` when the relationship is missing): entries sharing one graph are analysed
+/// once, and only the verdict is kept, not the references it came from.
+fn chart_sheet_usable<'p>(wb: &Workbook, p: &'p Preserved, s: &'p PreservedSheet, usable: &mut HashMap<&'p str, bool>) -> bool {
     let same = |a: &str, b: &str| a.chars().take(31).collect::<String>().eq_ignore_ascii_case(&b.chars().take(31).collect::<String>());
     if wb.sheets.iter().any(|w| same(&w.name, &s.name)) {
         return false;
     }
-    let Some(root) = p.rels.iter().find(|r| r.id == s.rel_id) else { return false };
-    let parts = reachable(p, root);
-    let mut formulas = vec![];
-    for q in p.parts.iter().filter(|q| parts.contains(&normalize(&q.name)) && q.content_type.contains("chart")) {
-        if let Ok(x) = xml::parse(&q.data) {
-            chart_formulas(&x, &mut formulas);
+    *usable.entry(s.rel_id.as_str()).or_insert_with(|| {
+        let Some(root) = p.rels.iter().find(|r| r.id == s.rel_id) else { return false };
+        let parts = reachable(p, root);
+        let mut formulas = vec![];
+        for q in p.parts.iter().filter(|q| parts.contains(&normalize(&q.name)) && q.content_type.contains("chart")) {
+            if let Ok(x) = xml::parse(&q.data) {
+                chart_formulas(&x, &mut formulas);
+            }
         }
-    }
-    formulas.iter().flat_map(|f| sheet_names_in(f)).all(|n| wb.sheets.iter().any(|w| same(&w.name, &n)))
+        formulas.iter().flat_map(|f| sheet_names_in(f)).all(|n| wb.sheets.iter().any(|w| same(&w.name, &n)))
+    })
 }
 
 /// Writes the workbook's kept parts and relationships. Chart sheets that can't be written back
 /// (see [`chart_sheet_usable`]) are left out with the parts only they reach.
 pub fn write_workbook(wb: &Workbook, wb_rels: &mut Rels, out: &mut Out) -> WorkbookKept {
     let Some(p) = wb.preserved.as_deref() else { return WorkbookKept::default() };
-    let (live, dropped): (Vec<&PreservedSheet>, Vec<&PreservedSheet>) = p.sheets.iter().partition(|s| chart_sheet_usable(wb, p, s));
+    let (live, dropped): (Vec<&PreservedSheet>, Vec<&PreservedSheet>) = {
+        let mut refs = HashMap::new();
+        p.sheets.iter().partition(|s| chart_sheet_usable(wb, p, s, &mut refs))
+    };
     let root_of = |s: &PreservedSheet| p.rels.iter().find(|r| r.id == s.rel_id);
     let dropped_ids: HashSet<&str> = dropped.iter().map(|s| s.rel_id.as_str()).collect();
     // Parts reached only by dropped chart sheets go with them.

crates/xlsx/src/tests/passthrough_tests.rs

@@ -696,6 +696,32 @@ fn chartsheet_package() -> Vec<u8> {
     ])
 }
 
+/// Thousands of chart-sheet entries naming one chart sheet's relationship: its graph is walked
+/// and its charts parsed once when saving, not once per entry.
+#[test]
+fn chart_sheet_entries_sharing_a_graph_are_checked_once() {
+    let bytes = chartsheet_package();
+    let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap();
+    let mut parts: Vec<(String, Vec<u8>)> = Vec::new();
+    for i in 0..z.len() {
+        let mut f = z.by_index(i).unwrap();
+        let mut data = Vec::new();
+        f.read_to_end(&mut data).unwrap();
+        parts.push((f.name().to_string(), data));
+    }
+    let extra: String = (0..3_000).map(|i| format!(r#"<sheet name="Copy{i}" sheetId="{}" r:id="rId2"/>"#, 100 + i)).collect();
+    for (name, data) in &mut parts {
+        if name == "xl/workbook.xml" {
+            *data = String::from_utf8(data.clone()).unwrap().replace("</sheets>", &format!("{extra}</sheets>")).into_bytes();
+        }
+    }
+    let refs: Vec<(&str, &[u8])> = parts.iter().map(|(n, d)| (n.as_str(), d.as_slice())).collect();
+    let (wb, _) = read_xlsx(&make_zip(&refs)).unwrap();
+    let u = Unpacked::new(&write_xlsx(&wb).unwrap());
+    u.assert_valid();
+    assert!(written_sheets(&u).iter().any(|(n, _)| n == "Chart1"));
+}
+
 /// The sheets of a written workbook: (name, state) in file order.
 fn written_sheets(u: &Unpacked) -> Vec<(String, String)> {
     let wb = xml::parse(u.parts.get("xl/workbook.xml").unwrap()).unwrap();