address review: key extract cache by exact block, count live MOCORO objects, PDF link budget

Every SORTENTSTABLE rebuilt its whole entity store, so many small tables naming one large space made import quadratic. Each space is now sorted once, by the last table naming it, which already decided its order.

Co-authored-by: Brandon Thomas <echelon@gmail.com>

apps/cadcraft/src/control_server.rs

@@ -250,17 +250,18 @@ fn parse_line(bytes: &[u8]) -> Line {
         Ok(v) => v,
         Err(e) => return Line::Reject(format!("bad JSON ({e}); expected one JSON request object per line; closing the connection")),
     };
-    let Some(obj) = msg.as_object() else {
+    let Value::Object(mut obj) = msg else {
         return Line::Reject("not a control request (expected a JSON object); closing the connection".into());
     };
-    let Some(method) = obj.get("method").and_then(Value::as_str) else {
+    let Some(Value::String(method)) = obj.remove("method") else {
         return Line::Reject("not a control request (missing string `method`); closing the connection".into());
     };
+    // Moved out of the parsed object, not cloned: `params` can be most of the line.
     Line::Request {
-        id: obj.get("id").cloned().unwrap_or(Value::Null),
-        method: method.to_string(),
-        params: obj.get("params").cloned().unwrap_or(json!({})),
-        token: obj.get("token").and_then(Value::as_str).map(str::to_string),
+        id: obj.remove("id").unwrap_or(Value::Null),
+        method,
+        params: obj.remove("params").unwrap_or(json!({})),
+        token: obj.remove("token").and_then(|t| if let Value::String(t) = t { Some(t) } else { None }),
     }
 }
 

crates/engine/src/cmd/array.rs

@@ -177,8 +177,16 @@ fn create(s: &mut Session, objs: &[Handle], base: Vec2, p: ArrayParams, assoc: b
     Ok(json!({ "created": n }))
 }
 
+/// The most objects EXPLODE of one associative array makes from its source objects (ten times the
+/// default MAXARRAY).
+const MAX_EXPLODED_OBJECTS: usize = 1_000_000;
+
 /// EXPLODE of an associative array: every item becomes the objects of its block (the source
 /// objects, or a replacement block's reference), in world coordinates. `None` for other objects.
+///
+/// An array that would explode into more than [`MAX_EXPLODED_OBJECTS`] objects (items times the
+/// objects of the source block, which a file sets freely) is not expanded here: `None`, so EXPLODE
+/// takes the array block's own items.
 pub(crate) fn explode_array(d: &Drawing, e: &Entity) -> Option<Vec<Entity>> {
     let EntityKind::Insert(ins) = &e.kind else { return None };
     let p = d.array_params(e)?;
@@ -192,7 +200,12 @@ pub(crate) fn explode_array(d: &Drawing, e: &Entity) -> Option<Vec<Entity>> {
             _ => None,
         };
         match source {
-            Some((im, b)) => out.extend(b.entities.iter().map(|se| placed(&item, se, &im))),
+            Some((im, b)) => {
+                if out.len().saturating_add(b.entities.len()) > MAX_EXPLODED_OBJECTS {
+                    return None;
+                }
+                out.extend(b.entities.iter().map(|se| placed(&item, se, &im)));
+            }
             None => out.push(item),
         }
     }

crates/engine/src/cmd/blocks/extract.rs

@@ -2,7 +2,7 @@
 //! block name and values, or one row per reference with its handle, layer, position and
 //! rotation — returned, written to a CSV file and/or inserted as a table. Tags match in any case.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 
 use cadcraft_doc::{EntityKind, Insert};
 use cadcraft_geom::Vec2;
@@ -88,17 +88,34 @@ fn matches(spec: &str, text: &str) -> bool {
     spec.split(',').any(|p| wildcard(p.trim(), text))
 }
 
-/// The attribute values of `ins`: its own, then the constant ones of its block.
-fn values(d: &cadcraft_doc::Drawing, ins: &Insert) -> Vec<(String, String)> {
-    let mut v: Vec<(String, String)> = ins.attribs.iter().map(|a| (a.tag.to_ascii_uppercase(), a.text.value.clone())).collect();
-    if let Some(b) = d.block(&ins.block) {
-        for e in b.entities.iter() {
-            if let EntityKind::AttDef(a) = &e.kind
-                && a.constant
-                && !v.iter().any(|(t, _)| t.eq_ignore_ascii_case(&a.tag))
-            {
-                v.push((a.tag.to_ascii_uppercase(), a.text.value.clone()));
-            }
+/// The constant attribute values of a block definition whose tags `tags` selects.
+fn constants(b: &cadcraft_doc::Block, tags: &str) -> Vec<(String, String)> {
+    let mut v: Vec<(String, String)> = Vec::new();
+    for e in b.entities.iter() {
+        if let EntityKind::AttDef(a) = &e.kind
+            && a.constant
+            && matches(tags, &a.tag.to_ascii_uppercase())
+            && !v.iter().any(|(t, _)| t.eq_ignore_ascii_case(&a.tag))
+        {
+            v.push((a.tag.to_ascii_uppercase(), a.text.value.clone()));
+        }
+    }
+    v
+}
+
+/// The attribute values of `ins` that `o.tags` selects: its own, then the constant ones of its
+/// block (`consts`, read once per block). Values no column shows are not copied.
+fn values(ins: &Insert, consts: &[(String, String)], tags: &str) -> Vec<(String, String)> {
+    let mut v: Vec<(String, String)> = ins
+        .attribs
+        .iter()
+        .map(|a| (a.tag.to_ascii_uppercase(), a))
+        .filter(|(t, _)| matches(tags, t))
+        .map(|(t, a)| (t, a.text.value.clone()))
+        .collect();
+    for (t, val) in consts {
+        if !v.iter().any(|(k, _)| k.eq_ignore_ascii_case(t)) {
+            v.push((t.clone(), val.clone()));
         }
     }
     v
@@ -113,6 +130,7 @@ fn found(s: &Session, p: &Value, o: &Options) -> Result<Vec<Found>> {
         d.model.iter().chain(d.layouts.iter().flat_map(|l| l.entities.iter())).cloned().collect()
     };
     let mut out = Vec::new();
+    let mut consts: HashMap<String, Vec<(String, String)>> = HashMap::new();
     for e in &ents {
         let EntityKind::Insert(ins) = &e.kind else { continue };
         let anonymous = ins.block.starts_with('*') || d.block(&ins.block).is_some_and(|b| b.anonymous);
@@ -121,12 +139,17 @@ fn found(s: &Session, p: &Value, o: &Options) -> Result<Vec<Found>> {
         }
         let block = d.block(&ins.block).map(|b| b.name.clone()).unwrap_or_else(|| ins.block.clone());
         out.push(Found {
-            block,
             handle: e.handle.hex(),
             layer: e.common.layer.clone(),
             at: ins.insert.xy(),
             rotation: ins.rotation.to_degrees(),
-            values: values(d, ins),
+            block: block.clone(),
+            values: values(
+                ins,
+                // Keyed by the resolved definition's exact name: `Part` and `PART` can both exist.
+                consts.entry(block.clone()).or_insert_with(|| d.block(&ins.block).map(|b| constants(b, &o.tags)).unwrap_or_default()),
+                &o.tags,
+            ),
         });
     }
     Ok(out)

crates/engine/src/cmd/file/compare.rs

@@ -8,7 +8,7 @@ use std::collections::HashMap;
 use std::hash::{Hash, Hasher};
 use std::sync::Arc;
 
-use cadcraft_doc::{Drawing, Entity, EntityKind, Handle, MAX_BLOCK_DEPTH, Space, entity_bounds};
+use cadcraft_doc::{BlockBoundsCache, Drawing, Entity, EntityKind, Handle, MAX_BLOCK_DEPTH, Space, entity_bounds_cached};
 use cadcraft_geom::{Bounds2, Vec2};
 use serde_json::{Value, json};
 
@@ -375,16 +375,20 @@ fn run_compare(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let Diff { added, removed, changed, common, truncated } = diff(&cur, &other, &space);
     let mut boxes = Vec::new();
-    let mut bound = |d: &Drawing, h: Handle| {
-        if let Some(b) = d.entity(h).map(|e| entity_bounds(d, e, 0)).filter(|b| !b.is_empty() && b.min.is_finite() && b.max.is_finite()) {
+    // One block-bounds cache for each drawing, so a block many differing entities refer to is
+    // measured once, not once per entity.
+    let (mut cur_cache, mut other_cache) = (BlockBoundsCache::default(), BlockBoundsCache::default());
+    fn bound<'a>(boxes: &mut Vec<Bounds2>, d: &'a Drawing, h: Handle, cache: &mut BlockBoundsCache<'a>) {
+        if let Some(b) = d.entity(h).map(|e| entity_bounds_cached(d, e, 0, cache)).filter(|b| !b.is_empty() && b.min.is_finite() && b.max.is_finite())
+        {
             boxes.push(b);
         }
-    };
-    added.iter().for_each(|h| bound(&cur, *h));
-    removed.iter().for_each(|h| bound(&other, *h));
+    }
+    added.iter().for_each(|h| bound(&mut boxes, &cur, *h, &mut cur_cache));
+    removed.iter().for_each(|h| bound(&mut boxes, &other, *h, &mut other_cache));
     for (a, b, _, _) in &changed {
-        bound(&cur, *a);
-        bound(&other, *b);
+        bound(&mut boxes, &cur, *a, &mut cur_cache);
+        bound(&mut boxes, &other, *b, &mut other_cache);
     }
     let regions = regions(&boxes);
     let ch: Vec<Value> = changed

crates/engine/src/cmd/modify2/mocoro.rs

@@ -108,6 +108,12 @@ pub(super) fn run_mocoro(s: &mut Session, p: &Value) -> Result<Value> {
     }
     // Every option is checked before the drawing changes.
     let ops = list.iter().enumerate().map(|(i, v)| parse_op(i, v)).collect::<Result<Vec<_>>>()?;
+    // Each Copy clones every selected object: the copies together stay within MAXARRAY, as an array's do.
+    // Erased handles are skipped by `apply`, so only live objects count (none: nothing is copied).
+    let live = counts(s, &objs)?.0;
+    if live > 0 {
+        crate::cmd::array::check_size(s, ops.iter().filter(|o| matches!(o, Op::Copy(_))).count() as u64, live)?;
+    }
     let mut copies = Vec::new();
     for op in ops {
         copies.extend(apply(s, &objs, &mut base, op)?);

crates/engine/tests/it/mocoro.rs

@@ -233,3 +233,17 @@ fn options_on_objects_that_are_gone_record_nothing() {
     assert!(lines(&s).is_empty());
     assert_eq!(undo_len(&s), undo, "no empty undo step");
 }
+
+#[test]
+fn json_form_copies_stay_within_maxarray() {
+    let mut s = Session::new();
+    let l = line(&mut s, [0.0, 0.0], [10.0, 0.0]);
+    // 5,000 Copy options of 30 objects would be 150,000 copies, over the default MAXARRAY of 100,000.
+    let others: Vec<String> = (0..29).map(|i| line(&mut s, [0.0, f64::from(i + 1)], [10.0, f64::from(i + 1)])).collect();
+    let mut hs = vec![l];
+    hs.extend(others);
+    let ops: Vec<Value> = (0..5_000).map(|i| json!({"copy": [0, i]})).collect();
+    let e = s.execute("mocoro", &json!({"handles": hs, "base": [0, 0], "ops": ops})).unwrap_err();
+    assert!(e.to_string().contains("MAXARRAY"), "{e}");
+    assert_eq!(lines(&s).len(), 30, "nothing was copied");
+}

crates/io/src/dxf_read/document.rs

@@ -333,7 +333,17 @@ pub(super) fn apply_sortents(
     paper_for: &HashMap<String, String>,
     first_layout: &str,
 ) {
-    for (owner, map) in tables {
+    // Each sort rebuilds its whole space, so a space named by many tables is sorted once, by the
+    // last of them (which decided its order anyway); repeated tables made import quadratic.
+    let target = |owner: &String| {
+        let up = brs.get(owner)?.to_ascii_uppercase();
+        Some(if up.starts_with("*PAPER_SPACE") { paper_for.get(&up).cloned().unwrap_or_else(|| first_layout.to_string()) + "\0paper" } else { up })
+    };
+    let last: HashMap<String, usize> = tables.iter().enumerate().filter_map(|(i, (owner, _))| Some((target(owner)?, i))).collect();
+    for (i, (owner, map)) in tables.iter().enumerate() {
+        if target(owner).and_then(|t| last.get(&t)) != Some(&i) {
+            continue;
+        }
         let Some(name) = brs.get(owner) else { continue };
         let up = name.to_ascii_uppercase();
         // A sort handle of 0 draws last (ezdxf `reorder`, which notes AutoCAD defines it, undocumented).

crates/io/src/pdf.rs

@@ -50,6 +50,8 @@ const MAX_LAYER_NAME: usize = 1000;
 /// Most link annotations in one file, and the longest link target (bytes).
 const MAX_LINKS: usize = 10_000;
 const MAX_URL: usize = 2048;
+/// The most link annotations in one PDF, over all pages (a page holds at most [`MAX_LINKS`]).
+const MAX_FILE_ANNOTATIONS: usize = 100_000;
 /// Longest title and page label written (characters).
 const MAX_TEXT: usize = 1000;
 
@@ -357,7 +359,10 @@ fn sheet_name(space: &Space) -> String {
 /// Write a vector PDF with one page per space (at most [`MAX_PAGES`]), each labelled with its
 /// layout name. The options apply to every sheet; layouts use their own page setups.
 pub fn pdf_sheets(d: &Drawing, spaces: &[Space], o: &PdfOptions) -> Result<Vec<u8>> {
-    let items: Vec<(Space, PdfOptions)> = spaces.iter().take(MAX_PAGES + 1).map(|s| (s.clone(), o.clone())).collect();
+    // Links are file-wide (`pdf_items` reads them from `o`), so a sheet's own options carry none:
+    // a copy of the list per sheet multiplied it by the sheet count.
+    let sheet = PdfOptions { links: Vec::new(), ..o.clone() };
+    let items: Vec<(Space, PdfOptions)> = spaces.iter().take(MAX_PAGES + 1).map(|s| (s.clone(), sheet.clone())).collect();
     pdf_items(d, &items, o)
 }
 
@@ -453,8 +458,16 @@ fn write_pdf(d: &Drawing, items: &[(Space, PdfOptions)], o: &PdfOptions, text_as
         let mut writer = Writer::new(settings, metadata);
         let mut fonts = fonts::Fonts::new(!text_as_geometry);
         let mut groups = Groups::default();
+        // Link annotations of the whole file: each page keeps its own copy of the targets, so the
+        // pages times the links must not multiply without a bound.
+        let mut annotations_left = MAX_FILE_ANNOTATIONS;
         for (s, po) in items {
-            let page = sheet(drawn, s, po, &links)?;
+            // Once the budget is spent no link is built (scanned and cloned) for later pages.
+            let mut page = sheet(drawn, s, po, if annotations_left == 0 { &links[..0] } else { &links })?;
+            // Only targets `write_page` accepts count against the budget.
+            page.links.retain(|(_, url)| uri(url).is_some());
+            page.links.truncate(annotations_left);
+            annotations_left -= page.links.len();
             let doc = writer.document_mut();
             // Groups are added to the document, so they must exist before the page starts.
             groups.add(doc, d, &page.layers);
@@ -625,7 +638,8 @@ pub(crate) fn placement(space: &Space, bounds: &Bounds2, fit: bool, sheet: &Shee
 /// The hyperlinks to plot: the options' own, then (with [`PdfOptions::hyperlinks`]) those the
 /// drawing file gave its entities as `PE_URL` extended data, at most [`MAX_LINKS`] in all.
 fn all_links(d: &Drawing, o: &PdfOptions) -> Vec<(Handle, String)> {
-    let mut out: Vec<(Handle, String)> = o.links.iter().take(MAX_LINKS).cloned().collect();
+    // A target past MAX_URL is refused at the annotation anyway; it is left out before it is copied.
+    let mut out: Vec<(Handle, String)> = o.links.iter().filter(|(_, u)| u.len() <= MAX_URL).take(MAX_LINKS).cloned().collect();
     if !o.hyperlinks {
         return out;
     }

crates/io/src/tests.rs

@@ -337,6 +337,28 @@ fn entity_transparency_roundtrips_as_group_440() {
     assert_eq!(d.model.iter().map(|e| e.common.transparency).collect::<Vec<_>>(), [Transparency::ByLayer, Transparency::Percent(90)]);
 }
 
+/// Several SORTENTSTABLEs for one space sort it once, in the last table's order.
+#[test]
+fn repeated_sortents_tables_use_the_last() {
+    let table =
+        |a: u32, b: u32| format!("0\nSORTENTSTABLE\n5\n{:X}\n100\nAcDbSortentsTable\n330\n1F\n331\nA\n5\n{a:X}\n331\nB\n5\n{b:X}\n", 0x100 + a);
+    let mut t = String::from(
+        "0\nSECTION\n2\nTABLES\n0\nTABLE\n2\nBLOCK_RECORD\n0\nBLOCK_RECORD\n5\n1F\n2\n*Model_Space\n0\nENDTAB\n0\nENDSEC\n\
+         0\nSECTION\n2\nENTITIES\n0\nLINE\n5\nA\n330\n1F\n10\n0\n20\n0\n11\n1\n21\n1\n0\nLINE\n5\nB\n330\n1F\n10\n0\n20\n0\n11\n2\n21\n2\n0\nENDSEC\n\
+         0\nSECTION\n2\nOBJECTS\n0\nDICTIONARY\n5\nC\n",
+    );
+    for _ in 0..1000 {
+        t.push_str(&table(2, 1));
+    }
+    t.push_str(&table(1, 2));
+    t.push_str("0\nENDSEC\n0\nEOF\n");
+    let d = read(t.as_bytes(), "s.dxf").unwrap();
+    assert_eq!(d.model.iter().map(|e| e.handle.0).collect::<Vec<_>>(), [0xA, 0xB]);
+    let reversed = t.replacen(&table(1, 2), &table(2, 1), 1);
+    let d = read(reversed.as_bytes(), "s.dxf").unwrap();
+    assert_eq!(d.model.iter().map(|e| e.handle.0).collect::<Vec<_>>(), [0xB, 0xA]);
+}
+
 /// A table declaring 10 000 rows and no columns (or the reverse) is read as an empty table.
 #[test]
 fn table_with_an_empty_dimension_allocates_nothing() {

crates/ui-egui/src/managers.rs

@@ -306,7 +306,7 @@ fn point_style(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Action
 enum Field {
     Num(&'static str, &'static str, f64),
     Bool(&'static str, &'static str, bool),
-    Choice(&'static str, &'static str, String, Vec<String>),
+    Choice(&'static str, &'static str, String, std::sync::Arc<[String]>),
 }
 
 struct Style {
@@ -371,7 +371,8 @@ fn mleader_style(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Acti
         no_drawing(ctx, "Multileader Style", open);
         return None;
     };
-    let text_styles: Vec<String> = d.text_styles.iter().map(|s| s.name.clone()).collect();
+    // One list shared by every row: a copy per style multiplied the two counts.
+    let text_styles: std::sync::Arc<[String]> = d.text_styles.iter().map(|s| s.name.clone()).collect();
     let mut styles: Vec<Style> = d
         .mleader_styles
         .iter()
@@ -382,7 +383,7 @@ fn mleader_style(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Acti
                 Field::Num("textHeight", "Text height", s.text_height),
                 Field::Num("landingGap", "Landing gap", s.landing_gap),
                 Field::Num("dogleg", "Landing length", s.dogleg),
-                Field::Choice("textStyle", "Text style", s.text_style.clone(), text_styles.clone()),
+                Field::Choice("textStyle", "Text style", s.text_style.clone(), std::sync::Arc::clone(&text_styles)),
             ],
         })
         .collect();
@@ -451,7 +452,7 @@ fn style_manager(ctx: &egui::Context, open: &mut bool, title: &str, cmd: &'stati
                             ui.label(*label);
                             let mut picked = None;
                             craft_ui::ComboBox::from_id_salt((cmd, *key)).selected_text(v.as_str()).show_ui(ui, |ui| {
-                                for c in choices {
+                                for c in choices.iter() {
                                     if ui.selectable_label(c == v, c).clicked() && c != v {
                                         picked = Some(c.clone());
                                     }