address review: font walk breadth-first across roots, font file cache keyed by size and mtime and read outside the lock, first-subpath path measure with finite check, scratch dir guard

2026-11-11 06:01:23 +00:00 · 2026-11-10 21:46:52 -05:00
parent 603a4f5b60
commit 4d9a70388c
3 changed files with 121 additions and 36 deletions
@@ -107,22 +107,29 @@ fn extra_dirs(var: Option<&std::ffi::OsStr>) -> Vec<PathBuf> {
    var.map(|v| std::env::split_paths(v).filter(|p| !p.as_os_str().is_empty()).collect()).unwrap_or_default()
 }

-fn walk(dir: &std::path::Path, depth: u32, out: &mut Vec<PathBuf>, seen: &mut std::collections::HashSet<PathBuf>) {
-    // Each real folder is listed once: symlinks fanning out to the same folders would otherwise
-    // be walked again along every path to them (exponentially many within the depth bound).
-    if !seen.insert(std::fs::canonicalize(dir).unwrap_or_else(|_| dir.to_path_buf())) {
-        return;
-    }
-    let Ok(rd) = std::fs::read_dir(dir) else { return };
-    for e in rd.flatten() {
-        let p = e.path();
-        if p.is_dir() {
-            // Bounded, so a symlink loop ends.
-            if depth < 8 {
-                walk(&p, depth + 1, out, seen);
+/// Font files under `roots`, each real folder listed once. Breadth first from every root, so a
+/// folder reached by several routes is listed at its shallowest and keeps the most depth below it.
+fn walk(roots: &[PathBuf], out: &mut Vec<PathBuf>) {
+    let mut seen = std::collections::HashSet::new();
+    let mut queue: std::collections::VecDeque<(PathBuf, u32)> = roots.iter().map(|r| (r.clone(), 0)).collect();
+    while let Some((dir, depth)) = queue.pop_front() {
+        // Symlinks fanning out to the same folders would otherwise be walked again along every
+        // path to them (exponentially many within the depth bound).
+        if !seen.insert(std::fs::canonicalize(&dir).unwrap_or_else(|_| dir.clone())) {
+            continue;
+        }
+        let Ok(rd) = std::fs::read_dir(&dir) else { continue };
+        for e in rd.flatten() {
+            let p = e.path();
+            if p.is_dir() {
+                // Bounded, so a symlink loop ends.
+                if depth < 8 {
+                    queue.push_back((p, depth + 1));
+                }
+            } else if p.extension().and_then(std::ffi::OsStr::to_str).is_some_and(|x| matches!(x.to_ascii_lowercase().as_str(), "ttf" | "otf" | "ttc" | "otc"))
+            {
+                out.push(p);
            }
-        } else if p.extension().and_then(std::ffi::OsStr::to_str).is_some_and(|x| matches!(x.to_ascii_lowercase().as_str(), "ttf" | "otf" | "ttc" | "otc")) {
-            out.push(p);
        }
    }
 }
@@ -130,10 +137,7 @@ fn walk(dir: &std::path::Path, depth: u32, out: &mut Vec<PathBuf>, seen: &mut st
 impl FontSource for DirectorySource {
    fn faces(&self) -> Vec<FaceInfo> {
        let mut files = Vec::new();
-        let mut seen = std::collections::HashSet::new();
-        for d in &self.dirs {
-            walk(d, 0, &mut files, &mut seen);
-        }
+        walk(&self.dirs, &mut files);
        files.sort();
        files.dedup();
        let mut out = Vec::new();
@@ -216,15 +220,23 @@ pub struct VMetrics {
 /// A font file's bytes, shared by every face of it that is loaded (the faces of a collection
 /// each held their own copy of the whole file).
 fn file_bytes(p: &std::path::Path) -> Option<Arc<Vec<u8>>> {
-    static FILES: std::sync::LazyLock<std::sync::Mutex<std::collections::HashMap<PathBuf, std::sync::Weak<Vec<u8>>>>> =
-        std::sync::LazyLock::new(Default::default);
-    let mut files = FILES.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
-    if let Some(b) = files.get(p).and_then(std::sync::Weak::upgrade) {
+    type Key = (PathBuf, u64, Option<std::time::SystemTime>);
+    static FILES: std::sync::LazyLock<std::sync::Mutex<std::collections::HashMap<Key, std::sync::Weak<Vec<u8>>>>> = std::sync::LazyLock::new(Default::default);
+    let lock = || FILES.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+    // Keyed by size and modification time too: a file replaced at the same path is read afresh.
+    let m = std::fs::metadata(p).ok()?;
+    let key: Key = (p.to_path_buf(), m.len(), m.modified().ok());
+    if let Some(b) = lock().get(&key).and_then(std::sync::Weak::upgrade) {
        return Some(b);
    }
+    // Read outside the lock: a slow file must not hold up every other face's first load.
    let b = Arc::new(std::fs::read(p).ok()?);
+    let mut files = lock();
+    if let Some(b) = files.get(&key).and_then(std::sync::Weak::upgrade) {
+        return Some(b);
+    }
    files.retain(|_, w| w.strong_count() > 0);
-    files.insert(p.to_path_buf(), Arc::downgrade(&b));
+    files.insert(key, Arc::downgrade(&b));
    Some(b)
 }

@@ -568,21 +568,65 @@ fn unknown_face_id_returns_bundled_default() {
    assert!(Arc::ptr_eq(&fallback, &face(0)));
 }

+/// A scratch folder this test created itself (never one that already exists), removed on drop.
+struct Scratch(std::path::PathBuf);
+impl Scratch {
+    fn new(tag: &str) -> Self {
+        let d = (0u32..)
+            .map(|n| std::env::temp_dir().join(format!("effectcraft-{tag}-{}-{n}", std::process::id())))
+            .find_map(|d| match std::fs::create_dir(&d) {
+                Ok(()) => Some(d),
+                Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => None,
+                Err(e) => panic!("create scratch dir {}: {e}", d.display()),
+            })
+            .unwrap();
+        Scratch(d)
+    }
+}
+impl Drop for Scratch {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.0);
+    }
+}
+
 #[cfg(unix)]
 #[test]
 fn a_folder_reached_through_many_links_is_listed_once() {
    // Six links back to the folder itself: 6^8 listings within the depth bound before.
-    let d = std::env::temp_dir().join(format!("effectcraft-font-walk-{}", std::process::id()));
-    let _ = std::fs::remove_dir_all(&d);
-    std::fs::create_dir_all(&d).unwrap();
+    let scratch = Scratch::new("font-walk");
+    let d = &scratch.0;
    std::fs::write(d.join("a.ttf"), b"x").unwrap();
    for i in 0..6 {
-        std::os::unix::fs::symlink(&d, d.join(format!("l{i}"))).unwrap();
+        std::os::unix::fs::symlink(d, d.join(format!("l{i}"))).unwrap();
    }
-    let (mut out, mut seen) = (Vec::new(), std::collections::HashSet::new());
+    let mut out = Vec::new();
    let start = std::time::Instant::now();
-    walk(&d, 0, &mut out, &mut seen);
+    walk(std::slice::from_ref(d), &mut out);
    assert!(start.elapsed() < std::time::Duration::from_secs(5));
    assert_eq!(out.len(), 1);
-    let _ = std::fs::remove_dir_all(&d);
+}
+
+#[test]
+fn a_folder_inside_another_root_keeps_its_own_depth() {
+    // root/a/b/c/d/e/f/g/h is also a root: the walk of `root` reaches it at depth 8 and could not
+    // go below it, so it must be listed from the shallower visit.
+    let scratch = Scratch::new("font-depth");
+    let root = &scratch.0;
+    let deep = root.join("a/b/c/d/e/f/g/h");
+    std::fs::create_dir_all(deep.join("sub")).unwrap();
+    std::fs::write(deep.join("sub/x.ttf"), b"x").unwrap();
+    let mut out = Vec::new();
+    walk(&[root.clone(), deep.clone()], &mut out);
+    assert_eq!(out, vec![deep.join("sub/x.ttf")]);
+}
+
+#[test]
+fn a_font_file_replaced_at_the_same_path_is_read_again() {
+    let scratch = Scratch::new("font-bytes");
+    let p = scratch.0.join("a.ttf");
+    std::fs::write(&p, b"old").unwrap();
+    let old = file_bytes(&p).unwrap();
+    std::fs::write(&p, b"new, longer").unwrap();
+    assert_eq!(file_bytes(&p).unwrap().as_slice(), b"new, longer");
+    assert_eq!(old.as_slice(), b"old");
 }
@@ -21,12 +21,24 @@ impl PathMeasure {
        let mut done = false;
        // The tolerance grows with the path's size (0.05 for ordinary layer-sized paths): a
        // path with huge coordinates would otherwise flatten into hundreds of millions of points.
-        let bbox = kurbo::Shape::bounding_box(path);
-        let size = bbox.width().max(bbox.height());
-        if !size.is_finite() {
-            done = true;
+        // Only the first subpath is measured, so only it sets the tolerance and gets flattened.
+        let mut first = BezPath::new();
+        for el in path.iter() {
+            if matches!(el, PathEl::MoveTo(_)) && !first.elements().is_empty() {
+                break;
+            }
+            first.push(el);
+            if matches!(el, PathEl::ClosePath) {
+                break;
+            }
        }
-        kurbo::flatten(path.iter(), 0.05_f64.max(size * 1e-6), |el| {
+        // A non-finite coordinate (an infinite control point too) measures nothing.
+        if !first.is_finite() {
+            return PathMeasure::default();
+        }
+        let bbox = kurbo::Shape::bounding_box(&first);
+        let size = bbox.width().max(bbox.height());
+        kurbo::flatten(first.iter(), 0.05_f64.max(size * 1e-6), |el| {
            if done {
                return;
            }
@@ -148,6 +160,23 @@ mod tests {
        q.move_to((0.0, 0.0));
        q.curve_to((f64::INFINITY, 0.0), (1.0, 1.0), (2.0, 0.0));
        assert_eq!(PathMeasure::new(&q, false).pts.len(), 0);
+        // Even with finite endpoints, an infinite control point is refused before flattening.
+        let mut r = BezPath::new();
+        r.move_to((0.0, 0.0));
+        r.curve_to((f64::INFINITY, f64::INFINITY), (f64::INFINITY, 0.0), (1.0, 0.0));
+        assert_eq!(PathMeasure::new(&r, false).pts.len(), 0);
+    }
+
+    #[test]
+    fn a_distant_second_subpath_does_not_coarsen_the_first() {
+        let mut p = BezPath::new();
+        p.move_to((0.0, 0.0));
+        p.curve_to((0.0, 100.0), (100.0, 100.0), (100.0, 0.0));
+        p.move_to((1e9, 1e9));
+        p.line_to((1e9 + 1.0, 1e9));
+        let m = PathMeasure::new(&p, false);
+        assert!(m.pts.len() > 10, "{}", m.pts.len());
+        assert!(m.length() > 150.0, "{}", m.length());
    }

    #[test]