Engine: renaming macros with clashing names on open takes near-linear work

Opening a workbook gave each clashing macro name a suffix by trying " (2)", " (3)"... in turn,
and each try scanned every macro accepted so far. A file with 1,000 macros of the same
255-character name made about 300 million name comparisons.

Load now keeps a set of the names in use (ASCII case-insensitive, as find compares them) and,
for each shortened name and suffix width, the next suffix to try, so each suffix is looked up
about once. The result is unchanged: the smallest free suffix, within the name length limit,
and the renamed macro still passes the final checks. unique_name keeps its signature. A test
opens, saves and reopens 1,000 same-name macros in JSON and xlsx and keeps them all, and
another counts lookups instead of timing them.

crates/engine/src/macros/mod.rs

@@ -7,7 +7,7 @@
 //! the open document. Scripts made with Automate › New Script or Record Actions are a separate,
 //! older feature with their own storage; macros neither read nor change them.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap, HashSet};
 
 use gridcraft_model::{DefinedName, Macro, Workbook};
 use serde_json::{Value as Json, json};
@@ -142,6 +142,7 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
     // All of them go through the same limits, and a clashing name gets a suffix.
     let mut all = std::mem::take(&mut wb.macros);
     all.extend(found);
+    let mut taken = Taken::default();
     for m in all {
         if wb.macros.len() >= gridcraft_model::MAX_MACROS {
             bad += 1;
@@ -149,12 +150,15 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
         }
         let mut m = m;
         let checked = m.check_limits().and_then(|()| {
-            m.name = unique_name(wb, m.name.trim());
+            m.name = taken.unique(m.name.trim());
             // The macro as it is kept, renamed, must still pass, or the next save would lose it.
             m.check_limits().and_then(|()| check_size(&m))
         });
         match checked {
-            Ok(()) => wb.macros.push(m),
+            Ok(()) => {
+                taken.insert(&m.name);
+                wb.macros.push(m);
+            }
             Err(_) => bad += 1,
         }
     }
