From 4d9a70388cd25ab83ef2d40f2aa6e5ed47b1d68e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 00:13:43 +0000 Subject: [PATCH] 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 --- crates/text/src/fonts.rs | 60 ++++++++++++++++++++-------------- crates/text/src/fonts/tests.rs | 58 ++++++++++++++++++++++++++++---- crates/text/src/path_text.rs | 39 +++++++++++++++++++--- 3 files changed, 121 insertions(+), 36 deletions(-) diff --git a/crates/text/src/fonts.rs b/crates/text/src/fonts.rs index 7337f04f7..760ed8481 100644 --- a/crates/text/src/fonts.rs +++ b/crates/text/src/fonts.rs @@ -107,22 +107,29 @@ fn extra_dirs(var: Option<&std::ffi::OsStr>) -> Vec { 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, seen: &mut std::collections::HashSet) { - // 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) { + 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, seen: &mut st impl FontSource for DirectorySource { fn faces(&self) -> Vec { 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>> { - static FILES: std::sync::LazyLock>>>> = - 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); + static FILES: std::sync::LazyLock>>>> = 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) } diff --git a/crates/text/src/fonts/tests.rs b/crates/text/src/fonts/tests.rs index 00dd0d9e8..d70a709d9 100644 --- a/crates/text/src/fonts/tests.rs +++ b/crates/text/src/fonts/tests.rs @@ -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"); } diff --git a/crates/text/src/path_text.rs b/crates/text/src/path_text.rs index 5689f2c9e..7455456ad 100644 --- a/crates/text/src/path_text.rs +++ b/crates/text/src/path_text.rs @@ -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]