LRC-LIB-SMARTCOLL: smart albums testing smart albums stay linear

From final review: the loop guard bounded the depth of smart albums
testing smart albums, but not repeated work. Each album tested by the
next one twice ("all of: album is X(k-1), album is X(k-1)") doubled the
cost per level, so 25 levels meant 2^25 evaluations per photo, enough
to freeze the sidebar's counts. Any agent or catalog file could build
such a chain.

rules::smart_album_holds remembers each smart album's answer for each
photo during one outermost question, and forgets them when it ends (a
drop guard, however it ends). A regression test counts a 26-deep
doubled chain in well under its 2 s limit; before the fix it ran for
minutes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Nathaniel Ledford
2026-10-09 10:54:57 -04:00
co-authored by Claude Opus 5.5
parent 4598d2c186
commit a51224e6d4
3 changed files with 54 additions and 1 deletions
+1 -1
View File
@@ -325,7 +325,7 @@ impl Catalog {
pub fn album_contains(&self, id: AlbumId, p: &Photo) -> bool {
match self.albums.get(&id) {
// guarded: a smart album testing smart albums can't loop or recurse without end
Some(Album { smart: Some(rules), .. }) => !p.deleted && rules::evaluating(id, || rules.matches(p, self)).unwrap_or(false),
Some(Album { smart: Some(rules), .. }) => !p.deleted && rules::smart_album_holds(id, p.id, || rules.matches(p, self)),
Some(a) => a.photos.contains(&p.id),
None => false,
}
+31
View File
@@ -413,6 +413,37 @@ thread_local! {
static EVALUATING: std::cell::RefCell<Vec<crate::AlbumId>> = const { std::cell::RefCell::new(Vec::new()) };
}
thread_local! {
/// While one outermost question is answered (is this photo in album X?): what each smart album
/// it leads to answered for each photo, so an album tested many times along the way (X tests Y
/// twice, Y tests Z twice…) is worked out once, not 2^depth times. Cleared when it ends.
static ANSWERS: std::cell::RefCell<std::collections::HashMap<(crate::AlbumId, crate::PhotoId), bool>> =
std::cell::RefCell::new(std::collections::HashMap::new());
}
/// Whether `photo` is in smart album `id`, by `matches` (its rules), remembered for the rest of the
/// outermost question and guarded by [`evaluating`] (a loop or a chain too deep holds nothing).
pub(crate) fn smart_album_holds(id: crate::AlbumId, photo: crate::PhotoId, matches: impl FnOnce() -> bool) -> bool {
if let Some(known) = ANSWERS.with_borrow(|a| a.get(&(id, photo)).copied()) {
return known;
}
// the outermost question forgets the answers when it ends, however it ends
struct Forget(bool);
impl Drop for Forget {
fn drop(&mut self) {
if self.0 {
ANSWERS.with_borrow_mut(std::collections::HashMap::clear);
}
}
}
let _forget = Forget(EVALUATING.with_borrow(Vec::is_empty));
let answer = evaluating(id, matches);
if let Some(a) = answer {
ANSWERS.with_borrow_mut(|known| known.insert((id, photo), a));
}
answer.unwrap_or(false)
}
/// How deep smart albums may test smart albums before the innermost counts as holding nothing.
const MAX_ALBUM_DEPTH: usize = 32;
+22
View File
@@ -565,3 +565,25 @@ fn summaries_name_albums() {
let f = Filter { rule_set: Some(rules), ..Default::default() };
assert!(f.describe_with(&c).contains("album isn't “Excluded Photos”"));
}
/// A chain of smart albums each testing the one before twice ("all of: album is X(k-1), album is
/// X(k-1)") costs a pass per album, not 2^depth: each album's answer for a photo is worked out once
/// per question. 25 albums deep stays instant.
#[test]
fn chains_of_smart_albums_stay_linear() {
let mut c = Catalog::new();
let smart = |id: u64, f: Filter| Album { smart: Some(Box::new(f)), ..Album::new(AlbumId(id), "X") };
c.apply(Op::AddAlbum { album: smart(1, Filter::default()) }).unwrap();
for k in 2..=26u64 {
let f: Filter = serde_json::from_value(serde_json::json!({"ruleSet": {"rules": [
{"field": "album", "op": "is", "value": k - 1}, {"field": "album", "op": "is", "value": k - 1}]}}))
.unwrap();
c.apply(Op::AddAlbum { album: smart(k, f) }).unwrap();
}
for i in 0..20 {
photo(&mut c, &format!("p{i}.jpg"), "2026-09-01T10:00:00");
}
let start = std::time::Instant::now();
assert_eq!(c.album_count(AlbumId(26)), 20);
assert!(start.elapsed() < std::time::Duration::from_secs(2), "took {:?}", start.elapsed());
}