From a09d3f6934d43b4da9f9d592d674c685c1508537 Mon Sep 17 00:00:00 2001 From: Bryan Mark Date: Fri, 16 Oct 2026 14:26:53 +0900 Subject: [PATCH] Memory: undo steps share picture and macro bytes instead of copying them Every edit copies the workbook for its undo step (copy-on-write: sheets and 64-row bands of cells are shared). A floating picture's bytes (Image.data) and the VBA project (Workbook.vba_project) were plain Vec, so each edit on a sheet with a picture, and every edit in a workbook with macros, kept another full copy of them, up to the 100 steps of the history. They are Arc> now, as cell pictures already are, with base64 helpers for serde (b64::shared, b64::opt). JSON and XLSX are unchanged. Measured on macOS 27 (Apple M3 Max), gridcraft-cli, kernel peak footprint: a 19 MB picture and 50 cell edits on its sheet, 992 -> 22 MB; 10 edits, 215 -> 21 MB. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +++- crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/print.rs | 2 +- crates/engine/src/io.rs | 2 +- crates/engine/src/tests.rs | 31 ++++++++++++++++++++++++++++- crates/model/src/b64.rs | 26 ++++++++++++++++++++---- crates/model/src/features.rs | 7 ++++--- crates/model/src/workbook.rs | 5 +++-- crates/ui-egui/tests/interaction.rs | 6 +++--- crates/xlsx/src/drawing.rs | 2 +- crates/xlsx/src/read.rs | 2 +- crates/xlsx/src/sheet_write.rs | 2 +- crates/xlsx/src/tests/fixtures.rs | 2 +- crates/xlsx/src/tests/roundtrip.rs | 6 +++--- crates/xlsx/src/write.rs | 2 +- 15 files changed, 76 insertions(+), 25 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..b9846d57f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-16 · **Change:** minor (undo steps share picture and macro bytes) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-16 | Memory: undo steps share a sheet's picture bytes and the workbook's VBA project instead of copying them: 50 edits on a sheet with a 20 MB picture keep 22 MB instead of 992 MB | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | Progress log: undo steps share picture and macro bytes | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d878e4469..38028b0e6 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -977,7 +977,7 @@ fn insert_picture(s: &mut Session, p: &Json) -> Result { // Excel inserts pictures as "Move but don't size with cells" (`editAs="oneCell"`). mode: anchor_mode_param(p).unwrap_or(gridcraft_model::AnchorMode::MoveOnly), }, - data, + data: data.into(), mime, alt: str_param(p, "alt").unwrap_or("").into(), }; diff --git a/crates/engine/src/cmd/print.rs b/crates/engine/src/cmd/print.rs index a10f19d46..0b8d22ade 100644 --- a/crates/engine/src/cmd/print.rs +++ b/crates/engine/src/cmd/print.rs @@ -2000,7 +2000,7 @@ mod tests { wb.sheet_mut(0).unwrap().images.push(Image { id, anchor: Anchor { cell: CellRef::new(5, 5), dx: 0.0, dy: 0.0, width: 40.0, height: 40.0, mode: gridcraft_model::AnchorMode::MoveAndSize }, - data: png, + data: png.into(), mime: "image/png".into(), alt: String::new(), }); diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 3e84036cb..067a15441 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -517,7 +517,7 @@ mod tests { #[test] fn macros_are_saved_only_into_xlsm() { let mut wb = Workbook::new(); - wb.vba_project = Some(b"vba".to_vec()); + wb.vba_project = Some(b"vba".to_vec().into()); let has_macros = |path: &str| { let bytes = save_bytes(&wb, path).unwrap(); gridcraft_xlsx::read_xlsx(&bytes).unwrap().0.vba_project.is_some() diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..7b335c474 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2549,7 +2549,7 @@ fn add_image(s: &mut Session, cell: &str, mode: gridcraft_model::AnchorMode) -> wb.sheet_mut(0).unwrap().images.push(gridcraft_model::Image { id, anchor: gridcraft_model::Anchor { cell: CellRef::parse(cell).unwrap(), dx: 0.0, dy: 0.0, width: 40.0, height: 40.0, mode }, - data: tiny_png(), + data: tiny_png().into(), mime: "image/png".into(), alt: String::new(), }); @@ -2560,6 +2560,35 @@ fn image_cell(s: &Session, id: u32) -> Option { s.doc().unwrap().wb.active().unwrap().images.iter().find(|i| i.id == id).map(|i| i.anchor.cell) } +/// Every undo step holds a copy of the workbook. A picture's bytes and the VBA project are shared +/// between those copies instead of copied into each: a 20 MB picture used to cost another 20 MB +/// for every cell typed on its sheet, up to 100 steps (2 GB). +#[test] +fn undo_steps_share_picture_and_macro_bytes() { + let mut s = s(); + s.execute("insert.picture", json!({"base64": crate::io::base64_encode(&tiny_png()), "at": "C3"})).unwrap(); + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).vba_project = Some(b"vba".to_vec().into()); + for i in 1..=3 { + s.execute("cell.set", json!({"cell": format!("A{i}"), "input": i.to_string()})).unwrap(); + } + let d = s.doc().unwrap(); + let live = &d.wb.active().unwrap().images[0].data; + let vba = d.wb.vba_project.as_ref().unwrap(); + assert_eq!(**live, tiny_png()); + assert!(d.undo.len() >= 3); + for step in d.undo.iter().filter(|h| !h.wb.active().unwrap().images.is_empty()) { + assert!(std::sync::Arc::ptr_eq(&step.wb.active().unwrap().images[0].data, live), "{}", step.label); + } + for step in d.undo.iter().rev().take(3) { + assert!(std::sync::Arc::ptr_eq(step.wb.vba_project.as_ref().unwrap(), vba), "{}", step.label); + } + // Undo and redo still give the picture back as it was. + s.execute("edit.undo", json!({"steps": 3})).unwrap(); + assert_eq!(*s.doc().unwrap().wb.active().unwrap().images[0].data, tiny_png()); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(*s.doc().unwrap().wb.active().unwrap().images[0].data, tiny_png()); +} + /// Issue #5: images anchored in cells follow their row when the sheet is sorted. #[test] fn sort_moves_anchored_images_with_their_rows() { diff --git a/crates/model/src/b64.rs b/crates/model/src/b64.rs index 30db4b43d..b377fe451 100644 --- a/crates/model/src/b64.rs +++ b/crates/model/src/b64.rs @@ -49,22 +49,40 @@ pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> decode(&s).ok_or_else(|| serde::de::Error::custom("invalid base64")) } -/// Base64 for optional byte blobs (VBA projects): `null`/missing stays `None`. +/// Base64 for byte blobs shared through `Arc` (floating pictures), so copies of a workbook, such as +/// its undo steps, share one payload. +pub mod shared { + use std::sync::Arc; + + use serde::{Deserializer, Serializer}; + + pub fn serialize(v: &Arc>, s: S) -> Result { + super::serialize(v, s) + } + + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>, D::Error> { + super::deserialize(d).map(Arc::new) + } +} + +/// Base64 for optional shared byte blobs (VBA projects): `null`/missing stays `None`. pub mod opt { + use std::sync::Arc; + use serde::{Deserialize, Deserializer, Serializer}; - pub fn serialize(v: &Option>, s: S) -> Result { + pub fn serialize(v: &Option>>, s: S) -> Result { match v { Some(data) => s.serialize_str(&super::encode(data)), None => s.serialize_none(), } } - pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>, D::Error> { + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result>>, D::Error> { let s = Option::::deserialize(d)?; match s { None => Ok(None), - Some(s) => super::decode(&s).map(Some).ok_or_else(|| serde::de::Error::custom("invalid base64")), + Some(s) => super::decode(&s).map(|v| Some(Arc::new(v))).ok_or_else(|| serde::de::Error::custom("invalid base64")), } } } diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b51f7bc98..2dafb1c5c 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -508,9 +508,10 @@ pub struct Chart { pub struct Image { pub id: u32, pub anchor: Anchor, - /// Encoded bytes (PNG/JPEG), base64 in JSON. - #[serde(with = "crate::b64")] - pub data: Vec, + /// Encoded bytes (PNG/JPEG), base64 in JSON. Shared, so a copy of the sheet (an undo step) + /// doesn't copy the picture. + #[serde(with = "crate::b64::shared")] + pub data: std::sync::Arc>, pub mime: String, pub alt: String, } diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 828ca692e..fd300b9fa 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -106,9 +106,10 @@ pub struct Workbook { pub cell_styles: Vec<(String, crate::style::Style)>, /// VBA project (`xl/vbaProject.bin`) carried through unchanged. GridCraft does not run /// macros; the blob is preserved so that saving a macro-enabled workbook never destroys the - /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. + /// macros it arrived with. base64 in JSON. `None` = the workbook has no macros. Shared, so a + /// copy of the workbook (every edit's undo step) doesn't copy it. #[serde(default, with = "crate::b64::opt")] - pub vba_project: Option>, + pub vba_project: Option>>, /// Field separator of the CSV this workbook was read from, so saving it as CSV writes the same /// separator back (a `;` file stays a `;` file). `None` = not read from CSV: CSV saves use `,`. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index b40769cf8..0af83753b 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -750,7 +750,7 @@ fn column_autocomplete_can_be_turned_off() { /// An .xlsm carrying a VBA project, saved to bytes. fn macro_workbook_bytes() -> Vec { let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec()); + wb.vba_project = Some(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec().into()); gridcraft_engine::io::save_bytes(&wb, "macros.xlsm").unwrap() } @@ -798,7 +798,7 @@ fn plain_open_warning_becomes_an_info_notice() { fn macros_dialog_lists_vba_and_scripts() { let mut s = blank(); let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1].into()); s.active_mut().unwrap().wb = std::sync::Arc::new(wb); let mut h = harness(s); h.state_mut().open_dialog("macros", json!({})); @@ -814,7 +814,7 @@ fn macros_dialog_lists_vba_and_scripts() { fn status_bar_shows_macro_badge() { let mut s = blank(); let mut wb = gridcraft_engine::model::Workbook::new(); - wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]); + wb.vba_project = Some(vec![0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1].into()); s.active_mut().unwrap().wb = std::sync::Arc::new(wb); // Rendering the status bar must not panic with a VBA project present. let mut h = harness(s); diff --git a/crates/xlsx/src/drawing.rs b/crates/xlsx/src/drawing.rs index 6dbd17e7e..8cd63873a 100644 --- a/crates/xlsx/src/drawing.rs +++ b/crates/xlsx/src/drawing.rs @@ -156,7 +156,7 @@ pub fn read_drawing(cx: &mut Ctx<'_>, part: &str, sheet: &mut Sheet) -> Result<( }; let alt = obj.path(&["nvPicPr", "cNvPr"]).and_then(|c| c.attr("descr")).unwrap_or("").to_string(); let id = cx.next_object_id(); - sheet.images.push(Image { id, anchor, data, mime: mime_for(&target).to_string(), alt }); + sheet.images.push(Image { id, anchor, data: data.into(), mime: mime_for(&target).to_string(), alt }); } "graphicFrame" => { let Some(gd) = obj.path(&["graphic", "graphicData"]) else { continue }; diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..160e838d6 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -331,7 +331,7 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { if let Some(r) = find_rel(&wb_rels, "vbaProject").cloned() { match cx.pkg.read(&r.target) { Ok(Some(data)) => { - wb.vba_project = Some(data); + wb.vba_project = Some(data.into()); cx.warn("macros (VBA project) were kept but not executed"); } Ok(None) => cx.warn("macros (VBA project) reference a missing part and were dropped"), diff --git a/crates/xlsx/src/sheet_write.rs b/crates/xlsx/src/sheet_write.rs index 78e1afaae..b5fcaa052 100644 --- a/crates/xlsx/src/sheet_write.rs +++ b/crates/xlsx/src/sheet_write.rs @@ -502,7 +502,7 @@ pub fn write_sheet(wb: &Workbook, si: usize, selected: bool, out: &mut Out) -> ( let ext = image_ext(&img.mime, &img.data); let name = format!("xl/media/image{}.{ext}", out.images); out.default_type(ext, image_content_type(ext)); - out.part(&name, None, img.data.clone()); + out.part(&name, None, img.data.to_vec()); let rid = drels.add("image", &format!("../media/image{}.{ext}", out.images)); objs.push(Obj::Image(img, rid)); } diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 39ea30951..4ab178159 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -381,7 +381,7 @@ fn drawing_with_picture_and_chart() { let (wb, _) = read_xlsx(&bytes).unwrap(); let s = wb.sheet(0).unwrap(); let img = &s.images[0]; - assert_eq!(img.data, png); + assert_eq!(*img.data, png); assert_eq!(img.mime, "image/png"); assert_eq!(img.alt, "logo"); assert_eq!(img.anchor.cell, at("B3")); diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 901802905..5cd835878 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -326,7 +326,7 @@ fn sample() -> Workbook { id: 10, // A non-default anchor mode, to prove it survives an xlsx round-trip. anchor: Anchor { cell: at("C10"), dx: 5.0, dy: 4.0, width: 120.0, height: 80.0, mode: AnchorMode::MoveOnly }, - data: png, + data: png.into(), mime: "image/png".into(), alt: "Logo \"x\"".into(), }); @@ -737,7 +737,7 @@ fn vba_project_roundtrip() { // carries the macro-enabled content type plus the vbaProject relationship. let vba = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1 fake OLE2 vbaProject".to_vec(); let mut wb = Workbook::new(); - wb.vba_project = Some(vba.clone()); + wb.vba_project = Some(vba.clone().into()); let bytes = write_xlsx(&wb).unwrap(); assert_eq!(write_xlsx(&wb).unwrap(), bytes, "output is deterministic"); @@ -756,7 +756,7 @@ fn vba_project_roundtrip() { assert!(rels.contains("relationships/vbaProject"), "vbaProject relationship: {rels}"); let (back, rep) = read_xlsx(&bytes).unwrap(); - assert_eq!(back.vba_project.as_deref(), Some(vba.as_slice()), "vbaProject survives the read"); + assert_eq!(back.vba_project.as_deref(), Some(&vba), "vbaProject survives the read"); assert!(rep.warnings.iter().any(|w| w.contains("not executed")), "{:?}", rep.warnings); // A workbook without macros stays macro-free and free of VBA warnings. diff --git a/crates/xlsx/src/write.rs b/crates/xlsx/src/write.rs index 06761f01d..6920bec55 100644 --- a/crates/xlsx/src/write.rs +++ b/crates/xlsx/src/write.rs @@ -404,7 +404,7 @@ pub fn write_xlsx(wb: &Workbook, kind: XlsxKind) -> Result, IoError> { // A macro-free package can't hold a VBA project (Excel refuses such a file). if let Some(data) = wb.vba_project.as_ref().filter(|_| kind.is_macro_enabled()) { wb_rels.add("vbaProject", "vbaProject.bin"); - out.part("xl/vbaProject.bin", Some(CT_VBA), data.clone()); + out.part("xl/vbaProject.bin", Some(CT_VBA), data.to_vec()); } out.part("xl/_rels/workbook.xml.rels", None, wb_rels.xml().into_bytes());