From 8b47a9bd4bf4c56c5b662fd143ae66e80548bec4 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 09:55:06 +0000 Subject: [PATCH] address review: font files shared through file_bytes with the open file's metadata; run lookup by bisection only for ordered runs --- crates/text/src/fonts.rs | 46 +++++++++++++++------------------ crates/text/src/fonts/tests.rs | 10 +++++-- crates/text/src/layout.rs | 43 ++++++++++++++++++------------ crates/text/src/layout/tests.rs | 11 +++++--- 4 files changed, 64 insertions(+), 46 deletions(-) diff --git a/crates/text/src/fonts.rs b/crates/text/src/fonts.rs index 760ed8481..c0eea0bd7 100644 --- a/crates/text/src/fonts.rs +++ b/crates/text/src/fonts.rs @@ -168,17 +168,6 @@ fn info_from(n: FaceNames, data: FaceData, origin: &'static str) -> FaceInfo { } } -/// A font file's bytes and its metadata from the same open file (so the metadata describes the -/// bytes, not a later version of the file). -fn read_with_metadata(path: &std::path::Path) -> std::io::Result<(Vec, std::fs::Metadata)> { - use std::io::Read; - let mut f = std::fs::File::open(path)?; - let meta = f.metadata()?; - let mut bytes = Vec::new(); - f.read_to_end(&mut bytes)?; - Ok((bytes, meta)) -} - /// A loaded (or loadable) face. pub struct Face { pub id: FaceId, @@ -218,26 +207,33 @@ 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>> { +/// each held their own copy of the whole file), with the metadata of the open file they were read +/// from (so it describes the bytes, not a later version of the file). +fn file_bytes(p: &std::path::Path) -> std::io::Result<(Arc>, std::fs::Metadata)> { + use std::io::Read; type Key = (PathBuf, u64, Option); - static FILES: std::sync::LazyLock>>>> = std::sync::LazyLock::new(Default::default); + type Entry = (std::sync::Weak>, std::fs::Metadata); + static FILES: std::sync::LazyLock>> = std::sync::LazyLock::new(Default::default); let lock = || FILES.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let hit = |files: &std::collections::HashMap, key: &Key| files.get(key).and_then(|(w, m)| Some((w.upgrade()?, m.clone()))); + let mut f = std::fs::File::open(p)?; // 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 m = f.metadata()?; 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); + if let Some(found) = hit(&lock(), &key) { + return Ok(found); } // 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 bytes = Vec::new(); + f.read_to_end(&mut bytes)?; + let b = Arc::new(bytes); let mut files = lock(); - if let Some(b) = files.get(&key).and_then(std::sync::Weak::upgrade) { - return Some(b); + if let Some(found) = hit(&files, &key) { + return Ok(found); } - files.retain(|_, w| w.strong_count() > 0); - files.insert(key, Arc::downgrade(&b)); - Some(b) + files.retain(|_, (w, _)| w.strong_count() > 0); + files.insert(key, (Arc::downgrade(&b), m.clone())); + Ok((b, m)) } impl Face { @@ -248,9 +244,9 @@ impl Face { FaceData::Static(b) => Some(FaceBytes::Static(b)), FaceData::Shared(b) => Some(FaceBytes::Shared(b.clone())), FaceData::File(p) => { - let (bytes, meta) = read_with_metadata(p).ok()?; + let (bytes, meta) = file_bytes(p).ok()?; let _ = self.file_meta.set(meta); - Some(FaceBytes::Shared(Arc::new(bytes))) + Some(FaceBytes::Shared(bytes)) } }) .as_ref() diff --git a/crates/text/src/fonts/tests.rs b/crates/text/src/fonts/tests.rs index d70a709d9..2e04e5f0e 100644 --- a/crates/text/src/fonts/tests.rs +++ b/crates/text/src/fonts/tests.rs @@ -625,8 +625,14 @@ 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(); + let (old, old_meta) = file_bytes(&p).unwrap(); + // Faces of one file share its bytes, and the metadata is that of the file the bytes came from. + let (again, _) = file_bytes(&p).unwrap(); + assert!(Arc::ptr_eq(&old, &again)); + assert_eq!(old_meta.len(), 3); std::fs::write(&p, b"new, longer").unwrap(); - assert_eq!(file_bytes(&p).unwrap().as_slice(), b"new, longer"); + let (new, new_meta) = file_bytes(&p).unwrap(); + assert_eq!(new.as_slice(), b"new, longer"); + assert_eq!(new_meta.len(), 11); assert_eq!(old.as_slice(), b"old"); } diff --git a/crates/text/src/layout.rs b/crates/text/src/layout.rs index 0686d1658..6ab393cd9 100644 --- a/crates/text/src/layout.rs +++ b/crates/text/src/layout.rs @@ -628,15 +628,23 @@ fn is_hanging_open(c: char) -> bool { matches!(c, '"' | '\'' | '\u{2018}' | '\u{201c}' | '(' | '\u{ab}') } +/// Whether each run starts at or after the end of the one before it (the runs of a document): +/// the precondition of [`run_at`]'s bisection. +fn runs_ordered(runs: &[(Range, TextStyle)]) -> bool { + runs.windows(2).all(|w| w[0].0.end <= w[1].0.start) +} + /// The style run index covering global byte `b` (the run before it at a boundary, for empty -/// paragraphs at the end). -fn run_at(runs: &[(Range, TextStyle)], b: usize) -> usize { - // Runs from a document are ordered and contiguous: the last one starting at or before `b` - // holds it, found by bisection (a scan from the first run per character was quadratic). - // Anything else (a gap, unordered runs) takes the scan as before. - let i = runs.partition_point(|(r, _)| r.start <= b).saturating_sub(1); - if runs.get(i).is_some_and(|(r, _)| r.contains(&b)) { - return i; +/// paragraphs at the end). `ordered` is [`runs_ordered`] of `runs`. +fn run_at(runs: &[(Range, TextStyle)], ordered: bool, b: usize) -> usize { + // Ordered, non-overlapping runs: the last one starting at or before `b` is the only one that + // can hold it, found by bisection (a scan from the first run per character was quadratic). + // Anything else (a gap, overlapping or unordered runs) takes the scan, whose first match wins. + if ordered { + let i = runs.partition_point(|(r, _)| r.start <= b).saturating_sub(1); + if runs.get(i).is_some_and(|(r, _)| r.contains(&b)) { + return i; + } } runs.iter().position(|(r, _)| b >= r.start && b < r.end).unwrap_or_else(|| runs.iter().rposition(|(r, _)| r.start <= b).unwrap_or(0)) } @@ -714,6 +722,7 @@ impl<'a> Para<'a> { text: &'a str, base: usize, runs: &'a [(Range, TextStyle)], + ordered: bool, primaries: &'a [Resolved], variation_ids: &'a [u32], para: &'a ParagraphStyle, @@ -722,7 +731,7 @@ impl<'a> Para<'a> { let default_level = para.rtl.map(|r| if r { Level::rtl() } else { Level::ltr() }); let bidi = BidiInfo::new(text, default_level); let base_rtl = bidi.paragraphs[0].level.is_rtl(); - let chars = char_forms(text, base, runs, primaries, &bidi); + let chars = char_forms(text, base, runs, ordered, primaries, &bidi); let items = build_items(text, &chars, runs); // Prefix widths and the preceding non-whitespace boundary make candidate line widths O(1). // Every-line composition evaluates O(n²) candidate breaks; summing each candidate's bytes @@ -1028,10 +1037,10 @@ impl<'a> Para<'a> { } /// Each char with its shaped form, face, direction and run. -fn char_forms(text: &str, base: usize, runs: &[(Range, TextStyle)], primaries: &[Resolved], bidi: &BidiInfo) -> Vec { +fn char_forms(text: &str, base: usize, runs: &[(Range, TextStyle)], ordered: bool, primaries: &[Resolved], bidi: &BidiInfo) -> Vec { text.char_indices() .map(|(b, c)| { - let si = run_at(runs, base + b); + let si = run_at(runs, ordered, base + b); let style = &runs[si].1; let primary = primaries[si].face; let face_of = |ch: char| if ch.is_whitespace() { primary } else { fonts::fallback_for(ch, primary) }; @@ -1090,13 +1099,14 @@ fn paragraph( text: &str, base: usize, runs: &[(Range, TextStyle)], + ordered: bool, primaries: &[Resolved], variation_ids: &[u32], para: &ParagraphStyle, width: Option, ) -> Vec { if text.is_empty() { - let si = run_at(runs, base); + let si = run_at(runs, ordered, base); let pm = style_metrics(primaries, runs, si); // No text to resolve a direction from: the explicit one. let rtl = para.rtl == Some(true); @@ -1117,7 +1127,7 @@ fn paragraph( underline: None, }]; } - let p = Para::new(text, base, runs, primaries, variation_ids, para, width); + let p = Para::new(text, base, runs, ordered, primaries, variation_ids, para, width); let ranges = p.break_ranges(); let nlines = ranges.len(); ranges.into_iter().enumerate().map(|(li, r)| p.place_line(li, r, li + 1 == nlines)).collect() @@ -1174,18 +1184,19 @@ pub fn layout_rich_uncached(text: &str, runs: &[(Range, TextStyle)], para }); let runs = cleared.as_deref().unwrap_or(runs); let variation_ids: Vec = runs.iter().map(|(_, s)| crate::variable::intern(&s.variations)).collect(); + let ordered = runs_ordered(runs); let mut lay = Layout { missing_font: primaries.iter().any(|p| p.missing), text_len: text.len(), ..Default::default() }; let mut plines: Vec<(usize, ParaLine)> = Vec::new(); for (pi, span) in paragraph_ranges(text).enumerate() { let p = text.get(span.bytes.clone()).unwrap_or_default(); - plines.extend(paragraph(p, span.bytes.start, runs, &primaries, &variation_ids, para_at(pi), width).into_iter().map(|l| (pi, l))); + plines.extend(paragraph(p, span.bytes.start, runs, ordered, &primaries, &variation_ids, para_at(pi), width).into_iter().map(|l| (pi, l))); } let mut baseline = 0.0f32; for (i, (pi, pl)) in plines.into_iter().enumerate() { let para = para_at(pi); if i == 0 { // Vertical type keeps its first column at the font ascent (not measured in AE). - let vertical = runs.get(run_at(runs, pl.range.start)).is_some_and(|r| r.1.vertical) + let vertical = runs.get(run_at(runs, ordered, pl.range.start)).is_some_and(|r| r.1.vertical) || runs.iter().any(|r| r.1.vertical && r.0.start < pl.range.end && r.0.end > pl.range.start); baseline = if width.is_some() { if vertical { pl.ascent } else { pl.first } } else { 0.0 }; } else { @@ -1205,7 +1216,7 @@ pub fn layout_rich_uncached(text: &str, runs: &[(Range, TextStyle)], para })); let x = shift + pl.left_trim; if let Some((u0, u1)) = pl.underline { - let si = run_at(runs, pl.range.start); + let si = run_at(runs, ordered, pl.range.start); let pm = fonts::face(primaries[si].face).metrics(runs[si].1.size); lay.underlines.push([u0 + shift, baseline + pm.underline_pos, u1 + shift, baseline + pm.underline_pos + pm.underline_thickness]); } diff --git a/crates/text/src/layout/tests.rs b/crates/text/src/layout/tests.rs index 33c2a3c89..0ea8e7474 100644 --- a/crates/text/src/layout/tests.rs +++ b/crates/text/src/layout/tests.rs @@ -513,9 +513,14 @@ fn run_lookup_by_bisection_matches_the_scan() { }; let runs: Vec<(Range, TextStyle)> = (0..50).map(|i| (i * 3..i * 3 + 3, st(10.0))).collect(); let gapped: Vec<(Range, TextStyle)> = (0..50).map(|i| (i * 4..i * 4 + 3, st(10.0))).collect(); + // Overlapping runs: the first run holding a byte wins, as in the scan. + let overlapping: Vec<(Range, TextStyle)> = vec![(0..10, st(10.0)), (5..15, st(20.0)), (12..20, st(30.0))]; + assert!(runs_ordered(&runs) && runs_ordered(&gapped) && !runs_ordered(&overlapping)); for b in 0..220 { - assert_eq!(run_at(&runs, b), scan(&runs, b), "{b}"); - assert_eq!(run_at(&gapped, b), scan(&gapped, b), "{b}"); + assert_eq!(run_at(&runs, true, b), scan(&runs, b), "{b}"); + assert_eq!(run_at(&gapped, true, b), scan(&gapped, b), "{b}"); + assert_eq!(run_at(&overlapping, false, b), scan(&overlapping, b), "{b}"); } - assert_eq!(run_at(&[], 5), 0); + assert_eq!(run_at(&overlapping, false, 6), 0); + assert_eq!(run_at(&[], true, 5), 0); }