From 7c0f804783cea8ddf79cbd5efb0f1b742da4b177 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 29 Sep 2026 10:15:52 +0200 Subject: [PATCH] fix(fonts): make a font cache invalidation reach the rayon workers clear_all_media_caches ran on the calling thread. The font caches are thread-local, so it emptied main's copy and left every rayon worker -- the threads that actually render frames -- holding the old one. Under --watch, editing a custom font file kept serving the previous face and the previous glyph widths for the rest of the process. A rayon::broadcast would only reach the pool's current threads and would have to race with the work they are doing. A generation counter does not: FONT_GENERATION is global, each thread-local cache remembers the generation it was filled at, and it drops its contents on the first access after a bump. No thread has to be interrupted, and a worker spawned later starts on the current generation by construction. Both font caches were also missing from the function entirely -- CUSTOM_TYPEFACES predates it, and the fallback cache added in 5b81cd3 inherited the same gap. Both are GenerationCache now, and the fallback one is hoisted out of its function body so the two sit together. The registry had the same staleness one level down: register_custom_font_variant skipped a slot that was already filled, so even the global registry kept the previous file's bytes. That skip is not gratuitous -- one FontEntry can resolve to several files, and registering_the_same_weight_twice_keeps_the_first pins that two of them landing on the same weight must not fight. Both intents hold now: a variant records the generation it was registered in, so first-wins applies within a load and a later load supersedes it. load_custom_fonts opens a generation, which is what makes "a load" a thing the registry can see. It returns early on an empty list: a scenario with no custom fonts reloads constantly under --watch, and bumping there would clear every worker's cache for nothing. --- .../src/engine/renderer/fonts.rs | 187 ++++++++++++++++-- crates/rustmotion/src/cli/commands/render.rs | 1 + 2 files changed, 170 insertions(+), 18 deletions(-) diff --git a/crates/rustmotion-core/src/engine/renderer/fonts.rs b/crates/rustmotion-core/src/engine/renderer/fonts.rs index ff4d4e6..f995275 100644 --- a/crates/rustmotion-core/src/engine/renderer/fonts.rs +++ b/crates/rustmotion-core/src/engine/renderer/fonts.rs @@ -11,8 +11,39 @@ use super::google_fonts::{font_cache_dir, resolve_google_font}; thread_local! { static THREAD_FONT_MGR: FontMgr = FontMgr::default(); - static CUSTOM_TYPEFACES: RefCell> = - RefCell::new(HashMap::new()); + static CUSTOM_TYPEFACES: RefCell> = + RefCell::new(GenerationCache::new()); + static FALLBACK_CACHE: RefCell>> = + RefCell::new(GenerationCache::new()); +} + +static FONT_GENERATION: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +pub fn invalidate_font_caches() { + FONT_GENERATION.fetch_add(1, std::sync::atomic::Ordering::Release); +} + +struct GenerationCache { + generation: u64, + map: HashMap, +} + +impl GenerationCache { + fn new() -> Self { + Self { + generation: FONT_GENERATION.load(std::sync::atomic::Ordering::Acquire), + map: HashMap::new(), + } + } + + fn current(&mut self) -> &mut HashMap { + let generation = FONT_GENERATION.load(std::sync::atomic::Ordering::Acquire); + if self.generation != generation { + self.map.clear(); + self.generation = generation; + } + &mut self.map + } } pub fn font_mgr() -> FontMgr { @@ -24,6 +55,7 @@ struct CustomFontVariant { data: Vec, weight: i32, italic: bool, + generation: u64, } fn custom_font_registry() -> &'static Mutex>> { @@ -35,16 +67,21 @@ pub fn register_custom_font_variant(family: &str, data: Vec, weight: i32, it let mut reg = custom_font_registry() .lock() .unwrap_or_else(|e| e.into_inner()); + let generation = FONT_GENERATION.load(std::sync::atomic::Ordering::Acquire); let variants = reg.entry(family.to_string()).or_default(); - if !variants - .iter() - .any(|v| v.weight == weight && v.italic == italic) + let replacement = CustomFontVariant { + data, + weight, + italic, + generation, + }; + match variants + .iter_mut() + .find(|v| v.weight == weight && v.italic == italic) { - variants.push(CustomFontVariant { - data, - weight, - italic, - }); + Some(existing) if existing.generation == generation => {} + Some(existing) => *existing = replacement, + None => variants.push(replacement), } } @@ -73,7 +110,7 @@ fn custom_typeface(family: &str, style: FontStyle) -> Option { let italic = style.slant() != skia_safe::font_style::Slant::Upright; let cache_key = (family.to_string(), weight, italic); CUSTOM_TYPEFACES.with(|cache| { - if let Some(tf) = cache.borrow().get(&cache_key) { + if let Some(tf) = cache.borrow_mut().current().get(&cache_key) { return Some(tf.clone()); } let data = { @@ -85,11 +122,16 @@ fn custom_typeface(family: &str, style: FontStyle) -> Option { }; let sk_data = skia_safe::Data::new_copy(&data); let tf = font_mgr().new_from_data(&sk_data, None)?; - cache.borrow_mut().insert(cache_key, tf.clone()); + cache.borrow_mut().current().insert(cache_key, tf.clone()); Some(tf) }) } +#[cfg(test)] +fn what_this_thread_sees_in_its_fallback_cache() -> usize { + FALLBACK_CACHE.with(|cache| cache.borrow_mut().current().len()) +} + pub fn resolve_custom_typeface(family: &str, style: FontStyle) -> Option { custom_typeface(family, style) } @@ -120,6 +162,10 @@ pub fn resolve_font_entry(entry: &FontEntry) -> Result> } pub fn load_custom_fonts(fonts: &[FontEntry]) { + if fonts.is_empty() { + return; + } + invalidate_font_caches(); let font_mgr = font_mgr(); for entry in fonts { match resolve_font_entry(entry) { @@ -209,20 +255,16 @@ pub fn fallback_typeface_for_char( style: FontStyle, c: char, ) -> Option { - thread_local! { - static FALLBACK_CACHE: RefCell>> = - RefCell::new(HashMap::new()); - } let weight = *style.weight(); let italic = style.slant() != skia_safe::font_style::Slant::Upright; let key = (primary_family.to_string(), weight, italic, c as u32); FALLBACK_CACHE.with(|cache| { - if let Some(hit) = cache.borrow().get(&key) { + if let Some(hit) = cache.borrow_mut().current().get(&key) { return hit.clone(); } let resolved = font_mgr().match_family_style_character(primary_family, style, &[], c as i32); - cache.borrow_mut().insert(key, resolved.clone()); + cache.borrow_mut().current().insert(key, resolved.clone()); resolved }) } @@ -430,3 +472,112 @@ mod tests { assert_eq!(paths.len(), 1); } } + +#[cfg(test)] +mod cache_invalidation_reaches_every_thread_tests { + use super::*; + use rayon::prelude::*; + use std::sync::atomic::Ordering; + + fn a_face(marker: u8) -> Vec { + let mut data = vec![ + 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, 0x10, 0x00, 0x03, 0x00, 0x04, + ]; + data.push(marker); + data + } + + fn fill_this_threads_fallback_cache() -> usize { + for c in ['a', 'e', 'i', 'o', 'u', 'y', 'z'] { + fallback_typeface_for_char("rm-test-no-such-family", FontStyle::normal(), c); + } + what_this_thread_sees_in_its_fallback_cache() + } + + #[test] + fn a_rayon_worker_drops_its_own_copy_when_the_generation_moves() { + let before: Vec = (0..256) + .into_par_iter() + .map(|_| fill_this_threads_fallback_cache()) + .collect(); + assert!( + before.iter().any(|&len| len > 0), + "test setup: the workers must actually have filled a thread-local cache, or the \ + assertion below proves nothing" + ); + + invalidate_font_caches(); + + let after: Vec = (0..256) + .into_par_iter() + .map(|_| what_this_thread_sees_in_its_fallback_cache()) + .collect(); + assert!( + after.iter().all(|&len| len == 0), + "the clearing is lazy, so this is what each worker sees on its next lookup — and \ + that is the guarantee: clearing on the calling thread alone leaves the threads \ + that actually render frames serving the stale face forever: {after:?}" + ); + } + + #[test] + fn a_generation_cache_holds_its_entries_until_the_generation_moves() { + let mut cache: GenerationCache = GenerationCache::new(); + cache.current().insert(1, 10); + cache.current().insert(2, 20); + assert_eq!( + cache.current().len(), + 2, + "nothing moved, nothing is dropped" + ); + invalidate_font_caches(); + assert_eq!( + cache.current().len(), + 0, + "the next access after a bump is what clears it — a thread that is busy rendering \ + does not have to be interrupted for its cache to go stale" + ); + } + + #[test] + fn a_second_file_in_the_same_load_does_not_clobber_the_first() { + let family = "rm-test-same-load"; + invalidate_font_caches(); + register_custom_font_variant(family, a_face(1), 400, false); + register_custom_font_variant(family, a_face(2), 400, false); + assert_eq!( + custom_font_bytes(family, 400, false), + Some(a_face(1)), + "one FontEntry can resolve to several files; two of them landing on the same weight \ + and slant must not fight, the first still wins" + ); + } + + #[test] + fn the_next_load_supersedes_the_variant_the_previous_one_registered() { + let family = "rm-test-reload"; + invalidate_font_caches(); + register_custom_font_variant(family, a_face(1), 400, false); + + invalidate_font_caches(); + register_custom_font_variant(family, a_face(2), 400, false); + assert_eq!( + custom_font_bytes(family, 400, false), + Some(a_face(2)), + "editing the file under --watch is a new load, and its bytes must win — being \ + skipped is what kept serving the previous face for the rest of the process" + ); + } + + #[test] + fn loading_an_empty_font_list_does_not_throw_away_every_threads_cache() { + let generation = FONT_GENERATION.load(Ordering::Acquire); + load_custom_fonts(&[]); + assert_eq!( + FONT_GENERATION.load(Ordering::Acquire), + generation, + "a scenario with no custom fonts reloads constantly under --watch; bumping there \ + would clear every worker's font cache for nothing" + ); + } +} diff --git a/crates/rustmotion/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index 7a54598..7c553f7 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -12,6 +12,7 @@ fn clear_all_media_caches() { engine::clear_asset_cache(); engine::gif_cache().clear(); engine::video_frame_cache().clear(); + rustmotion_core::engine::renderer::invalidate_font_caches(); } #[allow(clippy::too_many_arguments)]