@@ -181,19 +185,66 @@ fn macro_item(n: &DefinedName) -> Option<(String, Option<u64>)> {
 /// `name`, or `name (2)`, `name (3)`… if a macro already has it. A long name is shortened to make
 /// room for the suffix, so the result stays within [`gridcraft_model::MAX_MACRO_NAME`] characters.
 pub fn unique_name(wb: &Workbook, name: &str) -> String {
-    let name = if name.is_empty() { "Macro" } else { name };
-    if find(wb, name).is_none() {
-        return name.to_string();
+    let mut taken = Taken::default();
+    for m in &wb.macros {
+        taken.insert(&m.name);
     }
-    (2..)
-        .map(|n| {
+    taken.unique(name)
+}
+
+/// Macro names in use, compared as [`find`] compares them, and the next suffix to try for each
+/// shortened name and suffix width, so naming many clashing macros takes near-linear work.
+#[derive(Default)]
+struct Taken {
+    names: HashSet<String>,
+    next: HashMap<(String, usize), u64>,
+    /// Name lookups made, for tests.
+    probes: usize,
+}
+
+impl Taken {
+    fn key(name: &str) -> String {
+        name.trim().to_ascii_lowercase()
+    }
+
+    fn insert(&mut self, name: &str) {
+        self.names.insert(Taken::key(name));
+    }
+
+    fn contains(&mut self, name: &str) -> bool {
+        self.probes = self.probes.saturating_add(1);
+        self.names.contains(&Taken::key(name))
+    }
+
+    /// The first of `name`, `name (2)`, `name (3)`… not in use.
+    fn unique(&mut self, name: &str) -> String {
+        let name = if name.is_empty() { "Macro" } else { name };
+        if !self.contains(name) {
+            return name.to_string();
+        }
+        let mut n: u64 = 2;
+        loop {
             let suffix = format!(" ({n})");
             let keep = gridcraft_model::MAX_MACRO_NAME.saturating_sub(suffix.chars().count());
             let base: String = name.chars().take(keep).collect();
-            format!("{}{suffix}", base.trim_end())
-        })
-        .find(|n| find(wb, n).is_none())
-        .unwrap_or_else(|| name.to_string())
+            let base = base.trim_end();
+            // Every candidate for this base and suffix width below `next` is in use already
+            // (names are only added), so the search goes on from there.
+            let key = (Taken::key(base), suffix.len());
+            let next = self.next.get(&key).copied().unwrap_or(n);
+            if next > n {
+                n = next;
+                continue;
+            }
+            let Some(after) = n.checked_add(1) else { return name.to_string() };
+            self.next.insert(key, after);
+            let candidate = format!("{base}{suffix}");
+            if !self.contains(&candidate) {
+                return candidate;
+            }
+            n = after;
+        }
+    }
 }
 
 #[cfg(test)]

crates/engine/src/macros/tests.rs

@@ -317,3 +317,53 @@ fn a_macro_in_a_newer_format_is_dropped_not_rewritten() {
     assert!(back.names.is_empty(), "{:?}", names_of(&back));
     check("book.xlsx", &back, &warnings);
 }
+
+#[test]
+fn many_macros_with_one_name_all_survive_save_and_reopen() {
+    // MAX_MACROS macros with the same name, at the length limit, so the suffixes run from " (2)"
+    // to " (1000)" and the kept part of the name shrinks as they widen.
+    let long = "é".repeat(MAX_MACRO_NAME);
+    for file in ["same.json", "same.xlsx"] {
+        let mut wb = Workbook::new();
+        wb.macros = (0..MAX_MACROS).map(|i| Macro { name: long.clone(), description: format!("{i}"), ..Macro::default() }).collect();
+        let (first, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&wb, file).unwrap()).unwrap();
+        assert!(warnings.is_empty(), "{file}: {warnings:?}");
+        assert_eq!(first.macros.len(), MAX_MACROS, "{file}");
+        let names: std::collections::HashSet<String> = first.macros.iter().map(|m| m.name.to_lowercase()).collect();
+        assert_eq!(names.len(), MAX_MACROS, "{file}: every name is different");
+        assert!(first.macros.iter().all(|m| m.name.chars().count() <= MAX_MACRO_NAME && m.check_limits().is_ok()), "{file}");
+        assert_eq!(first.macros[0].name, long);
+        assert_eq!(first.macros[9].name, format!("{} (10)", "é".repeat(MAX_MACRO_NAME - 5)));
+        assert_eq!(first.macros[MAX_MACROS - 1].name, format!("{} (1000)", "é".repeat(MAX_MACRO_NAME - 7)));
+        assert!(first.macros.iter().enumerate().all(|(i, m)| m.description == format!("{i}")), "{file}: order kept");
+        let (again, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&first, file).unwrap()).unwrap();
+        assert!(warnings.is_empty(), "{file}: {warnings:?}");
+        assert_eq!(again.macros, first.macros, "{file}");
+    }
+}
+
+#[test]
+fn naming_clashing_macros_takes_linear_work() {
+    // Each suffix is looked up about once, not once per earlier macro with the same name.
+    let mut taken = Taken::default();
+    // Names a file may already use, in the way of the first suffixes.
+    for n in 2..=500 {
+        taken.insert(&format!("Same ({n})"));
+    }
+    let mut out = Vec::new();
+    for _ in 0..MAX_MACROS {
+        let name = taken.unique("SAME");
+        taken.insert(&name);
+        out.push(name);
+    }
+    assert_eq!(out[..3], ["SAME", "SAME (501)", "SAME (502)"]);
+    assert!(taken.probes <= 3 * MAX_MACROS, "{} lookups", taken.probes);
+    // The smallest free suffix is still the one used.
+    let mut taken = Taken::default();
+    for name in ["A", "A (3)"] {
+        taken.insert(name);
+    }
+    assert_eq!(taken.unique("a"), "a (2)");
+    taken.insert("a (2)");
+    assert_eq!(taken.unique("a"), "a (4)");
+}