From fec89459e08ef24f162d6317478c18b89d553004 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Tue, 20 Oct 2026 17:05:06 -0400 Subject: [PATCH 1/4] PERF-SMART: test a smart album's rules cheapest first A rule set is now tested cheapest rule first (numbers, choices and yes/no, then lists and dates, then text and paths, then smart albums), worked out once per prepared filter and per smart album, not per photo. Photos matched are the same; the editor still shows the user's order. While a smart album tests itself, or a chain of smart albums reaches the depth limit, rules are tested as written, since there an answer can depend on which album is asked first. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/lib.rs | 90 ++++++- crates/catalog/src/query.rs | 46 +++- crates/catalog/src/rules.rs | 64 +++++ crates/catalog/src/tests_rule_order.rs | 324 +++++++++++++++++++++++++ docs/whats-new.md | 9 + 5 files changed, 517 insertions(+), 16 deletions(-) create mode 100644 crates/catalog/src/tests_rule_order.rs diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 453b68a69..5a80021f2 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -294,6 +294,10 @@ pub struct Catalog { /// Lookups derived from the above (never saved), see [`index`]. #[serde(skip)] index: index::Indexes, + /// Each smart album's rules ready to run (cheapest rule first), worked out when first asked and + /// forgotten when an op adds or removes an album or changes rules. + #[serde(skip)] + plans: PlansMemo, } /// What of a photo an op changed: enough to tell which tallies of the library an edit leaves as they were (a @@ -332,6 +336,32 @@ impl Aspects { /// The most photos remembered as touched; older changes are then answered with "anything may have changed". const MAX_TOUCHED: usize = 8192; +/// [`Catalog::plans`] as last worked out: derived from the albums, like [`index::Indexes`], so no +/// part of what a catalog is; a copy starts without it. +#[derive(Default)] +struct PlansMemo { + plans: std::sync::OnceLock>, + guard: std::sync::OnceLock, +} + +impl Clone for PlansMemo { + fn clone(&self) -> Self { + Self::default() + } +} + +impl PartialEq for PlansMemo { + fn eq(&self, _: &PlansMemo) -> bool { + true + } +} + +impl std::fmt::Debug for PlansMemo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("PlansMemo") + } +} + /// A short memory of what the latest ops changed, so that what is derived from the catalog (the grid's list) can /// follow a small edit without being worked out again from every photo. #[derive(Clone, Debug, Default)] @@ -442,12 +472,61 @@ impl Catalog { /// no smart album). pub fn album_contains(&self, id: AlbumId, p: &Photo) -> bool { match self.albums.get(&id) { - Some(Album { smart: Some(rules), .. }) => self.smart_album_has(id, rules, &rules.prepare_one(), p), + Some(Album { smart: Some(rules), .. }) => match self.plans().get(&id) { + Some(plan) => self.smart_album_has(id, rules, plan, p), + None => self.smart_album_has(id, rules, &rules.prepare_one(self), p), + }, Some(a) => self.album_lists(a.id, p.id), None => false, } } + /// Every smart album's [`Filter::prepare_one`], worked out once for the albums as they are + /// (so a photo asked about does not prepare the album's rules again). + fn plans(&self) -> &std::collections::HashMap { + self.plans.plans.get_or_init(|| self.albums.values().filter_map(|a| a.smart.as_ref().map(|rules| (a.id, rules.prepare_one(self)))).collect()) + } + + /// Whether rules must be tested as written, not cheapest first ([`Filter::prepare`]): some smart + /// album tests itself (a saved or damaged catalog; the app refuses to make one), or a chain of + /// smart albums testing smart albums is as long as [`rules::MAX_ALBUM_DEPTH`], where the + /// innermost hold nothing. There an answer can depend on which album was asked first (the + /// answers memo keeps what was worked out under the cut), so reordering could change who + /// matches. Worked out once for the albums as they are. + pub(crate) fn keeps_written_order(&self) -> bool { + *self.plans.guard.get_or_init(|| { + self.albums.values().any(|a| a.smart.is_some() && self.album_reaches(a.id, a.id)) || self.longest_chain() >= rules::MAX_ALBUM_DEPTH + }) + } + + /// The most smart albums one smart album leads through, itself included, by the smart albums + /// its rules and album filter test. Only asked of albums that lead back to none. + fn longest_chain(&self) -> usize { + let tests = |id: &AlbumId| -> Vec { + let tested = self.albums.get(id).and_then(|a| a.smart.as_deref()).map(Filter::albums_tested).unwrap_or_default(); + tested.into_iter().filter(|t| self.albums.get(t).is_some_and(|a| a.smart.is_some())).collect() + }; + let mut chain: std::collections::HashMap = std::collections::HashMap::new(); + for root in self.albums.values().filter(|a| a.smart.is_some()).map(|a| a.id) { + let mut stack = vec![root]; + while let Some(a) = stack.last().copied() { + if chain.contains_key(&a) { + stack.pop(); + continue; + } + let kids = tests(&a); + let pending: Vec = kids.iter().copied().filter(|k| !chain.contains_key(k)).collect(); + if pending.is_empty() { + chain.insert(a, 1 + kids.iter().filter_map(|k| chain.get(k)).copied().max().unwrap_or(0)); + stack.pop(); + } else { + stack.extend(pending); + } + } + } + chain.values().copied().max().unwrap_or(0) + } + /// Whether smart album `id` with `rules` (as [`Filter::prepare`]d) holds `p`. fn smart_album_has(&self, id: AlbumId, rules: &Filter, prepared: &query::Prepared, p: &Photo) -> bool { // guarded: a smart album testing smart albums can't loop or recurse without end @@ -502,7 +581,7 @@ impl Catalog { pub fn album_photos(&self, id: AlbumId) -> Vec { match self.albums.get(&id) { Some(Album { smart: Some(rules), .. }) => { - let prepared = rules.prepare(); + let prepared = rules.prepare(self); self.photos.values().filter(|p| self.smart_album_has(id, rules, &prepared, p)).map(|p| p.id).collect() } Some(a) => a.photos.clone(), @@ -515,7 +594,7 @@ impl Catalog { match self.albums.get(&id) { // counted in place: no id list is built just for its length Some(Album { smart: Some(rules), .. }) => { - let prepared = rules.prepare(); + let prepared = rules.prepare(self); self.photos.values().filter(|p| self.smart_album_has(id, rules, &prepared, p)).count() } Some(a) => a.photos.iter().filter(|p| self.photos.get(p).is_some_and(|p| !p.deleted)).count(), @@ -779,6 +858,7 @@ impl Catalog { Op::SetHistoryCursor { id, cursor: std::mem::replace(&mut p.history_cursor, cursor) } } Op::AddAlbum { album } => { + self.plans = PlansMemo::default(); if self.albums.contains_key(&album.id) { return Err(CatalogError::Invalid(format!("album {:?} exists", album.id))); } @@ -800,6 +880,7 @@ impl Catalog { Op::RemoveAlbum { id } } Op::RemoveAlbum { id } => { + self.plans = PlansMemo::default(); if self.albums.values().any(|a| a.parent == Some(id)) { return Err(CatalogError::Invalid("folder is not empty".into())); } @@ -860,6 +941,7 @@ impl Catalog { Op::SetAlbumCover { id, cover: std::mem::replace(&mut a.cover, cover) } } Op::SetAlbumRules { id, rules } => { + self.plans = PlansMemo::default(); self.validate_rules(&rules)?; let a = self.album_mut(id)?; let Some(old) = a.smart.as_mut() else { @@ -1209,6 +1291,8 @@ mod tests_lock; #[cfg(test)] mod tests_repair; #[cfg(test)] +mod tests_rule_order; +#[cfg(test)] mod tests_scale; #[cfg(test)] mod tests_tally; diff --git a/crates/catalog/src/query.rs b/crates/catalog/src/query.rs index ff94268a8..5ed7c7dd1 100644 --- a/crates/catalog/src/query.rs +++ b/crates/catalog/src/query.rs @@ -143,6 +143,9 @@ fn shuffle_rank(seed: u64, id: PhotoId) -> u64 { pub(crate) struct Prepared { root: Option, only: Option>, + /// [`Filter::rule_set`] with its rules in the order to test them ([`crate::RuleSet::by_cost`]); + /// `None` tests them as written. + rules: Option, } /// The tokens of a search text: split at whitespace, except inside double quotes, so @@ -327,18 +330,35 @@ impl Filter { } pub fn matches(&self, p: &Photo, cat: &Catalog) -> bool { - self.matches_in(p, cat, &self.prepare_one()) + // one photo: its rules are tested as written (ordering them would cost more than it saves) + self.matches_in(p, cat, &Prepared { root: self.library_root(), only: None, rules: None }) } - /// [`Filter::prepare`] for testing one photo (the `only` ids are looked through, not hashed). - pub(crate) fn prepare_one(&self) -> Prepared { - Prepared { root: self.library_root(), only: None } + /// [`Filter::prepare`] for testing one photo at a time, with what each needs worked out once + /// (the `only` ids are looked through, not hashed). + pub(crate) fn prepare_one(&self, cat: &Catalog) -> Prepared { + Prepared { root: self.library_root(), only: None, rules: self.ordered_rules(cat) } + } + + /// The rule set to test, cheapest rule first: once per prepared filter, not per photo. `None` + /// (as written) while any smart album tests itself or a chain is too deep ([`Catalog::keeps_written_order`]), where the + /// order could change which photos match. + fn ordered_rules(&self, cat: &Catalog) -> Option { + if cat.keeps_written_order() { + return None; + } + self.rule_set.as_ref().map(|rs| rs.by_cost(cat)) } /// What [`Filter::matches_in`] needs worked out once for a pass over many photos rather than - /// once per photo: the library folder's identity and the `only` ids as a set. - pub(crate) fn prepare(&self) -> Prepared { - Prepared { root: self.library_root(), only: (!self.only.is_empty()).then(|| self.only.iter().copied().collect()) } + /// once per photo: the library folder's identity, the `only` ids as a set and the rules in the + /// order to test them. + pub(crate) fn prepare(&self, cat: &Catalog) -> Prepared { + Prepared { + root: self.library_root(), + only: (!self.only.is_empty()).then(|| self.only.iter().copied().collect()), + rules: self.ordered_rules(cat), + } } /// [`Filter::library_folder`] as a folder identity ([`folder_key`]); `None` without a choice. @@ -382,7 +402,7 @@ impl Filter { return false; } if let Some(rs) = &self.rule_set - && !rs.matches(p, cat) + && !prepared.rules.as_ref().unwrap_or(rs).matches(p, cat) { return false; } @@ -482,9 +502,9 @@ impl Catalog { /// album's rules, the filter bar), as long as they are still where the view looks (`within`: in the library, /// the album, the folder; not deleted). pub fn query_keeping(&self, filter: &Filter, sort: &Sort, keep: &std::collections::HashSet, within: &Filter) -> Vec { - let prepared = filter.prepare(); + let prepared = filter.prepare(self); // `within` only decides for the few kept photos: worked out per photo, not hashed - let within_prepared = within.prepare_one(); + let within_prepared = within.prepare_one(self); let mut v: Vec<&Photo> = self .photos() .map(|p| p.as_ref()) @@ -502,7 +522,7 @@ impl Catalog { pub fn follow(&self, was: &[PhotoId], touched: &[PhotoId], filter: &Filter, sort: &Sort, within: Option<&Filter>) -> Vec { let listed: std::collections::HashSet = was.iter().copied().filter(|id| touched.binary_search(id).is_ok()).collect(); let mut ids: Vec = was.iter().copied().filter(|id| !listed.contains(id)).collect(); - let (prepared, within_prepared) = (filter.prepare_one(), within.map(Filter::prepare_one)); + let (prepared, within_prepared) = (filter.prepare_one(self), within.map(|w| w.prepare_one(self))); for id in touched { let Some(p) = self.photo(*id) else { continue }; let kept = within.zip(within_prepared.as_ref()).is_some_and(|(w, wp)| listed.contains(id) && w.matches_in(p, self, wp)); @@ -517,7 +537,7 @@ impl Catalog { /// How many photos match `filter`: [`Catalog::query`]'s length without its sorting. pub fn count(&self, filter: &Filter) -> usize { - let prepared = filter.prepare(); + let prepared = filter.prepare(self); self.photos().filter(|p| filter.matches_in(p, self, &prepared)).count() } @@ -574,7 +594,7 @@ impl Catalog { pub fn people_in(&self, filter: &Filter) -> Vec { let filter = Filter { person: None, ..filter.clone() }; let mut m: std::collections::HashMap = Default::default(); - let prepared = filter.prepare(); + let prepared = filter.prepare(self); for p in self.photos().filter(|p| filter.matches_in(p, self, &prepared)) { let mut seen: Vec = Vec::new(); for r in p.meta.regions.iter().filter(|r| r.kind == lightcraft_meta::RegionKind::Face) { diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index ec92dc724..d0ea85417 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -488,6 +488,12 @@ fn pass_answer(id: crate::AlbumId, photo: crate::PhotoId) -> Option { }) } +#[cfg(test)] +thread_local! { + /// Tests: test rules in the order they were written, as before [`RuleSet::by_cost`]. + pub(crate) static WRITTEN_ORDER: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + #[cfg(test)] thread_local! { /// How many times a smart album's rules were worked out on this thread (tests count them). @@ -1048,6 +1054,48 @@ impl Rule { } } +/// How much work one rule is, in tiers: [`RuleSet::by_cost`] tests the cheaper first. +const COST_SCALAR: u8 = 0; +const COST_SMALL: u8 = 1; +const COST_TEXT: u8 = 2; +const COST_ALBUM: u8 = 3; + +impl Rule { + /// A rule's tier: 0 numbers, choices and yes/no questions about one photo's own record (and + /// anything that is false at once: an unknown field or a missing album); 1 dates, keyword and + /// people lists, short single fields, an index lookup and a plain album's list (a folder holds no photos here: an empty lookup); 2 text built + /// from several fields and file paths (they copy and lower-case the text); 3 a smart album, + /// which runs a whole rule set for the photo. A group costs as much as its dearest rule. + fn cost(&self, cat: &Catalog) -> u8 { + match self { + Rule::Group { group } => group.rules.iter().map(|r| r.cost(cat)).max().unwrap_or(COST_SCALAR), + Rule::Field { field, value, .. } => match field.as_str() { + "album" => album_rule_id(value).map_or(COST_SCALAR, |id| match cat.albums.get(&id) { + None => COST_SCALAR, + Some(a) if a.smart.is_some() => COST_ALBUM, + Some(_) => COST_SMALL, + }), + "text" | "filePath" | "location" => COST_TEXT, + "keywords" | "person" | "personCount" | "stacked" | "sameContent" | "copyName" | "fileName" | "extension" | "format" | "title" + | "caption" | "altText" | "city" | "state" | "country" | "camera" | "lens" | "creator" | "copyright" | "captureDate" + | "importDate" | "editDate" | "shutterSpeed" | "aspect" => COST_SMALL, + _ => COST_SCALAR, + }, + } + } + + /// This rule with its nested groups ordered by cost, and its tier. + fn by_cost(&self, cat: &Catalog) -> (u8, Rule) { + match self { + Rule::Group { group } => { + let group = group.by_cost(cat); + (group.rules.iter().map(|r| r.cost(cat)).max().unwrap_or(COST_SCALAR), Rule::Group { group }) + } + field => (field.cost(cat), field.clone()), + } + } +} + impl RuleSet { pub fn matches(&self, p: &Photo, cat: &Catalog) -> bool { if self.rules.is_empty() { @@ -1060,6 +1108,22 @@ impl RuleSet { } } + /// This rule set with its rules (and those of nested groups) in the order to test them: the + /// cheapest first ([`Rule::cost`]), the user's order among equals. Rules are pure (they read + /// the photo and the catalog; a smart album's answer is the same however it is reached as long + /// as no smart album tests itself and no chain is cut by the depth limit, which callers check first: [`Catalog::keeps_written_order`]), + /// so for all, any and none the photos that match are the same in any order; only the work + /// differs. Worked out once for a pass, never per photo. The saved rules are untouched. + pub(crate) fn by_cost(&self, cat: &Catalog) -> RuleSet { + #[cfg(test)] + if WRITTEN_ORDER.get() { + return self.clone(); + } + let mut rules: Vec<(u8, Rule)> = self.rules.iter().map(|r| r.by_cost(cat)).collect(); + rules.sort_by_key(|(cost, _)| *cost); + RuleSet { mode: self.mode, rules: rules.into_iter().map(|(_, r)| r).collect() } + } + /// Whether matches depend on the clock ("in the last…" rules, nested groups included): the /// same photos can enter or leave the set without any catalog change. pub fn depends_on_now(&self) -> bool { diff --git a/crates/catalog/src/tests_rule_order.rs b/crates/catalog/src/tests_rule_order.rs new file mode 100644 index 000000000..fc13b36ce --- /dev/null +++ b/crates/catalog/src/tests_rule_order.rs @@ -0,0 +1,324 @@ +//! A rule set is tested cheapest rule first ([`RuleSet::by_cost`]), once per pass; what matches +//! is what testing the rules in the order they were written gives. +//! +//! Scenarios: +//! - Given random libraries (Unicode and Windows paths, nested groups, album rules that test +//! smart albums and missing albums), then every album, query and count gives what the written +//! order gives, for all, any and none. +//! - Given a library where smart albums test themselves (the app refuses to make one; a saved or +//! damaged catalog can hold it, and there the answer can depend on which album is asked first), +//! then the rules are tested as written, so the answers are the written order's, as before. +//! - Given `all[album is , ]`, then the smart album is never +//! asked about a photo: the cheap rule ends it first (written first, it ran for every photo). +//! - Given rules in any order, then the saved rules and the order the editor shows are untouched. + +use serde_json::json; + +use crate::rules::{EVALUATED, WRITTEN_ORDER}; +use crate::*; + +/// A small generator: the same sequence every run. +struct Rng(u64); +impl Rng { + fn next(&mut self, n: u64) -> u64 { + self.0 = self.0.wrapping_mul(6364136223846793005).wrapping_add(1442695040888963407); + (self.0 >> 33) % n + } + fn pick<'a, T>(&mut self, of: &'a [T]) -> &'a T { + &of[self.next(of.len() as u64) as usize] + } +} + +const PATHS: &[&str] = &[ + "/Volumes/share/photos/Asie/Japon/Été/IMG_0001.jpg", + "/Volumes/share/photos/ASIE/japon/IMG_0002.JPG", + "C:\\Users\\me\\Pictures\\Ünïcode\\Tokyo\\IMG_0003.jpg", + "/Volumes/share/photos/Straße/İstanbul/IMG_0004.cr2", + "/Volumes/share/photos/東京/渋谷/IMG_0005.jpg", + "/Volumes/share/photos/Asie/Chine/clip.mp4", + "relative/dir/IMG_0007.jpg", +]; + +const WORDS: &[&str] = + &["asie", "Japon", "été", "ÜNÏCODE", "tokyo", "straße", "i̇stanbul", "東京", "photos\\asie", "photos/Asie/", " ", "", "img_000"]; + +fn library(rng: &mut Rng) -> (Catalog, Vec) { + let mut c = Catalog::new(); + let ids: Vec = (0..24) + .map(|i| { + let id = c.alloc_photo_id(); + let path = if i == 5 { None } else { Some(rng.pick(PATHS).to_string()) }; + let source = path.map_or(Source::Demo { scene: 1 }, |path| Source::File { path }); + let name = format!("IMG_{i:04}.{}", ["jpg", "cr2", "mp4"][rng.next(3) as usize]); + let mut p = Photo::new(id, source, &name, "JPEG", 6000, 4000, "2026-09-30T10:00:00"); + p.rating = rng.next(6) as u8; + p.flag = [Flag::None, Flag::Pick, Flag::Reject][rng.next(3) as usize]; + p.label = [None, Some(ColorLabel::Red), Some(ColorLabel::Blue)][rng.next(3) as usize]; + p.meta.keywords = (0..rng.next(4)).map(|k| format!("kw{}", k + rng.next(2))).collect(); + p.captured = Some(format!("20{}-0{}-1{}T10:00:00", 10 + rng.next(16), 1 + rng.next(9), rng.next(9))); + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id + }) + .collect(); + (c, ids) +} + +fn leaf(rng: &mut Rng, albums: &[AlbumId]) -> serde_json::Value { + let field = |f: &str, op: &str, v: serde_json::Value| json!({"field": f, "op": op, "value": v}); + match rng.next(14) { + 0 => field("rating", ["gte", "lte", "is", "gt", "lt"][rng.next(5) as usize], json!(rng.next(6))), + 1 => field("flag", ["is", "isNot"][rng.next(2) as usize], json!(["pick", "reject", "none"][rng.next(3) as usize])), + 2 => field("label", ["is", "isNot"][rng.next(2) as usize], json!(["red", "blue", "none"][rng.next(3) as usize])), + 3 => field("kind", ["is", "isNot"][rng.next(2) as usize], json!(["image", "raw", "video"][rng.next(3) as usize])), + 4 => field("keywordCount", ["lte", "gte"][rng.next(2) as usize], json!(rng.next(4))), + 5 => field("keywords", ["contains", "notContains", "isEmpty"][rng.next(3) as usize], json!(format!("kw{}", rng.next(3)))), + 6 => field("captureDate", ["before", "after"][rng.next(2) as usize], json!(format!("20{}", 10 + rng.next(16)))), + 7 | 8 => field("filePath", ["contains", "notContains", "startsWith", "is", "isEmpty"][rng.next(5) as usize], json!(*rng.pick(WORDS))), + 9 => field("fileName", "contains", json!(["img_000", ".cr2", "mp4"][rng.next(3) as usize])), + 10 => field("text", ["contains", "notContains"][rng.next(2) as usize], json!(*rng.pick(WORDS))), + // an album that may not exist (99) or may be on a loop + _ => { + let album = if rng.next(8) == 0 { AlbumId(99) } else { *rng.pick(albums) }; + field("album", ["is", "isNot"][rng.next(2) as usize], json!(album.0)) + } + } +} + +fn rule_set(rng: &mut Rng, albums: &[AlbumId], depth: u32) -> serde_json::Value { + let rules: Vec = (0..rng.next(5)) + .map(|_| if depth < 2 && rng.next(4) == 0 { json!({"group": rule_set(rng, albums, depth + 1)}) } else { leaf(rng, albums) }) + .collect(); + let mode = ["all", "any", "none"][rng.next(3) as usize]; + json!({"match": mode, "rules": rules}) +} + +fn filter(rules: serde_json::Value) -> Filter { + Filter { rule_set: Some(serde_json::from_value(rules).unwrap()), ..Default::default() } +} + +/// What a catalog answers for every album, photo and a few queries. +fn answers(c: &Catalog, albums: &[AlbumId], photos: &[PhotoId], queries: &[Filter]) -> String { + let mut out = String::new(); + for a in albums { + let held: Vec = photos.iter().map(|p| c.album_contains(*a, c.photo(*p).unwrap())).collect(); + out += &format!("{a:?} {held:?} {:?} {}\n", c.album_photos(*a), c.album_count(*a)); + } + for p in photos { + out += &format!("{p:?} {:?}\n", c.albums_of(*p)); + } + for q in queries { + out += &format!("{:?} {}\n", c.query(q, &Sort::default()), c.count(q)); + } + out +} + +#[test] +fn the_cheapest_first_order_matches_what_the_written_order_matches() { + let chained = same_answers_in_random_libraries(Rng(11), false); + assert!(chained > 20, "the generator makes smart albums that test smart albums: {chained}"); +} + +#[test] +fn a_library_with_loops_answers_as_written_and_does_not_hang() { + let looping = same_answers_in_random_libraries(Rng(5), true); + assert!(looping > 20, "the generator makes loops: {looping}"); +} + +/// Albums of random libraries and queries answer the same cheapest first and as written; returns +/// how many smart albums tested a smart album (`loops`: one that leads back to itself). +fn same_answers_in_random_libraries(mut rng: Rng, loops: bool) -> usize { + let mut chained = 0; + for round in 0..150 { + let (mut c, photos) = library(&mut rng); + let plain = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { photos: photos.iter().copied().step_by(3).collect(), ..Album::new(plain, "Plain") } }).unwrap(); + let folder = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { folder: true, ..Album::new(folder, "F") } }).unwrap(); + let smarts: Vec = (0..6) + .map(|i| { + let id = c.alloc_album_id(); + let parent = (i % 3 == 0).then_some(folder); + c.apply(Op::AddAlbum { album: Album { parent, smart: Some(Box::default()), ..Album::new(id, format!("S{i}")) } }).unwrap(); + id + }) + .collect(); + let albums: Vec = [plain, folder].into_iter().chain(smarts.iter().copied()).collect(); + for (i, s) in smarts.iter().enumerate() { + // (without loops a smart album tests only the albums made before it) + let testable = if loops { &albums[..] } else { &albums[..2 + i] }; + let _ = c.apply(Op::SetAlbumRules { id: *s, rules: Box::new(filter(rule_set(&mut rng, testable, 0))) }); + } + let queries: Vec = (0..4).map(|_| filter(rule_set(&mut rng, &albums, 0))).collect(); + // (the copy starts without what the catalog worked out, so each is worked out under its own order) + let written = c.clone(); + let ordered_answers = answers(&c, &albums, &photos, &queries); + WRITTEN_ORDER.set(true); + let written_answers = answers(&written, &albums, &photos, &queries); + WRITTEN_ORDER.set(false); + assert_eq!(ordered_answers, written_answers, "round {round}: {}", c.to_snapshot()); + let reaching = |s: &AlbumId| { + smarts.iter().any(|t| if loops { c.album_reaches(*t, *s) && c.album_reaches(*s, *t) } else { t != s && c.album_reaches(*s, *t) }) + }; + chained += smarts.iter().filter(|s| reaching(s)).count(); + assert_eq!(c.keeps_written_order(), smarts.iter().any(|s| c.album_reaches(*s, *s))); + assert!(loops || !c.keeps_written_order(), "round {round}: {}", c.to_snapshot()); + } + chained +} + +#[test] +fn a_cheap_rule_that_fails_ends_the_test_before_a_smart_album_runs() { + let mut c = Catalog::new(); + let photos: Vec = (0..20) + .map(|i| { + let id = c.alloc_photo_id(); + let p = Photo::new(id, Source::File { path: format!("/a/b/{i}.jpg") }, &format!("{i}.jpg"), "JPEG", 6000, 4000, "2026-09-30T10:00:00"); + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id + }) + .collect(); + let inner = c.alloc_album_id(); + c.apply(Op::AddAlbum { + album: Album { + smart: Some(Box::new(filter(json!({"rules": [{"field": "filePath", "op": "contains", "value": "/a/"}]})))), + ..Album::new(inner, "Inner") + }, + }) + .unwrap(); + let rules = |first: serde_json::Value, second: serde_json::Value| json!({"match": "all", "rules": [first, second]}); + let album = json!({"field": "album", "op": "is", "value": inner.0}); + // no photo is rated 6 stars (a rating is at most 5) + let never = json!({"field": "rating", "op": "gte", "value": 6}); + let outer = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::new(filter(rules(album.clone(), never.clone())))), ..Album::new(outer, "Outer") } }) + .unwrap(); + EVALUATED.set(0); + assert_eq!(c.album_count(outer), 0); + // (the outer album's own rules count as one run for each photo; its inner album must run for none) + assert_eq!(EVALUATED.get(), photos.len(), "only the outer album ran, once per photo"); + // the same photos whichever way round the rules are written + let swapped = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::new(filter(rules(never, album)))), ..Album::new(swapped, "Swapped") } }).unwrap(); + assert_eq!((c.album_count(swapped), c.album_photos(swapped)), (0, vec![])); +} + +#[test] +fn rules_are_ordered_by_cost_without_touching_the_saved_ones() { + let mut c = Catalog::new(); + let plain = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album::new(plain, "Plain") }).unwrap(); + let smart = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(smart, "Smart") } }).unwrap(); + let written: RuleSet = serde_json::from_value(json!({"match": "all", "rules": [ + {"field": "album", "op": "is", "value": smart.0}, + {"field": "filePath", "op": "contains", "value": "x"}, + {"group": {"match": "any", "rules": [ + {"field": "album", "op": "isNot", "value": smart.0}, + {"field": "rating", "op": "gte", "value": 3}]}}, + {"field": "keywords", "op": "contains", "value": "k"}, + {"field": "album", "op": "is", "value": plain.0}, + {"field": "rating", "op": "gte", "value": 3}, + {"field": "album", "op": "is", "value": 99}, + {"field": "flag", "op": "is", "value": "pick"}]})) + .unwrap(); + let before = written.clone(); + let ordered = written.by_cost(&c); + assert_eq!(written, before, "the saved rules stay as the user wrote them"); + let names: Vec = ordered + .rules + .iter() + .map(|r| match r { + Rule::Group { .. } => "group".to_string(), + Rule::Field { field, op, .. } => format!("{field} {op}"), + }) + .collect(); + // scalars (a missing album is false at once), then lists and a plain album, then text, then + // smart albums and the group that holds one; equals keep the written order + assert_eq!(names, ["rating gte", "album is", "flag is", "keywords contains", "album is", "filePath contains", "album is", "group"]); + let inner: Vec<&str> = match ordered.rules.get(7) { + Some(Rule::Group { group }) => group.rules.iter().map(|r| if let Rule::Field { field, .. } = r { field.as_str() } else { "group" }).collect(), + _ => Vec::new(), + }; + assert_eq!(inner, ["rating", "album"], "nested groups are ordered too"); +} + +/// B = all[album is C], C = all[album isNot B], A = all[any[album is C, flag is pick], album is B]: +/// asked in the order the rules are written, a picked photo is not in A; asked cheapest first +/// (the flag, then the albums) it was. With a loop the order stays as written. +#[test] +fn smart_albums_that_test_each_other_keep_the_written_order() { + let mut c = Catalog::new(); + let mut p = Photo::new(c.alloc_photo_id(), Source::File { path: "/a/1.jpg".into() }, "1.jpg", "JPEG", 6000, 4000, "2026-09-30T10:00:00"); + p.flag = Flag::Pick; + let photo = p.id; + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let ids: Vec = ["A", "B", "C"] + .iter() + .map(|name| { + let id = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(id, *name) } }).unwrap(); + id + }) + .collect(); + let (a, b, cc) = (ids[0], ids[1], ids[2]); + let album = |op: &str, id: AlbumId| json!({"field": "album", "op": op, "value": id.0}); + let set = |c: &mut Catalog, id: AlbumId, rules: serde_json::Value| c.apply(Op::SetAlbumRules { id, rules: Box::new(filter(rules)) }).unwrap(); + set(&mut c, b, json!({"match": "all", "rules": [album("is", cc)]})); + set(&mut c, cc, json!({"match": "all", "rules": [album("isNot", b)]})); + set( + &mut c, + a, + json!({"match": "all", "rules": [ + {"group": {"match": "any", "rules": [album("is", cc), {"field": "flag", "op": "is", "value": "pick"}]}}, + album("is", b)]}), + ); + let written = c.clone(); + let ask = |c: &Catalog| { + ids.iter().map(|id| (c.album_contains(*id, c.photo(photo).unwrap()), c.album_photos(*id), c.album_count(*id))).collect::>() + }; + WRITTEN_ORDER.set(true); + let as_written = ask(&written); + WRITTEN_ORDER.set(false); + assert!(c.keeps_written_order()); + assert_eq!(ask(&c), as_written); +} + +/// A = all[any[album is D1, flag is pick], album is W], D1 tests D2 … D30, D30 tests W, W tests Z, +/// Z = rating ≥ 0: asked from A, the chain is cut by the depth limit, and asked cheapest first (the +/// flag, then the albums) the cut falls elsewhere. Chains that long keep the written order. +#[test] +fn a_chain_as_deep_as_the_limit_keeps_the_written_order() { + let mut c = Catalog::new(); + let mut p = Photo::new(c.alloc_photo_id(), Source::File { path: "/a/1.jpg".into() }, "1.jpg", "JPEG", 6000, 4000, "2026-09-30T10:00:00"); + p.flag = Flag::Pick; + let photo = p.id; + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let new_smart = |c: &mut Catalog, name: &str| { + let id = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::default()), ..Album::new(id, name) } }).unwrap(); + id + }; + let a = new_smart(&mut c, "A"); + let chain: Vec = (1..=30).map(|i| new_smart(&mut c, &format!("D{i}"))).collect(); + let (w, z) = (new_smart(&mut c, "W"), new_smart(&mut c, "Z")); + let album = |id: AlbumId| json!({"field": "album", "op": "is", "value": id.0}); + let set = |c: &mut Catalog, id: AlbumId, rules: serde_json::Value| c.apply(Op::SetAlbumRules { id, rules: Box::new(filter(rules)) }).unwrap(); + set( + &mut c, + a, + json!({"match": "all", "rules": [{"group": {"match": "any", "rules": [album(chain[0]), {"field": "flag", "op": "is", "value": "pick"}]}}, album(w)]}), + ); + for pair in chain.windows(2) { + set(&mut c, pair[0], json!({"match": "all", "rules": [album(pair[1])]})); + } + set(&mut c, chain[29], json!({"match": "all", "rules": [album(w)]})); + set(&mut c, w, json!({"match": "all", "rules": [album(z)]})); + set(&mut c, z, json!({"match": "all", "rules": [{"field": "rating", "op": "gte", "value": 0}]})); + let written = c.clone(); + let ask = |c: &Catalog| [a, w, z, chain[0]].map(|id| (c.album_contains(id, c.photo(photo).unwrap()), c.album_photos(id), c.album_count(id))); + WRITTEN_ORDER.set(true); + let as_written = ask(&written); + WRITTEN_ORDER.set(false); + assert!(c.keeps_written_order()); + assert_eq!(ask(&c), as_written); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index c6c259200..c24c4c516 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -157,6 +157,15 @@ the full count took about 14 s instead of about 23 s on a heavily loaded machine; the rest is the text rules of the albums themselves. The counts are the same as before, including for albums that loop through each other. +### Smart albums open and count faster on large libraries +- A smart album that tests another smart album and also a file path or other text (for example "in Rejects, and the + path contains …") now checks the quick rules first and the slow ones last, so most photos are ruled out before the + other album is asked about them. The photos in the album and their order are the same, and the rules still show in + the order you wrote them. (A library whose smart albums test each other in a circle, which LightCraft doesn't let + you make, or test each other 32 deep, keeps testing the rules as written.) On a 312,000-photo test library, opening + one such album went from about 224 ms to about 100 ms, and opening 51 of them in turn from about 19 s to about + 5.8 s (a busy machine, so rough numbers). + ### Sharper resampling in HDR and panorama merges - Merged photos (HDR and panorama) are resampled with a sharper filter (Lanczos-3 instead of bilinear). Moving a photo by half a pixel used to lose a third of its finest detail; now it keeps most of it, without halos around From e331229740b56d6d1f8ef6e014ab7a91df5d5e64 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Tue, 20 Oct 2026 17:35:36 -0400 Subject: [PATCH 2/4] PERF-SMART: a repair forgets what was worked out from the albums Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/lib.rs | 2 ++ crates/catalog/src/tests_repair.rs | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 5a80021f2..3b6b88449 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -1231,6 +1231,8 @@ impl Catalog { if !changed.is_empty() { // stacks and albums changed outside `apply`: the lookups are built again when asked self.index = index::Indexes::default(); + // (and what was worked out from the albums, the guard of [`Self::keeps_written_order`] too) + self.plans = PlansMemo::default(); } changed } diff --git a/crates/catalog/src/tests_repair.rs b/crates/catalog/src/tests_repair.rs index 9feb038a5..2c079d0ac 100644 --- a/crates/catalog/src/tests_repair.rs +++ b/crates/catalog/src/tests_repair.rs @@ -218,3 +218,27 @@ fn an_album_filed_under_another_id_gets_its_own() { assert_eq!((a.id, a.parent), (inner, Some(folder))); assert_eq!(c.album_children(Some(folder)).iter().map(|a| a.id).collect::>(), vec![inner]); } + +/// What was worked out from the albums (whether rules may be tested cheapest first) is worked out +/// again when a repair changes the albums. +#[test] +fn a_repair_forgets_what_was_worked_out_from_the_albums() { + let (mut c, _) = sound(); + let smart = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::default()), ..album(smart.0, None, false, vec![]) } }).unwrap(); + assert!(!c.keeps_written_order(), "worked out, and remembered"); + // damaged outside `apply`: the smart album tests itself, and an album is filed under another id + let rules: Filter = + serde_json::from_value(serde_json::json!({"ruleSet": {"match": "all", "rules": [{"field": "album", "op": "is", "value": smart.0}]}})) + .unwrap(); + if let Some(a) = c.albums.get_mut(&smart) { + a.smart = Some(Box::new(rules)); + } + let inner = c.albums().find(|a| !a.folder && a.smart.is_none()).unwrap().id; + if let Some(mut a) = c.albums.remove(&inner) { + a.id = AlbumId(900); + c.albums.insert(inner, a); + } + assert!(!c.repair().is_empty()); + assert!(c.keeps_written_order(), "the loop the repair left is seen"); +} From f4f2348fc164f70977484bb1de104926aa8c09ba Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 22 Oct 2026 07:15:53 -0400 Subject: [PATCH 3/4] PERF-SMART: the depth-limit test states the expected answers --- crates/catalog/src/tests_rule_order.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/catalog/src/tests_rule_order.rs b/crates/catalog/src/tests_rule_order.rs index fc13b36ce..281e058be 100644 --- a/crates/catalog/src/tests_rule_order.rs +++ b/crates/catalog/src/tests_rule_order.rs @@ -291,6 +291,7 @@ fn a_chain_as_deep_as_the_limit_keeps_the_written_order() { let mut c = Catalog::new(); let mut p = Photo::new(c.alloc_photo_id(), Source::File { path: "/a/1.jpg".into() }, "1.jpg", "JPEG", 6000, 4000, "2026-09-30T10:00:00"); p.flag = Flag::Pick; + p.rating = 0; // (Z, rating at least 0, holds for it) let photo = p.id; c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); let new_smart = |c: &mut Catalog, name: &str| { @@ -318,6 +319,9 @@ fn a_chain_as_deep_as_the_limit_keeps_the_written_order() { let ask = |c: &Catalog| [a, w, z, chain[0]].map(|id| (c.album_contains(id, c.photo(photo).unwrap()), c.album_photos(id), c.album_count(id))); WRITTEN_ORDER.set(true); let as_written = ask(&written); + // (the expected answers, written down: the same evaluator gives both sides below, so a + // regression that lets the 33rd album in would otherwise change both alike) + assert_eq!(as_written.each_ref().map(|answer| answer.0), [false, true, true, true], "the depth limit cuts the chain only while A is asked"); WRITTEN_ORDER.set(false); assert!(c.keeps_written_order()); assert_eq!(ask(&c), as_written); From 9673a69bec4e94c934018c9558771989dcf51c87 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 29 Oct 2026 13:35:34 -0400 Subject: [PATCH 4/4] PERF-SMART: test that adding or removing an album forgets the reorder guard; fix comment wording and What's New figure --- crates/catalog/src/rules.rs | 10 +++---- crates/catalog/src/tests_rule_order.rs | 37 ++++++++++++++++++++++++++ docs/whats-new.md | 4 +-- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index d0ea85417..44d2fbe58 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -1062,9 +1062,9 @@ const COST_ALBUM: u8 = 3; impl Rule { /// A rule's tier: 0 numbers, choices and yes/no questions about one photo's own record (and - /// anything that is false at once: an unknown field or a missing album); 1 dates, keyword and + /// anything that is settled at once: an unknown field or a missing album); 1 dates, keyword and /// people lists, short single fields, an index lookup and a plain album's list (a folder holds no photos here: an empty lookup); 2 text built - /// from several fields and file paths (they copy and lower-case the text); 3 a smart album, + /// from several fields and file paths (long strings, matched for each photo); 3 a smart album, /// which runs a whole rule set for the photo. A group costs as much as its dearest rule. fn cost(&self, cat: &Catalog) -> u8 { match self { @@ -1076,9 +1076,9 @@ impl Rule { Some(_) => COST_SMALL, }), "text" | "filePath" | "location" => COST_TEXT, - "keywords" | "person" | "personCount" | "stacked" | "sameContent" | "copyName" | "fileName" | "extension" | "format" | "title" - | "caption" | "altText" | "city" | "state" | "country" | "camera" | "lens" | "creator" | "copyright" | "captureDate" - | "importDate" | "editDate" | "shutterSpeed" | "aspect" => COST_SMALL, + "keywords" | "person" | "personCount" | "stacked" | "copyName" | "fileName" | "extension" | "format" | "title" | "caption" + | "altText" | "city" | "state" | "country" | "camera" | "lens" | "creator" | "copyright" | "captureDate" | "importDate" + | "editDate" | "shutterSpeed" | "aspect" => COST_SMALL, _ => COST_SCALAR, }, } diff --git a/crates/catalog/src/tests_rule_order.rs b/crates/catalog/src/tests_rule_order.rs index 281e058be..d3f448674 100644 --- a/crates/catalog/src/tests_rule_order.rs +++ b/crates/catalog/src/tests_rule_order.rs @@ -326,3 +326,40 @@ fn a_chain_as_deep_as_the_limit_keeps_the_written_order() { assert!(c.keeps_written_order()); assert_eq!(ask(&c), as_written); } + +/// What was worked out from the albums (whether rules may be tested cheapest first) is worked out +/// again after an album is added or removed: 31 smart albums in a chain are fine, a 32nd on top of +/// it reaches the depth limit (the written order from then on), and taking it away lifts that. +#[test] +fn adding_or_removing_an_album_forgets_whether_the_rules_may_be_reordered() { + let mut c = Catalog::new(); + let mut p = Photo::new(c.alloc_photo_id(), Source::File { path: "/a/1.jpg".into() }, "1.jpg", "JPEG", 6000, 4000, "2026-09-30T10:00:00"); + p.flag = Flag::Pick; + let photo = p.id; + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let album = |id: AlbumId| json!({"field": "album", "op": "is", "value": id.0}); + let smart = |c: &mut Catalog, name: &str, rules: serde_json::Value| { + let id = c.alloc_album_id(); + c.apply(Op::AddAlbum { album: Album { smart: Some(Box::new(filter(rules))), ..Album::new(id, name) } }).unwrap(); + id + }; + // S1 tests S2 … S30 tests S31, which tests nothing: 31 albums + let mut chain = vec![smart(&mut c, "S31", json!({"match": "all", "rules": []}))]; + for i in (1..=30).rev() { + let next = chain[chain.len() - 1]; + chain.push(smart(&mut c, &format!("S{i}"), json!({"match": "all", "rules": [album(next)]}))); + } + let bottom = chain[0]; + let top = chain[30]; + let ask = |c: &Catalog| [top, bottom].map(|id| (c.album_contains(id, c.photo(photo).unwrap()), c.album_photos(id), c.album_count(id))); + assert!(!c.keeps_written_order(), "31 albums in a chain are under the limit (worked out, and remembered)"); + let before = ask(&c); + let head = smart(&mut c, "S0", json!({"match": "all", "rules": [album(top)]})); + assert!(c.keeps_written_order(), "a 32nd album on top of the chain reaches the depth limit"); + assert_eq!(c.clone().keeps_written_order(), c.keeps_written_order(), "as a catalog that worked it out afresh"); + assert_eq!(ask(&c), ask(&c.clone())); + c.apply(Op::RemoveAlbum { id: head }).unwrap(); + assert!(!c.keeps_written_order(), "without it the chain is under the limit again"); + assert_eq!(c.clone().keeps_written_order(), c.keeps_written_order()); + assert_eq!(ask(&c), before); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index c24c4c516..a271c066d 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -163,8 +163,8 @@ other album is asked about them. The photos in the album and their order are the same, and the rules still show in the order you wrote them. (A library whose smart albums test each other in a circle, which LightCraft doesn't let you make, or test each other 32 deep, keeps testing the rules as written.) On a 312,000-photo test library, opening - one such album went from about 224 ms to about 100 ms, and opening 51 of them in turn from about 19 s to about - 5.8 s (a busy machine, so rough numbers). + 51 such albums in turn went from about 19 s to about 5.8 s (about 377 ms to 113 ms each; a busy machine, + so rough numbers). ### Sharper resampling in HDR and panorama merges - Merged photos (HDR and panorama) are resampled with a sharper filter (Lanczos-3 instead of bilinear). Moving a