From a711c7aa999e7966b069f05dad86f29c240e4aed Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 8 May 2026 11:13:49 -0700 Subject: [PATCH 1/3] perf(picker): refresh stale Log preview cache in background after disk hit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Log preview's cached `raw_log` bakes in `git log --format=%C(auto)%d` ref decorations at write time. The cache key is `(branch_head_sha, w, h)`, so another ref starting or stopping pointing at the cached SHA — most commonly a squash-merge landing `main` at the cached commit — leaves the decoration text stale even though the key is still valid. Mitigate by enqueuing a background refresh on every disk-cache hit. A dedicated `picker-preview-refresh` thread drains an mpsc channel of refresh tasks serially, calling `compute_log_raw_and_stats` and overwriting both the disk file and the in-memory `PreviewCache` DashMap entry. Skim re-invokes the preview callback on every item-change, so a user navigating row A → B → A within a session sees the refreshed preview on return; a fresh `wt switch` always reads the disk-refreshed content. The refresh thread runs separately from the rayon precompute pool so foreground first-paint work never competes with refreshes for worker slots. `wait_for_idle` covers refresh tasks via the existing `PendingGuard` pattern: producer increments `pending` before sending, consumer decrements via guard drop after running. Co-Authored-By: Claude --- src/commands/picker/items.rs | 64 +++++-- src/commands/picker/preview_cache.rs | 13 ++ src/commands/picker/preview_orchestrator.rs | 177 +++++++++++++++++++- 3 files changed, 241 insertions(+), 13 deletions(-) diff --git a/src/commands/picker/items.rs b/src/commands/picker/items.rs index 2743531198..57d88bf410 100644 --- a/src/commands/picker/items.rs +++ b/src/commands/picker/items.rs @@ -268,7 +268,7 @@ impl WorktreeSkimItem { ) -> String { match mode { PreviewMode::WorkingTree => Self::compute_working_tree_preview(repo, item, width), - PreviewMode::Log => Self::compute_log_preview(repo, item, width, height), + PreviewMode::Log => Self::compute_log_preview(repo, item, width, height).0, PreviewMode::BranchDiff => Self::compute_branch_diff_preview(repo, item, width), PreviewMode::UpstreamDiff => Self::compute_upstream_diff_preview(repo, item, width), PreviewMode::Summary => Self::loading_placeholder(PreviewMode::Summary), @@ -442,12 +442,40 @@ impl WorktreeSkimItem { /// key out of `main`'s SHA — a `git fetch` advancing `origin/main` /// doesn't invalidate any entry — while preserving correctness as /// `main` and wall-clock advance. + /// Render the Log preview and report whether the disk cache was hit. + /// The orchestrator uses the flag to schedule a background refresh — + /// see [`Self::refresh_log_preview`] and the `LogCacheEntry` + /// docstring for why decorations baked into `raw_log` need + /// refreshing even though the cache key is correct. pub(super) fn compute_log_preview( repo: &Repository, item: &ListItem, width: usize, height: usize, + ) -> (String, bool) { + Self::compute_log_preview_inner(repo, item, width, height, false) + } + + /// Force-recompute the Log preview, bypassing the disk-cache read but + /// writing through. Returns the freshly rendered string. Called by the + /// orchestrator's refresh worker after a cache hit so the next visit + /// sees decorations that match current ref topology. + pub(super) fn refresh_log_preview( + repo: &Repository, + item: &ListItem, + width: usize, + height: usize, ) -> String { + Self::compute_log_preview_inner(repo, item, width, height, true).0 + } + + fn compute_log_preview_inner( + repo: &Repository, + item: &ListItem, + width: usize, + height: usize, + force_recompute: bool, + ) -> (String, bool) { // Minimum preview width to show timestamps (adds ~7 chars: space + 4-char time + space) // Note: preview is typically 50% of terminal width, so 50 = 100-col terminal const TIMESTAMP_WIDTH_THRESHOLD: usize = 50; @@ -461,7 +489,10 @@ impl WorktreeSkimItem { let branch = item.branch_name(); let reset = Reset; let Some(default_branch) = repo.default_branch() else { - return cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no commits\n"); + return ( + cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no commits\n"), + false, + ); }; // merge-base / rev-list run on every call — they're how the @@ -473,7 +504,10 @@ impl WorktreeSkimItem { // the preview is supplementary, users can still select worktrees // even if a probe fails. let Ok(merge_base_output) = repo.run_command(&["merge-base", &default_branch, head]) else { - return cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no commits\n"); + return ( + cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no commits\n"), + false, + ); }; let merge_base = merge_base_output.trim(); let is_default_branch = branch == default_branch; @@ -500,15 +534,22 @@ impl WorktreeSkimItem { // Cacheable: the raw `git log --graph` output plus per-commit // stats. Both are pure functions of (head, width, height); on a // disk-cache hit we skip the `git log` and `git diff-tree` calls - // entirely. On miss we compute and write through. - let entry = match preview_cache::read_log(repo, head, width, height) { + // entirely. On miss we compute and write through. `force_recompute` + // bypasses the read (the refresh path) but always writes. + let cached = if force_recompute { + None + } else { + preview_cache::read_log(repo, head, width, height) + }; + let was_disk_hit = cached.is_some(); + let entry = match cached { Some(cached) => cached, None => { let Some(fresh) = Self::compute_log_raw_and_stats(repo, head, log_limit, show_timestamps) else { // Match prior behavior: empty output on `git log` failure. - return String::new(); + return (String::new(), false); }; preview_cache::write_log(repo, head, width, height, &fresh); fresh @@ -517,7 +558,7 @@ impl WorktreeSkimItem { let (processed, _hashes) = process_log_with_dimming(&entry.raw_log, unique_commits.as_ref()); - if show_timestamps { + let rendered = if show_timestamps { // `format_log_output` reads `epoch_now()` so relative-time // strings ("5m" / "2h" / "3d") track wall-clock on every call, // even when serving from cache. @@ -525,7 +566,8 @@ impl WorktreeSkimItem { } else { // Strip hash markers (SOH...NUL) since we're not using format_log_output strip_hash_markers(&processed) - } + }; + (rendered, was_disk_hit) } /// Run `git log --graph` and (when timestamps are shown) `batch_fetch_stats`, @@ -784,7 +826,7 @@ mod tests { let item = item_at(&repo, "feature"); assert!(super::preview_cache::read_log(&repo, item.head(), 80, 24).is_none()); - let _ = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24); + let _ = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24).0; let entry = super::preview_cache::read_log(&repo, item.head(), 80, 24) .expect("cache populated after first compute"); assert!( @@ -829,7 +871,7 @@ mod tests { // which removes that escape. The dim SGR `\x1b[2m` is unsuitable // because `format_log_output` already wraps every relative-time // column in dim, so it appears even in bright lines. - let before = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24); + let before = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24).0; let before_subject_line = before .lines() .find(|l| l.contains("feature commit")) @@ -847,7 +889,7 @@ mod tests { .unwrap(); repo.run_command(&["checkout", "feature"]).unwrap(); - let after = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24); + let after = WorktreeSkimItem::compute_log_preview(&repo, &item, 80, 24).0; let after_subject_line = after .lines() .find(|l| l.contains("feature commit")) diff --git a/src/commands/picker/preview_cache.rs b/src/commands/picker/preview_cache.rs index bc1109ef38..a40423ac25 100644 --- a/src/commands/picker/preview_cache.rs +++ b/src/commands/picker/preview_cache.rs @@ -49,6 +49,19 @@ const KIND: &str = "picker-preview"; /// `batch_fetch_stats`. The render path re-runs `process_log_with_dimming` /// against fresh `unique_commits` and `format_log_output` against /// `epoch_now()`, so output stays correct as `main` and wall-clock advance. +/// +/// `raw_log` also embeds `git log --format=%C(auto)%d` ref decorations +/// (e.g. `HEAD -> feature, main`), which are *not* SHA-deterministic — +/// another ref starting or stopping pointing at the same commit (most +/// commonly: a squash merge landing `main` at the cached SHA) would +/// leave the decoration text stale even though the cache key is still +/// valid. The orchestrator mitigates this with a background refresh: +/// every disk-cache hit on the Log mode enqueues a task on a dedicated +/// thread that re-runs `compute_log_raw_and_stats`, overwrites this +/// entry, and updates the in-memory `PreviewCache`. The cached entry +/// served on the *current* render is still potentially stale — refresh +/// is async — but the next visit to the same row reads fresh content. +/// See `commands::picker::preview_orchestrator::spawn_refresh_thread`. #[derive(Serialize, Deserialize)] pub(super) struct LogCacheEntry { pub raw_log: String, diff --git a/src/commands/picker/preview_orchestrator.rs b/src/commands/picker/preview_orchestrator.rs index 78dc667552..579bf1d9ba 100644 --- a/src/commands/picker/preview_orchestrator.rs +++ b/src/commands/picker/preview_orchestrator.rs @@ -8,6 +8,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::mpsc; use std::time::Duration; use dashmap::DashMap; @@ -26,10 +27,29 @@ impl Drop for PendingGuard { } } +/// Work item for the dedicated refresh thread. Carries everything the +/// thread needs to recompute a Log preview and overwrite both caches — +/// no shared orchestrator handle, so the thread can outlive any +/// borrow. +struct RefreshTask { + item: Arc, + dims: (usize, usize), + cache: PreviewCache, + repo: Repository, +} + pub(super) struct PreviewOrchestrator { pub(super) cache: PreviewCache, pool: Arc, pending: Arc, + /// Refresh queue. Sender is cloned into rayon worker closures; the + /// receiver is owned by a single std::thread spawned in `new`. Tasks + /// land here when a Log preview compute hit the disk cache so the + /// `raw_log`'s embedded ref decorations get refreshed (see the + /// `LogCacheEntry` docstring). Serial drain on a thread separate from + /// the rayon pool means refreshes never compete with foreground + /// precompute for worker slots. + refresh_tx: mpsc::Sender, /// Repository used by preview compute. Captured once at construction /// so background tasks see a stable repo binding, and so unit tests /// can inject a `TestRepo`-rooted `Repository` instead of relying on @@ -47,10 +67,13 @@ impl PreviewOrchestrator { .build() .expect("failed to build picker preview rayon pool"), ); + let pending = Arc::new(AtomicUsize::new(0)); + let refresh_tx = spawn_refresh_thread(Arc::clone(&pending)); Self { cache, pool, - pending: Arc::new(AtomicUsize::new(0)), + pending, + refresh_tx, repo, } } @@ -62,6 +85,12 @@ impl PreviewOrchestrator { /// happens outside any DashMap lock so skim's UI thread (which calls /// `preview()` synchronously and reads via `DashMap::get`) is never /// blocked on a shard write held across git/pager subprocesses. + /// + /// Log mode that hits the disk cache also enqueues a refresh task on + /// the dedicated refresh thread so the `raw_log`'s embedded + /// decorations are recomputed before the next visit. See the + /// `LogCacheEntry` docstring for why the disk cache itself is + /// SHA-keyed but decoration text drifts. pub(super) fn spawn_preview( &self, item: Arc, @@ -71,13 +100,45 @@ impl PreviewOrchestrator { let cache = Arc::clone(&self.cache); let (w, h) = dims; let repo = self.repo.clone(); + let refresh_tx = self.refresh_tx.clone(); + let pending = Arc::clone(&self.pending); self.spawn_task(move || { let cache_key = (item.branch_name().to_string(), mode); if cache.contains_key(&cache_key) { return; } - let value = WorktreeSkimItem::compute_and_page_preview(&repo, &item, mode, w, h); + let (value, log_disk_hit) = match mode { + // Log doesn't go through the diff pager (see + // `compute_and_page_preview`), so calling the + // status-reporting variant directly here is equivalent + // to going through the dispatcher. + PreviewMode::Log => WorktreeSkimItem::compute_log_preview(&repo, &item, w, h), + _ => ( + WorktreeSkimItem::compute_and_page_preview(&repo, &item, mode, w, h), + false, + ), + }; cache.insert(cache_key, value); + if log_disk_hit { + // Increment pending *before* handing off so `wait_for_idle` + // can't race past while the task is in flight on the + // channel — the refresh thread decrements via + // `PendingGuard` after running the task. + pending.fetch_add(1, Ordering::SeqCst); + let task = RefreshTask { + item: Arc::clone(&item), + dims: (w, h), + cache: Arc::clone(&cache), + repo: repo.clone(), + }; + if refresh_tx.send(task).is_err() { + // Refresh thread already exited (orchestrator dropped). + // Roll back the pending bump we just added so + // `wait_for_idle` doesn't hang forever waiting for a + // task that will never run. + pending.fetch_sub(1, Ordering::SeqCst); + } + } }); } @@ -188,6 +249,45 @@ impl PreviewOrchestrator { } } +/// Spawn the dedicated refresh worker thread. +/// +/// Lifecycle: lives as long as any clone of the returned sender. The +/// orchestrator holds one sender plus clones one into every rayon +/// closure that might enqueue a refresh; when the orchestrator is +/// dropped and all in-flight rayon closures finish, the channel closes +/// and `recv()` returns `Err`, ending the loop. +/// +/// Why a separate thread instead of submitting refreshes back into the +/// rayon pool: rayon has no priority — a refresh task and a +/// foreground-precompute task on the same pool compete for the same +/// worker slots, so refreshes can delay first-paint. A serial +/// dedicated thread guarantees refreshes never preempt rayon workers. +fn spawn_refresh_thread(pending: Arc) -> mpsc::Sender { + let (tx, rx) = mpsc::channel::(); + std::thread::Builder::new() + .name("picker-preview-refresh".into()) + .spawn(move || { + while let Ok(task) = rx.recv() { + // PendingGuard pairs with the producer's `pending.fetch_add(1)` + // before sending, so a panic inside the task still + // releases the counter and `wait_for_idle` doesn't hang. + let _guard = PendingGuard(Arc::clone(&pending)); + let RefreshTask { + item, + dims: (w, h), + cache, + repo, + } = task; + let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let rendered = WorktreeSkimItem::refresh_log_preview(&repo, &item, w, h); + cache.insert((item.branch_name().to_string(), PreviewMode::Log), rendered); + })); + } + }) + .expect("spawn picker-preview-refresh thread"); + tx +} + #[cfg(test)] mod tests { use super::*; @@ -293,6 +393,79 @@ mod tests { ); } + /// Disk-cache hit on a Log preview enqueues a background refresh that + /// overwrites both the disk file and the in-memory DashMap. Seed the + /// disk cache with a stale `LogCacheEntry` containing a marker — + /// after `spawn_preview` + `wait_for_idle`, neither cache should + /// hold the marker, because the refresh thread re-ran + /// `compute_log_raw_and_stats` and wrote real git-log output. + /// + /// `wait_for_idle` covers the refresh thread's task because the + /// producer increments `pending` before sending and the refresh + /// thread decrements via `PendingGuard` after running. + #[test] + fn log_disk_hit_triggers_background_refresh() { + let (t, item) = dirty_worktree_item(); + let repo = Repository::at(t.path()).unwrap(); + + let stale = super::super::preview_cache::LogCacheEntry { + raw_log: "STALE_MARKER\n".to_string(), + stats: std::collections::HashMap::new(), + }; + super::super::preview_cache::write_log(&repo, item.head(), 80, 24, &stale); + + let orch = orch_for(&t); + orch.spawn_preview(Arc::clone(&item), PreviewMode::Log, (80, 24)); + orch.wait_for_idle(); + + let disk = super::super::preview_cache::read_log(&repo, item.head(), 80, 24) + .expect("disk cache present after refresh"); + assert!( + !disk.raw_log.contains("STALE_MARKER"), + "refresh should overwrite stale disk entry, got raw_log: {:?}", + disk.raw_log + ); + + let in_memory = orch + .cache + .get(&("main".to_string(), PreviewMode::Log)) + .expect("in-memory entry present") + .clone(); + assert!( + !in_memory.contains("STALE_MARKER"), + "refresh should overwrite stale in-memory entry, got: {in_memory:?}" + ); + } + + /// Non-Log modes have content-addressed cache keys (BranchDiff is + /// `(base_sha, branch_sha, w)`, UpstreamDiff similar) and no + /// decoration drift, so a disk-cache hit on those modes must NOT + /// enqueue a Log refresh. Seed the disk Log cache with stale content + /// and spawn a BranchDiff preview — the disk Log cache must remain + /// stale because the refresh thread never received a task. + #[test] + fn non_log_modes_do_not_trigger_log_refresh() { + let (t, item) = dirty_worktree_item(); + let repo = Repository::at(t.path()).unwrap(); + + let stale = super::super::preview_cache::LogCacheEntry { + raw_log: "STALE_MARKER\n".to_string(), + stats: std::collections::HashMap::new(), + }; + super::super::preview_cache::write_log(&repo, item.head(), 80, 24, &stale); + + let orch = orch_for(&t); + orch.spawn_preview(Arc::clone(&item), PreviewMode::BranchDiff, (80, 24)); + orch.wait_for_idle(); + + let disk = super::super::preview_cache::read_log(&repo, item.head(), 80, 24) + .expect("disk Log cache untouched"); + assert_eq!( + disk.raw_log, "STALE_MARKER\n", + "non-Log spawn must not trigger Log refresh" + ); + } + #[test] fn dump_cache_json_format() { let t = TestRepo::new(); From ebdcfb91ea2bfd3440ee84b151c0694e020fb903 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 8 May 2026 11:40:58 -0700 Subject: [PATCH 2/3] fix(picker): address auto-reviewer findings on background-refresh PR Three changes prompted by review on #2646: - Inline `compute_preview` into `compute_and_page_preview`. The dispatcher's `Log` arm became dead after the orchestrator started bypassing it directly, so `compute_and_page_preview` now returns `(String, bool)` and the orchestrator no longer special-cases Log. Removes the dead arm flagged by codecov at items.rs:271. - Replace the `let-else return (String::new(), false)` on `git log` failure with `unwrap_or_default()`, requiring `LogCacheEntry: Default`. The empty-entry path now flows through the same render pipeline, producing the same effective output without a separate untestable defensive return (codecov gap at items.rs:552). - Refresh thread skips `cache.insert` when `refresh_log_preview` returns an empty string, so a transient `git log` failure during refresh can't poison the in-memory cache with `""` and wipe out the value the producer just inserted. Also drops an unreachable channel-send-error rollback in `spawn_preview`: the refresh thread holds the receiver for the orchestrator's full lifetime and `&self` proves the orchestrator is alive, so the send always succeeds. `expect()` makes the invariant explicit. Co-Authored-By: Claude --- src/commands/picker/items.rs | 76 ++++++++++----------- src/commands/picker/preview_cache.rs | 2 +- src/commands/picker/preview_orchestrator.rs | 35 ++++------ 3 files changed, 54 insertions(+), 59 deletions(-) diff --git a/src/commands/picker/items.rs b/src/commands/picker/items.rs index 57d88bf410..216345a734 100644 --- a/src/commands/picker/items.rs +++ b/src/commands/picker/items.rs @@ -233,7 +233,10 @@ impl WorktreeSkimItem { format!("○ {verb} {label}. Press {key} again to refresh.\n") } - /// Compute preview and apply pager for diff modes. + /// Compute preview and apply pager for diff modes. Returns the + /// display-ready string and (for Log) whether the disk cache was a + /// hit — the orchestrator uses the flag to schedule a background + /// refresh. /// /// Both the inline cache-miss path and background pre-computation use this so /// that the cache always stores display-ready content (no pager subprocess @@ -244,34 +247,33 @@ impl WorktreeSkimItem { mode: PreviewMode, width: usize, height: usize, - ) -> String { - let content = Self::compute_preview(repo, item, mode, width, height); + ) -> (String, bool) { match mode { - PreviewMode::WorkingTree | PreviewMode::BranchDiff | PreviewMode::UpstreamDiff => { - if let Some(pager_cmd) = diff_pager() { - pipe_through_pager(&content, pager_cmd, width) - } else { - content - } - } - _ => content, + PreviewMode::WorkingTree => ( + Self::page_diff(Self::compute_working_tree_preview(repo, item, width), width), + false, + ), + PreviewMode::Log => Self::compute_log_preview(repo, item, width, height), + PreviewMode::BranchDiff => ( + Self::page_diff(Self::compute_branch_diff_preview(repo, item, width), width), + false, + ), + PreviewMode::UpstreamDiff => ( + Self::page_diff( + Self::compute_upstream_diff_preview(repo, item, width), + width, + ), + false, + ), + PreviewMode::Summary => (Self::loading_placeholder(PreviewMode::Summary), false), } } - /// Compute raw preview for any mode. - pub(super) fn compute_preview( - repo: &Repository, - item: &ListItem, - mode: PreviewMode, - width: usize, - height: usize, - ) -> String { - match mode { - PreviewMode::WorkingTree => Self::compute_working_tree_preview(repo, item, width), - PreviewMode::Log => Self::compute_log_preview(repo, item, width, height).0, - PreviewMode::BranchDiff => Self::compute_branch_diff_preview(repo, item, width), - PreviewMode::UpstreamDiff => Self::compute_upstream_diff_preview(repo, item, width), - PreviewMode::Summary => Self::loading_placeholder(PreviewMode::Summary), + fn page_diff(content: String, width: usize) -> String { + if let Some(pager_cmd) = diff_pager() { + pipe_through_pager(&content, pager_cmd, width) + } else { + content } } @@ -542,19 +544,17 @@ impl WorktreeSkimItem { preview_cache::read_log(repo, head, width, height) }; let was_disk_hit = cached.is_some(); - let entry = match cached { - Some(cached) => cached, - None => { - let Some(fresh) = - Self::compute_log_raw_and_stats(repo, head, log_limit, show_timestamps) - else { - // Match prior behavior: empty output on `git log` failure. - return (String::new(), false); - }; - preview_cache::write_log(repo, head, width, height, &fresh); - fresh - } - }; + // On `git log` failure (effectively unreachable here — merge-base + // already validated `head` upstream), fall back to a default + // entry. `process_log_with_dimming` + `format_log_output` handle + // the empty `raw_log` gracefully and produce empty output, the + // same effective behavior as the prior explicit early return. + let entry = cached.unwrap_or_else(|| { + let fresh = Self::compute_log_raw_and_stats(repo, head, log_limit, show_timestamps) + .unwrap_or_default(); + preview_cache::write_log(repo, head, width, height, &fresh); + fresh + }); let (processed, _hashes) = process_log_with_dimming(&entry.raw_log, unique_commits.as_ref()); diff --git a/src/commands/picker/preview_cache.rs b/src/commands/picker/preview_cache.rs index a40423ac25..1ad400c9b3 100644 --- a/src/commands/picker/preview_cache.rs +++ b/src/commands/picker/preview_cache.rs @@ -62,7 +62,7 @@ const KIND: &str = "picker-preview"; /// served on the *current* render is still potentially stale — refresh /// is async — but the next visit to the same row reads fresh content. /// See `commands::picker::preview_orchestrator::spawn_refresh_thread`. -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Default)] pub(super) struct LogCacheEntry { pub raw_log: String, /// Empty when `width < TIMESTAMP_WIDTH_THRESHOLD` (the no-timestamp diff --git a/src/commands/picker/preview_orchestrator.rs b/src/commands/picker/preview_orchestrator.rs index 579bf1d9ba..a3b4a8a794 100644 --- a/src/commands/picker/preview_orchestrator.rs +++ b/src/commands/picker/preview_orchestrator.rs @@ -107,23 +107,17 @@ impl PreviewOrchestrator { if cache.contains_key(&cache_key) { return; } - let (value, log_disk_hit) = match mode { - // Log doesn't go through the diff pager (see - // `compute_and_page_preview`), so calling the - // status-reporting variant directly here is equivalent - // to going through the dispatcher. - PreviewMode::Log => WorktreeSkimItem::compute_log_preview(&repo, &item, w, h), - _ => ( - WorktreeSkimItem::compute_and_page_preview(&repo, &item, mode, w, h), - false, - ), - }; + let (value, log_disk_hit) = + WorktreeSkimItem::compute_and_page_preview(&repo, &item, mode, w, h); cache.insert(cache_key, value); if log_disk_hit { // Increment pending *before* handing off so `wait_for_idle` // can't race past while the task is in flight on the // channel — the refresh thread decrements via - // `PendingGuard` after running the task. + // `PendingGuard` after running the task. The send always + // succeeds: the refresh thread holds the receiver for the + // orchestrator's full lifetime, and `&self` here proves + // we're alive. pending.fetch_add(1, Ordering::SeqCst); let task = RefreshTask { item: Arc::clone(&item), @@ -131,13 +125,9 @@ impl PreviewOrchestrator { cache: Arc::clone(&cache), repo: repo.clone(), }; - if refresh_tx.send(task).is_err() { - // Refresh thread already exited (orchestrator dropped). - // Roll back the pending bump we just added so - // `wait_for_idle` doesn't hang forever waiting for a - // task that will never run. - pending.fetch_sub(1, Ordering::SeqCst); - } + refresh_tx + .send(task) + .expect("refresh thread receiver alive for orchestrator lifetime"); } }); } @@ -280,7 +270,12 @@ fn spawn_refresh_thread(pending: Arc) -> mpsc::Sender } = task; let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { let rendered = WorktreeSkimItem::refresh_log_preview(&repo, &item, w, h); - cache.insert((item.branch_name().to_string(), PreviewMode::Log), rendered); + // Skip empty results so a transient `git log` failure + // doesn't poison the in-memory cache with `""` and + // wipe out the value the producer just inserted. + if !rendered.is_empty() { + cache.insert((item.branch_name().to_string(), PreviewMode::Log), rendered); + } })); } }) From c3214313a68f2027dbda60de8fd361419b8d314f Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 8 May 2026 17:17:24 -0700 Subject: [PATCH 3/3] refactor(picker): submit Log refresh into rayon pool, drop dedicated thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the dedicated `picker-preview-refresh` std::thread + mpsc channel with `pool.spawn_fifo` into the existing rayon pool. The refresh closure runs the same `refresh_log_preview` + DashMap insert (with the empty- string guard) that the dedicated thread did, just in a different worker. `spawn_fifo` puts the task on the worker's local FIFO deque so already- submitted foreground precompute drains first. PendingGuard accounting moves inline into the closure so `wait_for_idle` still covers refreshes. Net effect: deletes RefreshTask, spawn_refresh_thread, the mpsc field, the panic-catch wrapper, and the channel-send-error rollback — ~100 LOC removed. The original argument for a separate thread (avoid contention with foreground precompute) doesn't survive scrutiny: refreshes are submitted from inside rayon closures during foreground work, so they land after the submitting task either way. Co-Authored-By: Claude --- src/commands/picker/preview_cache.rs | 14 +-- src/commands/picker/preview_orchestrator.rs | 111 ++++---------------- 2 files changed, 28 insertions(+), 97 deletions(-) diff --git a/src/commands/picker/preview_cache.rs b/src/commands/picker/preview_cache.rs index 1ad400c9b3..7e0b0c2a21 100644 --- a/src/commands/picker/preview_cache.rs +++ b/src/commands/picker/preview_cache.rs @@ -55,13 +55,13 @@ const KIND: &str = "picker-preview"; /// another ref starting or stopping pointing at the same commit (most /// commonly: a squash merge landing `main` at the cached SHA) would /// leave the decoration text stale even though the cache key is still -/// valid. The orchestrator mitigates this with a background refresh: -/// every disk-cache hit on the Log mode enqueues a task on a dedicated -/// thread that re-runs `compute_log_raw_and_stats`, overwrites this -/// entry, and updates the in-memory `PreviewCache`. The cached entry -/// served on the *current* render is still potentially stale — refresh -/// is async — but the next visit to the same row reads fresh content. -/// See `commands::picker::preview_orchestrator::spawn_refresh_thread`. +/// valid. The orchestrator mitigates this by enqueuing a background +/// refresh task on the picker's rayon pool whenever a Log preview hit +/// the disk cache; the refresh re-runs `compute_log_raw_and_stats`, +/// overwrites this entry, and updates the in-memory `PreviewCache`. +/// The cached entry served on the *current* render is still potentially +/// stale — refresh is async — but the next visit to the same row reads +/// fresh content. See `commands::picker::preview_orchestrator::spawn_preview`. #[derive(Serialize, Deserialize, Default)] pub(super) struct LogCacheEntry { pub raw_log: String, diff --git a/src/commands/picker/preview_orchestrator.rs b/src/commands/picker/preview_orchestrator.rs index a3b4a8a794..090cfcec44 100644 --- a/src/commands/picker/preview_orchestrator.rs +++ b/src/commands/picker/preview_orchestrator.rs @@ -8,7 +8,6 @@ use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::mpsc; use std::time::Duration; use dashmap::DashMap; @@ -27,29 +26,10 @@ impl Drop for PendingGuard { } } -/// Work item for the dedicated refresh thread. Carries everything the -/// thread needs to recompute a Log preview and overwrite both caches — -/// no shared orchestrator handle, so the thread can outlive any -/// borrow. -struct RefreshTask { - item: Arc, - dims: (usize, usize), - cache: PreviewCache, - repo: Repository, -} - pub(super) struct PreviewOrchestrator { pub(super) cache: PreviewCache, pool: Arc, pending: Arc, - /// Refresh queue. Sender is cloned into rayon worker closures; the - /// receiver is owned by a single std::thread spawned in `new`. Tasks - /// land here when a Log preview compute hit the disk cache so the - /// `raw_log`'s embedded ref decorations get refreshed (see the - /// `LogCacheEntry` docstring). Serial drain on a thread separate from - /// the rayon pool means refreshes never compete with foreground - /// precompute for worker slots. - refresh_tx: mpsc::Sender, /// Repository used by preview compute. Captured once at construction /// so background tasks see a stable repo binding, and so unit tests /// can inject a `TestRepo`-rooted `Repository` instead of relying on @@ -67,13 +47,10 @@ impl PreviewOrchestrator { .build() .expect("failed to build picker preview rayon pool"), ); - let pending = Arc::new(AtomicUsize::new(0)); - let refresh_tx = spawn_refresh_thread(Arc::clone(&pending)); Self { cache, pool, - pending, - refresh_tx, + pending: Arc::new(AtomicUsize::new(0)), repo, } } @@ -86,11 +63,11 @@ impl PreviewOrchestrator { /// `preview()` synchronously and reads via `DashMap::get`) is never /// blocked on a shard write held across git/pager subprocesses. /// - /// Log mode that hits the disk cache also enqueues a refresh task on - /// the dedicated refresh thread so the `raw_log`'s embedded - /// decorations are recomputed before the next visit. See the - /// `LogCacheEntry` docstring for why the disk cache itself is - /// SHA-keyed but decoration text drifts. + /// Log mode that hits the disk cache also enqueues a refresh task + /// (via `pool.spawn_fifo`, so it lands behind in-flight foreground + /// precompute) to recompute the embedded ref decorations before the + /// next visit. See the `LogCacheEntry` docstring for why the disk + /// cache itself is SHA-keyed but decoration text drifts. pub(super) fn spawn_preview( &self, item: Arc, @@ -100,7 +77,7 @@ impl PreviewOrchestrator { let cache = Arc::clone(&self.cache); let (w, h) = dims; let repo = self.repo.clone(); - let refresh_tx = self.refresh_tx.clone(); + let pool = Arc::clone(&self.pool); let pending = Arc::clone(&self.pending); self.spawn_task(move || { let cache_key = (item.branch_name().to_string(), mode); @@ -111,23 +88,21 @@ impl PreviewOrchestrator { WorktreeSkimItem::compute_and_page_preview(&repo, &item, mode, w, h); cache.insert(cache_key, value); if log_disk_hit { - // Increment pending *before* handing off so `wait_for_idle` - // can't race past while the task is in flight on the - // channel — the refresh thread decrements via - // `PendingGuard` after running the task. The send always - // succeeds: the refresh thread holds the receiver for the - // orchestrator's full lifetime, and `&self` here proves - // we're alive. pending.fetch_add(1, Ordering::SeqCst); - let task = RefreshTask { - item: Arc::clone(&item), - dims: (w, h), - cache: Arc::clone(&cache), - repo: repo.clone(), - }; - refresh_tx - .send(task) - .expect("refresh thread receiver alive for orchestrator lifetime"); + let guard = PendingGuard(Arc::clone(&pending)); + let item = Arc::clone(&item); + let cache = Arc::clone(&cache); + let repo = repo.clone(); + pool.spawn_fifo(move || { + let _g = guard; + let rendered = WorktreeSkimItem::refresh_log_preview(&repo, &item, w, h); + // Skip empty results so a transient `git log` failure + // doesn't poison the in-memory cache with "" and wipe + // out the value the producer just inserted. + if !rendered.is_empty() { + cache.insert((item.branch_name().to_string(), PreviewMode::Log), rendered); + } + }); } }); } @@ -239,50 +214,6 @@ impl PreviewOrchestrator { } } -/// Spawn the dedicated refresh worker thread. -/// -/// Lifecycle: lives as long as any clone of the returned sender. The -/// orchestrator holds one sender plus clones one into every rayon -/// closure that might enqueue a refresh; when the orchestrator is -/// dropped and all in-flight rayon closures finish, the channel closes -/// and `recv()` returns `Err`, ending the loop. -/// -/// Why a separate thread instead of submitting refreshes back into the -/// rayon pool: rayon has no priority — a refresh task and a -/// foreground-precompute task on the same pool compete for the same -/// worker slots, so refreshes can delay first-paint. A serial -/// dedicated thread guarantees refreshes never preempt rayon workers. -fn spawn_refresh_thread(pending: Arc) -> mpsc::Sender { - let (tx, rx) = mpsc::channel::(); - std::thread::Builder::new() - .name("picker-preview-refresh".into()) - .spawn(move || { - while let Ok(task) = rx.recv() { - // PendingGuard pairs with the producer's `pending.fetch_add(1)` - // before sending, so a panic inside the task still - // releases the counter and `wait_for_idle` doesn't hang. - let _guard = PendingGuard(Arc::clone(&pending)); - let RefreshTask { - item, - dims: (w, h), - cache, - repo, - } = task; - let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - let rendered = WorktreeSkimItem::refresh_log_preview(&repo, &item, w, h); - // Skip empty results so a transient `git log` failure - // doesn't poison the in-memory cache with `""` and - // wipe out the value the producer just inserted. - if !rendered.is_empty() { - cache.insert((item.branch_name().to_string(), PreviewMode::Log), rendered); - } - })); - } - }) - .expect("spawn picker-preview-refresh thread"); - tx -} - #[cfg(test)] mod tests { use super::*;