From ca6bda717897e5a3ca3d27390cd7dbaa409995c3 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 17:16:15 +0200 Subject: [PATCH 1/6] perf(log): prepare the walk once and page from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The paged log rebuilt its revwalk on every page, and in libgit2 1.9.7 a topologically sorted walk is not incremental in any sense: setting a sort order sets `walk->limited`, so `prepare_walk` runs `limit_list` over the whole reachable graph and `sort_in_topological_order` materialises the COMPLETE ordered list before the first oid comes out. Asking that walk for 500 commits and asking it for all 1.5 million cost the same thing — so one walk per page paid the identical price per page. It showed as a per-page cost flat in depth: on torvalds/linux page one was 15.95 s and page ten 157.67 s, ten times one page. git pays for that sort once and then skips. So the walk is prepared once and its output kept, and every later page is a slice of it. The ref map goes the same way: `collect_ref_map` enumerated and peeled every ref per page to decorate 500 rows, 16x git's own work on a repository with 7,001 refs. **Why:** nothing here can make the walk itself cheaper — the benchmark already ruled out the commit-graph, which libgit2's revwalk does not read (#473, #476). What was available was to stop throwing the finished sort away. Two invalidation stories, and they are different on purpose. A first page is keyed by (refspec, start oids), so any ref that moves is a different key and misses. A continuation is keyed by the frontier it was emitted with and does not consult refs at all — resuming from a cursor never did, and the set a frontier reaches is made of commits, which are immutable. The ref map is keyed by a fingerprint of every ref name and target, so a `git tag` typed in a terminal invalidates it exactly like one made in the app. The cache is a pure accelerator: everything in it is derivable from disk, a poisoned mutex degrades to a miss rather than failing a page, and closing a repository drops it. `log_walk_cache.rs` holds that property down by draining the same history through one backend and through a fresh backend per page and comparing the two, and asserts the counters separately — because every transparency test passes just as well against a cache that never hits. One behaviour is now visible that was always there: when two commits share a second, which lane comes first is not something either walk promises. libgit2 orders the topological queue by time through a binary heap with an unstable comparator, so it depends on insertion order, and a walk restarted from a cursor inserts differently from one that ran straight through. What holds either way — every commit exactly once, no parent before its child — is asserted against a fixture built entirely inside one second. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/git/libgit2.rs | 328 ++++++++++++++++--- src-tauri/src/git/log_cache.rs | 515 ++++++++++++++++++++++++++++++ src-tauri/src/git/mod.rs | 1 + src-tauri/tests/log_walk_cache.rs | 514 +++++++++++++++++++++++++++++ 4 files changed, 1313 insertions(+), 45 deletions(-) create mode 100644 src-tauri/src/git/log_cache.rs create mode 100644 src-tauri/tests/log_walk_cache.rs diff --git a/src-tauri/src/git/libgit2.rs b/src-tauri/src/git/libgit2.rs index 7698121..662768d 100644 --- a/src-tauri/src/git/libgit2.rs +++ b/src-tauri/src/git/libgit2.rs @@ -11,6 +11,7 @@ use uuid::Uuid; use crate::error::{AppError, AppResult}; use crate::git::image; +use crate::git::log_cache::{LogCache, RefMap, WalkKey, WalkOrder, MAX_ORDER}; use crate::git::ownership; use crate::git::repo_locks::RepoLock; use crate::git::shallow as shallow_mod; @@ -82,6 +83,9 @@ pub struct RebaseState { pub struct Libgit2Backend { repos: Mutex>>>, rebases: Mutex>, + /// What the paged log used to rebuild on every page (#473) — the prepared + /// walk and the ref decorations. Pure accelerator: see `git/log_cache.rs`. + log_cache: LogCache, /// Serializes the guard → write → cleanup window in `init` to prevent two /// concurrent calls on the same path from interfering. Without this lock, call /// A's cleanup could delete `.git` that call B just created, since the @@ -94,10 +98,16 @@ impl Libgit2Backend { Self { repos: Mutex::new(HashMap::new()), rebases: Mutex::new(HashMap::new()), + log_cache: LogCache::new(), init_lock: Mutex::new(()), } } + /// What the paged log's cache has done so far — see `CacheStats`. + pub fn log_cache_stats(&self) -> crate::git::log_cache::CacheStats { + self.log_cache.stats() + } + /// Clone the repo's own lock out of the map and RELEASE the map lock /// before running `f`. The map guard used to stay alive for the whole /// operation, so every git op in the process — any repository, any window — @@ -181,6 +191,112 @@ impl Libgit2Backend { self.repo_cell(repo_id)?.shared_mut(f) } + /// The ref decorations a log page stamps on its rows, rebuilt only when a + /// ref has actually moved (#473). + /// + /// `collect_ref_map` used to run on every page: on a repository with 7,001 + /// refs it was 16× git's own work for the same question, and most of a + /// page's cost on a repository whose history is short. The fingerprint is + /// one enumeration with no peeling; the map behind it is shared by `Arc`, + /// so several concurrent pages read one copy. + fn ref_map(&self, repo_id: &RepoId, repo: &Repository) -> Arc { + let fingerprint = ref_fingerprint(repo); + if let Some(map) = self.log_cache.ref_map(repo_id, fingerprint) { + return map; + } + let map = Arc::new(collect_ref_map(repo)); + self.log_cache + .put_ref_map(repo_id, fingerprint, Arc::clone(&map)); + map + } + + /// Which prepared walk answers this page, and where in it the page starts. + /// + /// `Ok(None)` is "nothing to walk" — an unborn HEAD, or a cursor whose + /// every oid is missing from a shallow clone — and the caller returns an + /// empty page for it, exactly as the walk-per-page version did. + fn page_plan( + &self, + repo_id: &RepoId, + repo: &Repository, + refspec: Option<&str>, + cursor: Option<&[String]>, + limit: usize, + ) -> AppResult> { + // A continuation. `refspec` is ignored here, as it always has been: + // the frontier already encodes the walk this page continues. + if let Some(frontier) = cursor.filter(|c| !c.is_empty()) { + let mut want = Vec::with_capacity(frontier.len()); + for raw in frontier { + want.push( + git2::Oid::from_str(raw).map_err(|_| AppError::InvalidRef(raw.clone()))?, + ); + } + let mut sorted = want.clone(); + sorted.sort(); + if let Some((key, order, offset)) = self.log_cache.resume(repo_id, &sorted, limit) { + // Seeded with the cursor rather than the walk's own start + // points: a lane this page stops short of has to survive into + // the NEXT cursor, and at this depth the live lanes are the + // ones the caller just handed back. + let seed = live_commits(repo, &want); + return Ok(Some(PagePlan { + order, + offset, + seed, + key, + })); + } + // A frontier oid can be missing in a shallow clone; skip it rather + // than failing the whole page. + let seed = live_commits(repo, &want); + if seed.is_empty() { + return Ok(None); + } + let key = WalkKey::new(None, &seed); + let order = Arc::new(self.prepare_walk(repo, &seed)?); + self.log_cache.insert(repo_id, key.clone(), Arc::clone(&order)); + return Ok(Some(PagePlan { + order, + offset: 0, + seed, + key, + })); + } + + // A first page. The refs name the walk, which is also what invalidates + // it: a ref that moved is a different set of start oids, so a different + // key, so a miss. + let starts = resolve_log_starts(repo, refspec)?; + if starts.is_empty() { + return Ok(None); + } + let key = WalkKey::new(refspec, &starts); + if let Some(order) = self.log_cache.first_page(repo_id, &key, limit) { + return Ok(Some(PagePlan { + seed: order.starts.clone(), + order, + offset: 0, + key, + })); + } + let order = Arc::new(self.prepare_walk(repo, &starts)?); + self.log_cache.insert(repo_id, key.clone(), Arc::clone(&order)); + Ok(Some(PagePlan { + order, + offset: 0, + seed: starts, + key, + })) + } + + /// `build_walk_order`, counted — the count is what proves the cache above + /// it is doing anything. + fn prepare_walk(&self, repo: &Repository, starts: &[git2::Oid]) -> AppResult { + self.log_cache.record_walk_prepared(); + build_walk_order(repo, starts) + } + /// The in-process blame: libgit2, HEAD, no subprocess. /// /// This is what a repository with no `blame.ignoreRevsFile` gets, and what @@ -2520,28 +2636,21 @@ fn resolve_commit<'a>(repo: &'a Repository, revspec: &str) -> AppResult, -) -> AppResult> { +/// Separate from pushing them onto a walk because the answer is also the +/// walk's IDENTITY: `log_cache` keys a prepared walk by these oids, so a ref +/// that moved is a different walk and misses (#473). Returned in resolution +/// order, deduplicated — the order is observable as the cursor's lane order. +fn resolve_log_starts(repo: &Repository, refspec: Option<&str>) -> AppResult> { match refspec { None => match repo.head() { - Ok(h) => { - let oid = h.peel_to_commit()?.id(); - walk.push(oid)?; - Ok(vec![oid]) - } + Ok(h) => Ok(vec![h.peel_to_commit()?.id()]), // Fresh repo, HEAD points at a branch with no commits yet → nothing // to walk. This is the only "empty log, not an error" case. Err(e) if e.code() == git2::ErrorCode::UnbornBranch => Ok(Vec::new()), @@ -2555,18 +2664,15 @@ fn push_log_start( // would hide the commits the user is actually sitting on). Tags are // deliberately out: this scope is about branches. Some(spec) if spec == REFSPEC_ALL => { - let mut starts = Vec::new(); - let mut push = |oid: git2::Oid, walk: &mut git2::Revwalk| -> AppResult<()> { - if starts.contains(&oid) { - return Ok(()); + let mut starts: Vec = Vec::new(); + let push = |oid: git2::Oid, starts: &mut Vec| { + if !starts.contains(&oid) { + starts.push(oid); } - walk.push(oid)?; - starts.push(oid); - Ok(()) }; if let Ok(head) = repo.head() { if let Ok(commit) = head.peel_to_commit() { - push(commit.id(), walk)?; + push(commit.id(), &mut starts); } } for glob in ["refs/heads/*", "refs/remotes/*/*"] { @@ -2575,23 +2681,107 @@ fn push_log_start( // A head's or remote head's target IS the commit oid — // no peel (an object read per ref, per page) needed. if let Some(oid) = r.target() { - push(oid, walk)?; + push(oid, &mut starts); } else if let Ok(commit) = r.peel_to_commit() { // A remote's symbolic HEAD (refs/remotes/origin/HEAD) // peels to a tip already pushed — dedup handles it. - push(commit.id(), walk)?; + push(commit.id(), &mut starts); } } } } Ok(starts) } - Some(spec) => { - let commit = resolve_commit(repo, spec)?; - walk.push(commit.id())?; - Ok(vec![commit.id()]) + Some(spec) => Ok(vec![resolve_commit(repo, spec)?.id()]), + } +} + +/// `resolve_log_starts`, pushed onto `walk`. Returns the oids actually pushed; +/// empty means "nothing to walk". The caller needs the oids, not just a flag: a +/// pushed start point that the page's limit stops short of has to survive into +/// the next cursor. +fn push_log_start( + repo: &Repository, + walk: &mut git2::Revwalk, + refspec: Option<&str>, +) -> AppResult> { + let starts = resolve_log_starts(repo, refspec)?; + for &oid in &starts { + walk.push(oid)?; + } + Ok(starts) +} + +/// Prepare one walk and keep what it produces (#473). +/// +/// This is the expensive call in the whole file on a large repository, and it +/// costs the same whether the caller wants 500 commits or every one of them: +/// libgit2's topological sort materialises the complete ordered list inside +/// `git_revwalk_next`'s first call. So it is drained rather than sipped, and +/// the result is what every page of this walk is then sliced out of. +/// +/// `MAX_ORDER` bounds the memory; a walk longer than that is kept as a prefix +/// and marked incomplete, which makes a page past the prefix fall back to +/// preparing a walk from its cursor — what every page did before this existed. +fn build_walk_order(repo: &Repository, starts: &[git2::Oid]) -> AppResult { + let mut walk = repo.revwalk()?; + walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; + for &oid in starts { + walk.push(oid)?; + } + let mut order = Vec::new(); + let mut complete = true; + for oid in walk { + if order.len() >= MAX_ORDER { + complete = false; + break; } + order.push(oid?); } + Ok(WalkOrder { + starts: starts.to_vec(), + order, + complete, + }) +} + +/// A fingerprint of the whole ref database: every ref's name and what it points +/// at, combined so that enumeration order cannot change the answer. +/// +/// This is what decides whether the cached ref decorations are still the truth +/// (#473). It reads names and targets and peels NOTHING, which is the entire +/// saving — peeling 500 annotated tags is an object read each, and that is the +/// work `collect_ref_map` does that this skips on a hit. +/// +/// A fingerprint rather than a write hook, because a `git tag` typed in a +/// terminal has to invalidate it exactly like one made in the app; and rather +/// than an mtime, because loose refs live in nested directories where no single +/// timestamp covers them all. +fn ref_fingerprint(repo: &Repository) -> u64 { + use std::hash::{Hash, Hasher}; + let Ok(refs) = repo.references() else { + // Unreadable refdb: a fingerprint nobody can match, so the map is + // rebuilt rather than a stale one being served. + return u64::MAX; + }; + let mut total: u64 = 0; + let mut count: u64 = 0; + for r in refs.flatten() { + let mut h = std::collections::hash_map::DefaultHasher::new(); + r.name_bytes().hash(&mut h); + match r.target() { + Some(oid) => oid.as_bytes().hash(&mut h), + // A symbolic ref (`refs/remotes/origin/HEAD`) has no direct target; + // what it names is what can change under it. + None => r.symbolic_target_bytes().hash(&mut h), + } + // Commutative, so `references()` may hand them over in any order it + // likes; the count is mixed in separately so that a ref whose hash is + // zero still moves the answer when it appears or disappears. + total = total.wrapping_add(h.finish()); + count += 1; + } + total.wrapping_mul(31).wrapping_add(count) } /// Accumulates the walk frontier while a page is emitted (#68 G11). @@ -2647,7 +2837,12 @@ impl FrontierBuilder { /// `None` ⟺ end of history: any parent we saw but did not walk IS more /// history, so an empty frontier means there is nothing left. - fn finish(self, repo: &Repository) -> Option> { + /// + /// Oids rather than strings because the cursor has a second reader now: + /// `log_cache` files the frontier it emitted against the position it + /// resumes at, and that lookup has to be exact (#473). The strings are made + /// at the edge, in `LogPage`. + fn finish(self, repo: &Repository) -> Option> { let mut out = Vec::new(); for p in self.candidates { if self.visited.contains(&p) { @@ -2660,7 +2855,7 @@ impl FrontierBuilder { .map(|odb| odb.exists(p)) .unwrap_or_else(|_| repo.find_commit(p).is_ok()); if exists { - out.push(p.to_string()); + out.push(p); } } if out.is_empty() { @@ -2671,6 +2866,35 @@ impl FrontierBuilder { } } +/// The frontier as it crosses IPC. +fn cursor_strings(frontier: &[git2::Oid]) -> Vec { + frontier.iter().map(git2::Oid::to_string).collect() +} + +/// The oids of `want` this repository actually has, in the order given. +/// +/// A cursor can name a commit a shallow clone does not carry; such a lane is +/// skipped rather than failing the page, which is what `push_page_start` has +/// always done. +fn live_commits(repo: &Repository, want: &[git2::Oid]) -> Vec { + want.iter() + .copied() + .filter(|oid| repo.find_commit(*oid).is_ok()) + .collect() +} + +/// Which prepared walk a page is a slice of, and where the slice starts. +struct PagePlan { + order: Arc, + offset: usize, + /// The live lanes at `offset` — what the frontier builder seeds with, so a + /// lane this page stops short of survives into the next cursor. + seed: Vec, + /// Where the order is filed, so the frontier this page emits can be filed + /// beside it. + key: WalkKey, +} + /// Seed a revwalk for a page: from the cursor frontier when resuming, /// otherwise from `refspec`/HEAD. An empty result means "nothing to walk". /// @@ -3342,6 +3566,10 @@ impl GitBackend for Libgit2Backend { // file handles once any in-flight op's clone finishes. An absent id is // success on purpose (see the trait doc). map.remove(repo_id); + // The log cache IS dropped, unlike `rebases` below: every entry in it + // is a megabytes-sized snapshot of a repository nobody has open any + // more, and all of it is derivable again from disk (#473). + self.log_cache.forget(repo_id); // `rebases` is deliberately left alone: its entries are bytes, not // handles, they are keyed by an id a re-open can never mint again, and // an in-progress rebase rehydrates from `.git/platypusgit-rebase.json` @@ -3589,22 +3817,19 @@ impl GitBackend for Libgit2Backend { limit: usize, ) -> AppResult { self.with_repo_read(repo_id, |repo| { - let ref_map = collect_ref_map(repo); - let mut walk = repo.revwalk()?; - walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; - let starts = push_page_start(repo, &mut walk, refspec, cursor)?; - if starts.is_empty() { + let ref_map = self.ref_map(repo_id, repo); + let Some(plan) = self.page_plan(repo_id, repo, refspec, cursor, limit)? else { return Ok(LogPage { commits: Vec::new(), next_cursor: None, }); - } + }; + let rows = plan.order.order[plan.offset..].iter().take(limit); let mut out = Vec::with_capacity(limit.min(4096)); let mut frontier = FrontierBuilder::new(limit.min(4096)); - frontier.seed(&starts); - for oid in walk.by_ref().take(limit) { - let oid = oid?; + frontier.seed(&plan.seed); + for &oid in rows { let commit = repo.find_commit(oid)?; let refs: Vec = ref_map.get(&oid).cloned().unwrap_or_default(); let mut info = commit_to_info(&commit); @@ -3613,8 +3838,21 @@ impl GitBackend for Libgit2Backend { out.push(info); } + let next = frontier.finish(repo); + // File the frontier against where it continues, so the page after + // this one is a slice of the same prepared walk rather than a + // second one (#473). + if let Some(f) = &next { + self.log_cache.remember_cursor( + repo_id, + &plan.key, + f.clone(), + plan.offset + out.len(), + ); + } + Ok(LogPage { - next_cursor: frontier.finish(repo), + next_cursor: next.as_deref().map(cursor_strings), commits: out, }) }) @@ -3689,7 +3927,7 @@ impl GitBackend for Libgit2Backend { }; self.with_repo(repo_id, |repo| { - let ref_map = collect_ref_map(repo); + let ref_map = self.ref_map(repo_id, repo); let mut walk = repo.revwalk()?; walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; let starts = push_page_start(repo, &mut walk, refspec, cursor)?; @@ -3784,7 +4022,7 @@ impl GitBackend for Libgit2Backend { out.push(info); } Ok(LogPage { - next_cursor: frontier.finish(repo), + next_cursor: frontier.finish(repo).as_deref().map(cursor_strings), commits: out, }) }) @@ -3792,7 +4030,7 @@ impl GitBackend for Libgit2Backend { fn commits_since(&self, repo_id: &RepoId, base: &str) -> AppResult> { self.with_repo(repo_id, |repo| { - let ref_map = collect_ref_map(repo); + let ref_map = self.ref_map(repo_id, repo); let head = match repo.head() { Ok(h) => h.peel_to_commit()?.id(), @@ -3842,7 +4080,7 @@ impl GitBackend for Libgit2Backend { limit: usize, ) -> AppResult> { self.with_repo(repo_id, |repo| { - let ref_map = collect_ref_map(repo); + let ref_map = self.ref_map(repo_id, repo); // `resolve_commit` maps a failure to InvalidRef with the offending // spec, so the UI can name the side the user typed wrong. let base_oid = resolve_commit(repo, base)?.id(); diff --git a/src-tauri/src/git/log_cache.rs b/src-tauri/src/git/log_cache.rs new file mode 100644 index 0000000..2aa0976 --- /dev/null +++ b/src-tauri/src/git/log_cache.rs @@ -0,0 +1,515 @@ +//! What `log_page` used to rebuild on every single page (#473). +//! +//! Two things, and the benchmark in `docs/dev/performance.md` measured both. +//! +//! **The walk.** `log_page` walks with `Sort::TIME | Sort::TOPOLOGICAL`, +//! because the commit graph's lane assignment needs a parent to come after +//! every one of its children. In libgit2 1.9.7 that sort is not incremental in +//! any sense: `git_revwalk_sorting` sets `walk->limited`, so `prepare_walk` +//! runs `limit_list` over the whole reachable graph and then +//! `sort_in_topological_order` materialises the COMPLETE ordered list — +//! before the first oid comes out. Asking that walk for 500 commits and asking +//! it for all 1.5 million of them therefore cost the same thing, and building +//! one walk per page means paying that identical price per page. It shows up +//! as a per-page cost that is flat in depth: on `torvalds/linux`, page one is +//! 15.95 s and page ten is 157.67 s — ten times one page, not one page plus a +//! little. git pays it once and then skips. +//! +//! So the fix is not to make the walk cheaper — nothing here can, see the +//! commit-graph finding in `docs/dev/performance.md` — it is to stop throwing +//! the finished sort away. A walk is prepared once, its output is kept as a +//! `Vec`, and every later page is a slice of it. +//! +//! **The ref map.** `collect_ref_map` enumerated and peeled every ref on every +//! page, to decorate 500 rows. On a repository with 7,001 refs and 2,000 +//! commits that was 16× git's own work for the same question, with history a +//! twenty-fifth of the size of the fixture beside it. It is cached against a +//! FINGERPRINT of the ref database rather than a timer or a write hook, so a +//! `git tag` typed in a terminal invalidates it exactly like one made in the +//! app. +//! +//! ## This cache is an accelerator and nothing else +//! +//! Every entry here is derivable from the repository on disk, so dropping all +//! of it must change nothing but the clock. That is the property to preserve +//! when editing this file, it is what `log_walk_cache.rs` asserts by draining +//! the same history cold and warm and comparing the two sequences, and it is +//! why a poisoned mutex here degrades to a miss instead of failing the page. +//! +//! ## Why a continuation may be served from a stale walk +//! +//! A first page is keyed by (refspec, start oids), so a ref that moves changes +//! the key and the next first page rebuilds. A CONTINUATION is not keyed that +//! way and does not need to be: resuming from a cursor ignores the refs +//! entirely (`push_page_start` does too), the frontier's reachable set is made +//! of commits, and a commit is immutable. New history can only be added as +//! CHILDREN of what is already there, never inside the set an existing +//! frontier reaches. So the tail of a walk prepared ten minutes ago is the +//! same tail it would have today. + +use std::collections::HashMap; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; + +use git2::Oid; + +use crate::git::types::{RefInfo, RepoId}; + +/// How much of one walk is kept, in commits. +/// +/// A bound, not a guess at what fits: the kernel's 1.48 M oids would be 30 MB +/// for one walk of one repository, and several tabs each hold their own. At +/// 100,000 an entry is 2 MB and covers two hundred 500-commit pages — further +/// than any human scrolls, and past it a page falls back to preparing a walk +/// from the cursor, which is what EVERY page did before this file existed. +pub const MAX_ORDER: usize = 100_000; + +/// How many distinct walks one repository remembers. +/// +/// Two, so that flipping the History scope between "All" and one branch — the +/// one alternation a user actually performs — does not evict on every switch. +const MAX_WALKS: usize = 2; + +/// How many emitted cursors one walk remembers, so a continuation can be +/// matched to the exact position it continues from. +/// +/// Unreachable in practice: `MAX_ORDER / 500` is 200 pages, so a walk runs out +/// of order long before it runs out of cursor slots. It exists so that a +/// pathological caller (`limit = 1`, scrolling forever) cannot grow the map +/// without bound. +const MAX_CURSORS: usize = 512; + +/// A commit's ref decorations, by the commit they point at. +pub type RefMap = HashMap>; + +/// Which walk this is: the question that was asked, and the commits it was +/// asked from. +/// +/// `starts` is SORTED, so two enumerations of the same refs in a different +/// order are the same key. It is the whole invalidation story for a first +/// page: any ref that moves changes an oid in here, which makes a new key, +/// which misses. +#[derive(Clone, PartialEq, Eq, Hash, Debug)] +pub struct WalkKey { + pub refspec: Option, + pub starts: Vec, +} + +impl WalkKey { + pub fn new(refspec: Option<&str>, starts: &[Oid]) -> Self { + let mut sorted = starts.to_vec(); + sorted.sort(); + Self { + refspec: refspec.map(str::to_string), + starts: sorted, + } + } +} + +/// One prepared walk's output, in the order the walk produced it. +#[derive(Debug)] +pub struct WalkOrder { + /// The start points that were pushed, in push order — what a first page + /// seeds its frontier with. Not `WalkKey::starts`, which is sorted for + /// hashing; the cursor's lane order is observable and should not shuffle. + pub starts: Vec, + /// Every oid the walk yielded, capped at `MAX_ORDER`. + pub order: Vec, + /// True when the walk ENDED inside the cap, so `order` is all of history + /// from `starts` and running off its end means the end of history. + pub complete: bool, +} + +impl WalkOrder { + /// Whether a page of `limit` starting at `offset` can be answered from + /// here. + /// + /// A short slice is only an answer when the walk is complete — then it IS + /// the end of history. An incomplete order that runs out mid-page would + /// hand back a page that stops for no reason the caller can see, so that + /// case rebuilds instead. + pub fn serves(&self, offset: usize, limit: usize) -> bool { + if self.complete { + offset <= self.order.len() + } else { + offset.saturating_add(limit) <= self.order.len() + } + } +} + +struct Walk { + key: WalkKey, + order: Arc, + /// A frontier this walk emitted → the index it resumes at. + /// + /// Keyed by the frontier itself, sorted, because that is what the caller + /// hands back. Matching by "the first oid of this frontier that appears in + /// some cached order" would be cheaper and is WRONG: two walks over the + /// same repository share commits, so a cursor from the "All" walk would + /// happily resolve against a single-branch walk and continue the wrong + /// history. A cursor we emitted is the only one we can place exactly. + cursors: HashMap, usize>, +} + +#[derive(Default)] +struct RepoEntry { + /// Most recently used first. + walks: Vec, + /// Ref-database fingerprint → the decorations built from it. + refs: Option<(u64, Arc)>, +} + +/// What the cache did, for anyone who has to prove it did it. +/// +/// A cache that never hits is invisible from the outside: pagination returns +/// exactly the same commits either way, which is the property this file is +/// built for and also the reason a functional test cannot tell the two apart. +/// The alternative — asserting on a clock — is a flake. So the counters are +/// public, `log_walk_cache.rs` reads them, and so does the benchmark. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CacheStats { + /// Pages served from an already-prepared walk. + pub hits: u64, + /// Walks prepared — the expensive thing this file exists to avoid. + pub walks_prepared: u64, + /// Ref maps built — every one of them enumerated and peeled every ref. + pub ref_maps_built: u64, +} + +/// Per-repository derived state for the paged log. +#[derive(Default)] +pub struct LogCache { + repos: Mutex>, + hits: AtomicU64, + walks_prepared: AtomicU64, + ref_maps_built: AtomicU64, +} + +impl LogCache { + pub fn new() -> Self { + Self::default() + } + + pub fn stats(&self) -> CacheStats { + CacheStats { + hits: self.hits.load(Ordering::Relaxed), + walks_prepared: self.walks_prepared.load(Ordering::Relaxed), + ref_maps_built: self.ref_maps_built.load(Ordering::Relaxed), + } + } + + /// Count a walk that had to be prepared. Called by the backend, which is + /// the only place that can know a build actually happened. + pub fn record_walk_prepared(&self) { + self.walks_prepared.fetch_add(1, Ordering::Relaxed); + } + + /// The order for a FIRST page of `key`, if one is cached and can answer it. + pub fn first_page(&self, repo: &RepoId, key: &WalkKey, limit: usize) -> Option> { + let mut repos = self.repos.lock().ok()?; + let entry = repos.get_mut(repo)?; + let at = entry.walks.iter().position(|w| &w.key == key)?; + if !entry.walks[at].order.serves(0, limit) { + return None; + } + let walk = entry.walks.remove(at); + let order = Arc::clone(&walk.order); + entry.walks.insert(0, walk); + self.hits.fetch_add(1, Ordering::Relaxed); + Some(order) + } + + /// The walk a cursor continues, and where in it the next page begins. + /// + /// `cursor` must be sorted — the caller sorts what it receives, and + /// `remember_cursor` sorts what it emits, so the two agree. + pub fn resume( + &self, + repo: &RepoId, + cursor: &[Oid], + limit: usize, + ) -> Option<(WalkKey, Arc, usize)> { + let mut repos = self.repos.lock().ok()?; + let entry = repos.get_mut(repo)?; + let at = entry + .walks + .iter() + .position(|w| w.cursors.contains_key(cursor))?; + let offset = *entry.walks[at].cursors.get(cursor)?; + if !entry.walks[at].order.serves(offset, limit) { + return None; + } + let walk = entry.walks.remove(at); + let out = (walk.key.clone(), Arc::clone(&walk.order), offset); + entry.walks.insert(0, walk); + self.hits.fetch_add(1, Ordering::Relaxed); + Some(out) + } + + /// File a freshly prepared walk, evicting the least recently used one. + pub fn insert(&self, repo: &RepoId, key: WalkKey, order: Arc) { + let Ok(mut repos) = self.repos.lock() else { + return; + }; + let entry = repos.entry(repo.clone()).or_default(); + entry.walks.retain(|w| w.key != key); + entry.walks.insert( + 0, + Walk { + key, + order, + cursors: HashMap::new(), + }, + ); + entry.walks.truncate(MAX_WALKS); + } + + /// Record that `cursor` continues `key` at `next`, so the page after it + /// can be a slice instead of a walk. + pub fn remember_cursor(&self, repo: &RepoId, key: &WalkKey, mut cursor: Vec, next: usize) { + let Ok(mut repos) = self.repos.lock() else { + return; + }; + let Some(entry) = repos.get_mut(repo) else { + return; + }; + let Some(walk) = entry.walks.iter_mut().find(|w| &w.key == key) else { + return; + }; + cursor.sort(); + // Cleared rather than evicted one by one: keeping an insertion order + // beside the map would double what the keys cost, to defend a bound + // that a 500-commit page cannot reach in the first place. + if walk.cursors.len() >= MAX_CURSORS { + walk.cursors.clear(); + } + walk.cursors.insert(cursor, next); + } + + /// The ref decorations for `fingerprint`, if that is still the ref database + /// we built them from. + pub fn ref_map(&self, repo: &RepoId, fingerprint: u64) -> Option> { + let repos = self.repos.lock().ok()?; + let (had, map) = repos.get(repo)?.refs.as_ref()?; + (*had == fingerprint).then(|| Arc::clone(map)) + } + + /// File a freshly built ref map. Called only after `collect_ref_map` ran, + /// which is what makes the counter here mean "a map was built". + pub fn put_ref_map(&self, repo: &RepoId, fingerprint: u64, map: Arc) { + self.ref_maps_built.fetch_add(1, Ordering::Relaxed); + let Ok(mut repos) = self.repos.lock() else { + return; + }; + repos.entry(repo.clone()).or_default().refs = Some((fingerprint, map)); + } + + /// Drop everything held for one repository — `close` calls this, so the + /// memory goes away with the tab rather than with the process. + pub fn forget(&self, repo: &RepoId) { + if let Ok(mut repos) = self.repos.lock() { + repos.remove(repo); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn oid(n: u8) -> Oid { + let mut raw = [0u8; 20]; + raw[19] = n; + Oid::from_bytes(&raw).unwrap() + } + + fn order(n: usize, complete: bool) -> Arc { + Arc::new(WalkOrder { + starts: vec![oid(0)], + order: (0..n).map(|i| oid(i as u8)).collect(), + complete, + }) + } + + fn repo() -> RepoId { + RepoId("r".into()) + } + + #[test] + fn a_first_page_hits_the_walk_it_was_filed_under() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(10, true)); + + assert!(c.first_page(&repo(), &key, 5).is_some()); + assert_eq!(c.stats().hits, 1); + } + + /// The whole invalidation story for a first page: a moved ref is a + /// different start oid, which is a different key, which is a miss. + #[test] + fn a_moved_start_point_is_a_different_walk() { + let c = LogCache::new(); + c.insert(&repo(), WalkKey::new(None, &[oid(1)]), order(10, true)); + + let moved = WalkKey::new(None, &[oid(2)]); + assert!(c.first_page(&repo(), &moved, 5).is_none()); + } + + /// …and the enumeration order of the refs is not part of that story. + #[test] + fn the_start_points_are_a_set_not_a_list() { + let c = LogCache::new(); + c.insert( + &repo(), + WalkKey::new(None, &[oid(1), oid(2), oid(3)]), + order(10, true), + ); + + let shuffled = WalkKey::new(None, &[oid(3), oid(1), oid(2)]); + assert!(c.first_page(&repo(), &shuffled, 5).is_some()); + } + + #[test] + fn a_different_refspec_is_a_different_walk() { + let c = LogCache::new(); + c.insert(&repo(), WalkKey::new(None, &[oid(1)]), order(10, true)); + + let scoped = WalkKey::new(Some("refs/heads/main"), &[oid(1)]); + assert!(c.first_page(&repo(), &scoped, 5).is_none()); + } + + #[test] + fn a_remembered_cursor_resumes_where_it_left_off() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(10, true)); + c.remember_cursor(&repo(), &key, vec![oid(5), oid(6)], 5); + + // Sorted the other way round: the caller hands back whatever order the + // frontier travelled in, and it must still match. + let (found, _, at) = c.resume(&repo(), &[oid(5), oid(6)], 2).expect("resume"); + assert_eq!(found, key); + assert_eq!(at, 5); + } + + /// A cursor nobody emitted is a miss, not a guess. Two walks over one + /// repository share commits, so "find an order containing these oids" + /// would place an "All" cursor inside a single-branch walk. + #[test] + fn an_unknown_cursor_does_not_resolve() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key, order(10, true)); + + assert!(c.resume(&repo(), &[oid(5)], 2).is_none()); + } + + /// An incomplete order cannot answer a page that runs off its end — that + /// page would stop early for a reason the caller cannot see. + #[test] + fn an_incomplete_order_refuses_a_page_it_cannot_fill() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(10, false)); + c.remember_cursor(&repo(), &key, vec![oid(9)], 9); + + assert!(c.resume(&repo(), &[oid(9)], 5).is_none(), "9 + 5 > 10"); + assert!(c.resume(&repo(), &[oid(9)], 1).is_some(), "9 + 1 == 10"); + } + + /// A COMPLETE order answers the same page happily: running out is the end + /// of history, which is a real answer. + #[test] + fn a_complete_order_answers_past_its_end() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(10, true)); + c.remember_cursor(&repo(), &key, vec![oid(9)], 9); + + assert!(c.resume(&repo(), &[oid(9)], 500).is_some()); + } + + #[test] + fn a_third_walk_evicts_the_least_recently_used_one() { + let c = LogCache::new(); + let a = WalkKey::new(None, &[oid(1)]); + let b = WalkKey::new(None, &[oid(2)]); + let d = WalkKey::new(None, &[oid(3)]); + c.insert(&repo(), a.clone(), order(10, true)); + c.insert(&repo(), b.clone(), order(10, true)); + // Touch `a` so `b` becomes the coldest. + c.first_page(&repo(), &a, 1).expect("a is cached"); + c.insert(&repo(), d.clone(), order(10, true)); + + assert!(c.first_page(&repo(), &a, 1).is_some(), "a was used last"); + assert!(c.first_page(&repo(), &d, 1).is_some(), "d is newest"); + assert!(c.first_page(&repo(), &b, 1).is_none(), "b was coldest"); + } + + /// Re-filing a key replaces it rather than growing a second copy, or two + /// pages of the same walk would evict everything else between them. + #[test] + fn re_inserting_a_key_replaces_it() { + let c = LogCache::new(); + let a = WalkKey::new(None, &[oid(1)]); + let b = WalkKey::new(None, &[oid(2)]); + c.insert(&repo(), a.clone(), order(10, true)); + c.insert(&repo(), a.clone(), order(10, true)); + c.insert(&repo(), b.clone(), order(10, true)); + + assert!(c.first_page(&repo(), &a, 1).is_some(), "a must survive"); + assert!(c.first_page(&repo(), &b, 1).is_some(), "b must survive"); + } + + #[test] + fn a_ref_map_survives_only_its_own_fingerprint() { + let c = LogCache::new(); + c.put_ref_map(&repo(), 7, Arc::new(RefMap::new())); + + assert!(c.ref_map(&repo(), 7).is_some()); + assert!(c.ref_map(&repo(), 8).is_none(), "the ref database moved"); + } + + #[test] + fn closing_a_repository_forgets_everything() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(10, true)); + c.put_ref_map(&repo(), 7, Arc::new(RefMap::new())); + + c.forget(&repo()); + + assert!(c.first_page(&repo(), &key, 1).is_none()); + assert!(c.ref_map(&repo(), 7).is_none()); + } + + /// Two repositories share this cache and must not share entries — the tab + /// next door is a different history under the same refspec. + #[test] + fn repositories_do_not_share_entries() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&RepoId("a".into()), key.clone(), order(10, true)); + + assert!(c.first_page(&RepoId("b".into()), &key, 1).is_none()); + } + + /// The cursor map is bounded, and a full one drops back to a miss rather + /// than growing. + #[test] + fn the_cursor_map_is_bounded() { + let c = LogCache::new(); + let key = WalkKey::new(None, &[oid(1)]); + c.insert(&repo(), key.clone(), order(MAX_ORDER, true)); + for i in 0..MAX_CURSORS + 1 { + c.remember_cursor(&repo(), &key, vec![oid(1), oid((i % 251) as u8)], i); + } + + let Ok(repos) = c.repos.lock() else { + panic!("poisoned") + }; + let held = repos[&repo()].walks[0].cursors.len(); + assert!(held <= MAX_CURSORS, "held {held} cursors"); + } +} diff --git a/src-tauri/src/git/mod.rs b/src-tauri/src/git/mod.rs index ba96e31..b1c55b3 100644 --- a/src-tauri/src/git/mod.rs +++ b/src-tauri/src/git/mod.rs @@ -8,6 +8,7 @@ pub mod hooks; pub mod image; pub mod libgit2; pub mod lfs; +pub mod log_cache; pub mod notes; pub mod ownership; pub mod rebase_plan; diff --git a/src-tauri/tests/log_walk_cache.rs b/src-tauri/tests/log_walk_cache.rs new file mode 100644 index 0000000..5006f5e --- /dev/null +++ b/src-tauri/tests/log_walk_cache.rs @@ -0,0 +1,514 @@ +//! The paged log's walk cache (#473). +//! +//! `log_page` used to prepare a fresh revwalk per page, and in libgit2 a +//! topologically sorted walk materialises its COMPLETE ordered list before it +//! yields anything — so every page paid for sorting all of history. Ten pages +//! into `torvalds/linux` cost two minutes and thirty-eight seconds, ten times +//! what one page cost. The walk is now prepared once and paged from. +//! +//! Two kinds of test here, and the split is the point. +//! +//! **Transparency.** Everything the cache holds is derivable from the +//! repository, so turning it off must change nothing but the clock. Each of +//! these drains the same history twice — once through one backend, where the +//! cache is hot, and once through a fresh backend per page, where it can never +//! hit — and compares the two. That is the pre-change behaviour, byte for +//! byte, held against the new one. +//! +//! **Effect.** Every transparency test above passes just as well against a +//! cache that never hits, which would make them worthless on their own. So the +//! rest assert on `log_cache_stats`: how many walks a drain prepared, and that +//! a moved ref makes the next one prepare another. Those are the tests that go +//! red when the cache stops working — and the ones to plant a violation +//! against before trusting an edit here. + +mod support; + +use platypusgit_lib::git::libgit2::Libgit2Backend; +use platypusgit_lib::git::types::{RefKind, RepoId}; +use platypusgit_lib::git::GitBackend; +use support::{linear_history, TempRepo}; + +/// One row as the UI sees it: the commit, and the pills stamped on it. +type Row = (String, Vec<(String, RefKind)>); + +fn rows(page: &platypusgit_lib::git::types::LogPage) -> Vec { + page.commits + .iter() + .map(|c| { + ( + c.oid.clone(), + c.refs.iter().map(|r| (r.name.clone(), r.kind)).collect(), + ) + }) + .collect() +} + +/// Walk all of history through ONE backend — the cache is hot from page two. +fn drain_warm(be: &Libgit2Backend, id: &RepoId, refspec: Option<&str>, page: usize) -> Vec { + let mut out = Vec::new(); + let mut cursor: Option> = None; + loop { + let p = be.log_page(id, refspec, cursor.as_deref(), page).unwrap(); + out.extend(rows(&p)); + match p.next_cursor { + Some(c) => cursor = Some(c), + None => break, + } + assert!(out.len() < 10_000, "pagination did not terminate"); + } + out +} + +/// Walk all of history through a FRESH backend per page, so no page is ever +/// served from a cache. This is what the code did before #473. +fn drain_cold(tr: &TempRepo, refspec: Option<&str>, page: usize) -> Vec { + let mut out = Vec::new(); + let mut cursor: Option> = None; + loop { + let (be, handle) = tr.open_with_backend(); + let p = be + .log_page(&handle.id, refspec, cursor.as_deref(), page) + .unwrap(); + out.extend(rows(&p)); + match p.next_cursor { + Some(c) => cursor = Some(c), + None => break, + } + assert!(out.len() < 10_000, "pagination did not terminate"); + } + out +} + +fn checkout(tr: &TempRepo, branch: &str) { + tr.repo.set_head(&format!("refs/heads/{branch}")).unwrap(); + let mut co = git2::build::CheckoutBuilder::new(); + co.force(); + tr.repo.checkout_head(Some(&mut co)).unwrap(); +} + +/// A commit on HEAD at an EXACT time, because the walk's order depends on it. +/// +/// libgit2 sorts the topological queue by commit time, and `git_pqueue` is a +/// binary heap whose comparator is not stable — so among commits sharing a +/// second, which one comes out first depends on the order they were inserted. +/// A fixture built with `Signature::now` makes every commit in the same second +/// and leaves the order of two live lanes genuinely ambiguous, which no test +/// can then pin. Every multi-lane fixture below therefore spaces its commits +/// out — `merged_history(3600)` — and the one case that deliberately does not, +/// `merged_history(0)`, asserts only what a tie can still promise. +fn commit_at(tr: &TempRepo, file: &str, message: &str, when: i64) -> git2::Oid { + support::fs::write_file(tr.path(), file, message); + let mut index = tr.repo.index().unwrap(); + index.add_path(std::path::Path::new(file)).unwrap(); + index.write().unwrap(); + let tree_oid = index.write_tree().unwrap(); + let tree = tr.repo.find_tree(tree_oid).unwrap(); + let sig = + git2::Signature::new("Test", "test@example.com", &git2::Time::new(when, 0)).unwrap(); + let head = tr.repo.head().ok().and_then(|h| h.peel_to_commit().ok()); + let parents: Vec<&git2::Commit> = head.iter().collect(); + tr.repo + .commit(Some("HEAD"), &sig, &sig, message, &tree, &parents) + .unwrap() +} + +/// initial → (main 1, main 2) and (feat 1, feat 2) → a real merge commit, so +/// the walk has two live lanes and a page boundary can land between them. +fn merged_history(spacing: i64) -> TempRepo { + let tr = TempRepo::fresh(); + commit_at(&tr, "README.md", "initial", 1_700_000_000); + { + let base = tr.repo.head().unwrap().peel_to_commit().unwrap(); + tr.repo.branch("feature", &base, false).unwrap(); + } + commit_at(&tr, "m1.txt", "main 1", 1_700_000_000 + spacing); + commit_at(&tr, "m2.txt", "main 2", 1_700_000_000 + 2 * spacing); + checkout(&tr, "feature"); + commit_at(&tr, "f1.txt", "feat 1", 1_700_000_000 + 3 * spacing); + commit_at(&tr, "f2.txt", "feat 2", 1_700_000_000 + 4 * spacing); + checkout(&tr, "main"); + { + let main_tip = tr.repo.head().unwrap().peel_to_commit().unwrap(); + let feat_tip = tr + .repo + .find_branch("feature", git2::BranchType::Local) + .unwrap() + .get() + .peel_to_commit() + .unwrap(); + let tree = main_tip.tree().unwrap(); + let sig = git2::Signature::new( + "Test", + "test@example.com", + &git2::Time::new(1_700_000_000 + 5 * spacing, 0), + ) + .unwrap(); + tr.repo + .commit( + Some("HEAD"), + &sig, + &sig, + "merge feature", + &tree, + &[&main_tip, &feat_tip], + ) + .unwrap(); + } + tr +} + +/// Every parent that appears in `rows` appears AFTER its child — the promise +/// the whole topological sort exists to make, and the one that survives a tie. +fn is_topological(tr: &TempRepo, rows: &[Row]) -> Result<(), String> { + let at: std::collections::HashMap<&str, usize> = rows + .iter() + .enumerate() + .map(|(i, (oid, _))| (oid.as_str(), i)) + .collect(); + for (i, (oid, _)) in rows.iter().enumerate() { + let commit = tr + .repo + .find_commit(git2::Oid::from_str(oid).unwrap()) + .unwrap(); + for parent in commit.parent_ids() { + if let Some(&j) = at.get(parent.to_string().as_str()) { + if j <= i { + return Err(format!("{oid} at {i} has parent {parent} at {j}")); + } + } + } + } + Ok(()) +} + +// ---------------------------------------------------------------- transparency + +/// The property everything else rests on: cached and uncached produce the same +/// history, page size by page size. Sizes that do and do not divide the 21 +/// commits, so a page boundary lands everywhere it can. +#[test] +fn a_cached_walk_pages_exactly_like_an_uncached_one() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 20); + let (be, handle) = tr.open_with_backend(); + + for page in [1, 2, 3, 5, 7, 21, 500] { + let warm = drain_warm(&be, &handle.id, None, page); + let cold = drain_cold(&tr, None, page); + assert_eq!(warm, cold, "page size {page}"); + assert_eq!(warm.len(), 21, "page size {page}"); + } +} + +/// The same, where a page boundary can fall between two live lanes — the case +/// the frontier cursor exists for, and the one an off-by-one in the resume +/// offset would corrupt without changing the commit COUNT. +#[test] +fn a_merge_pages_the_same_cached_and_uncached() { + let tr = merged_history(3600); + let (be, handle) = tr.open_with_backend(); + + for page in [1, 2, 3, 6, 500] { + let warm = drain_warm(&be, &handle.id, None, page); + let cold = drain_cold(&tr, None, page); + assert_eq!(warm, cold, "page size {page}"); + assert_eq!(warm.len(), 6, "page size {page}"); + } +} + +/// Every branch tip is a start point, so `--all` is the walk with the most +/// lanes and the most seeding to get wrong. +#[test] +fn the_all_scope_pages_the_same_cached_and_uncached() { + let tr = merged_history(3600); + let (be, handle) = tr.open_with_backend(); + + for page in [1, 2, 4, 500] { + let warm = drain_warm(&be, &handle.id, Some("--all"), page); + let cold = drain_cold(&tr, Some("--all"), page); + assert_eq!(warm, cold, "page size {page}"); + } +} + +/// When two lanes share a commit SECOND, which of them comes first is not +/// something either walk promises: libgit2 orders the topological queue by +/// time through a binary heap with an unstable comparator, so the answer +/// depends on insertion order — and a walk restarted from a cursor inserts in +/// a different order than one that ran straight through. That was true of the +/// walk-per-page version too; it is simply now visible, because one prepared +/// walk is self-consistent where ten of them were not. +/// +/// What must hold regardless, and is the actual contract of a paged log: every +/// commit exactly once, and no parent before its child. +#[test] +fn a_tie_in_commit_time_still_pages_every_commit_exactly_once() { + let tr = merged_history(0); + let (be, handle) = tr.open_with_backend(); + + for page in [1, 2, 3, 6, 500] { + let warm = drain_warm(&be, &handle.id, None, page); + let cold = drain_cold(&tr, None, page); + + let seen: std::collections::BTreeSet<&String> = warm.iter().map(|(o, _)| o).collect(); + assert_eq!(seen.len(), warm.len(), "page size {page}: a commit repeated"); + assert_eq!( + seen, + cold.iter().map(|(o, _)| o).collect(), + "page size {page}: a different set of commits", + ); + is_topological(&tr, &warm).unwrap_or_else(|e| panic!("page size {page}: {e}")); + } +} + +/// Ref decorations travel with the row, and they come from the cached ref map. +/// A map that went stale would drop the tag from the second read. +#[test] +fn a_tag_made_after_the_first_page_shows_up_on_the_next_one() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + let (be, handle) = tr.open_with_backend(); + + let before = be.log_page(&handle.id, None, None, 10).unwrap(); + assert!( + before.commits[0].refs.iter().all(|r| r.name != "v1"), + "the tag does not exist yet", + ); + + let head = tr.repo.head().unwrap().peel_to_commit().unwrap(); + tr.repo + .tag_lightweight("v1", head.as_object(), false) + .unwrap(); + + let after = be.log_page(&handle.id, None, None, 10).unwrap(); + let pills: Vec<&str> = after.commits[0] + .refs + .iter() + .map(|r| r.name.as_str()) + .collect(); + assert!(pills.contains(&"v1"), "got {pills:?}"); +} + +/// An annotated tag is the expensive half of `collect_ref_map` — it is the one +/// that has to be peeled — so it is the one most worth proving still arrives +/// after the map has been cached once. +#[test] +fn an_annotated_tag_made_later_shows_up_too() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + let (be, handle) = tr.open_with_backend(); + be.log_page(&handle.id, None, None, 10).unwrap(); + + let head = tr.repo.head().unwrap().peel_to_commit().unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + tr.repo + .tag("v2", head.as_object(), &sig, "release two", false) + .unwrap(); + + let after = be.log_page(&handle.id, None, None, 10).unwrap(); + let tagged = after.commits[0] + .refs + .iter() + .any(|r| r.name == "v2" && r.kind == RefKind::Tag); + assert!(tagged, "got {:?}", after.commits[0].refs); +} + +/// A commit made after a page was cached must appear in the next first page — +/// the invalidation that matters most, because it happens every time the user +/// commits. +#[test] +fn a_new_commit_appears_in_the_next_first_page() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + let (be, handle) = tr.open_with_backend(); + + let before = be.log_page(&handle.id, None, None, 10).unwrap(); + assert_eq!(before.commits.len(), 4); + + tr.add_commit("new.txt", "new\n", "brand new"); + + let after = be.log_page(&handle.id, None, None, 10).unwrap(); + assert_eq!(after.commits.len(), 5, "the new commit is missing"); + assert_eq!(after.commits[0].summary, "brand new"); +} + +/// Two scopes over one repository are two different walks. Asking for one +/// right after the other must not hand back the other one's history. +#[test] +fn two_scopes_do_not_share_a_walk() { + let tr = merged_history(3600); + let (be, handle) = tr.open_with_backend(); + + let head_only = be.log_page(&handle.id, None, None, 500).unwrap(); + let feature = be + .log_page(&handle.id, Some("feature"), None, 500) + .unwrap(); + let head_again = be.log_page(&handle.id, None, None, 500).unwrap(); + + assert_eq!(head_only.commits.len(), 6, "HEAD sees the merge"); + assert_eq!(feature.commits.len(), 3, "feature does not"); + assert_eq!( + head_only.commits.iter().map(|c| &c.oid).collect::>(), + head_again.commits.iter().map(|c| &c.oid).collect::>(), + "the second read of HEAD came back as something else", + ); +} + +// ---------------------------------------------------------------------- effect + +/// The point of the whole change: N pages, one walk. +/// +/// This is the test that fails if `page_plan` stops consulting the cache, and +/// the one the numbers in `docs/dev/performance.md` are downstream of. Twenty +/// one commits in pages of two is eleven pages. +#[test] +fn paging_a_whole_history_prepares_exactly_one_walk() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 20); + let (be, handle) = tr.open_with_backend(); + + let all = drain_warm(&be, &handle.id, None, 2); + assert_eq!(all.len(), 21); + + let stats = be.log_cache_stats(); + assert_eq!( + stats.walks_prepared, 1, + "eleven pages prepared {} walks", + stats.walks_prepared, + ); + assert_eq!(stats.hits, 10, "ten of the eleven pages should be slices"); +} + +/// …and the ref map is built once for those eleven pages, not eleven times. +#[test] +fn paging_a_whole_history_builds_exactly_one_ref_map() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 20); + let (be, handle) = tr.open_with_backend(); + + drain_warm(&be, &handle.id, None, 2); + + assert_eq!(be.log_cache_stats().ref_maps_built, 1); +} + +/// A repeated first page — what `refreshAll` issues on every refresh — must +/// not prepare a second walk. On the kernel that one fact is 15.9 seconds per +/// refresh. +#[test] +fn a_repeated_first_page_prepares_no_second_walk() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 10); + let (be, handle) = tr.open_with_backend(); + + for _ in 0..5 { + be.log_page(&handle.id, None, None, 500).unwrap(); + } + + assert_eq!(be.log_cache_stats().walks_prepared, 1); +} + +/// A commit invalidates the walk, because the start oid it is keyed by moved. +/// Without this the cache would be a correctness bug rather than a speed-up, +/// so it is asserted as a COUNT as well as by the commit showing up. +#[test] +fn a_commit_makes_the_next_first_page_prepare_a_new_walk() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + let (be, handle) = tr.open_with_backend(); + + be.log_page(&handle.id, None, None, 500).unwrap(); + assert_eq!(be.log_cache_stats().walks_prepared, 1); + + tr.add_commit("new.txt", "new\n", "brand new"); + be.log_page(&handle.id, None, None, 500).unwrap(); + + assert_eq!(be.log_cache_stats().walks_prepared, 2); +} + +/// A tag moves the ref fingerprint even though it moves no start point, so the +/// ref map is rebuilt and the walk is not. Both halves matter: rebuilding the +/// walk for a tag would throw away the expensive thing for the cheap reason. +#[test] +fn a_tag_rebuilds_the_ref_map_and_not_the_walk() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + let (be, handle) = tr.open_with_backend(); + + be.log_page(&handle.id, None, None, 500).unwrap(); + let before = be.log_cache_stats(); + + let head = tr.repo.head().unwrap().peel_to_commit().unwrap(); + tr.repo + .tag_lightweight("v1", head.as_object(), false) + .unwrap(); + be.log_page(&handle.id, None, None, 500).unwrap(); + let after = be.log_cache_stats(); + + assert_eq!(after.ref_maps_built, before.ref_maps_built + 1); + assert_eq!( + after.walks_prepared, before.walks_prepared, + "a tag is not a new walk", + ); +} + +/// Nothing changed, so nothing is rebuilt — the case that is true on almost +/// every call, and the one a fingerprint that is not stable would silently +/// ruin. +#[test] +fn an_unchanged_repository_rebuilds_nothing() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + { + let head = tr.repo.head().unwrap().peel_to_commit().unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + tr.repo.branch("other", &head, false).unwrap(); + tr.repo + .tag_lightweight("light", head.as_object(), false) + .unwrap(); + tr.repo + .tag("heavy", head.as_object(), &sig, "annotated", false) + .unwrap(); + } + let (be, handle) = tr.open_with_backend(); + + be.log_page(&handle.id, Some("--all"), None, 500).unwrap(); + let first = be.log_cache_stats(); + for _ in 0..4 { + be.log_page(&handle.id, Some("--all"), None, 500).unwrap(); + } + let after = be.log_cache_stats(); + + assert_eq!(after.walks_prepared, first.walks_prepared, "walk rebuilt"); + assert_eq!(after.ref_maps_built, first.ref_maps_built, "ref map rebuilt"); + assert_eq!(after.hits, first.hits + 4, "four pages, four slices"); +} + +/// A cursor from one backend handed to another has nothing to resume against, +/// and must still page correctly — the fallback every page took before #473, +/// and the one a cache eviction drops back to. +#[test] +fn a_cursor_with_no_cache_behind_it_still_pages() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 5); + let (first, handle) = tr.open_with_backend(); + + let page = first.log_page(&handle.id, None, None, 2).unwrap(); + let cursor = page.next_cursor.expect("more history"); + + let (second, other) = tr.open_with_backend(); + let resumed = second + .log_page(&other.id, None, Some(&cursor), 2) + .unwrap(); + + assert_eq!(resumed.commits.len(), 2); + assert_eq!( + second.log_cache_stats().walks_prepared, + 1, + "an unknown cursor has to prepare its own walk", + ); + let expected = drain_cold(&tr, None, 2); + assert_eq!( + resumed.commits.iter().map(|c| &c.oid).collect::>(), + expected[2..4].iter().map(|(o, _)| o).collect::>(), + ); +} From 7f22f5070cd94622aeff3e144cb2f95561bb4b67 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 18:35:40 +0200 Subject: [PATCH 2/6] perf(log): keep a continuation's decorations, and enumerate the refs once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to the ref-map half, both measured on the `refs` fixture (7,001 loose refs, 2,000 commits). **The peeling was not the expense.** #473 reads the per-page cost as "enumerating and peeling every ref", and the first cut of this cache followed that: validate by enumerating names and targets, skip the peel on a hit. Measured, one enumeration is 113 ms and the peel inside it is 10 ms of that — so validating cost 113 ms of a 117 ms page and the cache saved 13%. So a FIRST page validates the decorations and a CONTINUATION reuses what its first page saw. Once per refresh is proportionate: `branches` and `tags`, beside it in the same eleven-read fan-out, pay 187 ms and 149 ms for the same enumeration. Once per scroll is not. A scroll now shows one coherent snapshot of the decorations rather than re-reading 7,001 refs to redraw the same pills, and any refresh is a first page. **And the cold path enumerated everything twice.** Asking for a fingerprint before building the map means enumerating every ref for an answer that cannot match anything, then enumerating again inside `collect_ref_map`. That made a cold eleven-read fan-out SLOWER than the code this replaces — 219 ms against 364 ms — and the benchmark builds a fresh backend per `open_screen` sample, so that path is always the cold one. `collect_ref_map` now returns the fingerprint from the pass it was already making, and the validating pass runs only when there is something cached to validate. Cold is one pass, unchanged is one cheap pass, and only a ref that actually moved costs two. **Why:** the failure mode of two fingerprints drifting apart is invisible. Pagination stays correct, the decorations stay correct, and the cache simply never hits again — so `RefFingerprint` is one definition used by both paths and `the_two_fingerprint_paths_agree` asserts it through the only door there is: a repository holding every shape of ref, read twice, must rebuild nothing the second time. Planted a one-bit divergence and watched it go red. Co-Authored-By: Claude Opus 5 (1M context) --- docs/dev/architecture.md | 19 ++++++ docs/dev/backend.md | 77 ++++++++++++++++++++++++ src-tauri/src/git/libgit2.rs | 97 +++++++++++++++++++++++++------ src-tauri/src/git/log_cache.rs | 20 ++++--- src-tauri/tests/log_walk_cache.rs | 90 ++++++++++++++++++++++++++++ 5 files changed, 277 insertions(+), 26 deletions(-) diff --git a/docs/dev/architecture.md b/docs/dev/architecture.md index 0490213..0438616 100644 --- a/docs/dev/architecture.md +++ b/docs/dev/architecture.md @@ -274,6 +274,25 @@ git/ │ write still excludes everything. Carries its own unit tests │ with a fake handle, proving overlap by barrier rather than by │ wall clock. See backend.md +├── log_cache.rs What `log_page` used to rebuild on every page (#473): the +│ prepared WALK and the REF MAP. libgit2's topological sort is +│ not incremental — setting a sort order sets `walk->limited`, so +│ `prepare_walk` traverses the whole reachable graph and +│ materialises the complete ordered list before yielding one oid, +│ which is why 500 commits and 1.5 M cost the same and why a walk +│ per page paid that price per page (page 10 of the kernel: 157 +│ s). So the walk is prepared once, kept as a `Vec` capped at +│ MAX_ORDER, and later pages are slices. Two different +│ invalidations on purpose: a FIRST page is keyed by (refspec, +│ start oids) so a moved ref misses, a CONTINUATION is keyed by +│ the frontier it was emitted with and consults no refs at all +│ (the set a frontier reaches is commits, and commits are +│ immutable). The ref map is keyed by a fingerprint of every ref +│ name + target, so a `git tag` in a terminal invalidates it like +│ one made in the app. PURE ACCELERATOR: a poisoned mutex is a +│ miss, never a failed page, and `close` drops the lot. Carries +│ its own unit tests; `tests/log_walk_cache.rs` holds the +│ transparency property end to end. See backend.md, performance.md ├── update_refs.rs Stacked branches (#240) — git's `rebase --update-refs`, │ IMPLEMENTED not passed through, because our rebase is our own │ libgit2 replay with no `git rebase` process to hand a flag to. diff --git a/docs/dev/backend.md b/docs/dev/backend.md index 4324648..d37f70a 100644 --- a/docs/dev/backend.md +++ b/docs/dev/backend.md @@ -1544,6 +1544,83 @@ walk a `Vec` per commit — `{ name, kind }`, where `kind` is - `src-tauri/tests/log_decoration.rs` pins all of it, including a tag and a branch of the same name staying two distinct decorations. +`collect_ref_map` no longer runs per page — see below. + +## The paged log prepares ONE walk (#473) + +`log_page` used to build a fresh revwalk per page. That reads like an obvious +design until you look at what libgit2 does with a sort order: + +- `git_revwalk_sorting` sets `walk->limited` for ANY sorting but `NONE`, so + `prepare_walk` runs `limit_list` over the entire reachable graph; +- `sort_in_topological_order` then materialises the **complete** ordered list, + and only afterwards does `git_revwalk_next` start popping it. + +So asking that walk for 500 commits and asking it for all 1,482,923 of them +cost the same thing, and one walk per page paid that price per page. The +benchmark measured it as a per-page cost *flat in depth*: on `torvalds/linux` +page one was 15.95 s and page ten 157.67 s — ten times one page. git pays for +the same sort once and then `--skip`s. + +`git/log_cache.rs` keeps the finished sort instead. A walk is prepared once, +its output kept as a `Vec` capped at `MAX_ORDER` (100,000 — two hundred +pages, 2 MB), and every later page is a slice of it. The cursor contract is +unchanged: `next_cursor` is still the frontier, still self-describing, and a +cursor the cache cannot place still prepares its own walk. + +**There are two invalidation stories and they are deliberately different.** + +- A **first page** is keyed by `(refspec, start oids)`. Any ref that moves + changes an oid in the key, so the next first page misses and rebuilds. That + is the whole mechanism — there is no write hook to forget to call. +- A **continuation** is keyed by the frontier it was emitted with, and consults + no refs at all. It does not need to: resuming from a cursor has always + ignored `refspec` (`push_page_start`), and the set a frontier reaches is made + of commits, which are immutable. New history arrives as CHILDREN of what is + there, never inside that set. So a walk prepared ten minutes ago has the same + tail today. + +A cursor is placed by **exact match against the frontier we emitted**, never by +"find an order containing these oids". Two walks over one repository share +commits, so containment would happily resolve an `--all` cursor inside a +single-branch walk and continue the wrong history. + +**The ref map goes the same way**, and it was worth as much: `collect_ref_map` +enumerated and peeled every ref on every page to decorate 500 rows — 16× git's +own work on a repository with 7,001 refs and 2,000 commits. It is now cached +against a `ref_fingerprint`: one enumeration of every ref's name and target +with **no peeling**, combined commutatively so enumeration order cannot change +it. A fingerprint rather than a write hook, because a `git tag` typed in a +terminal has to invalidate it exactly like one made in the app; and rather than +an mtime, because loose refs live in nested directories that no single +timestamp covers. `commits_since` and `commits_between` read the same cache. + +**It is a pure accelerator, and that is a rule, not a description.** Everything +held is derivable from disk, a poisoned mutex degrades to a miss instead of +failing the page, and `close` drops the repository's entries. `tests/ +log_walk_cache.rs` holds it down by draining the same history twice — through +one backend where the cache is hot, and through a fresh backend per page where +it can never hit — and comparing the two. Those tests pass just as well against +a cache that never hits, which is why the file also asserts on +`log_cache_stats()`: eleven pages must prepare exactly one walk and build +exactly one ref map, and a commit must make the next first page prepare a +second. All four failure modes were planted and watched go red. + +**One behaviour is now visible that was always there.** When two commits share +a *second*, which lane comes out first is not something either walk promises: +libgit2 orders the topological queue by time through a `git_pqueue` whose +comparator is not stable, so the answer depends on insertion order — and a walk +restarted from a cursor inserts differently from one that ran straight through. +The walk-per-page version had the same freedom and used it at every page +boundary; one prepared walk is merely self-consistent where ten were not. What +holds either way is the actual contract — every commit exactly once, no parent +before its child — and that is what the same-second fixture asserts. + +**What is NOT cached: the filtered walk.** `log_filtered_page` still prepares +one walk per page. It can visit far more commits than it returns, so it can run +off the end of a capped prefix mid-page, and handling that well is a different +piece of work from this one. The ref-map half already applies to it. See #476. + ## Reading the log (#274) Where it is — `tauri_plugin_log`'s `LogDir` target, i.e. Tauri's `app_log_dir`: diff --git a/src-tauri/src/git/libgit2.rs b/src-tauri/src/git/libgit2.rs index 662768d..16444b6 100644 --- a/src-tauri/src/git/libgit2.rs +++ b/src-tauri/src/git/libgit2.rs @@ -199,12 +199,31 @@ impl Libgit2Backend { /// page's cost on a repository whose history is short. The fingerprint is /// one enumeration with no peeling; the map behind it is shared by `Arc`, /// so several concurrent pages read one copy. - fn ref_map(&self, repo_id: &RepoId, repo: &Repository) -> Arc { - let fingerprint = ref_fingerprint(repo); - if let Some(map) = self.log_cache.ref_map(repo_id, fingerprint) { - return map; + /// + /// `validate` is false for a CONTINUATION page, which reuses whatever the + /// walk's first page saw without asking again. That is measured, not a + /// guess: on the `refs` fixture, validating costs 113 ms of a 117 ms page, + /// because enumerating 7,001 loose refs is the whole expense and peeling + /// them is 10 ms of it. Paying that once per REFRESH is proportionate — + /// `branches` and `tags` beside it in the same fan-out pay 187 ms and 149 + /// ms for the same enumeration — and once per SCROLL is not. So a + /// continuation decorates with the refs its first page saw: a coherent + /// snapshot rather than a stale one, since the walk it pages is already a + /// snapshot and every refresh starts at a first page. + /// + /// Note the shape of the cold arm. Nothing cached means going straight to + /// `collect_ref_map`, whose own pass yields the fingerprint — asking for + /// one first would enumerate every ref twice for an answer that cannot + /// match anything, and that made a cold fan-out on `refs` slower than the + /// version this replaced. + fn ref_map(&self, repo_id: &RepoId, repo: &Repository, validate: bool) -> Arc { + if let Some((had, map)) = self.log_cache.ref_map(repo_id) { + if !validate || ref_fingerprint(repo) == had { + return map; + } } - let map = Arc::new(collect_ref_map(repo)); + let (map, fingerprint) = collect_ref_map(repo); + let map = Arc::new(map); self.log_cache .put_ref_map(repo_id, fingerprint, Arc::clone(&map)); map @@ -2758,15 +2777,34 @@ fn build_walk_order(repo: &Repository, starts: &[git2::Oid]) -> AppResult u64 { - use std::hash::{Hash, Hasher}; let Ok(refs) = repo.references() else { // Unreadable refdb: a fingerprint nobody can match, so the map is // rebuilt rather than a stale one being served. return u64::MAX; }; - let mut total: u64 = 0; - let mut count: u64 = 0; + let mut fp = RefFingerprint::default(); for r in refs.flatten() { + fp.add(&r); + } + fp.finish() +} + +/// The running fingerprint, so `ref_fingerprint` and `collect_ref_map` cannot +/// drift apart. +/// +/// One definition on purpose: the two have to agree exactly or the map built +/// by one is rejected by the other on the very next call, and nothing about +/// that failure is visible except the cache silently never hitting. +/// `the_two_fingerprint_paths_agree` pins it. +#[derive(Default)] +struct RefFingerprint { + total: u64, + count: u64, +} + +impl RefFingerprint { + fn add(&mut self, r: &git2::Reference<'_>) { + use std::hash::{Hash, Hasher}; let mut h = std::collections::hash_map::DefaultHasher::new(); r.name_bytes().hash(&mut h); match r.target() { @@ -2778,10 +2816,13 @@ fn ref_fingerprint(repo: &Repository) -> u64 { // Commutative, so `references()` may hand them over in any order it // likes; the count is mixed in separately so that a ref whose hash is // zero still moves the answer when it appears or disappears. - total = total.wrapping_add(h.finish()); - count += 1; + self.total = self.total.wrapping_add(h.finish()); + self.count += 1; + } + + fn finish(self) -> u64 { + self.total.wrapping_mul(31).wrapping_add(self.count) } - total.wrapping_mul(31).wrapping_add(count) } /// Accumulates the walk frontier while a page is emitted (#68 G11). @@ -2925,13 +2966,29 @@ fn push_page_start( } } -/// Map git2's per-ref lookup by target OID. Scans once per log call. -fn collect_ref_map(repo: &Repository) -> HashMap> { +/// Map git2's per-ref lookup by target OID, and the fingerprint of the ref +/// database it was built from. +/// +/// The fingerprint rides along rather than being asked for separately because +/// this scan is the expensive thing (#473): on a repository with 7,001 loose +/// refs, one enumeration is 113 ms and the peeling inside it is 10 ms of that. +/// A caller that computed a fingerprint first and then called this would +/// enumerate everything twice. +/// +/// No longer once per log call — see `Libgit2Backend::ref_map`. +fn collect_ref_map(repo: &Repository) -> (HashMap>, u64) { // A map, not a list: every log walk decorates each of its rows from this, // and a linear scan per row was O(refs x rows) with a clone per hit. let mut out: HashMap> = HashMap::new(); - if let Ok(refs) = repo.references() { + let mut fp = RefFingerprint::default(); + let Ok(refs) = repo.references() else { + // Unreadable refdb: a fingerprint nobody can match, so this is rebuilt + // next time rather than a stale map being served forever. + return (out, u64::MAX); + }; + { for r in refs.flatten() { + fp.add(&r); let name = match r.shorthand() { Ok(n) => n.to_string(), Err(_) => continue, @@ -2974,7 +3031,7 @@ fn collect_ref_map(repo: &Repository) -> HashMap> { } } } - out + (out, fp.finish()) } fn parse_reflog_op(raw_message: &str) -> (ReflogOp, String) { @@ -3817,7 +3874,9 @@ impl GitBackend for Libgit2Backend { limit: usize, ) -> AppResult { self.with_repo_read(repo_id, |repo| { - let ref_map = self.ref_map(repo_id, repo); + // A first page validates the decorations; a continuation reuses what + // its first page saw (#473). + let ref_map = self.ref_map(repo_id, repo, cursor.is_none()); let Some(plan) = self.page_plan(repo_id, repo, refspec, cursor, limit)? else { return Ok(LogPage { commits: Vec::new(), @@ -3927,7 +3986,7 @@ impl GitBackend for Libgit2Backend { }; self.with_repo(repo_id, |repo| { - let ref_map = self.ref_map(repo_id, repo); + let ref_map = self.ref_map(repo_id, repo, cursor.is_none()); let mut walk = repo.revwalk()?; walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; let starts = push_page_start(repo, &mut walk, refspec, cursor)?; @@ -4030,7 +4089,7 @@ impl GitBackend for Libgit2Backend { fn commits_since(&self, repo_id: &RepoId, base: &str) -> AppResult> { self.with_repo(repo_id, |repo| { - let ref_map = self.ref_map(repo_id, repo); + let ref_map = self.ref_map(repo_id, repo, true); let head = match repo.head() { Ok(h) => h.peel_to_commit()?.id(), @@ -4080,7 +4139,7 @@ impl GitBackend for Libgit2Backend { limit: usize, ) -> AppResult> { self.with_repo(repo_id, |repo| { - let ref_map = self.ref_map(repo_id, repo); + let ref_map = self.ref_map(repo_id, repo, true); // `resolve_commit` maps a failure to InvalidRef with the offending // spec, so the UI can name the side the user typed wrong. let base_oid = resolve_commit(repo, base)?.id(); diff --git a/src-tauri/src/git/log_cache.rs b/src-tauri/src/git/log_cache.rs index 2aa0976..ad8d900 100644 --- a/src-tauri/src/git/log_cache.rs +++ b/src-tauri/src/git/log_cache.rs @@ -286,12 +286,18 @@ impl LogCache { walk.cursors.insert(cursor, next); } - /// The ref decorations for `fingerprint`, if that is still the ref database - /// we built them from. - pub fn ref_map(&self, repo: &RepoId, fingerprint: u64) -> Option> { + /// The ref decorations this repository last built, and the fingerprint of + /// the ref database they were built from. + /// + /// The caller decides what to do with the pair, and that is the point: a + /// cold call must NOT enumerate the refs to produce a fingerprint it is + /// about to throw away — doing exactly that made a cold eleven-read + /// fan-out on the `refs` fixture slower than the version this replaced + /// (219 ms → 364 ms), because it enumerated 7,001 loose refs twice. + pub fn ref_map(&self, repo: &RepoId) -> Option<(u64, Arc)> { let repos = self.repos.lock().ok()?; let (had, map) = repos.get(repo)?.refs.as_ref()?; - (*had == fingerprint).then(|| Arc::clone(map)) + Some((*had, Arc::clone(map))) } /// File a freshly built ref map. Called only after `collect_ref_map` ran, @@ -467,8 +473,8 @@ mod tests { let c = LogCache::new(); c.put_ref_map(&repo(), 7, Arc::new(RefMap::new())); - assert!(c.ref_map(&repo(), 7).is_some()); - assert!(c.ref_map(&repo(), 8).is_none(), "the ref database moved"); + let (had, _) = c.ref_map(&repo()).expect("cached"); + assert_eq!(had, 7, "the fingerprint travels back with the map"); } #[test] @@ -481,7 +487,7 @@ mod tests { c.forget(&repo()); assert!(c.first_page(&repo(), &key, 1).is_none()); - assert!(c.ref_map(&repo(), 7).is_none()); + assert!(c.ref_map(&repo()).is_none()); } /// Two repositories share this cache and must not share entries — the tab diff --git a/src-tauri/tests/log_walk_cache.rs b/src-tauri/tests/log_walk_cache.rs index 5006f5e..a01af26 100644 --- a/src-tauri/tests/log_walk_cache.rs +++ b/src-tauri/tests/log_walk_cache.rs @@ -313,6 +313,53 @@ fn an_annotated_tag_made_later_shows_up_too() { assert!(tagged, "got {:?}", after.commits[0].refs); } +/// A continuation decorates with the refs its FIRST page saw, and the next +/// first page picks the new one up. +/// +/// This is the one place the cache is visible from outside, and it is a +/// deliberate trade measured on the `refs` fixture: validating the +/// decorations costs 113 ms of a 117 ms page there, because enumerating 7,001 +/// loose refs is the whole expense. Once per refresh is proportionate — the +/// `branches` and `tags` reads beside it in the same fan-out pay 187 ms and +/// 149 ms for the same enumeration — and once per scroll is not. A scroll +/// therefore shows one coherent snapshot of the decorations rather than a +/// different one per page. +#[test] +fn a_continuation_keeps_the_decorations_its_first_page_saw() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 5); + let (be, handle) = tr.open_with_backend(); + + let first = be.log_page(&handle.id, None, None, 2).unwrap(); + let cursor = first.next_cursor.clone().expect("more history"); + + // Tag a commit the NEXT page will contain. + let target = tr + .repo + .find_commit(git2::Oid::from_str(&cursor[0]).unwrap()) + .unwrap(); + tr.repo + .tag_lightweight("mid-scroll", target.as_object(), false) + .unwrap(); + + let next = be + .log_page(&handle.id, None, Some(&cursor), 2) + .unwrap(); + assert!( + next.commits[0].refs.iter().all(|r| r.name != "mid-scroll"), + "a continuation does not re-read the refs: {:?}", + next.commits[0].refs, + ); + + // …and a refresh — which is always a first page — does pick it up. + let refreshed = be.log_page(&handle.id, None, None, 500).unwrap(); + let tagged = refreshed + .commits + .iter() + .any(|c| c.refs.iter().any(|r| r.name == "mid-scroll")); + assert!(tagged, "a first page must validate the decorations"); +} + /// A commit made after a page was cached must appear in the next first page — /// the invalidation that matters most, because it happens every time the user /// commits. @@ -451,6 +498,49 @@ fn a_tag_rebuilds_the_ref_map_and_not_the_walk() { ); } +/// The two places a ref fingerprint is computed must agree. +/// +/// `collect_ref_map` produces one from its own pass and `ref_fingerprint` +/// computes one to validate it, and if the two ever disagree the map built by +/// one is rejected by the other on the very next call. Nothing about that is +/// visible from outside — pagination stays correct, the decorations stay +/// correct, and the cache simply never hits again. So it is asserted through +/// the only door there is: a repository with every shape of ref in it, read +/// twice, must rebuild nothing the second time. +#[test] +fn the_two_fingerprint_paths_agree() { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, 3); + { + let head = tr.repo.head().unwrap().peel_to_commit().unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + tr.repo.branch("topic", &head, false).unwrap(); + tr.repo + .tag_lightweight("light", head.as_object(), false) + .unwrap(); + tr.repo + .tag("heavy", head.as_object(), &sig, "annotated", false) + .unwrap(); + // A symbolic ref has no direct target, so it takes the other arm of the + // fingerprint — and of the map. + tr.repo + .reference_symbolic("refs/remotes/origin/HEAD", "refs/heads/main", true, "probe") + .unwrap(); + } + let (be, handle) = tr.open_with_backend(); + + be.log_page(&handle.id, Some("--all"), None, 500).unwrap(); + let first = be.log_cache_stats(); + be.log_page(&handle.id, Some("--all"), None, 500).unwrap(); + + assert_eq!( + be.log_cache_stats().ref_maps_built, + first.ref_maps_built, + "the validating fingerprint disagrees with the one the map was built \ + with, so the cache can never hit", + ); +} + /// Nothing changed, so nothing is rebuilt — the case that is true on almost /// every call, and the one a fingerprint that is not stable would silently /// ruin. From 1bb070e2ae532601eed03e4321f5705f09387495 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 18:40:34 +0200 Subject: [PATCH 3/6] docs: the paged log prepares one walk, and what may cache alongside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pointer would not serve here. The rule a future change breaks silently is that everything `git/log_cache.rs` holds is derivable from disk, so it has to stay a pure accelerator — and that a third cached thing needs its own answer to "what makes this wrong?", because the two already there answer it differently on purpose. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index cdef78e..17eb69b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -264,6 +264,16 @@ Each rule's full story (why, traps, tests that pin it) is in the named doc. - **The log is paged** — `s.commits` is a prefix of history, never the answer to "does X exist / is X an ancestor"; ask the backend. (`docs/dev/frontend.md`) +- **…and the backend prepares ONE walk for all those pages** (`git/log_cache.rs`, + #473). A sorted libgit2 revwalk materialises the whole ordered list before it + yields anything, so a walk per page cost the same per page — 157 s at page ten + of the kernel. Everything cached there is derivable from disk, so it must stay + a PURE ACCELERATOR: a lock failure is a miss and never a failed page, and the + proof is `log_walk_cache.rs` draining the same history warm and cold and + comparing. A first page is invalidated by its start oids, a continuation by + nothing (commits are immutable), the decorations by a ref fingerprint — add a + third cached thing and it needs its own answer to "what makes this wrong?". + (`docs/dev/backend.md`, `docs/dev/performance.md`) - **A commit row's columns have a YIELD ORDER, and it is the template.** Every track in `commitRowGrid` but the subject and the author is a fixed width, so a new fixed column — or a wider one — comes straight out of the subject, From b19486d2048765294ce9ef6b3ab9e265c23cdef0 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 18:54:35 +0200 Subject: [PATCH 4/6] perf(log): page commit search out of the prepared walk too log_filtered_page had both of the defects log_page had, for the same reason and in the same two lines: a revwalk built per page, and the ref map rebuilt per page. The ref-map half was already fixed; this is the walk. Search is where it hurt most. A search that matches nothing recent walks a long way before it fills a page -- and then the next page threw that prepared walk away and sorted all of history again. **Why it cannot just call `page_plan`.** A filtered page VISITS far more commits than it returns, so it can run off the end of an order capped at MAX_ORDER in the middle of a page, and a short page there is indistinguishable -- to the user and to the frontend -- from "no more matches exist". So `cached_filtered_plan` asks `WalkOrder::serves` with `usize::MAX`, which admits any offset into a COMPLETE order and none at all into a capped one, and a miss falls back to the revwalk-per-page the filtered log always had. A history past the cap is no worse off than before. **And why it must not prepare one speculatively.** Preparing a walk and then discovering it is capped pays for the topological sort twice, on exactly the repositories where that sort is most expensive. `LogCache::has_walk` answers "has this walk been prepared before" without counting a hit or reordering anything -- a different question from `first_page`'s "can this serve the page in hand". A filtered page prepares and files a walk in one case only: when nothing is filed for that key at all. The page was going to pay for a walk anyway, and a search should not depend on the log having been read first to be fast, even though in the app it always has been. Both sources run one `matches` closure through the generic `filtered_page`, because a search returning different matches depending on whether a walk happened to be cached is the one bug this must not have. The frontier is filed against an offset ONLY when the page came out of a cached order: a page that walked for itself ends somewhere no order has an index for. `log_filtered_cache.rs` keeps the same split as `log_walk_cache.rs` -- transparency by draining one search warm and cold and comparing, effect by asserting on the counters, because every transparency test here passes just as well against a cache that never hits. Verified by planting: serving every filtered page from its own walk takes `paging_a_search_prepares_one_walk` from 1 to 4. Refs #473 Co-Authored-By: Claude Opus 5 (1M context) --- docs/dev/backend.md | 38 +++- src-tauri/src/git/libgit2.rs | 257 +++++++++++++++++++++----- src-tauri/src/git/log_cache.rs | 23 +++ src-tauri/tests/log_filtered_cache.rs | 193 +++++++++++++++++++ 4 files changed, 462 insertions(+), 49 deletions(-) create mode 100644 src-tauri/tests/log_filtered_cache.rs diff --git a/docs/dev/backend.md b/docs/dev/backend.md index d37f70a..178e5ae 100644 --- a/docs/dev/backend.md +++ b/docs/dev/backend.md @@ -1616,10 +1616,40 @@ boundary; one prepared walk is merely self-consistent where ten were not. What holds either way is the actual contract — every commit exactly once, no parent before its child — and that is what the same-second fixture asserts. -**What is NOT cached: the filtered walk.** `log_filtered_page` still prepares -one walk per page. It can visit far more commits than it returns, so it can run -off the end of a capped prefix mid-page, and handling that well is a different -piece of work from this one. The ref-map half already applies to it. See #476. +**The filtered walk reads the same order.** `log_filtered_page` is commit +search, and it had both defects for the same reason and in the same two lines. +It is also where they hurt most: a search that matches nothing recent walks a +long way before it fills a page, and the next page threw that walk away and +sorted all of history again. + +It cannot go through `page_plan`, and the reason is worth keeping. A filtered +page visits far more commits than it returns, so it can run off the end of an +order capped at `MAX_ORDER` in the middle of a page — and a short page there is +indistinguishable, to the user and to the frontend, from "no more matches +exist". So `cached_filtered_plan` asks `WalkOrder::serves` with `usize::MAX`, +which admits any offset into a COMPLETE order and no offset at all into a capped +one; a miss falls back to the revwalk-per-page the filtered log always had. A +history past the cap is therefore no worse off than before #473. Filling the +page from the cached prefix and then continuing into a live walk is the version +that would help there too, and it needs a frontier the builder can hand back +mid-page. + +It must also not PREPARE a walk to find that out: preparing one and then +discovering it is capped pays for the topological sort twice, on exactly the +repositories where that sort is most expensive. `LogCache::has_walk` answers +"has this walk been prepared before" without counting a hit or moving anything, +which is a different question from `first_page`'s "can this serve the page in +hand". The one time a filtered page does prepare and file a walk is when nothing +is filed for that key at all — the page was going to pay for a walk anyway, and +a search should not depend on the log having been read first to be fast, even +though in the app it always has been. + +Both sources run one `matches` closure, through the generic `filtered_page`, +because a search that returned different matches depending on whether a walk +happened to be cached is the one bug this change must not have. The frontier it +emits is filed against an offset ONLY when the page came out of a cached order: +a page that walked for itself ends somewhere no order has an index for, and +filing that offset would put the next page at the wrong depth. ## Reading the log (#274) diff --git a/src-tauri/src/git/libgit2.rs b/src-tauri/src/git/libgit2.rs index 16444b6..da8d7a9 100644 --- a/src-tauri/src/git/libgit2.rs +++ b/src-tauri/src/git/libgit2.rs @@ -234,6 +234,102 @@ impl Libgit2Backend { /// `Ok(None)` is "nothing to walk" — an unborn HEAD, or a cursor whose /// every oid is missing from a shallow clone — and the caller returns an /// empty page for it, exactly as the walk-per-page version did. + /// The prepared walk a FILTERED page can be served from, if one is already + /// cached. Never prepares one. + /// + /// Separate from `page_plan` because a filtered page differs from a plain + /// one in two ways that both point here. + /// + /// **Only a COMPLETE order can answer it.** A search visits far more + /// commits than it returns, so it can run off the end of an order capped at + /// `MAX_ORDER` in the middle of a page — and a short page there is + /// indistinguishable, to the user and to the frontend, from "no more + /// matches exist". `usize::MAX` is how that is asked: `WalkOrder::serves` + /// admits any offset into a complete order and no offset at all into a + /// capped one, which is exactly the question. + /// + /// **It must not PREPARE one to find out.** Preparing a walk and then + /// discovering it is capped would pay for the topological sort twice, on + /// precisely the repositories where that sort is most expensive. A miss + /// here falls back to the revwalk-per-page the filtered log always had, so + /// a repository past the cap is no worse off than before #473 — filling the + /// page from the cached prefix and then continuing into a live walk is the + /// version that would help there too, and it needs a frontier the builder + /// can hand back mid-page. + fn cached_filtered_plan( + &self, + repo_id: &RepoId, + repo: &Repository, + refspec: Option<&str>, + cursor: Option<&[String]>, + ) -> AppResult> { + // A continuation. `refspec` is ignored here, as it always has been: the + // frontier already encodes the walk this page continues. + if let Some(frontier) = cursor.filter(|c| !c.is_empty()) { + let mut want = Vec::with_capacity(frontier.len()); + for raw in frontier { + want.push( + git2::Oid::from_str(raw).map_err(|_| AppError::InvalidRef(raw.clone()))?, + ); + } + let mut sorted = want.clone(); + sorted.sort(); + let Some((key, order, offset)) = self.log_cache.resume(repo_id, &sorted, usize::MAX) + else { + return Ok(None); + }; + // Seeded with the cursor rather than the walk's own start points: a + // lane this page stops short of has to survive into the NEXT + // cursor, and at this depth the live lanes are the ones the caller + // just handed back. + return Ok(Some(PagePlan { + order, + offset, + seed: live_commits(repo, &want), + key, + })); + } + + let starts = resolve_log_starts(repo, refspec)?; + if starts.is_empty() { + return Ok(None); + } + let key = WalkKey::new(refspec, &starts); + if let Some(order) = self.log_cache.first_page(repo_id, &key, usize::MAX) { + return Ok(Some(PagePlan { + seed: order.starts.clone(), + order, + offset: 0, + key, + })); + } + + // Nothing usable filed. If a walk for this key has been prepared + // before, the only reason it cannot serve is the cap, and preparing a + // second one would rediscover that at full price — so search on past + // the cap, exactly as this function did before #473. + if self.log_cache.has_walk(repo_id, &key) { + return Ok(None); + } + // Otherwise this is the first anyone has asked, and the page was going + // to pay for a walk regardless. Prepare it through the same path the + // plain log uses and file it, so the pages behind this one are slices + // rather than walks — a search must not depend on the log having been + // read first to be fast, even though in the app it always has been. + let order = Arc::new(self.prepare_walk(repo, &starts)?); + self.log_cache.insert(repo_id, key.clone(), Arc::clone(&order)); + if !order.complete { + return Ok(None); + } + Ok(Some(PagePlan { + seed: order.starts.clone(), + order, + offset: 0, + key, + })) + } + + fn page_plan( &self, repo_id: &RepoId, @@ -2925,6 +3021,61 @@ fn live_commits(repo: &Repository, want: &[git2::Oid]) -> Vec { } /// Which prepared walk a page is a slice of, and where the slice starts. +/// One page of a FILTERED log, read from whatever is handing out commits. +/// +/// Generic over the source because there are two and they must behave +/// identically: a slice of a prepared walk's order, and a revwalk this page +/// built for itself. Returns the rows, the frontier to resume from, and HOW +/// MANY commits were read — which is what the caller files the frontier +/// against, and it is not `commits.len()`, because a search reads far more +/// than it returns. +/// +/// Take from `rows` ONLY while there is room for another match. +/// `for oid in rows { if out.len() >= limit { break } … }` yields an oid first +/// and discards it on the break, without recording it in the frontier — and a +/// cursor start-point lost that way is in neither `visited` nor `candidates`, +/// so `finish` omits it and every commit reachable only through that lane +/// disappears from the log for good. The plain log sidesteps this by taking +/// exactly `limit`; a search cannot, because it decides per commit whether one +/// counts. +fn filtered_page( + repo: &Repository, + rows: I, + ref_map: &RefMap, + seed: &[git2::Oid], + limit: usize, + mut matches: impl FnMut(&Repository, &git2::Commit<'_>) -> AppResult, +) -> AppResult<(Vec, Option>, usize)> +where + I: Iterator>, +{ + let mut rows = rows; + let mut out = Vec::new(); + // The frontier tracks every VISITED commit, not just the matches: resuming + // from a match's parents would skip the non-matching commits between them + // and lose their ancestors entirely. + let mut frontier = FrontierBuilder::new(limit.min(4096)); + frontier.seed(seed); + let mut read = 0usize; + + while out.len() < limit { + let Some(oid) = rows.next() else { break }; + let oid = oid?; + let commit = repo.find_commit(oid)?; + frontier.visit(oid, commit.parent_ids()); + read += 1; + if !matches(repo, &commit)? { + continue; + } + let mut info = commit_to_info(&commit); + info.refs = ref_map.get(&oid).cloned().unwrap_or_default(); + out.push(info); + } + + Ok((out, frontier.finish(repo), read)) +} + + struct PagePlan { order: Arc, offset: usize, @@ -3986,45 +4137,29 @@ impl GitBackend for Libgit2Backend { }; self.with_repo(repo_id, |repo| { + // A first page validates the decorations; a continuation reuses + // what its first page saw (#473). let ref_map = self.ref_map(repo_id, repo, cursor.is_none()); - let mut walk = repo.revwalk()?; - walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; - let starts = push_page_start(repo, &mut walk, refspec, cursor)?; - if starts.is_empty() { - return Ok(LogPage { - commits: Vec::new(), - next_cursor: None, - }); - } - let mut out = Vec::new(); - // The frontier tracks every VISITED commit, not just the matches: - // resuming from a match's parents would skip the non-matching - // commits between them and lose their ancestors entirely. - let mut frontier = FrontierBuilder::new(limit.min(4096)); - frontier.seed(&starts); - // Pull from the walk ONLY while there is room for another match. - // `for oid in walk { if out.len() >= limit { break } … }` yields an oid - // first and discards it on the break, without recording it in the - // frontier — and a cursor start-point lost that way is in neither - // `visited` nor `candidates`, so `finish` omits it and every commit - // reachable only through that lane disappears from the log for good. - // `log_page` sidesteps this with `walk.by_ref().take(limit)`; the - // filtered walk cannot, because it decides per commit whether one - // counts towards the limit. - let mut walk = walk; - while out.len() < limit { - let Some(oid) = walk.next() else { break }; - let oid = oid?; - let commit = repo.find_commit(oid)?; - frontier.visit(oid, commit.parent_ids()); + // Which commits this page reads, and in what sequence: a prepared + // walk the log already paid for when one can answer the question, + // and otherwise a revwalk of this page's own — which is what EVERY + // filtered page did before #473. + let plan = self.cached_filtered_plan(repo_id, repo, refspec, cursor)?; + + // Whether a commit counts towards `limit`. Written once and handed + // to whichever source is reading, so the two cannot drift: a search + // returning different matches depending on whether a walk happened + // to be cached is the one bug this change must not have. + let matches = |repo: &Repository, commit: &git2::Commit<'_>| -> AppResult { + let oid = commit.id(); // sha prefix — cheap, check first. Compared nibble-by-nibble // off the raw bytes: `oid.to_string()` allocated 40 hex chars // for EVERY commit the walk visits, matching or not. if let Some(ref q) = sha_q { if !oid_has_hex_prefix(&oid, q) { - continue; + return Ok(false); } } @@ -4032,12 +4167,12 @@ impl GitBackend for Libgit2Backend { let ts = commit.time().seconds(); if let Some(since) = filter.since { if ts < since { - continue; + return Ok(false); } } if let Some(until) = filter.until { if ts > until { - continue; + return Ok(false); } } @@ -4047,7 +4182,7 @@ impl GitBackend for Libgit2Backend { let name = author.name().unwrap_or("").to_lowercase(); let email = author.email().unwrap_or("").to_lowercase(); if !name.contains(q.as_str()) && !email.contains(q.as_str()) { - continue; + return Ok(false); } } @@ -4055,14 +4190,14 @@ impl GitBackend for Libgit2Backend { if let Some(ref q) = message_q { let msg = commit.message().unwrap_or("").to_lowercase(); if !msg.contains(q.as_str()) { - continue; + return Ok(false); } } // path — expensive, check late. if let Some(ref p) = path_q { - if !commit_touches_path(repo, &commit, p)? { - continue; + if !commit_touches_path(repo, commit, p)? { + return Ok(false); } } @@ -4070,19 +4205,51 @@ impl GitBackend for Libgit2Backend { // commit, so it runs LAST: an author- or path-scoped search // only diffs the commits every cheaper filter already accepted. if let Some(ref m) = content_m { - if !commit_diff_matches_content(repo, &commit, m, path_q.as_deref())? { - continue; + if !commit_diff_matches_content(repo, commit, m, path_q.as_deref())? { + return Ok(false); } } - let refs: Vec = ref_map.get(&oid).cloned().unwrap_or_default(); - let mut info = commit_to_info(&commit); - info.refs = refs; - out.push(info); + Ok(true) + }; + + let (commits, next, at, filed) = match &plan { + Some(p) => { + let rows = p.order.order[p.offset..].iter().copied().map(Ok); + let (commits, next, read) = + filtered_page(repo, rows, &ref_map, &p.seed, limit, matches)?; + (commits, next, p.offset + read, Some(p.key.clone())) + } + None => { + let mut walk = repo.revwalk()?; + walk.set_sorting(Sort::TIME | Sort::TOPOLOGICAL)?; + let starts = push_page_start(repo, &mut walk, refspec, cursor)?; + if starts.is_empty() { + return Ok(LogPage { + commits: Vec::new(), + next_cursor: None, + }); + } + self.log_cache.record_walk_prepared(); + let rows = walk.map(|r| r.map_err(AppError::from)); + let (commits, next, read) = + filtered_page(repo, rows, &ref_map, &starts, limit, matches)?; + (commits, next, read, None) + } + }; + + // File the frontier against where it continues, so the page after + // this one slices the same prepared walk. ONLY when this page came + // out of a cached order: a page that walked for itself ends + // somewhere no order has an index for, and filing that offset + // against one would put the next page at the wrong depth. + if let (Some(f), Some(key)) = (&next, filed) { + self.log_cache.remember_cursor(repo_id, &key, f.clone(), at); } + Ok(LogPage { - next_cursor: frontier.finish(repo).as_deref().map(cursor_strings), - commits: out, + next_cursor: next.as_deref().map(cursor_strings), + commits, }) }) } diff --git a/src-tauri/src/git/log_cache.rs b/src-tauri/src/git/log_cache.rs index ad8d900..634731c 100644 --- a/src-tauri/src/git/log_cache.rs +++ b/src-tauri/src/git/log_cache.rs @@ -247,6 +247,29 @@ impl LogCache { } /// File a freshly prepared walk, evicting the least recently used one. + /// Whether a walk for `key` is already filed, whatever it can answer. + /// + /// Not the same question as `first_page`, and the difference is the point. + /// `first_page` says "can this serve the page in hand", and a CAPPED order + /// says no to a filtered page every time (see `cached_filtered_plan`). This + /// says "has this walk been prepared before", so a search does not prepare + /// a second one to rediscover that the history is longer than `MAX_ORDER` — + /// which would pay for the topological sort twice on every search, on + /// exactly the repositories where it is most expensive. + /// + /// Deliberately counts no hit and moves nothing: it is a question about the + /// cache, not a read from it. + pub fn has_walk(&self, repo: &RepoId, key: &WalkKey) -> bool { + self.repos + .lock() + .map(|repos| { + repos + .get(repo) + .is_some_and(|e| e.walks.iter().any(|w| &w.key == key)) + }) + .unwrap_or(false) + } + pub fn insert(&self, repo: &RepoId, key: WalkKey, order: Arc) { let Ok(mut repos) = self.repos.lock() else { return; diff --git a/src-tauri/tests/log_filtered_cache.rs b/src-tauri/tests/log_filtered_cache.rs new file mode 100644 index 0000000..2a4d92e --- /dev/null +++ b/src-tauri/tests/log_filtered_cache.rs @@ -0,0 +1,193 @@ +//! The FILTERED log pages out of the same prepared walk (#473). +//! +//! `log_filtered_page` had both of the defects `log_page` had, for the same +//! reason and in the same two lines: a revwalk built per page, and +//! `collect_ref_map` called per page. Commit search is the surface where that +//! hurts most on a large repository, because a search that matches nothing +//! recent walks a long way before it fills a page — and then the next page +//! threw that walk away and started the topological sort again. +//! +//! Same split as `log_walk_cache.rs`, and for the same reason. **Transparency** +//! drains one search warm and cold and compares, which is the pre-change +//! behaviour held against the new one. **Effect** asserts on the counters, +//! because every transparency test here passes just as well against a cache +//! that never hits. + +mod support; + +use platypusgit_lib::git::libgit2::Libgit2Backend; +use platypusgit_lib::git::types::{LogFilter, LogPage, RepoId}; +use platypusgit_lib::git::GitBackend; +use support::{linear_history, TempRepo}; + +/// Matches `commit 1`, `commit 10`…`commit 19` — 11 of 30, spread across the +/// history rather than bunched at one end, so a page boundary falls inside the +/// matches and the walk has to carry on past commits that do not match. +fn filter() -> LogFilter { + LogFilter { + message: Some("commit 1".into()), + ..Default::default() + } +} + +fn summaries(page: &LogPage) -> Vec { + page.commits.iter().map(|c| c.summary.clone()).collect() +} + +/// Page a search through ONE backend — the cache is hot from page two. +fn search_warm(be: &Libgit2Backend, id: &RepoId, page: usize) -> Vec { + let mut out = Vec::new(); + let mut cursor: Option> = None; + loop { + let p = be + .log_filtered_page(id, &filter(), None, cursor.as_deref(), page) + .unwrap(); + out.extend(summaries(&p)); + match p.next_cursor { + Some(c) => cursor = Some(c), + None => break, + } + assert!(out.len() < 10_000, "pagination did not terminate"); + } + out +} + +/// The same search, on a backend that has never seen this repository — so +/// every page pays for its own walk, exactly as every page used to. +fn search_cold(tr: &TempRepo, page: usize) -> Vec { + let mut out = Vec::new(); + let mut cursor: Option> = None; + loop { + let (be, handle) = tr.open_with_backend(); + let p = be + .log_filtered_page(&handle.id, &filter(), None, cursor.as_deref(), page) + .unwrap(); + out.extend(summaries(&p)); + match p.next_cursor { + Some(c) => cursor = Some(c), + None => break, + } + assert!(out.len() < 10_000, "pagination did not terminate"); + } + out +} + +fn history(n: usize) -> TempRepo { + let tr = TempRepo::with_initial_commit("hi\n"); + linear_history(&tr, n); + tr +} + +/// Transparency: the cache changes the clock and nothing else. +#[test] +fn a_paged_search_returns_the_same_commits_warm_and_cold() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + let warm = search_warm(&be, &handle.id, 4); + let cold = search_cold(&tr, 4); + + assert_eq!(warm, cold, "a cached search must return what a cold one does"); + assert_eq!(warm.len(), 11, "got {warm:?}"); +} + +/// Transparency, at a page size that puts a boundary between two matches. +#[test] +fn a_paged_search_agrees_with_an_unpaged_one() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + let paged = search_warm(&be, &handle.id, 3); + let whole: Vec = be + .log_filtered(&handle.id, &filter(), None, 100) + .unwrap() + .iter() + .map(|c| c.summary.clone()) + .collect(); + + assert_eq!(paged, whole, "paging a search must not drop or reorder a match"); +} + +/// Effect: the walk. THE test for the filtered half of #473 — planting the +/// violation (build a revwalk per filtered page, as it did) makes this one +/// walk per page instead. +#[test] +fn paging_a_search_prepares_one_walk() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + let found = search_warm(&be, &handle.id, 3); + + assert_eq!(found.len(), 11, "got {found:?}"); + assert_eq!( + be.log_cache_stats().walks_prepared, + 1, + "a search paged four deep must prepare ONE walk, not one per page", + ); +} + +/// Effect: the ref map. It decorated 500 rows by enumerating and peeling every +/// ref, once per page. +#[test] +fn paging_a_search_builds_one_ref_map() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + search_warm(&be, &handle.id, 3); + + assert_eq!( + be.log_cache_stats().ref_maps_built, + 1, + "a continuation must decorate with the refs its first page saw", + ); +} + +/// A search asks the same question of the same graph as the log beside it, so +/// it reads the walk the log already prepared rather than preparing a second. +#[test] +fn a_search_reuses_the_walk_the_log_prepared() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + be.log_page(&handle.id, None, None, 5).unwrap(); + assert_eq!(be.log_cache_stats().walks_prepared, 1); + + let hits = be + .log_filtered_page(&handle.id, &filter(), None, None, 100) + .unwrap(); + + assert_eq!(summaries(&hits).len(), 11); + assert_eq!( + be.log_cache_stats().walks_prepared, + 1, + "a search must not re-prepare a walk the log already materialised", + ); +} + +/// A commit moves HEAD, so the search's walk is rebuilt rather than served +/// from an order that no longer describes this repository. +#[test] +fn a_moved_head_re_prepares_the_search_walk() { + let tr = history(30); + let (be, handle) = tr.open_with_backend(); + + be.log_filtered_page(&handle.id, &filter(), None, None, 5) + .unwrap(); + assert_eq!(be.log_cache_stats().walks_prepared, 1); + + tr.add_commit("late.txt", "late\n", "commit 100"); + + let after = be + .log_filtered_page(&handle.id, &filter(), None, None, 5) + .unwrap(); + + assert_eq!( + after.commits[0].summary, "commit 100", + "a moved HEAD must not be searched from the cached order", + ); + assert_eq!( + be.log_cache_stats().walks_prepared, + 2, + "the walk must be re-prepared once HEAD has moved", + ); +} From b8e419ae03f0caa7f20e13042cc731ec440a9267 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 18:54:48 +0200 Subject: [PATCH 5/6] docs(perf): record what fixed findings 2 and 3, and why no after-numbers performance.md still described both defects as open. They are fixed, so the two findings now say what fixed them -- and carry the measurement the design rests on, which is the opposite of what "just cache it" usually costs: draining an ALREADY-PREPARED revwalk is 1.9 ms for the deep fixture's remaining 49,500 commits and 63.4 ms for the kernel's remaining 1,482,423, against 560.2 ms and 31.8 s to prepare. 0.2% more buys the whole order, which is why the cache holds a Vec and not the prepared Revwalk. Finding 3 keeps the two counter-intuitive things found on the way: the PEELING is not the expense (one enumeration on `refs` is 113 ms, the peel inside it 10 ms), and computing a ref fingerprint before building the map on a cold cache enumerates every ref twice and made a cold fan-out SLOWER than the code it replaced (219 -> 364 ms), which hides because open_screen builds a fresh backend per sample. **No after-numbers, deliberately, and the doc says so.** Two sessions were benchmarking this repository at once and $PGBENCH_HOME is one shared directory, so control rows moved 25-30% on operations neither change touches (wide status 5.38 -> 3.92 s, diff_commit 1.53 -> 1.12 s). That is not a result, it is two programs sharing a machine. The generated results block is untouched and still the "before" it always was; re-measure on a quiet machine with one session running before publishing an after. Refs #473 Co-Authored-By: Claude Opus 5 (1M context) --- docs/dev/performance.md | 77 ++++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/docs/dev/performance.md b/docs/dev/performance.md index 18dcf76..9ca6f6b 100644 --- a/docs/dev/performance.md +++ b/docs/dev/performance.md @@ -166,7 +166,7 @@ not a good answer, and publishing it is the point of the exercise: the user who opens a 1.4-million-commit repository and waits is the user this project was written for. -### 2. The log walk is not slow — the topological SORT is, and it is re-paid per page +### 2. The log walk is not slow — the topological SORT is, and it was re-paid per page The obvious reading of "our 500-commit page takes 252 ms and `git log -500` takes 41 ms" is that the walk is six times too slow. It is wrong, and it nearly @@ -179,8 +179,8 @@ not sort that way. Asked the same question, `git log --topo-order -500` costs this benchmark are `--topo-order` for exactly that reason, and the near miss is written up in the spec. -What survives is sharper. git pays for that sort **once and then skips**; we pay -it per page: +What survived was sharper. git pays for that sort **once and then skips**; we +paid it again on every page: | | `deep` (50k commits) | `torvalds/linux` (1.5M commits) | | --- | --- | --- | @@ -188,15 +188,44 @@ it per page: | our tenth page | 2.40 s | 157.67 s | | `git log --topo-order --skip=4500 -500` | 193 ms | 9.68 s | -The per-page cost is flat in depth — ten pages cost ten times one page — which -is the signature of restarting the walk rather than continuing it. The cursor -already carries the frontier, so the walk logically continues; it is the sort -that is rebuilt. +The per-page cost was flat in depth — ten pages cost ten times one page — which +is the signature of restarting the walk rather than continuing one. -### 3. The ref map is rebuilt on every page, too +#### The measurement the fix rests on -`log_page` calls `collect_ref_map(repo)` per call, enumerating and peeling every -ref so the page can decorate its 500 commits. Two fixtures isolate it, and the +Fixed in #473 by preparing the walk once and paging out of what it produced. +The reason that is affordable is one measurement, and it is worth recording +because it is the opposite of what "just cache it" usually costs. Draining a +revwalk that has **already been prepared** is very nearly free, because libgit2 +does the ordering during preparation and then hands commits out of a list it +already has: + +| | prepare + take(500) | drain the whole rest of the order | +| --- | --- | --- | +| `deep` (50,000 commits) | 560.2 ms | **1.9 ms** (49,500 more) | +| `torvalds/linux` (1,482,923 commits) | 31.8 s | **63.4 ms** (1,482,423 more) | + +0.2% more buys the entire order, which is why the cache holds a `Vec` +rather than the prepared `Revwalk` — holding the walk would mean holding a +`git2::Repository` alive between IPC calls, outliving the lock acquisition +`git/repo_locks.rs` orders every access by, to save that 0.2%. The source-level +reason it comes out this way is in `src-tauri/src/git/log_cache.rs`: +`git_revwalk_sorting` sets `walk->limited`, so `prepare_walk` runs `limit_list` +over the whole reachable graph and `sort_in_topological_order` materialises the +complete ordered list before the first oid is yielded. + +**The tables below have not been re-measured for that change**, and the numbers +in this section are the "before" they always were. Two sessions were benchmarking +this repository at once, and `$PGBENCH_HOME` is one shared directory: control +rows moved 25–30% on operations neither change touches (`wide` status 5.38 s → +3.92 s, `diff_commit` 1.53 s → 1.12 s), which is not a result, it is two +programs sharing a machine. Re-measure on a quiet machine, with one session +running, before publishing an after. + +### 3. The ref map was rebuilt on every page, too + +`log_page` called `collect_ref_map(repo)` per call, enumerating and peeling every +ref so the page could decorate its 500 commits. Two fixtures isolate it, and the variable between them is refs rather than history: | fixture | history | refs | ours | `git` work | ratio | @@ -204,9 +233,31 @@ variable between them is refs rather than history: | `deep` | 50,000 commits | 1 | 252 ms | 194 ms | 1.3× | | `refs` | 2,000 commits | 7,001 | 135 ms | 8.5 ms | **16×** | -Twenty-five times *less* history, and still most of the cost. This one looks -much cheaper to fix than the sort: cache the map per repository and invalidate -it on ref writes. +Twenty-five times *less* history, and still most of the cost. + +Fixed in #473 alongside the walk, and two things found on the way are worth +keeping, because both are counter-intuitive: + +* **The peeling is not the expense.** This issue was written as "enumerating and + peeling every ref"; measured on `refs`, one enumeration costs 113 ms and the + peel inside it costs 10 ms. So the map is validated on a FIRST page and reused + by the continuations behind it — revalidating per scroll cost 113 ms of a + 117 ms page, for a question that cannot have changed. +* **Do not compute a fingerprint before building the map on a cold cache.** It + enumerates every ref twice for an answer that cannot match anything, and it + made a cold eleven-read fan-out on `refs` *slower* than the code it replaced + (219 ms → 364 ms). It hides, too: `open_screen` builds a fresh backend per + sample, so that path is always the cold one. `collect_ref_map` returns the + fingerprint from its own pass for this reason. + +The same two defects were in `log_filtered_page`, which is commit search — the +surface where they hurt most, because a search that matches nothing recent walks +a long way before it fills a page, and the next page threw that walk away. It +reads the same prepared order now. One limit is deliberate and written down in +`cached_filtered_plan`: a search visits far more commits than it returns, so +only an order that is ALL of history can serve one, and a repository past +`MAX_ORDER` falls back to the walk-per-page it always had rather than to a short +page that would look like "no more matches exist". ### What is already good, and worth not breaking From ece254c19eff34d65da5d1b78552264ca646a424 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 18:57:59 +0200 Subject: [PATCH 6/6] docs(perf): a one-fixture bench run republishes the other three "Re-measure on a quiet machine" was not enough of a warning. The renderer publishes every result it finds in $PGBENCH_HOME/results, which is deliberate -- bench.sh says so, and it is what stops `pnpm bench --fixture deep` from silently deleting torvalds/linux from the published record -- but it also means a one-fixture run publishes fresh numbers for that fixture and whatever happens to be sitting beside it. **Why:** that is the shape a reviewer cannot catch. test/benchmark.test.ts checks the markdown against the JSON, so a record built from one fresh fixture and three stale ones is wrong and self-consistent, and passes. Refs #473 Co-Authored-By: Claude Opus 5 (1M context) --- docs/dev/performance.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/dev/performance.md b/docs/dev/performance.md index 9ca6f6b..9537178 100644 --- a/docs/dev/performance.md +++ b/docs/dev/performance.md @@ -222,6 +222,14 @@ rows moved 25–30% on operations neither change touches (`wide` status 5.38 s programs sharing a machine. Re-measure on a quiet machine, with one session running, before publishing an after. +And re-measure **all four fixtures**, or knowingly keep the rest. The renderer +publishes every result it finds in `$PGBENCH_HOME/results`, which is deliberate +— it is what stops `pnpm bench --fixture deep` from silently deleting +`torvalds/linux` from the record — but it also means a one-fixture run +publishes fresh numbers for that fixture and whatever happens to be sitting +beside it. `test/benchmark.test.ts` cannot catch that: it checks the markdown +against the JSON, and both would be wrong together. + ### 3. The ref map was rebuilt on every page, too `log_page` called `collect_ref_map(repo)` per call, enumerating and peeling every