address review: share the first inserted dictionary on a concurrent load

crates/engine/src/spell.rs

@@ -171,7 +171,9 @@ impl Dictionary {
                 ["COMPOUNDWORDMAX", n, ..] => d.compound_max = n.parse().unwrap_or(0),
                 ["REP", n] => {
                     for _ in 0..n.parse::<usize>().unwrap_or(0) {
-                        let r: Vec<&str> = lines.next().unwrap_or("").split_whitespace().collect();
+                        // The count is the file's claim: stop when the records run out.
+                        let Some(line) = lines.next() else { break };
+                        let r: Vec<&str> = line.split_whitespace().collect();
                         if let [_, a, b, ..] = r.as_slice() {
                             d.rep.push((a.replace('_', " "), b.replace('_', " ")));
                         }
@@ -182,7 +184,8 @@ impl Dictionary {
                     let Some(flag) = one_flag(Some(flag), mode) else { continue };
                     let cross = cross.eq_ignore_ascii_case("y");
                     for _ in 0..n.parse::<usize>().unwrap_or(0) {
-                        let r: Vec<&str> = lines.next().unwrap_or("").split_whitespace().collect();
+                        let Some(line) = lines.next() else { break };
+                        let r: Vec<&str> = line.split_whitespace().collect();
                         let [_, _, strip, add, rest @ ..] = r.as_slice() else { continue };
                         let (add, cont) = match add.split_once('/') {
                             Some((a, c)) => (a, parse_flags(c, mode)),
@@ -576,6 +579,15 @@ fn cache() -> &'static Cache {
     CACHE.get_or_init(Default::default)
 }
 
+/// Language strings remembered by [`dictionary`]; a string past it is resolved again on each call.
+const MAX_CACHED_LANGS: usize = 256;
+
+/// Parsed dictionaries by their affix file, so aliases of one language share it.
+fn by_files() -> &'static Mutex<HashMap<PathBuf, Arc<Dictionary>>> {
+    static M: OnceLock<Mutex<HashMap<PathBuf, Arc<Dictionary>>>> = OnceLock::new();
+    M.get_or_init(Default::default)
+}
+
 /// The dictionary for `lang` (a BCP-47 tag such as `en-US`), loaded once per process.
 pub fn dictionary(lang: &str) -> Option<Arc<Dictionary>> {
     let cache = cache();
@@ -586,10 +598,23 @@ pub fn dictionary(lang: &str) -> Option<Arc<Dictionary>> {
         return d.clone();
     }
     let loaded = find_in(&search_dirs(), lang).and_then(|(a, d)| {
-        let (a, d) = (std::fs::read(a).ok()?, std::fs::read(d).ok()?);
-        Some(Arc::new(Dictionary::parse(&a, &d)))
+        // Tags that resolve to the same files share one parsed dictionary.
+        if let Ok(m) = by_files().lock()
+            && let Some(dict) = m.get(&a)
+        {
+            return Some(dict.clone());
+        }
+        let (bytes_a, bytes_d) = (std::fs::read(&a).ok()?, std::fs::read(d).ok()?);
+        let dict = Arc::new(Dictionary::parse(&bytes_a, &bytes_d));
+        // A concurrent loader of the same files may have inserted first: share its copy.
+        if let Ok(mut m) = by_files().lock() {
+            return Some(m.entry(a).or_insert(dict).clone());
+        }
+        Some(dict)
     });
-    if let Ok(mut c) = cache.lock() {
+    if let Ok(mut c) = cache.lock()
+        && c.len() < MAX_CACHED_LANGS
+    {
         c.insert(key, loaded.clone());
     }
     loaded
@@ -880,6 +905,14 @@ SFX D Y 1\nSFX D 0 ed/S .\n";
         assert!(d.suggest("workingslides", 5).contains(&"working slides".to_string()));
     }
 
+    /// An affix header can claim billions of records; parsing stops where the file does.
+    #[test]
+    fn a_header_count_past_the_records_ends_with_them() {
+        let aff = b"SFX S Y 4000000000\nSFX S 0 s .\nREP 4000000000\nREP a b\n";
+        let d = Dictionary::parse(aff, b"1\ncat/S\n".as_slice());
+        assert!(d.check("cats"));
+    }
+
     /// #1175: compound splitting is quadratic in the word's length, so a word longer than
     /// `MAX_CHECK_CHARS` is not checked; shorter ones still are.
     #[test]

crates/metafile/src/canvas.rs

@@ -369,11 +369,6 @@ impl Canvas {
     }
 
     /// True once the item cap is hit; the readers stop there and keep what they have.
-    /// Bytes the canvas has retained so far.
-    pub fn used(&self) -> usize {
-        self.used
-    }
-
     pub fn full(&self) -> bool {
         self.full
     }
@@ -556,10 +551,6 @@ impl Canvas {
     /// of at most [`SHADE_RES`] cells on the longer side, laid out so that its one repeat covers
     /// the shape.
     pub fn shade_shape(&mut self, ops: &[Op], shade: &dyn Fn(P) -> [u8; 4]) {
-        // A full canvas keeps nothing more, so do not sample a tile for it.
-        if self.full {
-            return;
-        }
         let dev: Vec<Op> = ops.iter().map(|o| self.map_op(*o)).collect();
         let Some([l, t, r, b]) = crate::cliprect::ops_bbox(&dev) else { return };
         let side = (r - l).max(b - t);

crates/metafile/src/emfplus.rs

@@ -289,7 +289,7 @@ impl Plus {
             return None;
         }
         match typ {
-            0x4008 => self.object(flags, r, cv.used()),
+            0x4008 => self.object(flags, r),
             0x4009 | 0x4013 | 0x401A..=0x401C | 0x4036 => self.record_image_text_region(cv, typ, flags, r),
             0x4025..=0x4035 => self.record_state(cv, typ, flags, r),
             0x4010..=0x4012 | 0x4016..=0x4019 => self.record_curve(cv, typ, flags, r),
@@ -559,7 +559,7 @@ impl Plus {
         Some(())
     }
 
-    fn object(&mut self, flags: u16, r: Bytes, canvas_used: usize) -> Option<()> {
+    fn object(&mut self, flags: u16, r: Bytes) -> Option<()> {
         let id = usize::from(flags & 0xFF);
         let kind = (flags >> 8) & 0x7F;
         if flags & 0x8000 != 0 {
@@ -575,7 +575,7 @@ impl Plus {
             p.data.extend_from_slice(&chunk[..chunk.len().min(room)]);
             if p.data.len() >= p.total {
                 let obj = parse_object(p.kind, Bytes(&p.data));
-                self.store(p.id, obj, canvas_used);
+                self.store(p.id, obj);
             } else {
                 self.partial = Some(p);
             }
@@ -589,23 +589,22 @@ impl Plus {
             let room = p.total.saturating_sub(p.data.len());
             p.data.extend_from_slice(&r.0[..r.0.len().min(room)]);
             let obj = parse_object(kind, Bytes(&p.data));
-            self.store(id, obj, canvas_used);
+            self.store(id, obj);
             return Some(());
         }
         let obj = parse_object(kind, r);
-        self.store(id, obj, canvas_used);
+        self.store(id, obj);
         Some(())
     }
 
-    /// `canvas_used` is what the canvas already retains: objects and items share one budget.
-    fn store(&mut self, id: usize, obj: Option<Obj>, canvas_used: usize) {
+    fn store(&mut self, id: usize, obj: Option<Obj>) {
         let Some(slot) = self.objects.get_mut(id) else { return };
         if let Some(old) = slot.take() {
             self.object_bytes = self.object_bytes.saturating_sub(Self::obj_cost(&old));
         }
         if let Some(new_obj) = obj {
             let cost = Self::obj_cost(&new_obj);
-            if self.object_bytes.saturating_add(cost).saturating_add(canvas_used) <= crate::MAX_BYTES {
+            if self.object_bytes.saturating_add(cost) <= crate::MAX_BYTES {
                 self.object_bytes += cost;
                 *slot = Some(new_obj);
             }
@@ -616,7 +615,7 @@ impl Plus {
         match obj {
             Obj::Image(img) => match img.as_ref() {
                 ImageData::Bitmap(rgba) => rgba.px.len(),
-                ImageData::Meta(pic) => picture_cost(&pic.items),
+                ImageData::Meta(pic) => pic.items.len().saturating_mul(128),
             },
             Obj::Brush(Fill::Texture { img, .. }) => img.px.len(),
             Obj::Path(ops) => ops.len().saturating_mul(std::mem::size_of::<Op>()),
@@ -1277,33 +1276,6 @@ fn metafile(r: Bytes) -> Option<Picture> {
     pic.filter(|p| p.width > 0.0 && p.height > 0.0)
 }
 
-/// Bytes the items of a nested picture own: path segments, clip rectangles, pixels and text, with
-/// each shared pattern tile counted once.
-fn picture_cost(items: &[crate::Item]) -> usize {
-    use crate::Item;
-    let region = |r: &Option<Vec<[f32; 4]>>| r.as_ref().map_or(0, |v| v.len().saturating_mul(16));
-    let mut tiles = std::collections::HashSet::new();
-    let mut total = 0usize;
-    for item in items {
-        let own = match item {
-            Item::Path { segs, region: r, .. } => segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(region(r)),
-            Item::Bitmap { rgba, region: r, .. } => rgba.len().saturating_add(region(r)),
-            Item::Pattern { segs, tile, region: r, .. } => {
-                let tile_bytes = if tiles.insert(Arc::as_ptr(tile)) { tile.rgba.len() } else { 0 };
-                segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(tile_bytes).saturating_add(region(r))
-            }
-            Item::Text(t) => t
-                .text
-                .len()
-                .saturating_add(t.dx.len().saturating_mul(std::mem::size_of::<f32>()))
-                .saturating_add(t.font.family.len())
-                .saturating_add(region(&t.region)),
-        };
-        total = total.saturating_add(128).saturating_add(own);
-    }
-    total
-}
-
 fn bitmap(r: Bytes) -> Option<Rgba> {
     let (w, h, stride) = (r.u32(8)? as i32, r.u32(12)? as i32, r.u32(16)? as i32);
     let format = r.u32(20)?;
@@ -1581,17 +1553,3 @@ fn wrap_line(line: &[u16], width: f64, em: f64) -> Vec<Vec<u16>> {
     out.push(cur);
     out
 }
-
-#[cfg(test)]
-mod cost_tests {
-    use super::*;
-    use crate::{Item, Seg, Tile};
-
-    #[test]
-    fn a_shared_pattern_tile_is_charged_once() {
-        let tile = Arc::new(Tile { width: 512, height: 512, rgba: vec![0; 1 << 20], cell: [1.0, 1.0], origin: [0.0, 0.0] });
-        let item = || Item::Pattern { segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)], even_odd: false, tile: tile.clone(), region: None };
-        let items: Vec<Item> = (0..257).map(|_| item()).collect();
-        assert!(picture_cost(&items) < 2 << 20, "{}", picture_cost(&items));
-    }
-}

crates/metafile/src/place.rs

@@ -58,7 +58,6 @@ impl Picture {
         let k = (c[0].abs() + c[3].abs()) / 2.0;
         let mut out = Vec::with_capacity(self.items.len());
         let mut total_elements = 0usize;
-        let region_elements = std::cell::Cell::new(0usize);
         let mut cached_region: Option<(*const [f32; 4], usize, Option<Option<Arc<BezPath>>>)> = None;
         let mut get_region = |reg: &Option<Vec<[f32; 4]>>| -> Option<Option<Arc<BezPath>>> {
             let (ptr, len) = match reg {
@@ -72,15 +71,12 @@ impl Picture {
                 return res.clone();
             }
             let res = placed_region(&tf, reg);
-            // Each distinct clip region is expanded once; its elements count against the placement budget.
-            let n = res.as_ref().and_then(Option::as_ref).map_or(0, |p| p.elements().len());
-            region_elements.set(region_elements.get().saturating_add(n));
             cached_region = Some((ptr, len, res.clone()));
             res
         };
 
         for (index, item) in self.items.iter().enumerate() {
-            if total_elements.saturating_add(region_elements.get()) >= MAX_PLACED_ELEMENTS {
+            if total_elements >= MAX_PLACED_ELEMENTS {
                 break;
             }
             match item {
@@ -88,10 +84,6 @@ impl Picture {
                     let path = tf * bez(segs);
                     let Some(region) = get_region(region) else { continue };
                     if in_range(path.bounding_box()) {
-                        // This item's own region is expanded by now: it counts before it is kept.
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         let stroke = stroke.map(|(c, w)| (c, f64::from(w) * k));
                         out.push(PlacedItem::Path { path, fill: *fill, stroke, even_odd: *even_odd, region });
@@ -105,9 +97,6 @@ impl Picture {
                         * Affine::scale_non_uniform(f64::from(t.cell[0]), f64::from(t.cell[1]));
                     let det = tile.determinant();
                     if in_range(path.bounding_box()) && det.is_finite() && det.abs() > 1e-12 {
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         out.push(PlacedItem::Pattern { index, path, even_odd: *even_odd, tile, region });
                     }

crates/metafile/src/tests.rs

@@ -897,24 +897,6 @@ fn place_fits_the_frame_scales_strokes_and_drops_far_paths() {
     assert_eq!(stroke.map(|s| s.1), Some(8.0));
 }
 
-#[test]
-fn place_counts_the_items_own_clip_region_before_keeping_it() {
-    // A path of 499,999 elements nearly fills the 500,000 budget; a second one whose new clip
-    // region adds elements goes past it and is left out.
-    let mut segs = vec![Seg::Move(0.0, 0.0)];
-    segs.extend((0..499_998).map(|i| Seg::Line(1.0 + (i % 50) as f32, 1.0)));
-    let big = Item::Path { segs, fill: None, stroke: None, even_odd: false, region: None };
-    let small = Item::Path {
-        segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)],
-        fill: None,
-        stroke: None,
-        even_odd: false,
-        region: Some(vec![[0.0, 0.0, 50.0, 25.0], [10.0, 10.0, 20.0, 20.0]]),
-    };
-    let pic = crate::Picture { width: 100.0, height: 50.0, items: vec![big, small] };
-    assert_eq!(pic.place(kurbo::Rect::new(0.0, 0.0, 100.0, 50.0), [0.0; 4]).len(), 1);
-}
-
 #[test]
 fn place_keeps_only_finite_bounded_bitmaps_with_area() {
     let bitmap = |rect: [f32; 4]| Item::Bitmap { rect, width: 1, height: 1, rgba: vec![0; 4], region: None };
@@ -2092,15 +2074,3 @@ fn clip_polygon_work_bound() {
     let clipped = crate::cliprect::clip_polygon(&poly, &clip);
     assert!(clipped.is_empty(), "excessive polygon clip product rejected");
 }
-
-#[test]
-fn full_canvas_does_not_sample_gradient_tiles() {
-    let mut cv = Canvas::new();
-    let big = || Rgba { w: 1, h: 1, bottom_up: false, px: vec![0u8; MAX_BYTES / 2] };
-    for _ in 0..3 {
-        cv.bitmap([0.0, 0.0, 1.0, 1.0], big());
-    }
-    assert!(cv.full());
-    cv.shade_shape(&crate::geom::rect(0.0, 0.0, 10.0, 10.0), &|_| panic!("sampled a full canvas"));
-    assert_eq!(cv.items.len(), 2);
-}