collective
Browse / github.com/storytold/gridcraft / patch
perf: win back formula-entry and XLSX-save time lost since 23829d8c
perf: win back formula-entry and XLSX-save time lost since 23829d8c Formula entry and XLSX save got ~20% and ~13% slower over the ~880 commits since 23829d8c. This finds where the time went and wins most of it back. Output doesn't change. Measured with the calculation bench from #2651 (`crates/engine/examples/bench.rs`) at 100k rows on an M2 Pro, with main and this branch run in turn three times: | scenario | main | this PR | |---|---|---| | enter.formulas (paste 100k formulas) | ~420 ms | ~379 ms (−10%) | | xlsx.save (300k-formula model) | ~525 ms | ~478 ms (−9%) | The other scenarios don't change beyond noise. ## Changes - **`number_to_text`** (f29ac7d9e
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- nihilist-narwhal (imported from pull request #2654, open)
- Base
d8c5f5885fc5320f8f1023d5fdc9f484a185ba86- Size
- +53 −8 in 6 files
- Review
- pending
- Shared
- 2026-10-28
- Id
c6130594d54cd24cb02ab735
How it went
No other operator has reported using it yet.
Diff
raw · crates/core/src/value.rs crates/engine/src/cmd/data.rs crates/engine/src/lib.rs crates/formula/src/ast.rs crates/model/src/store.rs crates/xlsx/src/fmla.rs
@@ -359,8 +359,9 @@ pub fn number_to_text(n: f64) -> String { if !(1e-9..1e15).contains(&a) && a.round() != a || a >= 1e15 { return sci_text(n, 15); }- // A value just below 1e15 can round up to 1e15 at 15 significant digits.- if format!("{:.14e}", a).parse::<f64>().is_ok_and(|r| r >= 1e15) {+ // A value just below 1e15 can round up to 1e15 at 15 significant digits. Only that narrow+ // band pays for the exact formatting: it runs for every number literal a formula prints.+ if a >= 9.999_999_999_999_9e14 && format!("{:.14e}", a).parse::<f64>().is_ok_and(|r| r >= 1e15) { return sci_text(n, 15); } // 15 significant digits, trailing zeros trimmed.@@ -411,6 +412,9 @@ mod tests { assert_eq!(number_to_text(999999999999999.9), "1E+15"); assert_eq!(number_to_text(-999999999999999.9), "-1E+15"); assert_eq!(number_to_text(999999999999999.0), "999999999999999");+ assert_eq!(number_to_text(999999999999999.5), "1E+15");+ assert_eq!(number_to_text(999999999999999.4), "999999999999999");+ assert_eq!(number_to_text(999999999999989.9), "999999999999990"); } #[test]@@ -665,14 +665,15 @@ fn filter_blank(v: &Value) -> bool { /// A table's own filter is left to the table, and a row typed after a blank row does not count. /// A formula counts before it is calculated: this runs inside the edit, ahead of the recalc. pub(crate) fn grow_autofilter(sh: &mut Sheet, at: CellRef) {- if !sh.cells.get(at).is_some_and(|c| !filter_blank(&c.value) || c.formula.is_some()) {- return;- }+ // The AutoFilter checks come first: they are cheap, and most sheets have none. let Some(af) = sh.autofilter.as_ref() else { return }; let r = af.range; if r.end.row.checked_add(1) != Some(at.row) || at.col < r.start.col || at.col > r.end.col || sh.tables.iter().any(|t| t.range == r) { return; }+ if !sh.cells.get(at).is_some_and(|c| !filter_blank(&c.value) || c.formula.is_some()) {+ return;+ } let grown = RangeRef::new(r.start, CellRef::new(at.row, r.end.col)); if let Some(af) = sh.autofilter.as_mut() { af.range = grown;@@ -266,6 +266,19 @@ fn trim_to_budget(history: &mut Vec<HistoryEntry>, byte_budget: usize) { } } +/// A writer that only counts the bytes written to it.+struct ByteCounter(usize);++impl std::io::Write for ByteCounter {+ fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {+ self.0 = self.0.saturating_add(buf.len());+ Ok(buf.len())+ }+ fn flush(&mut self) -> std::io::Result<()> {+ Ok(())+ }+}+ impl DocState { /// A document for `wb`, which came from where `trust` says: [`Trust::FromFile`] for anything /// read from bytes (a file, a download, a recovery copy), [`Trust::Local`] only for a workbook@@ -557,7 +570,11 @@ impl Session { /// Journals a command, then trims the oldest entries past either cap. A recording in progress /// keeps pointing at the same entry (the steps the trim dropped are lost). fn push_journal(&mut self, id: &str, params: Json) {- let size = |e: &(String, Json)| serde_json::to_vec(e).map_or(0, |v| v.len());+ // Counted as it is written, without building the text: a paste's params can be MBs.+ let size = |e: &(String, Json)| {+ let mut counter = ByteCounter(0);+ serde_json::to_writer(&mut counter, e).map_or(0, |()| counter.0)+ }; // `journal` is public: if it was changed from outside, measure it again. if self.journal_sizes.len() != self.journal.len() { self.journal_sizes = self.journal.iter().map(size).collect();@@ -215,7 +215,9 @@ impl Expr { /// Visits every node depth-first, parents before children, left to right. pub fn walk<'a>(&'a self, f: &mut dyn FnMut(&'a Expr)) { // An explicit stack: a long operator chain must not recurse once per operand.- let mut pending = vec![self];+ // Room for a typical formula up front, so walking one costs one allocation, not several.+ let mut pending = Vec::with_capacity(8);+ pending.push(self); while let Some(e) = pending.pop() { f(e); match e {@@ -236,6 +238,19 @@ impl Expr { } /// Rewrites every node bottom-up, children left to right before their parent. pub fn map(self, f: &mut dyn FnMut(Expr) -> Expr) -> Expr {+ // One operator on a non-chain operand (`A1*2`), the common case, needs no spine: rewrite+ // the operands in their boxes.+ if let Expr::Binary(op, mut a, mut b) = self {+ if !matches!(*a, Expr::Binary(..)) {+ *a = std::mem::replace(&mut *a, Expr::Missing).map(f);+ *b = std::mem::replace(&mut *b, Expr::Missing).map(f);+ return f(Expr::Binary(op, a, b));+ }+ return Expr::Binary(op, a, b).map_chain(f);+ }+ self.map_chain(f)+ }+ fn map_chain(self, f: &mut dyn FnMut(Expr) -> Expr) -> Expr { // Unwind the left spine of a long operator chain into a list, then rebuild it from the // innermost operand outwards, in the order a recursive rewrite would visit the nodes. let mut spine = Vec::new();@@ -283,6 +298,12 @@ impl Expr { impl Clone for Expr { fn clone(&self) -> Self {+ // One operator on a non-chain operand needs no spine.+ if let Expr::Binary(op, a, b) = self+ && !matches!(**a, Expr::Binary(..))+ {+ return Expr::Binary(*op, a.clone(), b.clone());+ } // Copy the left spine of an operator chain in a loop (see [`Expr`]). let mut spine = Vec::new(); let mut node = self;@@ -233,11 +233,12 @@ impl CellStore { bytes = bytes.saturating_add(size_of::<Band>()); for (row, cols) in &band.rows { bytes = bytes.saturating_add(size_of::<u32>() + 3 * size_of::<usize>());+ let next_row = next.and_then(|b| b.rows.get(row)); for (col, cell) in cols { // Cells and BTreeMap nodes are duplicated with a cloned band even when // their formula/value Arcs are still shared with the next snapshot. bytes = bytes.saturating_add(size_of::<u32>() + size_of::<Cell>() + 3 * size_of::<usize>());- let next_cell = next.and_then(|b| b.rows.get(row)).and_then(|r| r.get(col));+ let next_cell = next_row.and_then(|r| r.get(col)); if let Some(formula) = &cell.formula && !next_cell.and_then(|c| c.formula.as_ref()).is_some_and(|f| Arc::ptr_eq(f, formula)) {@@ -30,6 +30,7 @@ fn prefix_params(e: Expr, scope: &[String]) -> Expr { Expr::Name(n) if scope.iter().any(|s| s.eq_ignore_ascii_case(&n)) => Expr::Name(format!("{PARAM_PREFIX}{n}")), Expr::Unary(op, x) => Expr::Unary(op, sub(x, scope)), Expr::Paren(x) => Expr::Paren(sub(x, scope)),+ Expr::Binary(op, a, b) if !matches!(*a, Expr::Binary(..)) => Expr::Binary(op, sub(a, scope), sub(b, scope)), Expr::Binary(op, a, b) => { // A long chain such as A1+A2+… nests on its left: follow it in a loop, not one // level of recursion per operand.