From 1c12b20c2abf3ea79b632c8c32e7428dfb111df1 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:44:13 +0200 Subject: [PATCH] perf(gif): decode a gif once across rayon workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triggering scenario: a scenario with one gif component, rendered to MP4. encode_with_ffmpeg (ffmpeg.rs:582) dispatches the FIRST chunk of batch_size = rayon::current_num_threads() * 2 frames — all frames 0..N of the same scene — to par_iter() at once. Every one of those workers calls Gif::paint_content, every one misses the still-empty gif_cache, and every one runs decode_composed_frames independently. Unlike icons and videos, GIFs are NOT preloaded (preload.rs only walks Component::Icon and Component::Video), so the first decode genuinely happens inside the parallel section. Each decode is itself unbounded: decode_composed_frames pushes one full-canvas RGBA copy per GIF frame with no cap (gif.rs:141). A 600-frame 800x600 GIF is 1.15 GB per decode; on a 16-core machine (batch_size 32) that is ~37 GB attempted concurrently, then 31 of the 32 results are thrown away when insert overwrites. The process OOM-kills before frame 33. Fix: Decode GIFs in the preload pass alongside icons and videos (preload.rs), so the parallel section only ever hits a warm cache. If lazy decode must stay, gate it on a per-key single-flight primitive (e.g. DashMap>> — insert the empty cell under the shard lock, then let exactly one worker fill it) and cap total decoded frames per source. Refs #220 --- crates/rustmotion-components/src/gif.rs | 112 +++++++++++++++++++++--- 1 file changed, 101 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index e1738e04..529624ca 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -86,6 +86,22 @@ fn clear_rect(composed: &mut [u8], canvas_w: u32, canvas_h: u32, frame: &gif::Fr /// `gif_cache` stores. type DecodedGif = (Vec<(Vec, u32, u32)>, Vec, f64); +/// Artificial per-decode stall, settable only from this file's own tests +/// (`DECODE_STALL_MS`) to open a deterministic race window around the +/// cache-miss branch without timing-dependent sleeps sprinkled through the +/// test itself. Zero by default, and compiled out entirely in a non-test +/// build — no production cost. +#[cfg(test)] +static DECODE_STALL_MS: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +#[cfg(test)] +fn stall_decode_for_tests() { + let stall_ms = DECODE_STALL_MS.load(std::sync::atomic::Ordering::SeqCst); + if stall_ms > 0 { + std::thread::sleep(std::time::Duration::from_millis(stall_ms)); + } +} + /// Decode a GIF into full-canvas RGBA frames, their cumulative end times, and /// the total duration. /// @@ -123,6 +139,9 @@ fn decode_composed_frames(src: &str) -> Option { let canvas_w = decoder.width() as u32; let canvas_h = decoder.height() as u32; + #[cfg(test)] + stall_decode_for_tests(); + let mut frames: Vec<(Vec, u32, u32)> = Vec::new(); let mut cumulative_times: Vec = Vec::new(); let mut accumulated = 0.0; @@ -163,6 +182,21 @@ fn decode_composed_frames(src: &str) -> Option { Some((frames, cumulative_times, accumulated)) } +/// Cache lookup with the actual decode folded in, single-flighted through +/// `DashMap::entry`: a vacant entry holds its shard's write lock for as long +/// as the closure runs, so a second caller racing the same cache-cold `src` +/// blocks on that lock instead of starting its own redundant decode. Decode +/// failure (`decode_composed_frames` returning `None`) leaves the entry +/// vacant — `or_try_insert_with` never calls `insert` on its `Err` path — +/// so a broken source is retried rather than permanently cached as absent. +fn cached_decode(src: &str) -> Option> { + gif_cache() + .entry(src.to_string()) + .or_try_insert_with(|| decode_composed_frames(src).map(Arc::new).ok_or(())) + .ok() + .map(|entry| entry.clone()) +} + impl Painter for Gif { fn paint_content( &self, @@ -171,17 +205,8 @@ impl Painter for Gif { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let gcache = gif_cache(); - - let cached = if let Some(cached) = gcache.get(&self.src) { - cached.clone() - } else { - let Some(decoded) = decode_composed_frames(&self.src) else { - return; - }; - let cached = Arc::new(decoded); - gcache.insert(self.src.clone(), cached.clone()); - cached + let Some(cached) = cached_decode(&self.src) else { + return; }; let (ref frames, ref cumulative_times, total_duration) = *cached; @@ -302,4 +327,69 @@ mod tests { frame.top = 3; assert_eq!(frame_rect(4, 4, &frame), (3, 3, 1, 1)); } + + /// Releases `DECODE_STALL_MS` back to zero even if the test body + /// panics mid-assertion, so a failing run never leaks a stall into + /// whatever other test in this binary decodes a GIF next. + struct StallGuard; + + impl Drop for StallGuard { + fn drop(&mut self) { + DECODE_STALL_MS.store(0, std::sync::atomic::Ordering::SeqCst); + } + } + + /// N callers racing a cache-cold `src` must decode it once, not N + /// times. A 120ms stall (`DECODE_STALL_MS`) right after the header is + /// read opens a race window wide enough that every thread reaches the + /// cache-miss branch before any of them can finish decoding and insert — + /// pre-fix, that means eight independent decodes, each producing its own + /// `Arc` allocation. Comparing pointers (not content — a deterministic + /// decode produces byte-identical content either way) is what proves + /// only one of the eight actually ran. + #[test] + fn concurrent_paints_of_the_same_uncached_gif_decode_exactly_once() { + let _guard = StallGuard; + DECODE_STALL_MS.store(120, std::sync::atomic::Ordering::SeqCst); + + let path = std::env::temp_dir().join(format!( + "rustmotion_gif_stampede_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + write_two_frame_gif(&path); + let src = path.to_str().expect("utf-8 path").to_string(); + + const THREADS: usize = 8; + let barrier = std::sync::Arc::new(std::sync::Barrier::new(THREADS)); + let handles: Vec<_> = (0..THREADS) + .map(|_| { + let barrier = barrier.clone(); + let src = src.clone(); + std::thread::spawn(move || { + barrier.wait(); + cached_decode(&src) + }) + }) + .collect(); + + let results: Vec>> = + handles.into_iter().map(|h| h.join().unwrap()).collect(); + std::fs::remove_file(&path).ok(); + + let first = results[0].as_ref().expect("gif must decode"); + for (i, result) in results.iter().enumerate() { + let result = result.as_ref().unwrap_or_else(|| { + panic!("thread {i} did not get a decoded result"); + }); + assert!( + Arc::ptr_eq(first, result), + "thread {i} observed a different Arc than thread 0 — the GIF was decoded \ + more than once for the same cache-cold source" + ); + } + } }