From fd53de7c46bf399c4979184444b37bb1d10eec9e Mon Sep 17 00:00:00 2001 From: vangel76 Date: Mon, 12 Oct 2026 22:09:09 +0200 Subject: [PATCH] Numfmt: TEXT() and chart labels parse each format code once (#806) Co-Authored-By: Claude Sonnet 5.5 --- ROADMAP.md | 1 + crates/chart/src/axis.rs | 2 +- crates/chart/src/resolve.rs | 2 +- crates/numfmt/src/lib.rs | 76 +++++++++++++++- crates/numfmt/src/tests.rs | 171 ++++++++++++++++++++++++++++++++++++ 5 files changed, 249 insertions(+), 3 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index e8e1c06..a751272 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -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-12 | `TEXT()` over a range and chart labels parse each number format code once instead of once per value (#806) | | 2026-10-12 | File › Print selects a system printer, copies, duplex and grayscale, submits PDF jobs through craft-print on a worker and shows spooler errors; browser printing and PDF export remain available | | 2026-10-11 | Command-sequence fuzzing: random command sequences checked against undo/redo, XLSX round-trip and full recalculation; fixes for the table, legacy-array, spill, AutoSum, Watch Window and sheet-copy bugs it found | | 2026-10-11 | Password-protected workbooks open: GridCraft asks for the password and decrypts Agile and Standard encryption (#11) | diff --git a/crates/chart/src/axis.rs b/crates/chart/src/axis.rs index eb4f733..8686bf7 100644 --- a/crates/chart/src/axis.rs +++ b/crates/chart/src/axis.rs @@ -122,7 +122,7 @@ impl Fmt { pub fn new(code: &str) -> Fmt { let t = code.trim(); let general = t.is_empty() || t.eq_ignore_ascii_case("general"); - Fmt { general, fmt: NumberFormat::parse(if general { "General" } else { t }) } + Fmt { general, fmt: NumberFormat::parse_cached(if general { "General" } else { t }) } } /// Formats `v`; `step` (axis labels) fixes the decimals for General. pub fn format(&self, v: f64, step: Option) -> String { diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index 2c17eff..b3de1ca 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -145,7 +145,7 @@ fn category_texts(wb: &Workbook, sheet: usize, formula: &str) -> Vec { if let Some(c) = at { let code = &wb.styles.get(s.style_id(c)).num_fmt.0; if !code.eq_ignore_ascii_case("general") { - return format_value(v, &NumberFormat::parse(code), wb.date_system).text; + return format_value(v, &NumberFormat::parse_cached(code), wb.date_system).text; } } } diff --git a/crates/numfmt/src/lib.rs b/crates/numfmt/src/lib.rs index 674e844..b4f259f 100644 --- a/crates/numfmt/src/lib.rs +++ b/crates/numfmt/src/lib.rs @@ -101,6 +101,8 @@ impl Default for NumberFormat { impl NumberFormat { /// Parses a format code. Never fails: unknown or malformed parts degrade to literal text. pub fn parse(code: &str) -> NumberFormat { + #[cfg(test)] + parse_cache::count_parse(); let trimmed = code.trim(); // `checkbox` marks a checkbox cell (Insert › Checkbox): TRUE/FALSE draw as a box, any other // value displays as General. @@ -294,6 +296,78 @@ pub fn format_value(v: &Value, fmt: &NumberFormat, sys: DateSystem) -> Formatted } } +/// A small per-thread cache of parsed format codes for callers that format many values with the +/// same code (`TEXT` over a range, chart labels). A parsed format depends on the code text only +/// (the date system and values are applied when formatting), so the code is the whole key. +mod parse_cache { + use std::cell::RefCell; + use std::collections::HashMap; + + use super::NumberFormat; + + /// Most codes kept per thread. A full cache is emptied, so memory stays bounded. + pub(super) const CAPACITY: usize = 64; + /// Longer codes are parsed every time and never stored. + pub(super) const MAX_CODE_LEN: usize = 256; + + thread_local! { + static CACHE: RefCell> = RefCell::new(HashMap::new()); + } + + pub(super) fn get(code: &str) -> NumberFormat { + if code.len() > MAX_CODE_LEN { + return NumberFormat::parse(code); + } + // Not usable (thread shutting down, re-entered): parse without the cache. + let cached = CACHE + .try_with(|c| { + let mut c = c.try_borrow_mut().ok()?; + if let Some(f) = c.get(code) { + return Some(f.clone()); + } + let f = NumberFormat::parse(code); + if c.len() >= CAPACITY { + c.clear(); + } + c.insert(code.to_string(), f.clone()); + Some(f) + }) + .ok() + .flatten(); + cached.unwrap_or_else(|| NumberFormat::parse(code)) + } + + #[cfg(test)] + thread_local! { + static PARSES: std::cell::Cell = const { std::cell::Cell::new(0) }; + } + + #[cfg(test)] + pub(super) fn count_parse() { + let _ = PARSES.try_with(|p| p.set(p.get() + 1)); + } + + /// Parses done on this thread so far (tests only). + #[cfg(test)] + pub(super) fn parses() -> usize { + PARSES.try_with(std::cell::Cell::get).unwrap_or(0) + } + + /// Empties this thread's cache (tests only). + #[cfg(test)] + pub(super) fn clear() { + let _ = CACHE.try_with(|c| c.borrow_mut().clear()); + } +} + +impl NumberFormat { + /// Like [`NumberFormat::parse`], but remembers recently parsed codes (per thread, bounded) so + /// formatting many values with one code parses it once. Same result as `parse`. + pub fn parse_cached(code: &str) -> NumberFormat { + parse_cache::get(code) + } +} + /// The `TEXT()` worksheet function: formats a value with a format code. Numeric text is /// converted to a number first; other text and booleans (as `TRUE`/`FALSE`) go through the text /// section, or are returned as is. @@ -301,7 +375,7 @@ pub fn text_function(v: &Value, code: &str, sys: DateSystem) -> Result return Err(e), Value::Empty => 0.0, diff --git a/crates/numfmt/src/tests.rs b/crates/numfmt/src/tests.rs index 6c2306c..ca61418 100644 --- a/crates/numfmt/src/tests.rs +++ b/crates/numfmt/src/tests.rs @@ -582,3 +582,174 @@ fn decimal_digits_keeps_fifteen_digits_and_truncates_a_tie() { let (d, e) = decimal_digits(999999999999999.6); assert_eq!((d, e), ([1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], 15)); } + +// ---- parse cache ---- + +/// `TEXT()` as it was before the cache: parse the code, then format. +fn text_uncached(v: &Value, code: &str, sys: DateSystem) -> Result { + if code.is_empty() { + return Ok(String::new()); + } + let fmt = NumberFormat::parse(code); + let n = match v.scalar() { + Value::Error(e) => return Err(e), + Value::Empty => 0.0, + Value::Number(n) => n, + Value::Bool(b) => return Ok(fmt.format_text(if b { "TRUE" } else { "FALSE" }).text), + Value::Text(t) => match gridcraft_core::parse::parse_number_text(&t) { + Some(n) => n, + None => return Ok(fmt.format_text(&t).text), + }, + Value::Array(_) => return Err(CellError::Value), + }; + if !n.is_finite() { + return Err(CellError::Num); + } + let (f, overflow) = fmt.format_number(n, sys); + if overflow { Err(CellError::Value) } else { Ok(f.text) } +} + +const CACHE_CODES: &[&str] = &[ + "0", + "0.00", + "#,##0.00", + "#,##0.00;[Red](#,##0.00)", + "$#,##0.00", + "0%", + "0.0%", + "0.00E+00", + "##0.0E+0", + "General", + "general", + "checkbox", + "@", + "0;@", + "0;-0;;@", + "00000", + "# ?/?", + "# ??/16", + "?/?", + "m/d/yyyy", + "yyyy-mm-dd hh:mm:ss", + "dddd, mmmm d, yyyy", + "h:mm AM/PM", + "[h]:mm:ss", + "mm:ss.000", + "[Red]0;[Blue]-0", + "[>100]0.0;[<5]0.000;0", + "[<=9999999]###-####;(###) ###-####", + "[$-409]d mmm yy", + "\"abc\"0", + "\\x0", + "*-0", + "_(* #,##0_)", + "0.0.0", + "[", + "\"abc", + "0;0;0;0;0", + "#,##0.00;[Red", + "[$-]0", + " 0.00 ", + "[Color99]0", + "0.00_);(0.00)", + "#,,\"M\"", + "yyyy", + "", +]; + +fn cache_values() -> Vec { + vec![ + Value::Number(0.0), + Value::Number(1234.5678), + Value::Number(-42.5), + Value::Number(0.5), + Value::Number(45000.75), + Value::Number(1e15), + Value::Number(1e-7), + Value::Number(-0.0001), + Value::Number(f64::INFINITY), + Value::from("abc"), + Value::from("12.5"), + Value::Bool(true), + Value::Empty, + Value::Error(CellError::NA), + ] +} + +#[test] +fn parse_cached_gives_the_same_results_cold_and_warm() { + assert!(CACHE_CODES.len() >= 40); + parse_cache::clear(); + let values = cache_values(); + for sys in [DateSystem::D1900, DateSystem::D1904] { + // Cold on the first system, warm on the second. + for code in CACHE_CODES { + let direct = NumberFormat::parse(code); + let cached = NumberFormat::parse_cached(code); + assert_eq!(cached.code(), direct.code(), "{code:?}"); + assert_eq!( + (cached.kind(), cached.is_date(), cached.is_text(), cached.is_general()), + (direct.kind(), direct.is_date(), direct.is_text(), direct.is_general()) + ); + for v in &values { + let want = text_uncached(v, code, sys); + assert_eq!(text_function(v, code, sys), want, "TEXT({v:?}, {code:?})"); + // Again, now certainly warm. + assert_eq!(text_function(v, code, sys), want, "warm TEXT({v:?}, {code:?})"); + assert_eq!(format_value(v, &cached, sys), format_value(v, &direct, sys), "{v:?} {code:?}"); + } + } + } +} + +#[test] +fn parse_cached_parses_each_code_once() { + parse_cache::clear(); + let before = parse_cache::parses(); + for i in 0..10_000 { + assert!(text_function(&Value::Number(f64::from(i) + 0.5), "0.00", S).is_ok()); + } + assert_eq!(parse_cache::parses() - before, 1); + + let before = parse_cache::parses(); + for i in 0..10_000 { + let code = if i % 2 == 0 { "0.00" } else { "#,##0.0" }; + assert!(text_function(&Value::Number(f64::from(i)), code, S).is_ok()); + } + // "0.00" is still cached from above: only the second code is new. + assert_eq!(parse_cache::parses() - before, 1); + + parse_cache::clear(); + let before = parse_cache::parses(); + for i in 0..10_000 { + let code = if i % 2 == 0 { "0.00" } else { "#,##0.0" }; + assert!(text_function(&Value::Number(f64::from(i)), code, S).is_ok()); + } + assert_eq!(parse_cache::parses() - before, 2); +} + +#[test] +fn parse_cache_is_bounded() { + parse_cache::clear(); + // More distinct codes than the capacity: all still format correctly... + let n = parse_cache::CAPACITY + 10; + for i in 0..n { + let c = format!("0.00\"{i}\""); + assert_eq!(text_function(&Value::Number(1.0), &c, S), Ok(format!("1.00{i}"))); + } + // ...the first ones were evicted (parsed again), the newest is still cached. + let before = parse_cache::parses(); + let newest = format!("0.00\"{}\"", n - 1); + assert!(text_function(&Value::Number(1.0), &newest, S).is_ok()); + assert_eq!(parse_cache::parses() - before, 0); + assert!(text_function(&Value::Number(1.0), "0.00\"0\"", S).is_ok()); + assert_eq!(parse_cache::parses() - before, 1); + + // A very long code is parsed on every call and not stored. + let long = format!("0\"{}\"", "x".repeat(parse_cache::MAX_CODE_LEN)); + let before = parse_cache::parses(); + for _ in 0..3 { + assert!(text_function(&Value::Number(1.0), &long, S).is_ok()); + } + assert_eq!(parse_cache::parses() - before, 3); +}