collective
Browse / github.com/storytold/cadcraft / patch
Avoid repeated alias scans during PGP imports
Avoid repeated alias scans during PGP imports Addresses the PGP import performance finding in #1933. PGP imports currently scan every retained alias for each accepted definition to implement last-definition-wins. Accumulate definitions directly in the existing `BTreeMap` and publish the completed map, avoiding those repeated scans. This preserves normalization, last-valid-definition-wins, validation and warning order, loaded counts, remembered-file behavior, and the existing capacity cutoff: at 10,000 aliases, the next valid definition still stops the import even if it is a duplicate. The separate JSON `set` behavior is unchanged. Compa
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- WillPastor (imported from pull request #2270, open)
- Base
d864a08b013353143086bd8bb568c26cb6256796- Size
- +143 −12 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
248fe42b55f27a48a64b025b
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/file/pgp.rs crates/engine/tests/aliases.rs docs/file-format-parity.md
@@ -63,8 +63,8 @@ pub fn alias_name(a: &str) -> std::result::Result<String, String> { } /// The aliases of a `.pgp` file's text, and its problems.-fn parse(text: &str) -> (Vec<(String, String)>, Vec<String>) {- let mut out: Vec<(String, String)> = Vec::new();+fn parse(text: &str) -> (BTreeMap<String, String>, Vec<String>) {+ let mut out = BTreeMap::new(); let mut warnings = Vec::new(); let mut warn = |no: usize, msg: String| { if warnings.len() < MAX_WARNINGS {@@ -100,13 +100,13 @@ fn parse(text: &str) -> (Vec<(String, String)>, Vec<String>) { warn(no, format!("{}: unknown command {}; skipped", alias.trim(), target.trim())); continue; };+ // Once full, stop at the next valid definition, even if it repeats an alias. if out.len() >= MAX_ALIASES { warn(no, format!("more than {MAX_ALIASES} aliases; the rest are skipped")); break; }- // A later line for the same alias wins, as it would when typed into the file twice.- out.retain(|(a, _)| *a != name);- out.push((name, id));+ // A later definition wins without scanning all the aliases accumulated so far.+ out.insert(name, id); } (out, warnings) }@@ -120,7 +120,7 @@ fn listing(warnings: &[String], added: usize) -> Value { fn load(text: &str, path: Option<String>) -> (usize, Vec<String>) { let (aliases, warnings) = parse(text); let mut st = store();- st.map = aliases.into_iter().collect();+ st.map = aliases; if path.is_some() { st.file = path; }@@ -5,6 +5,7 @@ use std::sync::Mutex; use cadcraft_engine::Session;+use cadcraft_engine::cmd::file::MAX_ALIASES; use cadcraft_engine::cmd::resolve_alias; use serde_json::json; @@ -21,6 +22,10 @@ ZZBAD, *NOSUCHCOMMAND LINE, *CIRCLE "; +fn alias_prefix(count: usize) -> String {+ (0..count).map(|i| format!("ZZP{i:05},*LINE\n")).collect()+}+ #[test] fn an_alias_file_adds_and_overrides_aliases() { let _one = ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner());@@ -58,17 +63,142 @@ fn reinit_reads_the_alias_file_again() { let dir = std::env::temp_dir().join(format!("cadcraft-aliases-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let file = dir.join("mine.pgp").to_string_lossy().into_owned();- std::fs::write(&file, "ZZR, *RECTANG\n").unwrap();+ std::fs::write(&file, "ZZR, *LINE\nzzr, *RECTANG\n").unwrap(); let mut s = Session::new();- s.execute("aliasedit", &json!({ "file": file })).unwrap();+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+ let r = s.execute("aliasedit", &json!({ "file": file })).unwrap();+ assert_eq!(r["loaded"], json!(1));+ assert_eq!(r["file"], json!(file));+ assert_eq!(r["warnings"], json!([])); assert_eq!(resolve_alias("zzr"), "rectang"); - std::fs::write(&file, "ZZR, *CIRCLE\n").unwrap();+ std::fs::write(&file, "ZZR, *MOVE\nzzr, *CIRCLE\n").unwrap(); s.cmdline("reinit").unwrap(); assert_eq!(resolve_alias("zzr"), "circle"); assert!(s.log.iter().any(|l| l.contains("1 command alias")), "{:?}", s.log);- let r = s.execute("reinit", &json!({ "text": "ZZS, *SCALE\n" })).unwrap();+ let r = s.execute("aliasedit", &json!({})).unwrap();+ assert_eq!(r["file"], json!(file));+ assert_eq!(r["aliases"], json!({ "zzr": "circle" }));+ let r = s.execute("reinit", &json!({ "text": "ZZS, *LINE\nzzs, *SCALE\n" })).unwrap(); assert_eq!(r["aliases"], json!({ "zzs": "scale" }));+ assert_eq!(r["loaded"], json!(1));+ assert_eq!(r["warnings"], json!([]));+ assert_eq!(r["file"], json!(file), "reloading text keeps the remembered file");+ let r = s.execute("reinit", &json!({})).unwrap();+ assert_eq!(r["aliases"], json!({ "zzr": "circle" }));+ assert_eq!(r["loaded"], json!(1));+ assert_eq!(r["file"], json!(file));+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+}++#[test]+fn valid_redefinitions_win_and_the_listing_stays_sorted() {+ let _one = ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner());+ let mut s = Session::new();+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+ let text = "ZZZ,*LINE\nZza,*LINE\nzzz,*CIRCLE\nZZA,*NO_SUCH_1933\nLINE,*CIRCLE\n";+ let r = s.execute("aliasedit", &json!({ "text": text })).unwrap();+ let aliases = json!({ "zza": "line", "zzz": "circle" });+ assert_eq!(r["aliases"], aliases);+ assert_eq!(r["loaded"], json!(2), "loaded counts unique aliases, not definitions");+ assert_eq!(r["file"], json!(null));+ assert_eq!(+ r["warnings"],+ json!(["line 4: ZZA: unknown command *NO_SUCH_1933; skipped", "line 5: LINE is a command name; it stays the command; skipped"])+ );+ assert_eq!(resolve_alias("ZZZ"), "circle");+ assert_eq!(resolve_alias("zza"), "line", "an invalid redefinition keeps the previous target");+ assert_eq!(resolve_alias("line"), "line");+ assert_eq!(serde_json::to_string(&r["aliases"]).unwrap(), r#"{"zza":"line","zzz":"circle"}"#);+ let listing = s.execute("aliasedit", &json!({})).unwrap();+ assert_eq!(listing, json!({ "aliases": aliases, "file": null, "loaded": 0, "warnings": [] }));++ let empty = s.execute("aliasedit", &json!({ "text": "" })).unwrap();+ assert_eq!(empty, json!({ "aliases": {}, "file": null, "loaded": 0, "warnings": [] }));+ assert_eq!(resolve_alias("zza"), "zza", "an empty import replaces the old set");+ assert_eq!(resolve_alias("zzz"), "zzz");+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+}++#[test]+fn duplicates_below_the_limit_leave_room_for_another_alias() {+ let _one = ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner());+ let mut s = Session::new();+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+ let mut text = alias_prefix(MAX_ALIASES - 1);+ for _ in 0..16 {+ text.push_str("ZZP00000,*MOVE\nzzp00001,*CIRCLE\n");+ }+ let r = s.execute("aliasedit", &json!({ "text": text })).unwrap();+ assert_eq!(r["loaded"], json!(MAX_ALIASES - 1));+ assert_eq!(r["aliases"].as_object().unwrap().len(), MAX_ALIASES - 1);+ assert_eq!(r["warnings"], json!([]));+ for i in 0..MAX_ALIASES - 1 {+ let expected = match i {+ 0 => "move",+ 1 => "circle",+ _ => "line",+ };+ assert_eq!(r["aliases"][format!("zzp{i:05}")], json!(expected));+ }++ text.push_str("ZZNEW,*PLINE\n");+ let r = s.execute("aliasedit", &json!({ "text": text })).unwrap();+ assert_eq!(r["loaded"], json!(MAX_ALIASES));+ assert_eq!(r["aliases"].as_object().unwrap().len(), MAX_ALIASES);+ assert_eq!(r["aliases"]["zznew"], json!("pline"));+ assert_eq!(r["aliases"]["zzp00000"], json!("move"));+ assert_eq!(r["aliases"]["zzp00001"], json!("circle"));+ assert_eq!(r["warnings"], json!([]), "duplicate definitions do not use up the alias limit");+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+}++#[test]+fn a_full_file_validates_lines_before_stopping_at_the_next_valid_definition() {+ let _one = ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner());+ let mut s = Session::new();+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+ let mut text = alias_prefix(MAX_ALIASES);+ text.push_str("; comment\nZZP00000,*NO_SUCH_1933\nLINE,*CIRCLE\nZZEXTERNAL, START NOTHING\nmalformed\nZZP00000,*MOVE\nZZAFTER,*CIRCLE\nanother malformed sentinel\n");+ let r = s.execute("aliasedit", &json!({ "text": text })).unwrap();+ assert_eq!(r["loaded"], json!(MAX_ALIASES));+ assert_eq!(r["aliases"].as_object().unwrap().len(), MAX_ALIASES);+ assert_eq!(r["aliases"]["zzp00000"], json!("line"), "the cap stops even a valid duplicate replacement");+ assert!(r["aliases"].get("zzafter").is_none(), "the remainder of the file is skipped");+ assert_eq!(+ r["warnings"],+ json!([+ format!("line {}: ZZP00000: unknown command *NO_SUCH_1933; skipped", MAX_ALIASES + 2),+ format!("line {}: LINE is a command name; it stays the command; skipped", MAX_ALIASES + 3),+ format!("line {}: ZZEXTERNAL is an external command; CADCraft doesn't run programs from alias files", MAX_ALIASES + 4),+ format!("line {}: `malformed` is not ALIAS, *COMMAND; skipped", MAX_ALIASES + 5),+ format!("line {}: more than {MAX_ALIASES} aliases; the rest are skipped", MAX_ALIASES + 6)+ ])+ );++ // Setting one existing alias through JSON has a different rule from importing a full file.+ let r = s.execute("aliasedit", &json!({ "set": { "ZZP00000": "move" } })).unwrap();+ assert_eq!(r["aliases"]["zzp00000"], json!("move"));+ assert_eq!(r["loaded"], json!(1));+ assert!(s.execute("aliasedit", &json!({ "set": { "ZZNEW": "line" } })).is_err());+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+}++#[test]+fn the_warning_limit_does_not_stop_a_later_valid_redefinition() {+ let _one = ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner());+ let mut s = Session::new();+ s.execute("aliasedit", &json!({ "clear": true })).unwrap();+ let mut text = String::from("ZZW,*LINE\n");+ for _ in 0..101 {+ text.push_str("ZZW,*NO_SUCH_1933\n");+ }+ text.push_str("zzw,*CIRCLE\n");+ let r = s.execute("aliasedit", &json!({ "text": text })).unwrap();+ assert_eq!(r["aliases"], json!({ "zzw": "circle" }));+ assert_eq!(r["loaded"], json!(1));+ let warnings: Vec<_> = (2..=101).map(|no| format!("line {no}: ZZW: unknown command *NO_SUCH_1933; skipped")).collect();+ assert_eq!(r["warnings"], json!(warnings), "only the first 100 warnings are returned"); s.execute("aliasedit", &json!({ "clear": true })).unwrap(); } @@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-22 · **Change:** minor (DIMASSOC objects for angular, diametric and radial dimensions) · **Target:** Autodesk AutoCAD 2027+> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-26 · **Change:** minor (PGP alias import performance) · **Target:** Autodesk AutoCAD 2027 Every format AutoCAD 2027 reads or writes, with CADCraft's support, fidelity and tests. Work items are in [gaps.md](gaps.md#file-formats); the weighted score feeds@@ -121,7 +121,7 @@ on read with a note, so such files no longer exceed the DXF limit (#1482). **DWG | Linetype (`.lin`) and hatch pattern (`.pat`) files | load user files | user files load (#552); linetype shape (SHX) elements are skipped | 1–2 | | Plot styles (`.ctb`, `.stb`), plotter configs (`.pc3`), `.pmp` | yes | plot style tables in our own JSON format ([plot-styles.md](plot-styles.md)) and kept in the drawing; plotter devices with papers and printable areas in our own JSON format ([plotters.md](plotters.md), #564); binary `.ctb`/`.stb`/`.pc3`/`.pmp` not read (no public spec) | 4–8 | | Scripts (`.scr`) | SCRIPT | script text through the CLI, MCP and control channel; no SCRIPT command reading a `.scr` file | 1–2 |-| Aliases (`.pgp`), CUI(x), workspaces | yes | user alias files and ALIASEDIT (#552); no CUI(x) or workspaces | 4–8 |+| Aliases (`.pgp`), CUI(x), workspaces | yes | user alias files and ALIASEDIT (#552); duplicate definitions replace aliases without scanning the accumulated list (#1933); no CUI(x) or workspaces | 4–8 | | SVG import (`.svg`, `.svgz`) | yes (via scripts) | OPEN reads shapes, paths, groups and Inkscape layers into entities (#636); no CSS classes, `use`, fills, text or images (what the drawing loses is named in a note on open, #627) | 2–4 | | AutoLISP (`.lsp`, `.fas`, `.vlx`), `.dll`/`.arx` plug-ins | yes | `.lsp` loads (APPLOAD, `(load)`, a dropped file; #758); no Visual LISP ActiveX (`vlax-`), DCL dialogs or reactors; `.fas`/`.vlx` are compiled and undocumented; no `.dll`/`.arx` | see [gaps.md](gaps.md) (E1) | | Data extraction (`.csv`, `.xls`) and data links | yes | DATAEXTRACTION/EATTEXT to CSV or a table (#433); table data links read CSV (#534); no `.xls`, no `.dxe` templates | 3–5 |@@ -140,6 +140,7 @@ our output in AutoCAD black-box. | Date | Change | Summary | |---|---|---|+| 2026-10-26 | minor | PGP alias imports replace duplicate definitions by keyed lookup, avoiding repeated scans of the accumulated aliases; validation, warnings and alias limits are unchanged (#1933). Parity estimates are unchanged. | | 2026-10-22 | minor | DXF/DWG save: angular, diametric and radial dimensions get standard DIMASSOC objects (one centre reference to the circle for radial/diametric, the definition points for angular), so AutoCAD follows their geometry; the standard objects of those kinds are read back too, without CADCraft's xdata (#1651) | | 2026-10-19 | minor | DXF/DWG save: angular dimension block text honors DIMDSEP, including per-dimension overrides (#1646) | | 2026-10-19 | minor | A file's own arrowhead blocks with standard names (AutoCAD's `_Dot`, `_DotSmall`), named only by a dimension style, are kept and saved as they were instead of being replaced by CADCraft's solids; PURGE keeps arrowhead blocks dimension styles name |