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/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/styles.rs

@@ -59,14 +59,11 @@ fn bool_prop(e: &El, name: &str) -> bool {
 
 const DEFAULT_FONT_FAMILY: u8 = 2;
 const MAX_FONT_FAMILY: u8 = 14;
-/// Longest font name kept, in characters (Excel's own limit). Every cell format copies its
-/// font, so a long name repeated by many formats would otherwise multiply in memory.
-const MAX_FONT_NAME: usize = 31;
 
 pub fn read_font(e: &El, palette: &[u32]) -> Font {
     let mut f = Font::default();
     if let Some(n) = e.child("name").or_else(|| e.child("rFont")).and_then(|n| n.attr("val")) {
-        f.name = n.chars().take(MAX_FONT_NAME).collect();
+        f.name = n.to_string();
     }
     if let Some(sz) = e.child("sz").and_then(|s| s.attr_f64("val")) {
         f.size = sz.clamp(1.0, 409.0) as f32;
@@ -280,14 +277,14 @@ fn read_protection(p: &El) -> Protection {
     Protection { locked: p.flag("locked", true), hidden: p.flag("hidden", false) }
 }
 
-fn num_fmt_code(id: u32, custom: &HashMap<u32, NumFmt>) -> NumFmt {
+fn num_fmt_code(id: u32, custom: &HashMap<u32, String>) -> NumFmt {
     if let Some(c) = custom.get(&id) {
-        return c.clone();
+        return NumFmt::new(c);
     }
     NumFmt::new(builtin_format(id).unwrap_or("General"))
 }
 
-fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, NumFmt>, checkboxes: &[bool]) -> Style {
+fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, String>, checkboxes: &[bool]) -> Style {
     let mut s = Style::default();
     if let Some(f) = xf.attr_u32("fontId").and_then(|i| fonts.get(i as usize)) {
         s.font = f.clone();
@@ -339,8 +336,6 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    // One shared copy of each custom code, which every format using it points to.
-    let shared_fmts: HashMap<u32, NumFmt> = fmts.iter().map(|(id, code)| (*id, NumFmt::new(code))).collect();
     // The scheme link only means something on a font in the body face; elsewhere it stays as a new font has it.
     let body_font = |e: &El| {
         let mut f = read_font(e, &palette);
@@ -350,14 +345,10 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
     let fonts: Vec<Font> = root.child("fonts").map(|f| f.kids("font").map(body_font).collect()).unwrap_or_default();
     let fills: Vec<Fill> = root.child("fills").map(|f| f.kids("fill").map(|e| read_fill(e, &palette, false)).collect()).unwrap_or_default();
     let borders: Vec<Borders> = root.child("borders").map(|f| f.kids("border").map(|e| read_border(e, &palette)).collect()).unwrap_or_default();
-    let xfs = root
-        .child("cellXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, checkboxes)).collect())
-        .unwrap_or_default();
-    let style_xfs: Vec<Style> = root
-        .child("cellStyleXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, &[])).collect())
-        .unwrap_or_default();
+    let xfs =
+        root.child("cellXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, checkboxes)).collect()).unwrap_or_default();
+    let style_xfs: Vec<Style> =
+        root.child("cellStyleXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, &[])).collect()).unwrap_or_default();
     let mut named = Vec::new();
     if let Some(cs) = root.child("cellStyles") {
         for c in cs.kids("cellStyle") {
@@ -368,14 +359,12 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    // Inline dxf format codes are interned: a code repeated by many dxfs is one allocation.
-    let mut inline_fmts = HashMap::new();
-    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &shared_fmts, &mut inline_fmts)).collect()).unwrap_or_default();
+    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &fmts)).collect()).unwrap_or_default();
     StylesIn { xfs, dxfs, palette, named, num_fmts: fmts }
 }
 
 /// One differential format (`dxf`): the style it sets over a cell's own.
-fn read_dxf<'a>(x: &'a El, palette: &[u32], fmts: &HashMap<u32, NumFmt>, inline_fmts: &mut HashMap<&'a str, NumFmt>) -> Style {
+fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, String>) -> Style {
     let mut s = Style::default();
     if let Some(e) = x.child("font") {
         let f = read_font(e, palette);
@@ -398,7 +387,7 @@ fn read_dxf<'a>(x: &'a El, palette: &[u32], fmts: &HashMap<u32, NumFmt>, inline_
     }
     if let Some(n) = x.child("numFmt") {
         if let Some(code) = n.attr("formatCode") {
-            s.num_fmt = inline_fmts.entry(code).or_insert_with(|| NumFmt::new(code)).clone();
+            s.num_fmt = NumFmt::new(code);
         } else if let Some(id) = n.attr_u32("numFmtId") {
             s.num_fmt = num_fmt_code(id, fmts);
         }
@@ -768,12 +757,10 @@ pub fn write_styles(wb: &Workbook, dxfs: &[Style], extra_fmts: &[String]) -> (St
         s.push_str("</numFmts>");
     }
     let _ = write!(s, "<fonts count=\"{}\">", fonts.len());
-    // Font names are cut to `MAX_FONT_NAME` on reading: so is the theme's, to compare with them.
-    let theme_minor: String = wb.theme.minor_font.chars().take(MAX_FONT_NAME).collect();
     for (i, f) in fonts.iter().enumerate() {
         // A Normal font in the theme's body face follows the theme, as Excel writes it (#805); so
         // does every other body-face font that kept the scheme link (#2517).
-        s.push_str(&font_xml(f, None, (i == 0 || f.minor) && f.name == theme_minor));
+        s.push_str(&font_xml(f, None, (i == 0 || f.minor) && f.name == wb.theme.minor_font));
     }
     let _ = write!(s, "</fonts><fills count=\"{}\">", fills.len());
     for f in &fills {

crates/xlsx/src/tests/fixtures.rs

@@ -737,36 +737,3 @@ fn every_skipped_drawing_object_is_reported() {
         assert_eq!(hits, 1, "object {n}: {:?}", rep.warnings);
     }
 }
-
-/// A theme body font longer than 31 characters (the length font names are cut to on reading)
-/// still links a body-face font to the theme when saving.
-#[test]
-fn a_font_cut_to_31_characters_keeps_the_scheme_link_to_a_longer_theme_font() {
-    let long: String = "B".repeat(40);
-    let mut wb = gridcraft_model::Workbook::default();
-    wb.theme.minor_font = long.clone();
-    let style = wb.styles.derive(StyleId::DEFAULT, |s| {
-        s.font.name = long.chars().take(31).collect();
-        s.font.minor = true;
-    });
-    let mut sheet = gridcraft_model::Sheet::new("S");
-    sheet.cells.set(at("A1"), gridcraft_model::Cell { value: Value::Number(1.0), formula: None, style, py: None });
-    wb.sheets.push(std::sync::Arc::new(sheet));
-    let out = crate::write_xlsx(&wb).unwrap();
-    let st = String::from_utf8(crate::package::Package::open(&out).unwrap().read("xl/styles.xml").unwrap().unwrap()).unwrap();
-    assert!(st.contains(r#"<scheme val="minor"/>"#), "{st}");
-}
-
-/// A number format code repeated by many differential formats is read into one allocation.
-#[test]
-fn inline_dxf_number_formats_are_shared() {
-    let code = format!("0.00{}", " \"x\"".repeat(50));
-    let dxfs: String = (0..3).map(|_| format!(r#"<dxf><numFmt numFmtId="170" formatCode="{}"/></dxf>"#, code.replace('"', "&quot;"))).collect();
-    let styles =
-        format!(r#"<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><dxfs count="3">{dxfs}</dxfs></styleSheet>"#);
-    let root = crate::xml::parse(styles.as_bytes()).unwrap();
-    let read = crate::styles::read_styles(&root, &[], "Calibri");
-    assert_eq!(read.dxfs.len(), 3);
-    assert_eq!(read.dxfs[0].num_fmt.as_str(), code);
-    assert!(read.dxfs.iter().all(|d| std::sync::Arc::ptr_eq(&d.num_fmt.0, &read.dxfs[0].num_fmt.0)));
-}

crates/xlsx/src/tests/malformed.rs

@@ -385,22 +385,3 @@ fn outline_summary_directions_round_trip() {
     let xml = String::from_utf8(crate::package::Package::open(&bytes).unwrap().read("xl/worksheets/sheet1.xml").unwrap().unwrap()).unwrap();
     assert!(!xml.contains("outlinePr"), "{xml}");
 }
-
-/// Many cell formats naming one long font and one long custom number format: the code is shared
-/// by every format and the font name is cut to Excel's 31 characters, so neither is copied per
-/// format.
-#[test]
-fn cell_formats_share_their_number_format_and_keep_short_font_names() {
-    let xml = format!(
-        r#"<styleSheet><numFmts><numFmt numFmtId="164" formatCode="{}"/></numFmts><fonts><font><name val="{}"/></font></fonts><cellXfs>{}</cellXfs></styleSheet>"#,
-        "0".repeat(100_000),
-        "F".repeat(100_000),
-        r#"<xf numFmtId="164" fontId="0"/>"#.repeat(20_000)
-    );
-    let root = crate::xml::parse(xml.as_bytes()).unwrap();
-    let st = crate::styles::read_styles(&root, &[], "Calibri");
-    assert_eq!(st.xfs.len(), 20_000);
-    assert!(st.xfs.iter().all(|s| std::sync::Arc::ptr_eq(&s.num_fmt.0, &st.xfs[0].num_fmt.0)));
-    assert_eq!(st.xfs[0].num_fmt.as_str().len(), 100_000);
-    assert!(st.xfs.iter().all(|s| s.font.name.chars().count() == 31));
-}

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();