From 12d26d53cc5c03832fddb109d79667623f3f2b11 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 07:06:18 -0500 Subject: [PATCH] xlsx: analyse each kept chart sheet's graph once per save (#3261) * xlsx: analyse each kept chart sheet's graph once per save 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. * address review: cache the chart-sheet usability verdict per relationship id instead of every reference * address review: cache chart-sheet analysis by the graph root, not the relationship id --- crates/xlsx/src/passthrough/write.rs | 28 +++++++++++++++------- crates/xlsx/src/tests/passthrough_tests.rs | 26 ++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/crates/xlsx/src/passthrough/write.rs b/crates/xlsx/src/passthrough/write.rs index cf11d0216..1670c66bb 100644 --- a/crates/xlsx/src/passthrough/write.rs +++ b/crates/xlsx/src/passthrough/write.rs @@ -260,27 +260,37 @@ pub(crate) fn sheet_names_in(formula: &str) -> Vec { /// 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 the normalized target of the chart sheet's relationship, whether its graph +/// refers only to sheets that exist (`false` when the relationship is missing): entries sharing one +/// graph, under the same or different relationship ids, are analysed once, and only the verdict is +/// kept, not the references it came from. +fn chart_sheet_usable(wb: &Workbook, p: &Preserved, s: &PreservedSheet, usable: &mut HashMap) -> bool { let same = |a: &str, b: &str| a.chars().take(31).collect::().eq_ignore_ascii_case(&b.chars().take(31).collect::()); 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(normalize(&root.target)).or_insert_with(|| { + 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. diff --git a/crates/xlsx/src/tests/passthrough_tests.rs b/crates/xlsx/src/tests/passthrough_tests.rs index a71b34422..d014319fb 100644 --- a/crates/xlsx/src/tests/passthrough_tests.rs +++ b/crates/xlsx/src/tests/passthrough_tests.rs @@ -696,6 +696,32 @@ fn chartsheet_package() -> Vec { ]) } +/// 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)> = 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#""#, 100 + i)).collect(); + for (name, data) in &mut parts { + if name == "xl/workbook.xml" { + *data = String::from_utf8(data.clone()).unwrap().replace("", &format!("{extra}")).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();