From ebb4023955124cc7defb09bba2db057fddeffd4d Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Tue, 27 Oct 2026 19:38:45 -0400 Subject: [PATCH] Fix scroll-back-to-top shimmer: prepend-on-submit job queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: the worker job queue was FIFO (mpsc). Scrolling down filled it with slow cold decodes of far cells; scrolling back up re-submitted the top cells as fast disk-cache hits that then waited behind the whole backlog — seconds of shimmer. Deterministic repro measured 6.3 s behind a 180-cold backlog (emulator UI showed the same while 220 cold jobs waited 0.5-0.9 s). Fix: job queue is now Mutex + Condvar; Engine::submit prepends each new batch so the most-recent intent (the viewport, or what you scrolled back to) is decoded before stale far-away prefetch. Cold backlog keeps warming the disk cache in the background (total work unchanged). - regression: thumb::tests::scroll_back_cache_hit_not_starved (6.3 s -> 130 ms) - dispatcher exits via a closed flag set in Engine::drop (replaces the mpsc channel-close shutdown) - HANDOFF: M3 fix note, prepend rationale, emulator realtest_extra dataset --- HANDOFF.md | 33 ++++++++--- src/thumb.rs | 156 +++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 170 insertions(+), 19 deletions(-) diff --git a/HANDOFF.md b/HANDOFF.md index c203537..c30895e 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -10,10 +10,13 @@ rediscovered. instant list paint, M2 thumbnail engine (turbojpeg DCT-shrink decode, worker pool, disk cache, cancellation), M3 grid UI (3-column ListView grid, shimmer placeholders, scroll-driven prefetch, memory-bound cell retention) all -verified: 13 unit tests, 5k synthetic engine test, on the emulator (150 -real JPEGs: first paint 0.6 s cold, warm relaunch from disk cache, 0 ANRs -through 16 fast swipes), and on a real Pixel 9 Pro (8,577 photos: grid -renders, scroll = 60 fps with 0 hitches). Next: M4 viewer. +verified: 14 unit tests (incl. the scroll-back FIFO-starvation regression), +5k synthetic engine test, on the emulator (real JPEGs: first paint 0.6 s +cold, warm relaunch from disk cache, 0 ANRs through 16 fast swipes), and on +a real Pixel 9 Pro (8,577 photos: grid renders, scroll = 60 fps with 0 +hitches). The reported scroll-back-to-top shimmer was root-caused (FIFO job +queue starvation) and fixed (prepend-on-submit) + regression-tested. Next: +M4 viewer. --- @@ -93,9 +96,12 @@ cargo run # desktop (Linux needs libfontconfig1+pkg-config) `/Pictures/realtest/` (150, generated with PIL). They were `touch`-ed to a future mtime so they sort to the TOP of the grid — when reasoning about "which cells should have images", remember only cells 0–149 are real; - everything below is stubs and correctly shows shimmer. `adb push` of a - directory nests a subfolder (push files individually or `mv` up); - `adb shell touch -d '…'` works for setting mtimes. The thumb cache is at + everything below is stubs and correctly shows shimmer. **`/Pictures/realtest_extra/` + holds 700 more real, decodable JPEGs (PIL, 1200–4032 px, mtime 2031 so they + sort to the very top, above the 150 in `realtest`) — added for the + scroll-back backlog repro; the real region is now ~850 files (283 rows). + `adb push` of a directory nests a subfolder (push files individually or + `mv` up); `adb shell touch -d '…'` works for setting mtimes. The thumb cache is at `/sdcard/Android/data/wow.gmp.gallery/files/thumbs` (wipe it to force re-decode). - Perf measurement: `adb shell perfetto` / `adb shell dumpsys gfxinfo wow.gmp.gallery`, @@ -407,6 +413,19 @@ pool threads), and `logcat` pool timing logs behind a `debug_timing` flag. (`apply_row_batch`) and are capped at 16 rows/tick to avoid ANR on fast scroll; `on_scroll` only re-anchors the active range + submits prefetch (no model mutation), all mutation is in the 10 Hz drain timer. + + **Scroll-back-to-top shimmer (fixed 2026-10-27, reported from the phone):** + scrolling down fills the worker queue with slow cold decodes of far cells; + scrolling back up re-submits the top cells as fast disk-cache HITS, but on + the old FIFO job queue they waited behind the whole cold backlog — seconds + of shimmer (deterministic repro: 6.3 s behind a 180-cold backlog). Fix: the + job queue is now a `Mutex` + `Condvar` and `Engine::submit` + **prepends** each new batch (most-recent intent first; stale far prefetch + can no longer starve the viewport). Regression test + `thumb::tests::scroll_back_cache_hit_not_starved` (6.3 s → 130 ms). Note + `mpsc` can't do this — it's FIFO-only; a priority queue needs the + mutex+condvar (dispatcher exits via a `closed` flag set in `Engine::drop`). + *Verified (emulator, 150 real JPEGs + 5k stubs): first paint 0.6 s cold; warm relaunch from disk cache; 0 ANRs through 16 fast swipes; backlog drains to 0 when idle. Verified on Pixel 9 Pro (release aarch64, 8,577 diff --git a/src/thumb.rs b/src/thumb.rs index a454026..2cc5535 100644 --- a/src/thumb.rs +++ b/src/thumb.rs @@ -15,12 +15,19 @@ //! valid) but drop the UI delivery. The queue is unbounded on purpose — //! backlog is visible in the timing logs (acceptance: it must not grow while //! scrolling). +//! +//! Queue ordering is PREPEND (not FIFO): `submit` puts each new batch at the +//! FRONT of the queue. The most recently requested content is what the user +//! is looking at (or scrolling back to); letting a backlog of stale far-away +//! cold decodes sit ahead of it starved fast disk-cache-hit re-submissions — +//! the scroll-back-to-top shimmer bug. Regression: `scroll_back_cache_hit_not_starved`. +use std::collections::VecDeque; use std::fs; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; use std::sync::mpsc; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Condvar, Mutex}; use std::time::Instant; use sha1::{Digest, Sha1}; @@ -60,7 +67,15 @@ pub struct JobResult { } pub struct Engine { - tx: mpsc::Sender, + /// Job queue: Mutex + Condvar (not mpsc — mpsc is FIFO-only). + /// NEW submissions are PREPENDED: the most recently requested content is + /// what the user is looking at (or scrolling back to); stale far-away + /// prefetch must not starve it. Concretely fixes the scroll-back-to-top + /// case where fast disk-cache-hit jobs waited seconds behind a backlog of + /// slow cold decodes (regression: `scroll_back_cache_hit_not_starved`). + jobs: Arc>>, + job_cv: Arc, + closed: Arc, /// Only the UI thread drains; the Mutex just makes Engine Sync so it can /// live in a process-wide OnceLock. rx: Mutex>, @@ -73,6 +88,13 @@ pub struct Engine { target: u32, } +impl Drop for Engine { + fn drop(&mut self) { + self.closed.store(true, Ordering::SeqCst); + self.job_cv.notify_all(); + } +} + /// Cache file for a job: `thumbs/{sha1(path|mtime|size|target)}.jpg` — the /// target is in the key so different render sizes coexist (M2 list 64 px, /// M3 grid 500 px). @@ -94,7 +116,9 @@ impl Engine { /// `target` px (longest side). pub fn new(cache_dir: PathBuf, workers: usize, target: u32) -> Result { fs::create_dir_all(&cache_dir).map_err(|e| format!("mkdir {}: {e}", cache_dir.display()))?; - let (job_tx, job_rx) = mpsc::channel::(); + let jobs = Arc::new(Mutex::new(VecDeque::::new())); + let job_cv = Arc::new(Condvar::new()); + let closed = Arc::new(AtomicBool::new(false)); let (res_tx, res_rx) = mpsc::channel::(); let gen = Arc::new(AtomicU64::new(1)); let in_flight = Arc::new(AtomicUsize::new(0)); @@ -120,12 +144,25 @@ impl Engine { } drop(res_tx); - // Dispatcher: single receiver for std::mpsc, round-robins to workers. + // Dispatcher: pops the job queue (front = most recent intent) and + // round-robins to workers. Exits when the engine is dropped and the + // queue has drained. + let (d_jobs, d_cv, d_closed) = (jobs.clone(), job_cv.clone(), closed.clone()); std::thread::Builder::new() .name("thumb-dispatch".into()) .spawn(move || { let mut next = 0usize; - for job in job_rx { + loop { + let job = { + let mut dq = d_jobs.lock().unwrap(); + while dq.is_empty() { + if d_closed.load(Ordering::SeqCst) { + return; // engine dropped, queue drained + } + dq = d_cv.wait(dq).unwrap(); + } + dq.pop_front().unwrap() + }; for _ in 0..workers_tx.len() { if workers_tx[next % workers_tx.len()].send(job.clone()).is_ok() { next += 1; @@ -138,7 +175,9 @@ impl Engine { .map_err(|e| e.to_string())?; Ok(Self { - tx: job_tx, + jobs, + job_cv, + closed, rx: Mutex::new(res_rx), gen, cache_dir, @@ -167,14 +206,21 @@ impl Engine { self.gen.fetch_add(1, Ordering::Relaxed) + 1 } + /// Submit jobs, PREPENDED to the queue: fresh submissions express the + /// user's current (or return) intent and must be decoded before stale + /// far-away prefetch. Within a batch the original order is preserved. pub fn submit(&self, jobs: &[Job]) { - for j in jobs { - self.in_flight.fetch_add(1, Ordering::Relaxed); - if self.tx.send(j.clone()).is_err() { - self.in_flight.fetch_sub(1, Ordering::Relaxed); - break; // engine gone + if jobs.is_empty() { + return; + } + { + let mut dq = self.jobs.lock().unwrap(); + for j in jobs.iter().rev() { + dq.push_front(j.clone()); } } + self.in_flight.fetch_add(jobs.len(), Ordering::Relaxed); + self.job_cv.notify_one(); } /// Non-blocking drain (UI thread). Stale-generation results are dropped. @@ -443,6 +489,92 @@ mod tests { let _ = p; } + /// Scroll-back bug (regression): a user scrolls down (queue fills with + /// slow cold decodes of far cells), then back to the top. The top cells' + /// jobs are fast disk-cache HITS but, on a FIFO queue, they wait behind + /// the entire cold backlog — the user sees shimmer for seconds. + /// Re-submitted (recently requested) work must be decoded first. + #[test] + fn scroll_back_cache_hit_not_starved() { + let dir = tempfile::tempdir().unwrap(); + let cache = dir.path().join("thumbs"); + std::env::set_var("GALLERY_DB_PATH", dir.path().join("gallery.db")); + + // Detailed (noise) JPEGs: non-trivial cold decodes, still small files. + let mk = |name: &str| -> Entry { + let (w, h) = (2048u32, 1536u32); + let mut rgb = Vec::with_capacity(3 * (w * h) as usize); + let mut x: u64 = 0x9e3779b97f4a7c15; + for _ in 0..(w * h) { + x = x + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + rgb.extend_from_slice(&[(x >> 33) as u8, (x >> 17) as u8, (x >> 1) as u8]); + } + let p = dir.path().join(name); + std::fs::write(&p, images::encode_jpeg(&rgb, w, h, 80).unwrap()).unwrap(); + Entry { + path: p.to_str().unwrap().into(), + mtime: 1, + size: std::fs::metadata(&p).unwrap().len() as i64, + } + }; + let top: Vec = (0..60).map(|i| mk(&format!("top{i:03}.jpg"))).collect(); + let far: Vec = (0..180).map(|i| mk(&format!("far{i:03}.jpg"))).collect(); + + let eng = Engine::new(cache.clone(), 4, GRID_TARGET).unwrap(); + let gen = eng.generation(); + let jobs = |es: &[Entry], base: usize| { + es.iter() + .enumerate() + .map(|(i, e)| Job { + id: base + i, + path: e.path.clone(), + mtime: e.mtime, + size: e.size, + gen, + }) + .collect::>() + }; + + // Warm the top 60 into the disk cache (they were painted earlier). + eng.submit(&jobs(&top, 0)); + let mut got = 0; + while got < 60 { + std::thread::sleep(std::time::Duration::from_millis(5)); + got += eng.drain().len(); + } + + // "Scroll down": 180 cold far jobs. "Scroll back up": 60 warm tops. + let t0 = Instant::now(); + eng.submit(&jobs(&far, 1000)); + eng.submit(&jobs(&top, 0)); + let mut tops = 0; + let mut total = 0; + let mut t_tops = None; + while total < 240 { + std::thread::sleep(std::time::Duration::from_millis(2)); + for r in eng.drain() { + total += 1; + if r.id < 60 && t_tops.is_none() { + tops += 1; + if tops == 60 { + t_tops = Some(t0.elapsed()); + } + } + } + } + let t_tops = t_tops.unwrap(); + eprintln!( + "scroll-back: all 60 cache-hit tops in {t_tops:?} behind a 180-cold-job backlog (total {:?})", + t0.elapsed() + ); + assert!( + t_tops.as_secs_f64() < 0.75, + "cache-hit re-entry waited {t_tops:?} behind the cold backlog — FIFO starvation" + ); + } + #[test] fn cache_key_stable_and_distinct() { let d = Path::new("/tmp/x");