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/render/src/images.rs

@@ -37,8 +37,7 @@ struct Cache {
     bad_order: VecDeque<usize>,
     bad_bytes: usize,
     /// GIFs by bytes address and adjustments (`None`: not animated); the bytes stay alive with them.
-    /// Decoded animations by (bytes, adjustments), each with the clock of its last use.
-    anims: HashMap<(usize, u64), (Arc<Vec<u8>>, Option<Arc<Anim>>, u64)>,
+    anims: HashMap<(usize, u64), (Arc<Vec<u8>>, Option<Arc<Anim>>)>,
 }
 
 /// Longest side, total decoded size and frame count kept for one animated GIF; the largest canvas
@@ -433,33 +432,16 @@ fn anim(bytes: &Arc<Vec<u8>>, adj: &PictureAdjust) -> Option<Arc<Anim>> {
         return None;
     }
     let key = (Arc::as_ptr(bytes) as usize, adjust_key(adj));
-    let hit = with(|c| {
-        c.clock += 1;
-        let clock = c.clock;
-        c.anims.get_mut(&key).map(|e| {
-            e.2 = clock;
-            e.1.clone()
-        })
-    });
-    if let Some(a) = hit {
+    if let Some(a) = with(|c| c.anims.get(&key).map(|e| e.1.clone())) {
         return a;
     }
     let a = decode_anim(bytes, adj).map(Arc::new);
     with(|c| {
-        // Make room by dropping the least recently used animations, not every one of them, so a
-        // slide whose animations overflow the cache does not decode all of them again each frame.
-        let new = a.as_ref().map_or(0, |a| a.bytes());
-        loop {
-            let held: usize = c.anims.values().filter_map(|e| e.1.as_ref()).map(|a| a.bytes()).sum();
-            if held + new <= GIF_CACHE_BUDGET && c.anims.len() < 64 {
-                break;
-            }
-            let Some(oldest) = c.anims.iter().min_by_key(|(_, e)| e.2).map(|(k, _)| *k) else { break };
-            c.anims.remove(&oldest);
+        let held: usize = c.anims.values().filter_map(|e| e.1.as_ref()).map(|a| a.bytes()).sum();
+        if held + a.as_ref().map_or(0, |a| a.bytes()) > GIF_CACHE_BUDGET || c.anims.len() >= 64 {
+            c.anims.clear();
         }
-        c.clock += 1;
-        let clock = c.clock;
-        c.anims.insert(key, (bytes.clone(), a.clone(), clock));
+        c.anims.insert(key, (bytes.clone(), a.clone()));
     });
     a
 }
@@ -553,26 +535,6 @@ mod tests {
         out.into_inner()
     }
 
-    /// A two-frame GIF whose first pixel is `n`, so each one has its own bytes.
-    fn gif(n: u8) -> Arc<Vec<u8>> {
-        use image::codecs::gif::GifEncoder;
-        let frame = |c: u8| image::Frame::new(image::RgbaImage::from_pixel(2, 2, image::Rgba([n, c, 0, 255])));
-        let mut out = Vec::new();
-        GifEncoder::new(&mut out).encode_frames([frame(0), frame(255)]).expect("encode");
-        Arc::new(out)
-    }
-
-    #[test]
-    fn a_full_animation_cache_drops_the_oldest_entry_not_all_of_them() {
-        let adj = PictureAdjust::default();
-        let gifs: Vec<_> = (0..70).map(gif).collect();
-        for g in &gifs {
-            assert!(anim(g, &adj).is_some());
-        }
-        let (len, newest) = with(|c| (c.anims.len(), c.anims.contains_key(&(Arc::as_ptr(&gifs[69]) as usize, adjust_key(&adj)))));
-        assert!(newest && len >= 32, "{len} cached");
-    }
-
     #[test]
     fn rgba_conversion_is_refused_past_the_cap() {
         let png = grey_png(100, 50);