From 825393eba1333687596740505b1efaa09ccb07b8 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 01:14:05 +0200 Subject: [PATCH 01/16] perf(save): stop the typed search allocating per property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property search walked the whole tree building, for every one of the ~1.4M properties in a real save, a String for the path segment, a String for the formatted value, and — once per query term — a lower-cased copy of the whole display path just to test a substring. Carry the path in a struct that borrows property names out of the tree and owns only the `[index]`/`{mapKey}` segments it has to format, keep a lower-cased twin of the display path in lockstep with it, and build the path and value only for a property that actually reaches the result page. Verified byte-identical against the previous walk over a real save: eight queries, including deep pages and the empty query, 3.1 MB of results. Hero attributes 871 ms -> 277 ms, the world clock 844 ms -> 274 ms. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/properties.rs | 300 +++++++++++++++++++---------- 1 file changed, 200 insertions(+), 100 deletions(-) diff --git a/crates/gore-save/src/properties.rs b/crates/gore-save/src/properties.rs index 262d87a3..77d020af 100644 --- a/crates/gore-save/src/properties.rs +++ b/crates/gore-save/src/properties.rs @@ -27,6 +27,7 @@ use crate::{CoreError, Reader}; use serde_json::{Value, json}; +use std::borrow::Cow; use std::collections::{BTreeMap, HashMap, HashSet}; use std::sync::Arc; @@ -658,16 +659,72 @@ pub fn search_properties( total: &mut total, hits: &mut hits, }; - walk_search( - &root.properties, - &mut Vec::new(), - &mut String::new(), - true, - &mut ctx, - ); + walk_search(&root.properties, &mut SearchPath::default(), true, &mut ctx); (hits, total) } +/// The path to the property currently being visited, carried down the walk and +/// unwound on the way back up. +/// +/// A real save holds well over a million properties, so this is the walk's hot +/// data structure and everything about it is shaped to avoid per-property +/// allocation: +/// +/// * `segments` borrows property names straight out of the tree and only owns +/// the `[index]` / `{mapKey}` segments it has to format, so descending into a +/// plain named property allocates nothing. +/// * `lower` is the lower-cased twin of `display`, maintained in lockstep. The +/// query terms are lower-cased once up front and matched against it, instead +/// of lower-casing the whole display path again at every leaf. +#[derive(Default)] +struct SearchPath<'a> { + segments: Vec>, + display: String, + lower: String, +} + +/// What [`SearchPath::push`] has to undo, so a pop restores the exact state. +struct SearchMark { + display_len: usize, + lower_len: usize, +} + +impl<'a> SearchPath<'a> { + /// Append one path segment (with the ` › ` separator when it joins a named + /// property to its parent) and return the mark that unwinds it. + fn push(&mut self, segment: Cow<'a, str>, separate: bool) -> SearchMark { + let mark = SearchMark { + display_len: self.display.len(), + lower_len: self.lower.len(), + }; + if separate && !self.display.is_empty() { + self.display.push_str(" › "); + self.lower.push_str(" › "); + } + self.display.push_str(&segment); + // Lower-casing per segment rather than per leaf is what makes the match + // cheap. It agrees with lower-casing the joined path: every segment is + // followed by a separator or the end of the string, so no character sits + // in a different word-final position in one form than in the other. + self.lower + .extend(segment.chars().flat_map(char::to_lowercase)); + self.segments.push(segment); + mark + } + + fn pop(&mut self, mark: SearchMark) { + self.segments.pop(); + self.display.truncate(mark.display_len); + self.lower.truncate(mark.lower_len); + } + + /// Materialize the owned path a hit carries. Only ever called for a property + /// that actually lands in the requested page. + fn to_owned_segments(&self) -> Vec { + self.segments.iter().map(|s| s.to_string()).collect() + } +} + struct SearchCtx<'a> { terms: &'a [String], offset: usize, @@ -677,13 +734,21 @@ struct SearchCtx<'a> { } impl SearchCtx<'_> { - /// Record a match: count it toward the total and push it if it falls inside - /// the requested page window. - fn record(&mut self, hit: PropertyHit) { + /// Whether the path built so far contains every query term. + fn matches(&self, path: &SearchPath) -> bool { + self.terms.iter().all(|term| path.lower.contains(term)) + } + + /// Record a match: count it toward the total and, only when it falls inside + /// the requested page, build the hit. The tree is always walked in full to + /// get an accurate total, so building hits lazily keeps the ~1.4M properties + /// a full scan visits from each paying for a path and value clone they would + /// only need if they were among the 50 rows actually returned. + fn record(&mut self, hit: impl FnOnce() -> PropertyHit) { let index = *self.total; *self.total += 1; if index >= self.offset && self.hits.len() < self.limit { - self.hits.push(hit); + self.hits.push(hit()); } } } @@ -723,71 +788,92 @@ fn scalar_display(value: &PropertyValue) -> Option { }) } -fn walk_search( - props: &[Property], - path: &mut Vec, - display: &mut String, +/// Whether [`scalar_display`] would produce a value — i.e. whether this is a +/// leaf the search reports rather than a container it descends into. The search +/// asks this first and only formats the value for a property that reaches the +/// result page, so the two must agree on every variant (asserted by +/// `scalar_display_agrees_with_is_scalar`). +fn is_scalar(value: &PropertyValue) -> bool { + matches!( + value, + PropertyValue::Int(_) + | PropertyValue::UInt32(_) + | PropertyValue::Int64(_) + | PropertyValue::Float(_) + | PropertyValue::Double(_) + | PropertyValue::Bool(_) + | PropertyValue::Byte(_) + | PropertyValue::Str(_) + | PropertyValue::Name(_) + | PropertyValue::Object(_) + | PropertyValue::Enum(_) + | PropertyValue::SoftObject(_) + ) +} + +/// Whether `name` occurs exactly once in `props`. Property lists are almost +/// always short, so a linear scan beats building a whole `HashMap` per list — +/// and the walk builds one per list, once per node in the tree. +fn occurs_once(props: &[Property], name: &str) -> bool { + let mut seen = 0usize; + for property in props { + if property.name == name { + seen += 1; + if seen > 1 { + return false; + } + } + } + seen == 1 +} + +fn walk_search<'a>( + props: &'a [Property], + path: &mut SearchPath<'a>, ancestors_addressable: bool, ctx: &mut SearchCtx, ) { - let mut name_counts = HashMap::<&str, usize>::new(); - for property in props { - *name_counts.entry(property.name.as_str()).or_default() += 1; - } for p in props { - let display_len = display.len(); - if !display.is_empty() { - display.push_str(" › "); - } - display.push_str(&p.name); - path.push(p.name.to_string()); - let addressable = - ancestors_addressable && name_counts.get(p.name.as_str()).copied() == Some(1); + let mark = path.push(Cow::Borrowed(p.name.as_str()), true); + let addressable = ancestors_addressable && occurs_once(props, &p.name); // Leaf value? - if let Some(value_display) = scalar_display(&p.value) { - if ctx.terms.iter().all(|t| display.to_lowercase().contains(t)) { - ctx.record(PropertyHit { - path: path.clone(), - display: display.clone(), + if is_scalar(&p.value) { + if ctx.matches(path) { + ctx.record(|| PropertyHit { + path: path.to_owned_segments(), + display: path.display.clone(), type_name: p.type_name.to_string(), - value_display, + value_display: scalar_display(&p.value).unwrap_or_default(), editable: addressable && scalar_editable(&p.value), }); } } else { - walk_value_search(&p.value, path, display, addressable, ctx); + walk_value_search(&p.value, path, addressable, ctx); } - path.pop(); - display.truncate(display_len); + path.pop(mark); } } -fn walk_value_search( - value: &PropertyValue, - path: &mut Vec, - display: &mut String, +fn walk_value_search<'a>( + value: &'a PropertyValue, + path: &mut SearchPath<'a>, ancestors_addressable: bool, ctx: &mut SearchCtx, ) { match value { PropertyValue::Struct(StructValue::Properties(inner)) => { - walk_search(inner, path, display, ancestors_addressable, ctx); + walk_search(inner, path, ancestors_addressable, ctx); } PropertyValue::Struct(StructValue::Instanced(Some(i))) => { - walk_search(&i.properties, path, display, ancestors_addressable, ctx); + walk_search(&i.properties, path, ancestors_addressable, ctx); } PropertyValue::ObjectInstances(objs) => { for (idx, obj) in objs.iter().enumerate() { - descend_indexed( - idx, - &obj.properties, - path, - display, - ancestors_addressable, - ctx, - ); + let mark = path.push(Cow::Owned(format!("[{idx}]")), false); + walk_search(&obj.properties, path, ancestors_addressable, ctx); + path.pop(mark); } } PropertyValue::Map { entries, .. } => { @@ -808,67 +894,33 @@ fn walk_value_search( Some(label) => format!("{{{label}}} [#{index}]"), None => format!("{{? #{index}}}"), }; - descend_value( - &segment, - value, - path, - display, - ancestors_addressable && unique, - ctx, - ); + descend_value(segment, value, path, ancestors_addressable && unique, ctx); } } PropertyValue::Array { elements } | PropertyValue::Set { elements, .. } => { for (idx, el) in elements.iter().enumerate() { - descend_value( - &format!("[{idx}]"), - el, - path, - display, - ancestors_addressable, - ctx, - ); + descend_value(format!("[{idx}]"), el, path, ancestors_addressable, ctx); } } _ => {} } } -fn descend_indexed( - idx: usize, - props: &[Property], - path: &mut Vec, - display: &mut String, - descendants_addressable: bool, - ctx: &mut SearchCtx, -) { - let display_len = display.len(); - let seg = format!("[{idx}]"); - display.push_str(&seg); - path.push(seg); - walk_search(props, path, display, descendants_addressable, ctx); - path.pop(); - display.truncate(display_len); -} - -fn descend_value( - seg: &str, - value: &PropertyValue, - path: &mut Vec, - display: &mut String, +fn descend_value<'a>( + seg: String, + value: &'a PropertyValue, + path: &mut SearchPath<'a>, descendants_addressable: bool, ctx: &mut SearchCtx, ) { - let display_len = display.len(); - display.push_str(seg); - path.push(seg.to_string()); - if let Some(value_display) = scalar_display(value) { - if ctx.terms.iter().all(|t| display.to_lowercase().contains(t)) { - ctx.record(PropertyHit { - path: path.clone(), - display: display.clone(), + let mark = path.push(Cow::Owned(seg), false); + if is_scalar(value) { + if ctx.matches(path) { + ctx.record(|| PropertyHit { + path: path.to_owned_segments(), + display: path.display.clone(), type_name: container_value_type(value).to_string(), - value_display, + value_display: scalar_display(value).unwrap_or_default(), // This hit's path ends on a `{mapKey}` or `[index]` segment. // `setValue` only resolves to tagged Property nodes and rejects // paths ending on a container element, so such scalars are not @@ -877,10 +929,9 @@ fn descend_value( }); } } else { - walk_value_search(value, path, display, descendants_addressable, ctx); + walk_value_search(value, path, descendants_addressable, ctx); } - path.pop(); - display.truncate(display_len); + path.pop(mark); } fn hex_guid(raw: &[u8; 16]) -> String { @@ -4220,6 +4271,55 @@ mod tests { assert_eq!(total_end, 2); } + /// The search asks `is_scalar` whether a value is a leaf and only calls + /// `scalar_display` for the properties that reach the result page. A variant + /// the two disagree about would either be silently dropped from the results + /// or reported with an empty value, so pin the agreement over one value of + /// every variant. + #[test] + fn scalar_display_agrees_with_is_scalar() { + let values = [ + PropertyValue::Int(1), + PropertyValue::UInt32(1), + PropertyValue::Int64(1), + PropertyValue::Float(1.0), + PropertyValue::Double(1.0), + PropertyValue::Bool(true), + PropertyValue::Byte(1), + PropertyValue::Str("s".into()), + PropertyValue::Name("n".into()), + PropertyValue::Object("o".into()), + PropertyValue::Enum("e".into()), + PropertyValue::SoftObject(SoftObjectPath { + package_name: "p".into(), + asset_name: "a".into(), + sub_path: String::new(), + }), + PropertyValue::Opaque(vec![1]), + PropertyValue::Array { elements: vec![] }, + PropertyValue::Set { + elements: vec![], + num_to_remove: 0, + }, + PropertyValue::Map { + entries: vec![], + num_to_remove: 0, + }, + PropertyValue::ObjectInstances(vec![]), + PropertyValue::Struct(StructValue::Properties(vec![])), + PropertyValue::Struct(StructValue::Instanced(None)), + PropertyValue::Struct(StructValue::GameplayTagContainer(vec![])), + PropertyValue::Struct(StructValue::Guid([0; 16])), + ]; + for value in &values { + assert_eq!( + is_scalar(value), + scalar_display(value).is_some(), + "is_scalar disagrees with scalar_display for {value:?}" + ); + } + } + #[test] fn search_marks_strings_editable() { // root class string is not a property; build a payload with a StrProperty From e051c72d0c1a6694f9c355bdd6d5398c3ccc7950 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 01:14:33 +0200 Subject: [PATCH 02/16] perf(save): load a save once, and answer a repeat read for free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a save and then walking its tabs cost around twelve seconds of core work on a real save, and every repeat paid it again. Four things: - `inspect_save` ran eleven independent read-only passes back to back: the FString scan, the typed parse, and one traversal each for the inventory, armor, slot integrity, progression, NPC, faction, skill and glossary blocks. They share no state, so they now run on scoped threads in two stages, and the load costs the longest pass instead of their sum. The inventory summary is split so its byte scan no longer waits for the typed-tree traversals it does not depend on. 7.7 s -> 1.7 s. - `private.skills.list` was the one read command that went around the parsed-root cache: it copied the whole decoded payload out of the byte cache and re-parsed it. 700 ms -> 60 ms. - No command result was cached, so re-opening a save or returning to a tab recomputed everything. Read commands now memoize their response under the request plus a content fingerprint of every file the answer depends on — for `inspect_save` that includes the sibling PersistentDataList.sav, which carries the slot's profile. Directory listings (`list_backups`, `scan_save_dir`) are deliberately excluded: they describe a folder, which changes without any save changing. A repeat read is now 2 ms, the cost of reading and hashing the file. - `list_backups` read and hashed every backup file one after another; a folder with a hundred-odd backups made that a tenth of a second on every save selection. The candidates are now described in parallel. 130 ms -> 50 ms (disk-bound, so this does not scale with cores). Verified byte-identical against the previous core: seventeen commands over a real save, including the preview and public-only inspects, 2.2 MB of responses. New integration tests pin the cache against serving a stale read after a write, after an out-of-band file replacement, and for a directory listing that must not be cached at all. Co-Authored-By: Claude Opus 5 --- crates/gore-save/examples/cmd_dump.rs | 40 ++ crates/gore-save/examples/tab_timer.rs | 126 +++++ crates/gore-save/src/lib.rs | 663 ++++++++++++++++------- crates/gore-save/tests/response_cache.rs | 189 +++++++ 4 files changed, 836 insertions(+), 182 deletions(-) create mode 100644 crates/gore-save/examples/cmd_dump.rs create mode 100644 crates/gore-save/examples/tab_timer.rs create mode 100644 crates/gore-save/tests/response_cache.rs diff --git a/crates/gore-save/examples/cmd_dump.rs b/crates/gore-save/examples/cmd_dump.rs new file mode 100644 index 00000000..5d293bd5 --- /dev/null +++ b/crates/gore-save/examples/cmd_dump.rs @@ -0,0 +1,40 @@ +//! Scratch research driver: dump the full response of every read command the +//! save editor issues, so an optimized core can be diffed against the previous +//! one. Read-only; writes only the dump file. Not shipped. + +fn main() { + let path = std::env::args().nth(1).expect("usage: cmd_dump "); + let out = std::env::args().nth(2).expect("usage: cmd_dump "); + let esc = serde_json::to_string(&path).unwrap(); + + let requests: Vec = vec![ + format!(r#"{{"command":"inspect_save","payload":{{"path":{esc},"includePrivate":true}}}}"#), + format!(r#"{{"command":"inspect_save","payload":{{"path":{esc},"includePrivate":true,"privateChunkLimit":4}}}}"#), + format!(r#"{{"command":"inspect_save","payload":{{"path":{esc}}}}}"#), + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"GameTime","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"AttributesByGlobalId {{Hero}}","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"","offset":900000,"limit":200}}}}"#), + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"","offset":0,"limit":50,"includeNodes":true,"source":"private"}}}}"#), + format!(r#"{{"command":"private.characters.list","payload":{{"path":{esc},"query":"","offset":0,"limit":100000}}}}"#), + format!(r#"{{"command":"private.skills.list","payload":{{"path":{esc},"actor":"Hero"}}}}"#), + format!(r#"{{"command":"private.npc.list","payload":{{"path":{esc},"query":"","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"private.factions.list","payload":{{"path":{esc}}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"quests","query":"","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"glossary","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"tutorials","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"story","query":"","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"knowledge","character":"Hero","query":"","offset":0,"limit":1000}}}}"#), + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"events","character":"Hero","query":"","offset":0,"limit":1000}}}}"#), + ]; + + let mut text = String::new(); + for request in &requests { + let response = gore_save::execute_json(request); + let parsed: serde_json::Value = serde_json::from_str(&response).unwrap(); + text.push_str(&format!("=== {request}\n")); + text.push_str(&serde_json::to_string_pretty(&parsed).unwrap()); + text.push('\n'); + } + std::fs::write(&out, &text).expect("write dump"); + println!("wrote {out} ({} bytes)", text.len()); +} diff --git a/crates/gore-save/examples/tab_timer.rs b/crates/gore-save/examples/tab_timer.rs new file mode 100644 index 00000000..b2dc089e --- /dev/null +++ b/crates/gore-save/examples/tab_timer.rs @@ -0,0 +1,126 @@ +//! Scratch research driver: replay the exact command sequence the save editor's +//! tabs issue and time each one through the public FFI entry point. Read-only. +//! Not shipped. + +use std::time::Instant; + +fn main() { + let path = std::env::args().nth(1).expect("usage: tab_timer "); + let esc = serde_json::to_string(&path).unwrap(); + + // (label, command, payload-json-without-path) + let steps: Vec<(&str, String)> = vec![ + ( + "inspect_save (initial load)", + format!(r#"{{"command":"inspect_save","payload":{{"path":{esc},"includePrivate":true}}}}"#), + ), + ( + "check_codec", + format!(r#"{{"command":"check_codec","payload":{{"path":{esc}}}}}"#), + ), + // Overview tab + ( + "OVERVIEW loadGameTime (search 'GameTime')", + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"GameTime","offset":0,"limit":1000}}}}"#), + ), + // Characters tab + ( + "CHARACTERS loadAllCharacters", + format!(r#"{{"command":"private.characters.list","payload":{{"path":{esc},"query":"","offset":0,"limit":100000}}}}"#), + ), + ( + "CHARACTERS loadHeroAttributes (search)", + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"AttributesByGlobalId {{Hero}}","offset":0,"limit":1000}}}}"#), + ), + ( + "CHARACTERS loadSkills (Hero)", + format!(r#"{{"command":"private.skills.list","payload":{{"path":{esc},"actor":"Hero"}}}}"#), + ), + ( + "CHARACTERS loadAllNpcActors", + format!(r#"{{"command":"private.npc.list","payload":{{"path":{esc},"query":"","offset":0,"limit":100000}}}}"#), + ), + // World tab + ( + "WORLD loadProgressionQuests", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"quests","query":"","offset":0,"limit":100}}}}"#), + ), + ( + "WORLD loadGlossary", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"glossary","offset":0,"limit":1000}}}}"#), + ), + ( + "WORLD loadProgressionTutorials", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"tutorials","offset":0,"limit":100}}}}"#), + ), + ( + "WORLD loadStoryState", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"story","query":"","offset":0,"limit":1000}}}}"#), + ), + ( + "WORLD loadFactions", + format!(r#"{{"command":"private.factions.list","payload":{{"path":{esc}}}}}"#), + ), + ( + "WORLD loadKnowledgeEntries (Hero)", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"knowledge","character":"Hero","query":"","offset":0,"limit":200}}}}"#), + ), + ( + "WORLD loadMemoryEvents (Hero)", + format!(r#"{{"command":"query_progression","payload":{{"path":{esc},"section":"events","character":"Hero","query":"","offset":0,"limit":200}}}}"#), + ), + // All data tab + ( + "ALLDATA browse (includeNodes)", + format!(r#"{{"command":"search_typed_properties","payload":{{"path":{esc},"query":"","offset":0,"limit":50,"includeNodes":true,"source":"private"}}}}"#), + ), + ( + "BACKUPS list_backups", + format!(r#"{{"command":"list_backups","payload":{{"path":{esc}}}}}"#), + ), + ]; + + // This box is rarely idle (a running game, a browser), and background load + // inflates every sample. Repeat each step and report the MINIMUM, which is + // the sample least contaminated by contention; the median is printed beside + // it so a wide spread is visible rather than hidden. + let runs: usize = std::env::args() + .nth(2) + .and_then(|v| v.parse().ok()) + .unwrap_or(5); + + println!("save: {path} ({runs} runs per step, reporting min)\n"); + println!("{:<44} {:>10} {:>10} {:>10}", "step", "min", "median", "resp KB"); + println!("{}", "-".repeat(78)); + + let mut min_total = 0.0f64; + for (label, request) in &steps { + let mut samples = Vec::with_capacity(runs); + let mut response = String::new(); + for _ in 0..runs { + let t = Instant::now(); + response = gore_save::execute_json(request); + samples.push(t.elapsed().as_secs_f64() * 1000.0); + } + samples.sort_by(|a, b| a.partial_cmp(b).unwrap()); + let min = samples[0]; + let median = samples[samples.len() / 2]; + + let ok = response.contains(r#""ok":true"#); + min_total += min; + println!( + "{:<44} {:>8.1}ms {:>8.1}ms {:>10.0} {}", + label, + min, + median, + response.len() as f64 / 1024.0, + if ok { "" } else { " <-- FAILED" }, + ); + if !ok { + let short: String = response.chars().take(160).collect(); + println!(" {short}"); + } + } + println!("{}", "-".repeat(78)); + println!("{:<44} {:>8.1}ms", "TOTAL (min)", min_total); +} diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 0c7cf6af..e92dcbcf 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -367,8 +367,25 @@ impl<'a> Reader<'a> { } pub fn execute_json(input: &str) -> String { + // A read command is a pure function of the files it reads, so an identical + // request against unchanged files can be answered from the last response. + // This is what makes re-opening a save, or returning to a tab, free. + let cache_key = read_response_cache_key(input); + if let Some(key) = &cache_key { + if let Some(hit) = cached_response(key) { + return hit; + } + } match execute_json_inner(input) { - Ok(data) => json!({ "ok": true, "data": data }).to_string(), + Ok(data) => { + let response = json!({ "ok": true, "data": data }).to_string(); + // Only successes are cached: a failure is usually transient (a file + // being written, a codec hiccup) and must stay retryable. + if let Some(key) = cache_key { + store_cached_response(key, &response); + } + response + } Err(err) => { let code = match &err { CoreError::InvalidRequest(_) => "INVALID_REQUEST", @@ -1939,40 +1956,15 @@ pub fn list_save_backups(path: &Path) -> Result, CoreError> } } - for (backup_path, file_name) in candidates { - let data = fs::read(&backup_path)?; - let metadata = fs::metadata(&backup_path)?; - let created_epoch = parse_backup_epoch(&file_name, &prefix); - let (status, player_save_name, slot_name) = - match inspect_bytes(&data, Some(&backup_path), false) { - Ok(info) => { - let public = info.get("public").cloned().unwrap_or_else(|| json!({})); - ( - "ok".to_string(), - public - .get("playerSaveName") - .and_then(Value::as_str) - .map(ToOwned::to_owned), - public - .get("slotName") - .and_then(Value::as_str) - .map(ToOwned::to_owned), - ) - } - Err(err) => (err.to_string(), None, None), - }; - backups.push(BackupListItem { - path: backup_path.display().to_string(), - name: names.get(&file_name).cloned(), - file_name, - file_size: metadata.len(), - sha1: sha1_hex(&data), - created_epoch, - status, - player_save_name, - slot_name, - scope: "save".to_string(), - }); + // Every candidate is read whole and hashed whole. A save folder that has + // been backed up for a while holds dozens of multi-megabyte files, and this + // listing sits in the load path — so read them side by side rather than one + // after another. Order is preserved, and a read error still aborts the whole + // listing exactly as a serial loop would. + for item in par_map(candidates, |candidate| { + describe_save_backup(candidate, &prefix, &names) + }) { + backups.push(item?); } backups.sort_by(|a, b| { b.created_epoch @@ -1982,6 +1974,48 @@ pub fn list_save_backups(path: &Path) -> Result, CoreError> Ok(backups) } +/// Read one save backup and describe it for the listing. Split out of +/// [`list_save_backups`] so the candidates can be described in parallel. +fn describe_save_backup( + (backup_path, file_name): (PathBuf, String), + prefix: &str, + names: &HashMap, +) -> Result { + let data = fs::read(&backup_path)?; + let metadata = fs::metadata(&backup_path)?; + let created_epoch = parse_backup_epoch(&file_name, prefix); + let (status, player_save_name, slot_name) = + match inspect_bytes(&data, Some(&backup_path), false) { + Ok(info) => { + let public = info.get("public").cloned().unwrap_or_else(|| json!({})); + ( + "ok".to_string(), + public + .get("playerSaveName") + .and_then(Value::as_str) + .map(ToOwned::to_owned), + public + .get("slotName") + .and_then(Value::as_str) + .map(ToOwned::to_owned), + ) + } + Err(err) => (err.to_string(), None, None), + }; + Ok(BackupListItem { + path: backup_path.display().to_string(), + name: names.get(&file_name).cloned(), + file_name, + file_size: metadata.len(), + sha1: sha1_hex(&data), + created_epoch, + status, + player_save_name, + slot_name, + scope: "save".to_string(), + }) +} + fn list_persistent_data_list_backups_for_save( path: &Path, ) -> Result, CoreError> { @@ -2031,48 +2065,12 @@ fn list_persistent_data_list_backups_for_save( } } - for (backup_path, file_name) in candidates { - let data = fs::read(&backup_path)?; - let metadata = fs::metadata(&backup_path)?; - let created_epoch = parse_backup_epoch(&file_name, &prefix); - let (status, player_save_name, slot_name) = - match inspect_bytes(&data, Some(&backup_path), false) { - Ok(_) => { - let persistent_slots = parse_persistent_slot_metadata(&data); - let slot_meta = persistent_slots.get(slot); - let player_save_name = slot_meta.and_then(|m| m.player_save_name.clone()); - let slot_name = slot_meta - .and_then(|m| m.slot_name.clone()) - .unwrap_or_else(|| slot.to_string()); - // inspect_bytes' GVAS branch only checks the magic and scans - // strings, so require a STRICT profile parse before reporting - // a restorable "ok": a truncated/manual backup that still - // contains the slot strings must not enable the Restore - // action (which would overwrite the live profile with corrupt - // bytes). Metadata is still surfaced for display. - let status = if parse_profile_file(&data).is_err() { - "invalid PersistentDataList structure".to_string() - } else if slot_meta.is_none() { - "selected slot metadata missing".to_string() - } else { - "ok".to_string() - }; - (status, player_save_name, Some(slot_name)) - } - Err(err) => (err.to_string(), None, Some(slot.to_string())), - }; - backups.push(BackupListItem { - path: backup_path.display().to_string(), - name: names.get(&file_name).cloned(), - file_name, - file_size: metadata.len(), - sha1: sha1_hex(&data), - created_epoch, - status, - player_save_name, - slot_name, - scope: "persistent_data_list".to_string(), - }); + // Read and parse the profile backups side by side, as the save backups above + // are: each one is read whole, hashed whole, and strictly profile-parsed. + for item in par_map(candidates, |candidate| { + describe_profile_backup(candidate, &prefix, &names, slot) + }) { + backups.push(item?); } backups.sort_by(|a, b| { b.created_epoch @@ -2082,6 +2080,59 @@ fn list_persistent_data_list_backups_for_save( Ok(backups) } +/// Read one PersistentDataList backup and describe it for the listing, from the +/// point of view of the save slot `slot`. Split out of +/// [`list_persistent_data_list_backups_for_save`] so the candidates can be +/// described in parallel. +fn describe_profile_backup( + (backup_path, file_name): (PathBuf, String), + prefix: &str, + names: &HashMap, + slot: &str, +) -> Result { + let data = fs::read(&backup_path)?; + let metadata = fs::metadata(&backup_path)?; + let created_epoch = parse_backup_epoch(&file_name, prefix); + let (status, player_save_name, slot_name) = + match inspect_bytes(&data, Some(&backup_path), false) { + Ok(_) => { + let persistent_slots = parse_persistent_slot_metadata(&data); + let slot_meta = persistent_slots.get(slot); + let player_save_name = slot_meta.and_then(|m| m.player_save_name.clone()); + let slot_name = slot_meta + .and_then(|m| m.slot_name.clone()) + .unwrap_or_else(|| slot.to_string()); + // inspect_bytes' GVAS branch only checks the magic and scans + // strings, so require a STRICT profile parse before reporting + // a restorable "ok": a truncated/manual backup that still + // contains the slot strings must not enable the Restore + // action (which would overwrite the live profile with corrupt + // bytes). Metadata is still surfaced for display. + let status = if parse_profile_file(&data).is_err() { + "invalid PersistentDataList structure".to_string() + } else if slot_meta.is_none() { + "selected slot metadata missing".to_string() + } else { + "ok".to_string() + }; + (status, player_save_name, Some(slot_name)) + } + Err(err) => (err.to_string(), None, Some(slot.to_string())), + }; + Ok(BackupListItem { + path: backup_path.display().to_string(), + name: names.get(&file_name).cloned(), + file_name, + file_size: metadata.len(), + sha1: sha1_hex(&data), + created_epoch, + status, + player_save_name, + slot_name, + scope: "persistent_data_list".to_string(), + }) +} + fn restore_backup(path: &Path, backup_path: &Path) -> Result { restore_backup_with_before_replace(path, backup_path, |_| Ok(())) } @@ -4743,6 +4794,58 @@ fn extract_script_paths(data: &[u8]) -> Vec { .collect() } +/// Wait for one scoped summary thread. A panic inside a summary is re-raised on +/// the caller's thread instead of being turned into a default value, so a bug in +/// one traversal can never quietly become an empty block in the response. +fn join(handle: std::thread::ScopedJoinHandle<'_, T>) -> T { + handle + .join() + .unwrap_or_else(|payload| std::panic::resume_unwind(payload)) +} + +/// Apply `work` to every item across scoped threads, returning the results in +/// the original order. For independent per-item work that is heavy enough to be +/// worth splitting — reading and hashing a folder full of save backups, say. +/// +/// Items are handed out in contiguous chunks, one chunk per thread, bounded by +/// the machine's parallelism so a folder with hundreds of entries does not spawn +/// hundreds of threads. +fn par_map(items: Vec, work: impl Fn(T) -> R + Sync) -> Vec +where + T: Send, + R: Send, +{ + let threads = std::thread::available_parallelism() + .map(|value| value.get()) + .unwrap_or(4) + .min(items.len()); + if threads <= 1 { + return items.into_iter().map(work).collect(); + } + let total = items.len(); + let chunk = total.div_ceil(threads); + // Owned chunks, so each thread consumes its own items by value. + let mut parts: Vec> = Vec::with_capacity(threads); + let mut rest = items; + while !rest.is_empty() { + let tail = rest.split_off(chunk.min(rest.len())); + parts.push(rest); + rest = tail; + } + let mut out = Vec::with_capacity(total); + std::thread::scope(|scope| { + let work = &work; + let handles: Vec<_> = parts + .into_iter() + .map(|part| scope.spawn(move || part.into_iter().map(work).collect::>())) + .collect(); + for handle in handles { + out.extend(join(handle)); + } + }); + out +} + fn inspect_private_payload( data: &[u8], path: Option<&Path>, @@ -4765,28 +4868,38 @@ fn inspect_private_payload( match decompress_private_payload_with_limit(data, stream, backend, private_chunk_limit) { Ok((payload, decoded_chunk_count)) => { let preview = decoded_chunk_count < stream.chunk_count; - // A full (non-preview) decode here is identical to what the typed - // property browser would re-decode on its first search. Seed the - // shared cache so the common inspect-then-browse path pays the - // ~20s decode only once per save. - if !preview { - if let Some(p) = path { - store_decoded_payload_cache(p, sha1_hex(data), payload.clone()); + // Everything below this point is a read-only pass over the same + // decoded bytes or the same parsed tree, and there are a dozen of + // them: the FString scan, the parse, and then one traversal each for + // the inventory, armor, slot-integrity, progression, NPC, faction and + // skill blocks. Run serially they added up to seconds of dead time on + // every load. They share no state, so they run on scoped threads and + // the load costs the longest pass rather than their sum. + // + // Stage 1: the FString scan and the typed parse both read `payload` + // and nothing else, so they overlap with each other. + let (refs, typed_result) = std::thread::scope(|scope| { + let scan = scope.spawn(|| scan_fstrings(&payload, 0)); + // A full (non-preview) decode here is identical to what the typed + // property browser would re-decode on its first search. Seed the + // shared cache so the common inspect-then-browse path pays the + // decode only once per save. + if !preview { + if let Some(p) = path { + store_decoded_payload_cache(p, sha1_hex(data), payload.clone()); + } } - } - let refs = scan_fstrings(&payload, 0); - let strings = refs - .iter() - .map(|reference| reference.value.clone()) - .filter(|value| !value.is_empty()) - .take(200) - .collect::>(); - let player = summarize_private_player_payload(&payload, &refs); - let typed_result: Option, CoreError>> = if preview { - None - } else { - Some(properties::parse_private_root(&payload).map(Arc::new)) - }; + let typed_result: Option, CoreError>> = + if preview { + None + } else { + Some(properties::parse_private_root(&payload).map(Arc::new)) + }; + // Never swallow a panic into a silently empty ref list — the + // summaries below would then report an empty save. + let refs = scan.join().unwrap_or_else(|e| std::panic::resume_unwind(e)); + (refs, typed_result) + }); // Seed the parsed-root cache with the parse we just did for the // summary. Without this the FIRST private read command after a load // (characters.list / npc.attributes / …) re-parses the whole payload @@ -4797,71 +4910,94 @@ fn inspect_private_payload( if let (Some(p), Some(Ok(root))) = (path, typed_result.as_ref()) { store_parsed_root_cache(p, sha1_hex(data), Arc::clone(root)); } - let main_container = typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .and_then(|r| main_container_summary(r)); - let armor_slot = typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .and_then(|r| armor_slot_summary(r)); - let misaligned = typed_result + let typed_parse = summarize_typed_parse_result(&payload, typed_result.as_ref()); + let typed_ok = typed_parse["status"] == "ok"; + let root = typed_result .as_ref() - .and_then(|r| r.as_ref().ok()) - .map(|r| misaligned_slot_containers(r)) - .unwrap_or_default(); - let inventory = summarize_private_inventory_payload( - &payload, - &refs, + .and_then(|result| result.as_ref().ok()) + .map(|root| root.as_ref()); + // Only the blocks that describe game state require a parse that + // covered the whole payload; the capability probes below are happy + // with any tree that parsed. + let verified_root = root.filter(|_| typed_ok); + + // Stage 2: one traversal per summary, all independent. + let ( + strings, + player, + inventory_scan, + main_container, + armor_slot, + misaligned, + hero_has_effects, + glossary_writable, + story_writable, + progression, + npc, + faction_guilds, + ) = std::thread::scope(|scope| { + let strings = scope.spawn(|| { + refs.iter() + .map(|reference| reference.value.clone()) + .filter(|value| !value.is_empty()) + .take(200) + .collect::>() + }); + let player = scope.spawn(|| summarize_private_player_payload(&payload, &refs)); + let inventory_scan = scope.spawn(|| scan_private_inventory(&payload, &refs)); + let main_container = scope.spawn(|| root.and_then(main_container_summary)); + let armor_slot = scope.spawn(|| root.and_then(armor_slot_summary)); + let misaligned = + scope.spawn(|| root.map(misaligned_slot_containers).unwrap_or_default()); + // `private.skills.set` needs the hero's ActiveEffects array as its + // edit target; apply_skill_set rejects the write otherwise. Gate + // the advertised capability on it so a guaranteed-to-fail op is + // never offered (e.g. a fresh save whose hero has no effects yet). + let hero_has_effects = scope.spawn(|| { + root.is_some_and(|root| skills::actor_has_active_effects(root, "Hero")) + }); + let glossary_writable = + scope.spawn(|| root.is_some_and(glossary_set_segment_writable)); + let story_writable = scope.spawn(|| root.is_some_and(story::is_writable)); + let progression = + scope.spawn(|| summarize_private_progression_overview(verified_root)); + // NPC capability block: the frontend feature-detects the + // "Attribute" tab from this. `hasNpcs` is true only when the typed + // parse succeeds and the save's _Attributes map yields at least + // one NPC. Attribute editing itself rides on the already-advertised + // `private.typed.setValue`; here we surface the two NPC-specific + // structural edits. + let npc = scope.spawn(|| summarize_private_npc_payload(verified_root)); + // Faction crime block: per-camp-guild crime counts for the player. + // The forgive edit is advertised only when at least one guild has + // an unforgiven Hero crime (so write_save never rejects an + // advertised op). + let faction_guilds = scope.spawn(|| { + verified_root + .map(factions::list_guild_crimes) + .unwrap_or_default() + }); + ( + join(strings), + join(player), + join(inventory_scan), + join(main_container), + join(armor_slot), + join(misaligned), + join(hero_has_effects), + join(glossary_writable), + join(story_writable), + join(progression), + join(npc), + join(faction_guilds), + ) + }); + let inventory = assemble_private_inventory( + inventory_scan, main_container.as_ref(), armor_slot.as_ref(), &misaligned, ); - let typed_parse = summarize_typed_parse_result(&payload, typed_result.as_ref()); - let typed_ok = typed_parse["status"] == "ok"; - // `private.skills.set` needs the hero's ActiveEffects array as its edit - // target; apply_skill_set rejects the write otherwise. Gate the - // advertised capability on it so a guaranteed-to-fail op is never - // offered (e.g. a fresh save whose hero has no effects yet). - let hero_has_effects = typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .map(|r| skills::actor_has_active_effects(r, "Hero")) - .unwrap_or(false); - let glossary_writable = typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .map(|r| glossary_set_segment_writable(r)) - .unwrap_or(false); - let progression = summarize_private_progression_overview( - typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .filter(|_| typed_ok) - .map(|r| r.as_ref()), - ); - // NPC capability block: the frontend feature-detects the "Attribute" - // tab from this. `hasNpcs` is true only when the typed parse succeeds - // and the save's _Attributes map yields at least one NPC. Attribute - // editing itself rides on the already-advertised - // `private.typed.setValue`; here we surface the two NPC-specific - // structural edits. - let npc = summarize_private_npc_payload( - typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .filter(|_| typed_ok) - .map(|r| r.as_ref()), - ); - // Faction crime block: per-camp-guild crime counts for the player. The - // forgive edit is advertised only when at least one guild has an - // unforgiven Hero crime (so write_save never rejects an advertised op). - let faction_guilds = typed_result - .as_ref() - .and_then(|r| r.as_ref().ok()) - .filter(|_| typed_ok) - .map(|r| factions::list_guild_crimes(r)) - .unwrap_or_default(); let any_unforgiven = faction_guilds.iter().any(|g| g.unforgiven > 0); let factions = json!({ "guilds": faction_guilds }); let mut writable = vec!["private.replaceFString"]; @@ -4880,11 +5016,7 @@ fn inspect_private_payload( // missing map entry and set member atomically. "private.knowledge.setEntry", ]); - if typed_result - .as_ref() - .and_then(|result| result.as_ref().ok()) - .is_some_and(|root| story::is_writable(root)) - { + if story_writable { writable.push("private.story.apply"); } // Hero skill edits (retarget / unlearn / learn a GameplayEffect @@ -5071,13 +5203,20 @@ fn apply_equipped_and_upgrades(items: &mut [Value], armor_slot: Option<&ArmorSlo /// exists so a pathological payload cannot produce unbounded JSON. const PLAYER_INVENTORY_ROW_LIMIT: usize = 4096; -fn summarize_private_inventory_payload( - payload: &[u8], - refs: &[FStringRef], - main_container: Option<&MainContainerSummary>, - armor_slot: Option<&ArmorSlotSummary>, - misaligned: &[(Vec, usize)], -) -> Value { +/// The half of the inventory summary that only reads the decoded bytes and the +/// FString scan. Split out from [`summarize_private_inventory_payload`] so +/// `inspect_save` can run it beside the typed-tree traversals it does not depend +/// on, instead of waiting for them. +struct InventoryScan { + script_paths: Vec, + properties: Vec, + candidates: Vec, + items: Vec, + item_stack_count: usize, + item_scope: &'static str, +} + +fn scan_private_inventory(payload: &[u8], refs: &[FStringRef]) -> InventoryScan { let script_paths = unique_strings( refs.iter().map(|r| r.value.as_str()).filter(|value| { value.starts_with("/Script/") && contains_any_ci(value, &["inventory", "item"]) @@ -5097,8 +5236,53 @@ fn summarize_private_inventory_payload( .filter(|value| looks_inventory_candidate(value)), 200, ); - let (mut items, item_stack_count, item_scope) = + let (items, item_stack_count, item_scope) = summarize_private_inventory_items(payload, refs, PLAYER_INVENTORY_ROW_LIMIT); + InventoryScan { + script_paths, + properties, + candidates, + items, + item_stack_count, + item_scope, + } +} + +/// The two halves back to back, as `inspect_save` composes them. Kept for the +/// tests that assert on a whole inventory block; the load path runs the halves +/// separately so the byte scan overlaps the typed-tree traversals. +#[cfg(test)] +fn summarize_private_inventory_payload( + payload: &[u8], + refs: &[FStringRef], + main_container: Option<&MainContainerSummary>, + armor_slot: Option<&ArmorSlotSummary>, + misaligned: &[(Vec, usize)], +) -> Value { + assemble_private_inventory( + scan_private_inventory(payload, refs), + main_container, + armor_slot, + misaligned, + ) +} + +/// Join the byte-level scan with the typed-tree facts that decide what is +/// editable. Cheap: no traversal of its own. +fn assemble_private_inventory( + scan: InventoryScan, + main_container: Option<&MainContainerSummary>, + armor_slot: Option<&ArmorSlotSummary>, + misaligned: &[(Vec, usize)], +) -> Value { + let InventoryScan { + script_paths, + properties, + candidates, + mut items, + item_stack_count, + item_scope, + } = scan; // Mark which rows can be deleted. removeItem addresses by path, so only a // path that occurs exactly once across the whole inventory is safe — a row // sharing its path with another container's stack must not offer delete, or @@ -5267,6 +5451,124 @@ fn summarize_typed_parse_result( } } +/// The read commands whose response is fully determined by the files they read, +/// and which are therefore safe to answer from [`RESPONSE_CACHE`]. +/// +/// Deliberately excluded: `scan_save_dir` and `list_backups` (they describe a +/// DIRECTORY, whose contents change without any save file changing), `check_codec` +/// (no file at all, and already instant) and every `loc_*` command (they read the +/// game installation, not the save). +const CACHEABLE_READ_COMMANDS: &[&str] = &[ + "inspect_save", + "search_typed_properties", + "query_progression", + "private.skills.list", + "private.npc.list", + "private.characters.list", + "private.npc.attributes", + "private.npc.position", + "private.npc.inventory", + "private.factions.list", +]; + +/// Identity of one cached response: the exact request, plus a content +/// fingerprint of every file that request's answer depends on. Any edit to the +/// save — by this editor, the game, or a cloud sync — changes the fingerprint +/// and misses, so a hit is always a byte-identity match and never a +/// trust-the-clock guess. Mirrors how the decode caches key themselves. +#[derive(PartialEq, Eq)] +struct ResponseCacheKey { + request: String, + fingerprint: String, +} + +struct CachedResponseEntry { + key: ResponseCacheKey, + /// Kept alongside the key so a write can drop this save's entries eagerly + /// instead of waiting for them to age out. + path: PathBuf, + response: String, +} + +/// Bounded so a long session cannot grow without limit. A whole save's worth of +/// editor queries is around fifteen entries and well under a megabyte, so these +/// hold several saves at once — switching back and forth stays free — while +/// staying negligible next to the decoded payload the core already keeps. +const RESPONSE_CACHE_MAX_ENTRIES: usize = 64; +const RESPONSE_CACHE_MAX_BYTES: usize = 16 * 1024 * 1024; + +static RESPONSE_CACHE: Mutex> = Mutex::new(Vec::new()); + +/// Build the cache identity for a request, or `None` when the command is not +/// cacheable, carries no save path, or its file cannot be read. +fn read_response_cache_key(input: &str) -> Option { + let value: Value = serde_json::from_str(input).ok()?; + let command = value.get("command")?.as_str()?; + if !CACHEABLE_READ_COMMANDS.contains(&command) { + return None; + } + let path = Path::new(value.get("payload")?.get("path")?.as_str()?); + let mut fingerprint = sha1_hex(&fs::read(path).ok()?); + // `inspect_save` also reports which profile owns the slot, and that lives in + // a sibling file rather than in the save. Fold it into the fingerprint so + // assigning a save to another profile is a miss, not a stale hit. + if command == "inspect_save" { + if let Some(bytes) = path + .parent() + .map(|dir| dir.join("PersistentDataList.sav")) + .and_then(|companion| fs::read(companion).ok()) + { + fingerprint.push_str(&sha1_hex(&bytes)); + } + } + Some(ResponseCacheKey { + request: input.to_string(), + fingerprint, + }) +} + +fn cached_response(key: &ResponseCacheKey) -> Option { + let guard = RESPONSE_CACHE.lock().unwrap_or_else(|e| e.into_inner()); + guard + .iter() + .find(|entry| &entry.key == key) + .map(|entry| entry.response.clone()) +} + +fn store_cached_response(key: ResponseCacheKey, response: &str) { + // The key was built from a request that carried a readable `payload.path`, + // so this re-read always resolves. + let save_path = serde_json::from_str::(&key.request) + .ok() + .and_then(|value| Some(PathBuf::from(value.get("payload")?.get("path")?.as_str()?))) + .unwrap_or_default(); + let mut guard = RESPONSE_CACHE.lock().unwrap_or_else(|e| e.into_inner()); + // A request that raced another thread to the same answer is already here. + if guard.iter().any(|entry| entry.key == key) { + return; + } + guard.push(CachedResponseEntry { + key, + path: save_path, + response: response.to_string(), + }); + // Oldest first: within one save every entry is wanted, so evicting by age + // drops the save the user has moved away from rather than the current one. + let mut bytes: usize = guard.iter().map(|entry| entry.response.len()).sum(); + while guard.len() > RESPONSE_CACHE_MAX_ENTRIES || bytes > RESPONSE_CACHE_MAX_BYTES { + let evicted = guard.remove(0); + bytes -= evicted.response.len(); + } +} + +/// Drop every cached response for `path`. Their fingerprints would miss anyway +/// once the file changes; this just releases the memory at the moment of the +/// write instead of leaving it to age out. +fn invalidate_response_cache(path: &Path) { + let mut guard = RESPONSE_CACHE.lock().unwrap_or_else(|e| e.into_inner()); + guard.retain(|entry| entry.path != path); +} + /// In-memory cache of the most recently decoded private payload. Decoding all /// chunks costs ~20s, so the typed property browser must not re-decode on every /// search/edit. Holds a single entry (the active save), bounded to one payload @@ -5329,6 +5631,9 @@ fn invalidate_decoded_payload_cache(path: &Path) { // The parsed tree is derived from the decoded bytes, so any write that // invalidates one must invalidate the other. invalidate_parsed_root_cache(path); + // Cached responses for this save are keyed by its content and would miss on + // their own; drop them here so the memory is released at the write. + invalidate_response_cache(path); } /// Search every typed property in the decoded private payload. Powers the @@ -6007,16 +6312,10 @@ fn skills_list_command( .and_then(Value::as_str) .filter(|s| !s.is_empty()) .unwrap_or(skills::HERO); - let data = fs::read(path)?; - if !data.starts_with(b"GSAV") { - return Err(CoreError::UnsupportedEdit( - "skill queries are only available for GSAV files".to_string(), - )); - } - let parts = split_gsav(&data)?; - let stream = parse_compressed_stream(&data, 13 + parts.public_payload.len())?; - let decoded = decoded_private_payload_cached(path, &data, &stream, backend)?; - let root = properties::parse_private_root(&decoded)?; + // Share the parsed tree with every other read command. This used to copy the + // whole decoded payload out of the byte cache and re-parse it, so opening the + // skills panel cost a full parse (~0.5 s) that the cache already held. + let root = decode_private_root_cached(path, backend)?; Ok(skills::list_skills(&root, actor)) } @@ -6182,7 +6481,7 @@ fn decode_private_root_cached( let data = fs::read(path)?; if !data.starts_with(b"GSAV") { return Err(CoreError::UnsupportedEdit( - "NPC commands are only available for GSAV files".to_string(), + "private reads are only available for GSAV files".to_string(), )); } let save_sha1 = sha1_hex(&data); diff --git a/crates/gore-save/tests/response_cache.rs b/crates/gore-save/tests/response_cache.rs new file mode 100644 index 00000000..1c74270d --- /dev/null +++ b/crates/gore-save/tests/response_cache.rs @@ -0,0 +1,189 @@ +//! The read-response cache must never answer for a file that has changed. +//! Requires a real GSAV save via GORE_SAVE; skips otherwise. +//! GORE_SAVE='C:\Users\Daniel\AppData\Local\G1R\Saved\SaveGames\G1R-011.sav' \ +//! cargo test --release -p gore-save --test response_cache -- --nocapture +use serde_json::{Value, json}; + +fn exec(req: Value) -> Value { + let resp: Value = serde_json::from_str(&gore_save::execute_json(&req.to_string())).unwrap(); + assert_eq!(resp["ok"], json!(true), "request failed: {resp}"); + resp["data"].clone() +} + +fn source_save() -> Option { + match std::env::var("GORE_SAVE") { + Ok(path) => Some(path), + Err(_) => { + eprintln!("GORE_SAVE not set; skipping"); + None + } + } +} + +/// Copy the save so a test can edit it without touching the user's file. +fn temp_copy(name: &str) -> Option<(tempfile::TempDir, String)> { + let source = source_save()?; + let dir = tempfile::tempdir().expect("temp dir"); + let target = dir.path().join(name); + std::fs::copy(&source, &target).expect("copy save"); + Some((dir, target.to_string_lossy().to_string())) +} + +fn inspect(path: &str) -> Value { + exec(json!({ + "command": "inspect_save", + "payload": { "path": path, "includePrivate": true }, + })) +} + +/// The typed path of the hero's first attribute base value, and its current +/// value — a scalar that `private.typed.setValue` can nudge in place. +fn first_hero_attribute(path: &str) -> (Vec, f64) { + let found = exec(json!({ + "command": "search_typed_properties", + "payload": { + "path": path, + "query": "AttributesByGlobalId {Hero}", + "offset": 0, + "limit": 1000, + }, + })); + let hit = found["results"] + .as_array() + .expect("results") + .iter() + .find(|hit| hit["type"] == "FloatProperty" && hit["editable"] == json!(true)) + .expect("no editable float attribute on the hero"); + let value: f64 = hit["value"].as_str().unwrap().parse().unwrap(); + (hit["path"].as_array().unwrap().clone(), value) +} + +/// A repeat of the same request must return exactly what the first one did — +/// the cache is a memo, not an approximation. +#[test] +fn repeated_reads_return_the_same_answer() { + let Some((_dir, path)) = temp_copy("G1R-cache-repeat.sav") else { + return; + }; + + for request in [ + json!({ "command": "inspect_save", "payload": { "path": path, "includePrivate": true } }), + json!({ "command": "private.characters.list", "payload": { "path": path } }), + json!({ "command": "private.skills.list", "payload": { "path": path, "actor": "Hero" } }), + json!({ "command": "private.factions.list", "payload": { "path": path } }), + json!({ + "command": "query_progression", + "payload": { "path": path, "section": "quests", "offset": 0, "limit": 100 }, + }), + json!({ + "command": "search_typed_properties", + "payload": { "path": path, "query": "GameTime", "offset": 0, "limit": 1000 }, + }), + ] { + let first = exec(request.clone()); + let second = exec(request.clone()); + assert_eq!(first, second, "second read differs for {request}"); + } +} + +/// The whole point of keying on content: once the save has been written, the +/// next read must reflect the new bytes rather than the memo of the old ones. +#[test] +fn a_write_is_never_served_a_stale_read() { + let Some((_dir, path)) = temp_copy("G1R-cache-write.sav") else { + return; + }; + + // Read first, so the pre-write answers are in the cache. + let (attribute_path, before) = first_hero_attribute(&path); + let _ = inspect(&path); + + // A real edit through the same entry point the editor uses, written back + // over the same file. + exec(json!({ + "command": "write_save", + "payload": { + "path": path, + "outputPath": path, + "backup": false, + "edits": [{ + "path": "private.typed.setValue", + "value": { "path": attribute_path, "value": before + 1.0 }, + }], + }, + })); + + let (_, after) = first_hero_attribute(&path); + assert_eq!( + after, + before + 1.0, + "the typed search served its pre-write value", + ); +} + +/// A save replaced behind the editor's back (a cloud sync, the game saving over +/// the slot) runs no write command, so only the content fingerprint can catch +/// it. +#[test] +fn an_external_replacement_is_not_served_from_cache() { + let Some((_dir, path)) = temp_copy("G1R-cache-external.sav") else { + return; + }; + let Some((_other_dir, other)) = temp_copy("G1R-cache-external-source.sav") else { + return; + }; + + let (attribute_path, before) = first_hero_attribute(&path); + + // Edit the OTHER copy, then move its bytes over the read one without going + // through any command that touches `path`. + exec(json!({ + "command": "write_save", + "payload": { + "path": other, + "outputPath": other, + "backup": false, + "edits": [{ + "path": "private.typed.setValue", + "value": { "path": attribute_path, "value": before + 2.0 }, + }], + }, + })); + std::fs::copy(&other, &path).expect("replace the save behind the core's back"); + + let (_, after) = first_hero_attribute(&path); + assert_eq!( + after, + before + 2.0, + "the typed search served the replaced file's cached response", + ); +} + +/// `list_backups` describes a directory, not the save, so it must not be cached: +/// removing a backup changes the answer while the save file is untouched. +#[test] +fn directory_listings_are_not_cached() { + let Some((dir, path)) = temp_copy("G1R-cache-backups.sav") else { + return; + }; + + let backup_dir = dir.path().join("goresave_backups"); + std::fs::create_dir_all(&backup_dir).expect("backup dir"); + std::fs::copy(&path, backup_dir.join("G1R-cache-backups.sav.bak.100")).expect("backup"); + + let listed = exec(json!({ "command": "list_backups", "payload": { "path": path } })); + let backups = listed["backups"].as_array().cloned().unwrap_or_default(); + assert!(!backups.is_empty(), "no backup was created to test with"); + + // Remove the backups outright; the save file itself is unchanged, so a + // save-keyed cache would happily serve the old listing. + std::fs::remove_dir_all(&backup_dir).expect("drop backups"); + + let relisted = exec(json!({ "command": "list_backups", "payload": { "path": path } })); + assert!( + relisted["backups"] + .as_array() + .is_none_or(|backups| backups.is_empty()), + "list_backups served a cached listing after the backups were removed", + ); +} From 7e3d2e9cf3c0f718e16a745a69855d4629d9c353 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 01:14:45 +0200 Subject: [PATCH 03/16] perf(save-editor): fetch every tab's data while the user reads the overview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each tab loaded its data on first paint, so the first click on Characters, World or All data sat on a spinner for as long as the query took. The core now answers a repeated read from a content-keyed cache, which makes it worth asking ahead of time. Once an inspection lands, the editor page starts a background warm-up that issues exactly the queries the panels will issue, ordered by how soon the user can reach them: the overview first, then Characters, World, and the property browser. It never touches the loading overlay or reports an error, and it stops the moment a newer load or a write supersedes it, so it can never make the user's own request wait behind the rest of the warm-up. The panels' page sizes move to `EditorPageSize` because the cache holds one response per exact request: a panel that quietly picked its own size would be warmed with an answer it never asks for. Two tests in player_events_hero_wiring_test forbade *every* progression query while the player had no hero id. Their subject — and their own comment — is the events query, which is still forbidden and still guarded; the other sections now legitimately load in the background. Co-Authored-By: Claude Opus 5 --- apps/save-editor/CHANGELOG.md | 5 + .../editor/domain/editor_notifier.dart | 119 ++++++++ .../lib/features/editor/ui/editor_page.dart | 11 +- .../features/editor/ui/progression_panel.dart | 8 +- .../test/player_events_hero_wiring_test.dart | 17 +- .../test/prefetch_tab_data_test.dart | 257 ++++++++++++++++++ 6 files changed, 410 insertions(+), 7 deletions(-) create mode 100644 apps/save-editor/test/prefetch_tab_data_test.dart diff --git a/apps/save-editor/CHANGELOG.md b/apps/save-editor/CHANGELOG.md index db547070..10974102 100644 --- a/apps/save-editor/CHANGELOG.md +++ b/apps/save-editor/CHANGELOG.md @@ -19,6 +19,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - Saving is much faster: a save with eight changed values took eleven seconds and now takes one. +- Opening a savegame is about four times faster. +- Tabs no longer load one by one. Everything they show is fetched in the + background as soon as the savegame opens, so switching tabs is immediate. +- Going back to a savegame, or to a tab already visited, no longer reloads + anything. ### Fixed diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index f9be4a1e..1023f2e8 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -25,6 +25,21 @@ import 'package:state_notifier/state_notifier.dart'; const _unchanged = Object(); +/// The page sizes the editor's panels ask the core for. +/// +/// These live here rather than in each panel because the core caches one +/// response per exact request, and [EditorNotifier.prefetchTabData] warms those +/// caches by issuing the panels' own queries ahead of time. A panel that quietly +/// chose its own size would be warmed with an answer it never asks for. +abstract final class EditorPageSize { + /// One screen of rows: knowledge entries, memory events, the property browser. + static const detail = 50; + + /// Fetched whole and then filtered/paged in the client: quests, tutorials, + /// story state. + static const fullList = 1000; +} + AppLocalizations _defaultEnglishLocalizations() => AppLocalizationsEn(); /// Sorts saves by in-game playtime (highest first). Slots with null playtime @@ -607,6 +622,110 @@ class EditorNotifier extends StateNotifier { /// starts. Future _coreQueue = Future.value(); + /// The inspection the background prefetch last ran for, so re-entering the + /// editor for an unchanged save does not queue the same warm-up twice. + SaveInspection? _prefetchedFor; + + /// The in-flight prefetch, exposed so a test can await the warm-up instead of + /// racing it. Production fires and forgets. + @visibleForTesting + Future? prefetchInFlight; + + /// Warm the core's caches for every tab of the freshly inspected save. + /// + /// The core answers a repeated read from a cache keyed by the save's content, + /// so running the panels' own queries here turns the first visit to a tab from + /// a fresh multi-hundred-millisecond traversal into a cache hit. Nothing here + /// touches [EditorState.isLoading] or reports an error: the user is looking at + /// the Overview tab while it runs, and a warm-up that fails simply leaves the + /// panel to load the normal way. + /// + /// The queries must match what the panels ask for, argument for argument — + /// the cache holds one response per exact request, so a warm-up with a + /// different page size would prime an answer nobody asks for. That is why the + /// page sizes live in [EditorPageSize] rather than in each panel. + void prefetchTabData() { + // The page listens for state changes to trigger this, and a change can still + // be delivered while the provider is being torn down (a hot restart, the + // window closing). Reading `state` then throws. + if (!mounted) return; + final inspection = state.inspection; + final path = state.selectedPath; + if (inspection == null || path == null) return; + if (identical(_prefetchedFor, inspection)) return; + _prefetchedFor = inspection; + prefetchInFlight = _prefetchTabData(path, inspection.path, _loadSeq); + } + + /// [inspectionPath] is the path as the INSPECTION spells it, which is what the + /// story panel pins its pages to; passing the selection's spelling instead + /// would warm a request the panel never makes. + Future _prefetchTabData( + String path, + String? inspectionPath, + int seq, + ) async { + // A newer load (or a write) has taken over: its own prefetch will run, and + // continuing here would only make the user's request wait behind ours. A + // disposed notifier stops it too — the editor is gone, and touching `state` + // after teardown throws. + bool superseded() => + !mounted || + seq != _loadSeq || + state.selectedPath != path || + state.isLoading; + + Future step(Future Function() load) async { + if (superseded()) return; + try { + await load(); + } catch (_) { + // A warm-up failure is not the user's problem; the panel will retry. + } + } + + // Ordered by how soon the user can reach the data: the Overview tab is + // already on screen, Characters is one click away, then World, then the + // property browser. + await step(loadGameTime); + // Also settles the hero GlobalId that the player's Events sub-tab needs. + await step(loadAllCharacters); + await step(loadHeroAttributes); + await step(loadSkills); + await step(loadAllNpcActors); + await step( + () => loadKnowledgeEntries( + const Actor.player().uniqueName, + limit: EditorPageSize.detail, + ), + ); + // The player's Events pane keys on the hero id the character index above + // settles. Without one there is nothing to warm — and nothing the pane will + // ask for either. + final heroId = superseded() ? null : state.heroGlobalId; + if (heroId != null) { + await step(() => loadMemoryEvents(heroId, limit: EditorPageSize.detail)); + } + await step(() => loadProgressionQuests(limit: EditorPageSize.fullList)); + await step(loadGlossary); + await step(loadProgressionTutorials); + await step( + () => loadStoryState( + includeUnset: true, + limit: EditorPageSize.fullList, + path: inspectionPath, + ), + ); + await step(loadFactions); + await step( + () => searchTypedProperties( + '', + limit: EditorPageSize.detail, + includeNodes: true, + ), + ); + } + bool get coreAvailable => _core.isAvailable; String get coreDescription => _core.description; diff --git a/apps/save-editor/lib/features/editor/ui/editor_page.dart b/apps/save-editor/lib/features/editor/ui/editor_page.dart index 57cfe8b3..00da3406 100644 --- a/apps/save-editor/lib/features/editor/ui/editor_page.dart +++ b/apps/save-editor/lib/features/editor/ui/editor_page.dart @@ -45,6 +45,9 @@ class _EditorPageState extends ConsumerState // manual Settings button stays available regardless. WidgetsBinding.instance.addPostFrameCallback((_) { unawaited(_maybePromptLocalizationExtract()); + // Covers a page that mounts with a save already inspected (a remount, a + // hot reload): the listener in build only sees LATER changes. + if (mounted) ref.read(editorProvider.notifier).prefetchTabData(); }); } @@ -110,6 +113,12 @@ class _EditorPageState extends ConsumerState Widget build(BuildContext context) { final state = ref.watch(editorProvider); final notifier = ref.read(editorProvider.notifier); + // A save has finished loading and its tabs are now reachable: warm the + // core's caches for them in the background so the first click on a tab + // shows data instead of a spinner. Listened to rather than called inline, + // because the warm-up writes editor state (the hero id the character index + // settles) and that must not happen during a build. + ref.listen(editorProvider, (previous, next) => notifier.prefetchTabData()); final uiScale = ref.watch(uiScaleProvider); final zoomPct = (uiScale * 100).round(); final scheme = Theme.of(context).colorScheme; @@ -2008,7 +2017,7 @@ class _AllDataPanelState extends State<_AllDataPanel> { TypedSearchResult? _result; bool _searching = false; int _requestSeq = 0; - int _pageSize = 50; + int _pageSize = EditorPageSize.detail; String _activeQuery = ''; String _source = 'private'; String _kind = 'all'; diff --git a/apps/save-editor/lib/features/editor/ui/progression_panel.dart b/apps/save-editor/lib/features/editor/ui/progression_panel.dart index 0514bc19..823b0964 100644 --- a/apps/save-editor/lib/features/editor/ui/progression_panel.dart +++ b/apps/save-editor/lib/features/editor/ui/progression_panel.dart @@ -237,7 +237,7 @@ class QuestsDetail extends ConsumerStatefulWidget { } class _QuestsDetailState extends ConsumerState { - static const _defaultPageSize = 50; + static const _defaultPageSize = EditorPageSize.detail; final TextEditingController _search = TextEditingController(); // Full quest list (fetched once with a large limit, no server filters): @@ -256,7 +256,7 @@ class _QuestsDetailState extends ConsumerState { QuestJournalSection? _sectionFilter; // The core clamps a query's `limit` to 1000, so the full quest list must be // pulled page-by-page rather than in one oversized request. - static const _fetchPageLimit = 1000; + static const _fetchPageLimit = EditorPageSize.fullList; @override void initState() { @@ -762,7 +762,7 @@ class KnowledgeDetail extends ConsumerStatefulWidget { } class _KnowledgeDetailState extends ConsumerState { - static const _defaultPageSize = 50; + static const _defaultPageSize = EditorPageSize.detail; String? _selectedCharacter; KnowledgeEntriesPage _entries = const KnowledgeEntriesPage(); @@ -1491,7 +1491,7 @@ class EventsDetail extends ConsumerStatefulWidget { } class _EventsDetailState extends ConsumerState { - static const _defaultPageSize = 50; + static const _defaultPageSize = EditorPageSize.detail; String? _selectedCharacter; MemoryEventsPage _events = const MemoryEventsPage(); diff --git a/apps/save-editor/test/player_events_hero_wiring_test.dart b/apps/save-editor/test/player_events_hero_wiring_test.dart index e47ed4ab..d943450e 100644 --- a/apps/save-editor/test/player_events_hero_wiring_test.dart +++ b/apps/save-editor/test/player_events_hero_wiring_test.dart @@ -159,8 +159,15 @@ void main() { ), findsWidgets, ); + // No EVENTS query specifically: other progression sections legitimately + // load in the background (the tab prefetch), but events cannot be asked + // for without an id. expect( - core.requests.where((r) => r.command == 'query_progression'), + core.requests.where( + (r) => + r.command == 'query_progression' && + r.payload['section'] == 'events', + ), isEmpty, ); @@ -227,8 +234,14 @@ void main() { ), findsNothing, ); + // As above: only the events section is forbidden, and only because there + // is no id to ask with. expect( - core.requests.where((r) => r.command == 'query_progression'), + core.requests.where( + (r) => + r.command == 'query_progression' && + r.payload['section'] == 'events', + ), isEmpty, ); }, diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart new file mode 100644 index 00000000..4b03ad80 --- /dev/null +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -0,0 +1,257 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:goresave/features/editor/domain/core_service.dart'; +import 'package:goresave/features/editor/domain/editor_notifier.dart'; + +/// Records every core request and answers the shell commands the editor needs +/// to reach a loaded save. Everything else returns a benign empty payload, so a +/// prefetch step that fails is indistinguishable from one that succeeds — which +/// is exactly what the "prefetch never surfaces an error" tests need. +class _RecordingCore implements GoresaveCoreService { + final requests = <({String command, Map payload})>[]; + + /// Completes for each in-flight command, so a test can hold the core mid-step + /// and observe what the prefetch does while a request is outstanding. + final Duration delay; + + _RecordingCore({this.delay = Duration.zero}); + + List get commands => [for (final r in requests) r.command]; + + Map? payloadFor(String command) => requests + .where((request) => request.command == command) + .map((request) => request.payload) + .firstOrNull; + + @override + String get description => 'prefetch-recording-core'; + + @override + bool get isAvailable => true; + + @override + Future> execute( + String command, { + Map payload = const {}, + }) async { + requests.add((command: command, payload: Map.from(payload))); + if (delay > Duration.zero) await Future.delayed(delay); + switch (command) { + case 'scan_save_dir': + return { + 'ok': true, + 'data': { + 'saveRoot': r'C:\tmp\saves', + 'saves': [ + { + 'path': r'C:\tmp\saves\G1R-001.sav', + 'slot': 'G1R-001', + 'format': 'GSAV', + 'fileSize': 1, + 'sha1': 'abc', + 'status': 'ok', + }, + ], + 'profiles': [], + }, + }; + case 'inspect_save': + return { + 'ok': true, + 'data': { + 'format': 'GSAV', + 'path': payload['path'], + 'slot': 'G1R-001', + 'size': 1, + 'sha1': 'abc', + 'private': { + 'status': 'decoded', + 'preview': false, + 'decompressedSize': 9, + 'typedParse': {'status': 'ok', 'propertyCount': 1, 'maxDepth': 1}, + 'player': {'playerName': 'Hero', 'attributes': []}, + }, + }, + }; + case 'list_backups': + return { + 'ok': true, + 'data': { + 'path': payload['path'], + 'backups': [], + 'companionBackups': [], + }, + }; + case 'private.characters.list': + return { + 'ok': true, + 'data': { + 'characters': [ + {'uniqueName': 'Hero', 'globalId': 'hero-global-id'}, + ], + 'total': 1, + }, + }; + default: + return {'ok': true, 'data': {}}; + } + } +} + +Future _loadedEditor(_RecordingCore core) async { + final notifier = EditorNotifier(core, saveDir: r'C:\tmp\saves'); + await pumpEventQueue(); + await notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); + await pumpEventQueue(); + core.requests.clear(); + return notifier; +} + +void main() { + test('prefetch warms every tab the loaded save can show', () async { + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + // One entry per panel that loads from the core on first paint. + expect( + core.commands.toSet(), + containsAll([ + 'search_typed_properties', // Overview clock + hero attributes + browser + 'private.characters.list', // Characters master list + 'private.skills.list', + 'private.npc.list', + 'query_progression', // quests, glossary, tutorials, story, knowledge + 'private.factions.list', + ]), + ); + // Every progression section a panel opens on. + final sections = [ + for (final request in core.requests) + if (request.command == 'query_progression') request.payload['section'], + ]; + expect( + sections.toSet(), + containsAll([ + 'knowledge', + 'events', + 'quests', + 'glossary', + 'tutorials', + 'story', + ]), + ); + }); + + test('prefetch asks for the page sizes the panels ask for', () async { + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + Map progression(String section) => core.requests + .firstWhere( + (request) => + request.command == 'query_progression' && + request.payload['section'] == section, + ) + .payload; + + // The core caches one response per exact request, so a prefetch at the + // wrong page size warms an answer no panel ever asks for. + expect(progression('knowledge')['limit'], EditorPageSize.detail); + expect(progression('events')['limit'], EditorPageSize.detail); + expect(progression('quests')['limit'], EditorPageSize.fullList); + expect(progression('story')['limit'], EditorPageSize.fullList); + expect(progression('story')['includeUnset'], isTrue); + + // The property browser's opening request: first page, node model, private + // source, no facet filters. + final browse = core.requests.firstWhere( + (request) => + request.command == 'search_typed_properties' && + request.payload['includeNodes'] == true, + ); + expect(browse.payload['query'], ''); + expect(browse.payload['offset'], 0); + expect(browse.payload['limit'], EditorPageSize.detail); + expect(browse.payload['source'], 'private'); + expect(browse.payload.containsKey('kind'), isFalse); + expect(browse.payload.containsKey('type'), isFalse); + expect(browse.payload.containsKey('editable'), isFalse); + }); + + test('prefetch runs once per inspection', () async { + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + final first = core.commands.length; + expect(first, greaterThan(0)); + + // The editor page calls this on every rebuild. + notifier.prefetchTabData(); + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + expect(core.commands.length, first, reason: 'prefetch repeated itself'); + }); + + test('a fresh inspection prefetches again', () async { + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + final first = core.commands.length; + + await notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); + await pumpEventQueue(); + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + expect(core.commands.length, greaterThan(first)); + }); + + test('prefetch never turns on the loading overlay', () async { + final core = _RecordingCore(delay: const Duration(milliseconds: 1)); + final notifier = await _loadedEditor(core); + + var sawLoading = false; + final removeListener = notifier.addListener((state) { + if (state.isLoading) sawLoading = true; + }, fireImmediately: false); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + removeListener(); + + expect(sawLoading, isFalse); + expect(notifier.state.error, isNull); + }); + + test('a newer load stops the prefetch instead of racing it', () async { + final core = _RecordingCore(delay: const Duration(milliseconds: 5)); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + // Supersede immediately: a second inspection means the panels will be + // rebuilt against a different inspection anyway, and every further prefetch + // request would only make the user's own load wait behind it. + final reinspect = notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); + await notifier.prefetchInFlight; + final duringPrefetch = core.commands + .where((command) => command != 'inspect_save' && command != 'list_backups') + .length; + await reinspect; + + expect( + duringPrefetch, + lessThan(6), + reason: 'prefetch kept queueing work behind a newer load', + ); + }); +} From b237794fb70d406f2847ad0c1e40718b46302371 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 14:55:06 +0200 Subject: [PATCH 04/16] fix(save-editor): warm the tabs of a save that is still loading its backups Two reports from the PR's automated reviewers, both real. Cursor: the warm-up never ran. `_inspect` publishes the inspection while it is still fetching the backup list, so the page's first state-change call arrived with the editor still loading. It claimed the inspection, every warm-up step bailed out on that flag, and the call that arrived once loading ended found the inspection already claimed and skipped it. Opening a save warmed nothing. Wait for the flag instead of spending the trigger on it: clearing it is itself a state change, so the page comes back. Codex: `private.npc.position` reports the recorded placement undo, which lives in a sidecar beside the save rather than inside it, and the response cache fingerprinted only the save. Restoring a backup puts back byte-identical save bytes alongside that backup's placement notes, which would then be served from the pre-restore cache entry. The two commands with such a dependency now declare it in one place, `response_companion_files`. Both are covered by tests that fail without the fix: opening a save driven only by state changes (as the page drives it) must warm the tabs, and a placement note recorded or cleared without touching the save must change what `private.npc.position` answers. One existing widget test forbade every events query while an orphan was selected. Its subject is the orphan, which has no GlobalId to ask with; the player's own events now legitimately load in the background, so it counts the queries the orphan selection itself caused. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 7 ++ .../test/player_events_hero_wiring_test.dart | 24 +++++-- .../test/prefetch_tab_data_test.dart | 32 +++++++++ crates/gore-save/src/lib.rs | 36 +++++++--- crates/gore-save/tests/response_cache.rs | 72 +++++++++++++++++++ 5 files changed, 155 insertions(+), 16 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index 1023f2e8..a7aec797 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -652,6 +652,13 @@ class EditorNotifier extends StateNotifier { final inspection = state.inspection; final path = state.selectedPath; if (inspection == null || path == null) return; + // The inspection lands BEFORE its load finishes — `_inspect` still has the + // backup list to fetch — and every warm-up step bails out while a load is in + // flight. Claiming the inspection here would therefore burn it on a warm-up + // that does nothing, and the identity check below would refuse to try again. + // Wait instead: clearing the loading flag is itself a state change, so the + // page calls this again, and that call starts the warm-up for real. + if (state.isLoading) return; if (identical(_prefetchedFor, inspection)) return; _prefetchedFor = inspection; prefetchInFlight = _prefetchTabData(path, inspection.path, _loadSeq); diff --git a/apps/save-editor/test/player_events_hero_wiring_test.dart b/apps/save-editor/test/player_events_hero_wiring_test.dart index d943450e..ad7a6b13 100644 --- a/apps/save-editor/test/player_events_hero_wiring_test.dart +++ b/apps/save-editor/test/player_events_hero_wiring_test.dart @@ -257,6 +257,18 @@ void main() { await tester.tap(find.widgetWithText(Tab, 'Characters')); await tester.pumpAndSettle(); + int eventsQueries() => core.requests + .where( + (r) => + r.command == 'query_progression' && + r.payload['section'] == 'events', + ) + .length; + // The player's own events legitimately load in the background (the tab + // prefetch). Count from here, so what follows measures only what + // selecting the orphan caused. + final beforeOrphan = eventsQueries(); + // Select the knowledge-only orphan from the trailing "Other" group. await tester.tap(find.text('Ghostvoice')); await tester.pumpAndSettle(); @@ -275,14 +287,12 @@ void main() { findsWidgets, ); expect(find.text('Select a character to see events'), findsNothing); - // And no events query was ever issued for the orphan. + // And no events query was issued for the orphan: it has no GlobalId, so + // there is nothing to ask with. expect( - core.requests.where( - (r) => - r.command == 'query_progression' && - r.payload['section'] == 'events', - ), - isEmpty, + eventsQueries(), + beforeOrphan, + reason: 'selecting the orphan issued an events query', ); }, ); diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index 4b03ad80..a1122023 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -183,6 +183,38 @@ void main() { expect(browse.payload.containsKey('editable'), isFalse); }); + test('opening a save warms its tabs, driven only by state changes', () async { + // The page does not call this at a chosen moment — it calls it on every + // state change. The FIRST change it sees is the inspection landing, which + // happens while `_inspect` is still fetching the backup list, so the editor + // is still loading and no warm-up can run yet. That moment must not consume + // the one trigger this inspection gets, or the call that arrives once + // loading ends finds the inspection already claimed, skips it, and no tab is + // ever warmed. + final core = _RecordingCore(); + final notifier = EditorNotifier(core, saveDir: r'C:\tmp\saves'); + await pumpEventQueue(); + core.requests.clear(); + + // Exactly what the editor page subscribes with. + final removeListener = notifier.addListener( + (_) => notifier.prefetchTabData(), + fireImmediately: false, + ); + await notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); + await pumpEventQueue(); + await notifier.prefetchInFlight; + removeListener(); + + expect( + core.commands, + contains('private.characters.list'), + reason: 'opening the save warmed nothing', + ); + expect(core.commands, contains('private.skills.list')); + expect(core.commands, contains('query_progression')); + }); + test('prefetch runs once per inspection', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index e92dcbcf..7cca85cd 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -5499,6 +5499,31 @@ const RESPONSE_CACHE_MAX_BYTES: usize = 16 * 1024 * 1024; static RESPONSE_CACHE: Mutex> = Mutex::new(Vec::new()); +/// The files besides the save itself that a command's answer is read from. +/// +/// A response is only safe to memoize under a fingerprint that covers everything +/// it was built from. These two commands each reach for one sidecar file that +/// can change while the save bytes stay exactly as they were — a profile +/// reassignment, or a backup restore that puts back byte-identical save bytes +/// alongside a different placement note. Fingerprinting the sidecar makes that a +/// miss instead of a stale hit. +/// +/// A missing sidecar contributes nothing, which is correct: "absent" and +/// "absent" fingerprint alike, and a sidecar that appears changes the answer and +/// the fingerprint together. +fn response_companion_files(command: &str, save_path: &Path) -> Vec { + match command { + // Reports which profile owns the slot, from the folder's profile file. + "inspect_save" => save_path + .parent() + .map(|dir| vec![dir.join("PersistentDataList.sav")]) + .unwrap_or_default(), + // Reports the recorded placement undo, from the placement notes. + "private.npc.position" => vec![placement::notes_path(save_path)], + _ => Vec::new(), + } +} + /// Build the cache identity for a request, or `None` when the command is not /// cacheable, carries no save path, or its file cannot be read. fn read_response_cache_key(input: &str) -> Option { @@ -5509,15 +5534,8 @@ fn read_response_cache_key(input: &str) -> Option { } let path = Path::new(value.get("payload")?.get("path")?.as_str()?); let mut fingerprint = sha1_hex(&fs::read(path).ok()?); - // `inspect_save` also reports which profile owns the slot, and that lives in - // a sibling file rather than in the save. Fold it into the fingerprint so - // assigning a save to another profile is a miss, not a stale hit. - if command == "inspect_save" { - if let Some(bytes) = path - .parent() - .map(|dir| dir.join("PersistentDataList.sav")) - .and_then(|companion| fs::read(companion).ok()) - { + for companion in response_companion_files(command, path) { + if let Ok(bytes) = fs::read(companion) { fingerprint.push_str(&sha1_hex(&bytes)); } } diff --git a/crates/gore-save/tests/response_cache.rs b/crates/gore-save/tests/response_cache.rs index 1c74270d..3d6ab754 100644 --- a/crates/gore-save/tests/response_cache.rs +++ b/crates/gore-save/tests/response_cache.rs @@ -159,6 +159,78 @@ fn an_external_replacement_is_not_served_from_cache() { ); } +/// `private.npc.position` reports the recorded placement undo, which lives in a +/// sidecar next to the save rather than inside it. The sidecar can change while +/// the save bytes stay exactly as they were — restoring a backup puts back +/// byte-identical bytes alongside that backup's placement notes — so the cache +/// key has to cover it. +#[test] +fn a_changed_placement_note_is_not_served_from_cache() { + let Some((_dir, path)) = temp_copy("G1R-cache-placement.sav") else { + return; + }; + let save = std::path::Path::new(&path); + + // Any NPC the save actually knows about. + let listed = exec(json!({ + "command": "private.npc.list", + "payload": { "path": path, "offset": 0, "limit": 1 }, + })); + let Some(npc) = listed["npcs"] + .as_array() + .and_then(|npcs| npcs.first()) + .and_then(|npc| npc["id"].as_str()) + .map(str::to_owned) + else { + eprintln!("save lists no NPCs; skipping"); + return; + }; + + let position = json!({ + "command": "private.npc.position", + "payload": { "path": path, "id": npc }, + }); + assert!( + exec(position.clone())["undo"].is_null(), + "the fixture already carries a placement note for {npc}", + ); + + // Record a note. Only the sidecar changes; the save file is untouched. + let before = std::fs::read(save).expect("read save"); + gore_save::placement::record( + save, + &[( + npc.clone(), + gore_save::placement::PlacementNote { + original_location: [1.0, 2.0, 3.0], + original_routine_class: None, + original_rotation: None, + written_location: [4.0, 5.0, 6.0], + written_rotation: None, + written_routine_class: None, + }, + )], + ) + .expect("record placement note"); + assert_eq!( + std::fs::read(save).expect("re-read save"), + before, + "recording a note must not touch the save", + ); + + assert!( + !exec(position.clone())["undo"].is_null(), + "private.npc.position served its pre-note answer from cache", + ); + + // And back the other way: dropping the note must surface again. + gore_save::placement::clear(save, std::slice::from_ref(&npc)).expect("clear placement note"); + assert!( + exec(position)["undo"].is_null(), + "private.npc.position served the removed note from cache", + ); +} + /// `list_backups` describes a directory, not the save, so it must not be cached: /// removing a backup changes the answer while the save file is untouched. #[test] From 758a7f3bfa5a660598a83ea0a208c801a77c65ed Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 15:07:46 +0200 Subject: [PATCH 05/16] fix(save): never bind a cached response to a fingerprint it does not describe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR: the response cache took its content fingerprint, then let the command read the same files again for itself. If a file changed between those two reads — the game saving over the slot, a cloud sync — the answer computed from the new bytes was stored under the old bytes' fingerprint. That entry is not stale for a moment; it answers every later read of the ORIGINAL bytes, so restoring them serves the wrong data for as long as the entry lives. Re-derive the fingerprint after the command and keep the response only when it still matches, so a stored entry always describes the content its key names. Costs one extra read and hash on a cache MISS, next to the hundreds of milliseconds the miss itself costs; a hit is unaffected. A cache hit needs no such re-check and does not get one: matching a fingerprint means the files hold byte-identical content to what the entry was built from, whatever happened in between. Not covered by a test. The window is between two reads that sit microseconds apart, so a test cannot land inside it reliably — a timing-based attempt passed just as well without the fix, which makes it worse than no test. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/lib.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 7cca85cd..27088beb 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -381,8 +381,23 @@ pub fn execute_json(input: &str) -> String { let response = json!({ "ok": true, "data": data }).to_string(); // Only successes are cached: a failure is usually transient (a file // being written, a codec hiccup) and must stay retryable. + // + // The command re-read the files itself, so it may have worked from + // bytes that arrived AFTER the fingerprint was taken — the game + // saving over the slot, a cloud sync, a restore. Storing that answer + // under the earlier fingerprint would not merely be stale for a + // moment: it would bind an answer to a content hash it does not + // describe, and every later read of those earlier bytes would be + // served the wrong answer for as long as the entry lives. So keep a + // response only when its inputs held still for the whole command. + // + // A cache HIT needs no such re-check: matching the fingerprint means + // the files hold byte-identical content to what the entry was built + // from, whatever has happened in between. if let Some(key) = cache_key { - store_cached_response(key, &response); + if read_response_cache_key(input).is_some_and(|current| current == key) { + store_cached_response(key, &response); + } } response } From 66d1fcd31215b951834ad3abd5868e37bb0133d5 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 15:16:35 +0200 Subject: [PATCH 06/16] fix(save-editor): warm the NPC roster without pre-filling its memo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR: `loadAllNpcActors` memoizes its roster for the lifetime of one inspection, and the warm-up was calling it, so the memo was filled at load time rather than when a panel first needed it. A save replaced in between — the game, a cloud sync — would then leave the first NPC panel showing a roster fetched from bytes no longer on disk, where before the fix it would have fetched against the current file. The memo itself predates this branch and is deliberate: the whole editor is pinned to one inspection, and every other panel reloads only when a new one lands. What did not belong there was the warm-up reaching into it. Page the roster into the CORE's cache instead and leave the memo to the panel that actually uses it: the paging it then repeats is answered from the warm cache, so the visit stays fast while the roster is derived from the file as of that moment. The paging loop moves to `_fetchAllNpcActors`, which both callers share. Only the memoizing one clears the memo slot on a failed page; the warm-up has no slot to clear, and clearing it behind a real load in flight would be wrong. Covered by a test that fails when the warm-up fills the memo. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 76 +++++++++++-------- .../test/prefetch_tab_data_test.dart | 34 +++++++++ 2 files changed, 80 insertions(+), 30 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index a7aec797..70a05f11 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -699,7 +699,12 @@ class EditorNotifier extends StateNotifier { await step(loadAllCharacters); await step(loadHeroAttributes); await step(loadSkills); - await step(loadAllNpcActors); + // Warms the CORE's cache without filling the Dart-side NPC memo. That memo + // is pinned to one inspection by design, so pre-filling it here would hand + // the first NPC panel a roster fetched seconds earlier; letting the panel + // fill it on first use keeps it derived from the file as of that moment, + // and the paging it repeats is answered from the warm core. + await step(() => _fetchAllNpcActors(path, dropMemoOnError: false)); await step( () => loadKnowledgeEntries( const Actor.player().uniqueName, @@ -3282,40 +3287,51 @@ class EditorNotifier extends StateNotifier { if (cached != null && identical(_allNpcActorsFor, inspection)) { return cached; } - final future = () async { - // The core clamps `private.npc.list` `limit` to 1000, but real saves have - // ~1484+ NPCs — a single request would silently drop everyone past the - // first page. PAGE through with an increasing offset, accumulating until - // we have `total`, then return one combined page. The decode is cached - // per-inspection in the core, so follow-up pages are cheap. - final npcs = []; - var offset = 0; - var total = 0; - while (true) { - final page = await loadNpcActors( - offset: offset, - limit: 1000, - path: pinnedPath, - ); - // Don't cache an error result — let the next call retry. - if (page.error != null) { - _invalidateNpcCache(); - return page; - } - npcs.addAll(page.npcs); - total = page.total; - offset += page.npcs.length; - // Stop once we've collected every NPC, or the core returns an empty - // page (defensive: never loop forever on a stuck/empty response). - if (page.npcs.isEmpty || offset >= total) break; - } - return NpcActorsPage(npcs: npcs, total: total, offset: 0, limit: total); - }(); + final future = _fetchAllNpcActors(pinnedPath, dropMemoOnError: true); _allNpcActorsFuture = future; _allNpcActorsFor = inspection; return future; } + /// Page the full NPC roster out of the core, without touching the memo. + /// + /// The core clamps `private.npc.list` `limit` to 1000, but real saves hold + /// ~1484+ NPCs — a single request would silently drop everyone past the first + /// page. Pages are accumulated until `total` is reached and returned as one. + /// [pinnedPath] fixes the file for the WHOLE fetch, so a save switch midway + /// cannot merge pages from two different files into one list. + /// + /// [dropMemoOnError] belongs to the memoizing caller: a failed load must not + /// stay cached, so it clears the memo slot the future was stored in. The + /// background warm-up passes false — it has no slot to clear, and clearing the + /// memo behind a real load in flight would be wrong. + Future _fetchAllNpcActors( + String? pinnedPath, { + required bool dropMemoOnError, + }) async { + final npcs = []; + var offset = 0; + var total = 0; + while (true) { + final page = await loadNpcActors( + offset: offset, + limit: 1000, + path: pinnedPath, + ); + if (page.error != null) { + if (dropMemoOnError) _invalidateNpcCache(); + return page; + } + npcs.addAll(page.npcs); + total = page.total; + offset += page.npcs.length; + // Stop once we've collected every NPC, or the core returns an empty page + // (defensive: never loop forever on a stuck/empty response). + if (page.npcs.isEmpty || offset >= total) break; + } + return NpcActorsPage(npcs: npcs, total: total, offset: 0, limit: total); + } + /// Load every attribute of a single NPC (by GlobalId) from the core /// `private.npc.attributes` command for the currently selected save. Real /// NPCs return ~46 rows. Each row carries the FULL typed Base/Current paths diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index a1122023..de5029b8 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -17,6 +17,9 @@ class _RecordingCore implements GoresaveCoreService { List get commands => [for (final r in requests) r.command]; + int commandCount(String command) => + requests.where((request) => request.command == command).length; + Map? payloadFor(String command) => requests .where((request) => request.command == command) .map((request) => request.payload) @@ -215,6 +218,37 @@ void main() { expect(core.commands, contains('query_progression')); }); + test('prefetch warms the core without filling the NPC memo', () async { + // `loadAllNpcActors` memoizes its roster for the lifetime of one inspection. + // The warm-up must not be what fills that memo: it runs seconds before the + // user opens an NPC panel, and a save replaced in between (the game, a cloud + // sync) would leave the panel showing a roster fetched from bytes that are + // no longer on disk. Warm the core instead, and let the panel's own call + // populate the memo from the file as of that moment. + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + final warmed = core.commandCount('private.npc.list'); + expect(warmed, greaterThan(0), reason: 'the NPC roster was not warmed'); + + // The panel's own call still goes to the core — proof the memo was empty — + // and is answered from the warm cache. + await notifier.loadAllNpcActors(); + expect( + core.commandCount('private.npc.list'), + greaterThan(warmed), + reason: 'the warm-up pre-filled the NPC memo', + ); + + // And it is a real memo from then on: a second call adds no request. + final afterPanel = core.commandCount('private.npc.list'); + await notifier.loadAllNpcActors(); + expect(core.commandCount('private.npc.list'), afterPanel); + }); + test('prefetch runs once per inspection', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); From 72c29dae04687db42866f5862e140fcaa532cfcb Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 15:32:29 +0200 Subject: [PATCH 07/16] fix(save): lower-case a search path the way the query was lower-cased MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR, and it disproves the reasoning I had written into the comment above the code. The search builds a lower-cased twin of the display path one segment at a time and matches query terms that went through `str::to_lowercase`. I lower-cased the segments char by char, arguing that segment boundaries never move a character into a different word position. That much is true, but it misses the point: `char::to_lowercase` has no context to apply, so it maps a word-final Σ to σ where `str::to_lowercase` maps it to ς. Searching "ΟΣ" normalized the query to "ος" and the path to "οσ", and the property could not be found by its own name. Lower-case each segment as a string. ASCII — everything a real save has in practice — takes a fast path that lower-cases in place, so the per-property allocation this walk exists to avoid stays avoided; only a non-ASCII segment allocates, and only that segment. Covered by a test over a word-final and a word-medial sigma; it fails on the previous mapping. Typed-search output over a real save is unchanged. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/properties.rs | 46 ++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/crates/gore-save/src/properties.rs b/crates/gore-save/src/properties.rs index 77d020af..cde1a782 100644 --- a/crates/gore-save/src/properties.rs +++ b/crates/gore-save/src/properties.rs @@ -703,11 +703,23 @@ impl<'a> SearchPath<'a> { } self.display.push_str(&segment); // Lower-casing per segment rather than per leaf is what makes the match - // cheap. It agrees with lower-casing the joined path: every segment is - // followed by a separator or the end of the string, so no character sits - // in a different word-final position in one form than in the other. - self.lower - .extend(segment.chars().flat_map(char::to_lowercase)); + // cheap. It must still agree with the query terms, which went through + // `str::to_lowercase` — and that applies context-sensitive mappings a + // char-by-char pass cannot: a word-final Σ becomes ς, never σ. + // + // Segment boundaries do not disturb those mappings here, because a + // segment is only ever followed by ` › `, `[`, `{`, or the end of the + // path — never by another cased letter — so a character that is + // word-final within its segment is word-final in the joined path too. + if segment.is_ascii() { + // The overwhelming majority, and free of context-sensitive + // mappings: lower-case in place instead of allocating per property. + let start = self.lower.len(); + self.lower.push_str(&segment); + self.lower[start..].make_ascii_lowercase(); + } else { + self.lower.push_str(&segment.to_lowercase()); + } self.segments.push(segment); mark } @@ -4320,6 +4332,30 @@ mod tests { } } + /// The search lower-cases the display path as it is built, one segment at a + /// time, and matches query terms that went through `str::to_lowercase`. The + /// two have to agree — and they only do if the segments are lower-cased the + /// same way. A word-final Σ is the case that tells them apart: + /// `str::to_lowercase` maps it to ς, while a char-by-char mapping always + /// yields σ, so an upper-case query would silently miss its own property. + #[test] + fn search_matches_a_word_final_sigma() { + let mut props = int_property("ΟΣ", 7); + props.extend_from_slice(&int_property("ΟΣΤΟΥΝ", 8)); + let payload = root("/Script/Test.Save", &props); + let parsed = parse_private_root(&payload).unwrap(); + + // Final position: the query normalizes to "ος", so the path must too. + let (hits, total) = search_properties(&parsed, "ΟΣ", 0, 100); + assert_eq!(total, 1, "an upper-case query missed its own property"); + assert_eq!(hits[0].display, "ΟΣ"); + + // Non-final position keeps the ordinary σ, and still matches. + let (hits, total) = search_properties(&parsed, "ΟΣΤΟΥΝ", 0, 100); + assert_eq!(total, 1); + assert_eq!(hits[0].display, "ΟΣΤΟΥΝ"); + } + #[test] fn search_marks_strings_editable() { // root class string is not a property; build a payload with a StrProperty From 3a2f02338f3438b3291c5f871d2ea3aba636c705 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 15:40:59 +0200 Subject: [PATCH 08/16] fix(save-editor): resume a warm-up that something else interrupted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR. The warm-up claimed its inspection when it started, and its steps skip while anything else holds the editor. So an operation that arrives mid-warm-up — a backup rename or delete bumps the load sequence, a codec check raises the loading flag — made every remaining step skip, and the identity check then refused to try again. The tabs those steps would have covered went back to loading the slow way for the rest of the session, which is exactly the stall this warm-up exists to remove. Retire an inspection only after a run that warmed every step. Anything less leaves the marker unset, so the next state change starts the sequence over; the steps that did complete come back from the core's cache, so a restart re-walks them in milliseconds. The marker can no longer double as the in-flight guard, and the warm-up itself changes editor state (the character index settles the hero id), so a second one would otherwise start mid-run. `_prefetchRunning` keeps it to one. Covered by a test that interrupts a warm-up and asserts the next trigger picks it up — and that a completed one is still retired. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 57 +++++++++++++++---- .../test/prefetch_tab_data_test.dart | 41 +++++++++++++ 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index 70a05f11..b9a821b7 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -622,10 +622,21 @@ class EditorNotifier extends StateNotifier { /// starts. Future _coreQueue = Future.value(); - /// The inspection the background prefetch last ran for, so re-entering the + /// The inspection the background prefetch warmed IN FULL, so re-entering the /// editor for an unchanged save does not queue the same warm-up twice. + /// + /// Set only once every step has run. A warm-up that was cut short — the user + /// renamed a backup, ran a codec check — leaves this null so the next state + /// change starts it again; the steps that did complete are answered from the + /// core's cache, so a restart re-walks them for a few milliseconds. SaveInspection? _prefetchedFor; + /// Whether a warm-up is running right now. [_prefetchedFor] cannot serve as + /// this flag any more (it is only set at the end), and the warm-up itself + /// changes editor state — the character index settles the hero id — so + /// without this a state change mid-warm-up would start a second one. + bool _prefetchRunning = false; + /// The in-flight prefetch, exposed so a test can await the warm-up instead of /// racing it. Production fires and forgets. @visibleForTesting @@ -659,31 +670,46 @@ class EditorNotifier extends StateNotifier { // Wait instead: clearing the loading flag is itself a state change, so the // page calls this again, and that call starts the warm-up for real. if (state.isLoading) return; + if (_prefetchRunning) return; if (identical(_prefetchedFor, inspection)) return; - _prefetchedFor = inspection; - prefetchInFlight = _prefetchTabData(path, inspection.path, _loadSeq); + _prefetchRunning = true; + prefetchInFlight = _prefetchTabData( + path, + inspection, + _loadSeq, + ).whenComplete(() => _prefetchRunning = false); } - /// [inspectionPath] is the path as the INSPECTION spells it, which is what the - /// story panel pins its pages to; passing the selection's spelling instead - /// would warm a request the panel never makes. + /// Warm every tab's query for [inspection], in reachability order. + /// + /// Steps are skipped, never queued, while something else holds the editor: + /// a warm-up that queued behind the user's own request would be the very + /// stall it exists to remove. A skipped step is not lost — the inspection is + /// then not marked warmed, so the next state change runs the sequence again + /// and the steps that did complete come back from the core's cache. Future _prefetchTabData( String path, - String? inspectionPath, + SaveInspection inspection, int seq, ) async { - // A newer load (or a write) has taken over: its own prefetch will run, and - // continuing here would only make the user's request wait behind ours. A - // disposed notifier stops it too — the editor is gone, and touching `state` - // after teardown throws. + // The story panel pins its pages to the path as the INSPECTION spells it; + // the selection's spelling would warm a request the panel never makes. + final inspectionPath = inspection.path; + // A newer load (or a write) has taken over: continuing would only make the + // user's request wait behind ours. A disposed notifier stops it too — the + // editor is gone, and touching `state` after teardown throws. bool superseded() => !mounted || seq != _loadSeq || state.selectedPath != path || state.isLoading; + var complete = true; Future step(Future Function() load) async { - if (superseded()) return; + if (superseded()) { + complete = false; + return; + } try { await load(); } catch (_) { @@ -736,6 +762,13 @@ class EditorNotifier extends StateNotifier { includeNodes: true, ), ); + + // Only a run that warmed everything retires this inspection. Anything less + // leaves the marker unset so the next state change picks the sequence up + // again — otherwise a warm-up interrupted by, say, a backup rename would + // leave the tabs it never reached loading the slow way for the rest of the + // session. + if (complete && mounted) _prefetchedFor = inspection; } bool get coreAvailable => _core.isAvailable; diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index de5029b8..de635b8c 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -266,6 +266,47 @@ void main() { expect(core.commands.length, first, reason: 'prefetch repeated itself'); }); + test('an interrupted warm-up is picked up again, not written off', () async { + // Something else taking the editor mid-warm-up — a backup rename bumps the + // load sequence, a codec check raises the loading flag — makes the + // remaining steps skip. The inspection must NOT count as warmed then, or + // the tabs those steps would have covered load the slow way for the rest of + // the session. + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + // Stand in for that interruption: the warm-up runs against a load sequence + // that has already moved on, so every step skips. + await notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); + await pumpEventQueue(); + core.requests.clear(); + notifier.prefetchTabData(); + final interrupted = notifier.prefetchInFlight; + await notifier.refreshBackups(); + await interrupted; + + final afterInterruption = core.commands + .where((command) => command != 'list_backups') + .length; + + // The next state change must start the sequence over rather than skip it. + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + + expect( + core.commands.where((command) => command != 'list_backups').length, + greaterThan(afterInterruption), + reason: 'the interrupted warm-up was never resumed', + ); + expect(core.commands, contains('private.characters.list')); + + // And once it does complete, it is retired: no third run. + final settled = core.commands.length; + notifier.prefetchTabData(); + await notifier.prefetchInFlight; + expect(core.commands.length, settled); + }); + test('a fresh inspection prefetches again', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); From 28d844e9e1fa70351c6b299554cf0c38e627bbb1 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 15:50:57 +0200 Subject: [PATCH 09/16] fix(save-editor): re-arm an interrupted warm-up instead of waiting to be called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor on the PR, against the previous commit's fix. Leaving the inspection unmarked when a run is cut short only helps if something calls the warm-up again — and a step still in flight holds `_prefetchRunning` past the moment the interrupting operation clears the loading flag. The state change that would have restarted the warm-up therefore bounces off that guard, and by the time the drained run releases it nothing is left to trigger anything. The tabs it never reached stayed cold for the session, which is what the previous commit set out to prevent. A run that ends without warming everything now re-arms itself. It cannot spin: a fresh run takes the current load sequence, so it can only be cut short by a NEW interruption, and the entry guards still decide whether it may start. The test moves to the ordering that actually exposes this — the first warm-up step outlives the interruption — and drives the notifier through the page's own listener, so the restart cannot be attributed to a trigger the test made by hand. It fails without the re-arm. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 19 +++-- .../test/prefetch_tab_data_test.dart | 72 ++++++++++++------- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index b9a821b7..daa1d8b2 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -673,11 +673,20 @@ class EditorNotifier extends StateNotifier { if (_prefetchRunning) return; if (identical(_prefetchedFor, inspection)) return; _prefetchRunning = true; - prefetchInFlight = _prefetchTabData( - path, - inspection, - _loadSeq, - ).whenComplete(() => _prefetchRunning = false); + prefetchInFlight = _prefetchTabData(path, inspection, _loadSeq).whenComplete( + () { + _prefetchRunning = false; + // A run that was cut short cannot simply wait for the next state + // change: a step already in flight keeps this flag up past the moment + // the interrupting operation clears the loading flag, so the state + // change that would have restarted the warm-up bounces off the guard + // above and never comes again. Re-arm here instead. This cannot spin — + // a fresh run takes the current load sequence, so it can only be cut + // short by a NEW interruption, and the guards decide whether it may + // start at all. + if (!identical(_prefetchedFor, inspection)) prefetchTabData(); + }, + ); } /// Warm every tab's query for [inspection], in reachability order. diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index de635b8c..2be1bd20 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -13,7 +13,12 @@ class _RecordingCore implements GoresaveCoreService { /// and observe what the prefetch does while a request is outstanding. final Duration delay; - _RecordingCore({this.delay = Duration.zero}); + /// One command held far longer than the rest, so a test can arrange for a + /// warm-up step to still be in flight when something else finishes. + final String? slowCommand; + static const _slowDelay = Duration(milliseconds: 120); + + _RecordingCore({this.delay = Duration.zero, this.slowCommand}); List get commands => [for (final r in requests) r.command]; @@ -37,7 +42,8 @@ class _RecordingCore implements GoresaveCoreService { Map payload = const {}, }) async { requests.add((command: command, payload: Map.from(payload))); - if (delay > Duration.zero) await Future.delayed(delay); + final wait = command == slowCommand ? _slowDelay : delay; + if (wait > Duration.zero) await Future.delayed(wait); switch (command) { case 'scan_save_dir': return { @@ -100,6 +106,18 @@ class _RecordingCore implements GoresaveCoreService { } } +/// Wait until no warm-up is left running. A run that was cut short re-arms +/// itself, which replaces `prefetchInFlight`, so awaiting it once is not enough. +Future _settledPrefetch(EditorNotifier notifier) async { + for (var i = 0; i < 20; i++) { + final inFlight = notifier.prefetchInFlight; + await inFlight; + await pumpEventQueue(); + if (identical(notifier.prefetchInFlight, inFlight)) return; + } + fail('the warm-up never settled'); +} + Future _loadedEditor(_RecordingCore core) async { final notifier = EditorNotifier(core, saveDir: r'C:\tmp\saves'); await pumpEventQueue(); @@ -266,44 +284,44 @@ void main() { expect(core.commands.length, first, reason: 'prefetch repeated itself'); }); - test('an interrupted warm-up is picked up again, not written off', () async { + test('an interrupted warm-up restarts itself', () async { // Something else taking the editor mid-warm-up — a backup rename bumps the // load sequence, a codec check raises the loading flag — makes the - // remaining steps skip. The inspection must NOT count as warmed then, or - // the tabs those steps would have covered load the slow way for the rest of - // the session. - final core = _RecordingCore(); + // remaining steps skip. The warm-up has to come back on its own: a step + // still in flight holds the run open past the moment that operation clears + // the loading flag, so the state change that would have restarted it + // bounces off the one-run-at-a-time guard and never comes again. + // The first warm-up step outlives the interruption, which is what puts the + // state change that would restart the warm-up before the run has ended. + final core = _RecordingCore(slowCommand: 'search_typed_properties'); final notifier = await _loadedEditor(core); - // Stand in for that interruption: the warm-up runs against a load sequence - // that has already moved on, so every step skips. - await notifier.inspect(r'C:\tmp\saves\G1R-001.sav'); - await pumpEventQueue(); - core.requests.clear(); + // The editor page's own wiring, so the restart cannot be attributed to a + // trigger this test made by hand. + final removeListener = notifier.addListener( + (_) => notifier.prefetchTabData(), + fireImmediately: false, + ); notifier.prefetchTabData(); - final interrupted = notifier.prefetchInFlight; + // Interrupt it: refreshBackups bumps the load sequence without producing a + // new inspection, so the remaining steps skip. It finishes — and clears the + // loading flag — while the first warm-up step is still outstanding. await notifier.refreshBackups(); - await interrupted; - - final afterInterruption = core.commands - .where((command) => command != 'list_backups') - .length; - - // The next state change must start the sequence over rather than skip it. - notifier.prefetchTabData(); - await notifier.prefetchInFlight; + await _settledPrefetch(notifier); + removeListener(); + // No further trigger of any kind — the warm-up must have re-armed itself. expect( - core.commands.where((command) => command != 'list_backups').length, - greaterThan(afterInterruption), + core.commands, + contains('private.characters.list'), reason: 'the interrupted warm-up was never resumed', ); - expect(core.commands, contains('private.characters.list')); + expect(core.commands, contains('query_progression')); - // And once it does complete, it is retired: no third run. + // And once it does complete, it is retired: another trigger adds nothing. final settled = core.commands.length; notifier.prefetchTabData(); - await notifier.prefetchInFlight; + await _settledPrefetch(notifier); expect(core.commands.length, settled); }); From 39914fa5c54fd6df613370d72ce40cf4cd50f078 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 16:01:37 +0200 Subject: [PATCH 10/16] docs(save): state what the cache-store re-check does and does not prove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR is right that equal fingerprints before and after a command do not prove the files held still: a replacement to other bytes and back again inside the command would pass the check and bind that other answer to the original content hash. Left as is, with the reasoning recorded where the check lives rather than a comment that overstates it. Closing it properly means handing the command the bytes the key was taken from instead of letting it read for itself — a change to how every read command receives its save, not to this check, and not one to make inside a performance branch. What remains needs a replacement landing in the moment between two reads that sit microseconds apart AND the original bytes returning before the command ends, both from outside the editor: a write or a restore performed here drops the save's cached responses outright. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/lib.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 27088beb..8ba1929a 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -388,12 +388,24 @@ pub fn execute_json(input: &str) -> String { // under the earlier fingerprint would not merely be stale for a // moment: it would bind an answer to a content hash it does not // describe, and every later read of those earlier bytes would be - // served the wrong answer for as long as the entry lives. So keep a - // response only when its inputs held still for the whole command. + // served the wrong answer for as long as the entry lives. Re-derive + // the fingerprint and keep the response only if it still matches. // - // A cache HIT needs no such re-check: matching the fingerprint means - // the files hold byte-identical content to what the entry was built - // from, whatever has happened in between. + // Be precise about what that does and does not establish. Equal + // fingerprints before and after do not PROVE the files held still: + // a replacement to other bytes and back again inside the command + // would pass. Closing that would mean handing the command the bytes + // this key was taken from instead of letting it read for itself — + // a change to how every read command receives its save, not to this + // check. What is left needs a replacement landing in the moment + // between these two reads AND the original bytes returning before + // the command ends, both from outside the editor: a write or a + // restore performed HERE drops this save's entries outright (see + // `invalidate_decoded_payload_cache`). + // + // A cache HIT needs no re-check at all: matching the fingerprint + // means the files hold byte-identical content to what the entry was + // built from, whatever has happened in between. if let Some(key) = cache_key { if read_response_cache_key(input).is_some_and(|current| current == key) { store_cached_response(key, &response); From 6b7291cb754ecc28116dd1940f70747885754819 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 16:12:26 +0200 Subject: [PATCH 11/16] fix(save-editor): warm every page of the tabs fetched whole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR. The quest and story panels fetch their section whole and filter it in the client, walking pages the core clamps to 1000. The warm-up asked for one page, so a save past that clamp would open those tabs on a cold traversal for every page after the first — spinner up, which is the wait this warm-up exists to remove. Walk the pages the way the panels do, mirroring their offset arithmetic: the core caches one response per exact request, so an offset they never ask for warms nothing. A save inside the clamp costs exactly one request, as before — the real save measured here holds 472 quests and 470 story entries, so this changes nothing today and holds when either outgrows the page. Covered by a test over a section with three pages and one with exactly one; it fails when only the first page is warmed. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 53 ++++++++++++++++-- .../test/prefetch_tab_data_test.dart | 55 ++++++++++++++++++- 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index daa1d8b2..e0c06975 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -753,15 +753,27 @@ class EditorNotifier extends StateNotifier { if (heroId != null) { await step(() => loadMemoryEvents(heroId, limit: EditorPageSize.detail)); } - await step(() => loadProgressionQuests(limit: EditorPageSize.fullList)); + await step( + () => _prefetchAllPages((offset) async { + final page = await loadProgressionQuests( + offset: offset, + limit: EditorPageSize.fullList, + ); + return (total: page.total, count: page.quests.length); + }), + ); await step(loadGlossary); await step(loadProgressionTutorials); await step( - () => loadStoryState( - includeUnset: true, - limit: EditorPageSize.fullList, - path: inspectionPath, - ), + () => _prefetchAllPages((offset) async { + final page = await loadStoryState( + includeUnset: true, + offset: offset, + limit: EditorPageSize.fullList, + path: inspectionPath, + ); + return (total: page.total, count: page.values.length); + }), ); await step(loadFactions); await step( @@ -772,6 +784,9 @@ class EditorNotifier extends StateNotifier { ), ); + // (see `_prefetchAllPages` for why the two full-list sections above walk + // their pages instead of warming the first one.) + // Only a run that warmed everything retires this inspection. Anything less // leaves the marker unset so the next state change picks the sequence up // again — otherwise a warm-up interrupted by, say, a backup rename would @@ -3335,6 +3350,32 @@ class EditorNotifier extends StateNotifier { return future; } + /// Warm every page a full-list panel will ask for. + /// + /// The quest and story panels fetch their section whole and filter it in the + /// client, walking pages of [EditorPageSize.fullList] until they have `total`. + /// The core clamps a page to 1000 and caches one response per exact request, + /// so warming only the first page leaves a save that has outgrown that clamp + /// to load its remaining pages cold on the tab's first visit — with the + /// panel's spinner up, which is the wait this warm-up exists to remove. + /// + /// [page] must issue the panel's own request for an offset and report that + /// page's `total` and item count. The offsets mirror the panels' arithmetic — + /// items collected so far — because a different offset warms a request they + /// never make. A save inside the clamp costs exactly one request, as before. + Future _prefetchAllPages( + Future<({int total, int count})> Function(int offset) page, + ) async { + var offset = 0; + while (true) { + final result = await page(offset); + offset += result.count; + // An empty page also covers the failure case, where the loader reports a + // zero total: never loop on a stuck or erroring section. + if (result.count == 0 || offset >= result.total) break; + } + } + /// Page the full NPC roster out of the core, without touching the memo. /// /// The core clamps `private.npc.list` `limit` to 1000, but real saves hold diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index 2be1bd20..bfeb812b 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -18,7 +18,15 @@ class _RecordingCore implements GoresaveCoreService { final String? slowCommand; static const _slowDelay = Duration(milliseconds: 120); - _RecordingCore({this.delay = Duration.zero, this.slowCommand}); + /// `query_progression` section to the total it reports, for the sections a + /// panel pages through. Absent sections answer with an empty payload. + final Map pagedSectionTotals; + + _RecordingCore({ + this.delay = Duration.zero, + this.slowCommand, + this.pagedSectionTotals = const {}, + }); List get commands => [for (final r in requests) r.command]; @@ -90,6 +98,25 @@ class _RecordingCore implements GoresaveCoreService { 'companionBackups': [], }, }; + case 'query_progression': + // Sections the panels fetch whole report a total past one page, so the + // warm-up has to walk them the way the panel will. + final section = payload['section']; + final total = pagedSectionTotals[section]; + if (total == null) return {'ok': true, 'data': {}}; + final offset = (payload['offset'] as int?) ?? 0; + final limit = (payload['limit'] as int?) ?? 0; + final count = (total - offset).clamp(0, limit); + final rows = List.generate(count, (i) => {}); + return { + 'ok': true, + 'data': { + 'total': total, + 'offset': offset, + 'limit': limit, + if (section == 'quests') 'quests': rows else 'values': rows, + }, + }; case 'private.characters.list': return { 'ok': true, @@ -267,6 +294,32 @@ void main() { expect(core.commandCount('private.npc.list'), afterPanel); }); + test('prefetch walks every page of a section the panel fetches whole', () async { + // Quests and story state are fetched whole and filtered client-side, in + // pages the core clamps to 1000. A save past that clamp would otherwise + // open those tabs on a cold traversal for every page after the first — + // with the panel's spinner up. + final core = _RecordingCore( + pagedSectionTotals: const {'quests': 2300, 'story': 1000}, + ); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await _settledPrefetch(notifier); + + List offsetsFor(String section) => [ + for (final request in core.requests) + if (request.command == 'query_progression' && + request.payload['section'] == section) + request.payload['offset'] as int, + ]; + + // 2300 over pages of 1000: the panel asks at 0, 1000, 2000 and stops. + expect(offsetsFor('quests'), [0, 1000, 2000]); + // Exactly one full page: the panel stops after it, so the warm-up must too. + expect(offsetsFor('story'), [0]); + }); + test('prefetch runs once per inspection', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); From 369bd5da7a570754baddeb3af5e1facdda47a06b Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 16:22:37 +0200 Subject: [PATCH 12/16] fix(save-editor): abandon a paged warm-up when the editor moves on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor on the PR, against the previous commit's paging. A walk checked for supersede once, before it began, and then kept issuing pages — so a save switch or a load partway through left it queueing requests ahead of the user's own, to warm offsets derived from the previous file's total that nothing would ask for. The quest walk also passed no path, so those later pages queried whichever save was selected by then. Check before every page, and pin the quest pages to the file the walk began against, as the story and NPC walks already were. The NPC walk gets the same supersede check; a real load passes none, because a panel that asked for the roster needs all of it rather than a prefix. `loadProgressionQuests` grows the `path` parameter its story counterpart already has. Defaulting to the current selection keeps the panels' requests — and so the cache keys — exactly as they were. Covered by a test that disturbs the editor from INSIDE the walk, on its first page. My first attempt disturbed it from outside and proved nothing: the interruption landed before the walk had even started, so the sequence skipped it wholesale and the counts matched either way. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 36 +++++++++++-- .../editor/ui/quests_detail_journal_test.dart | 1 + .../test/prefetch_tab_data_test.dart | 50 +++++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index e0c06975..88c8de97 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -739,7 +739,13 @@ class EditorNotifier extends StateNotifier { // the first NPC panel a roster fetched seconds earlier; letting the panel // fill it on first use keeps it derived from the file as of that moment, // and the paging it repeats is answered from the warm core. - await step(() => _fetchAllNpcActors(path, dropMemoOnError: false)); + await step( + () => _fetchAllNpcActors( + path, + dropMemoOnError: false, + superseded: superseded, + ), + ); await step( () => loadKnowledgeEntries( const Actor.player().uniqueName, @@ -754,10 +760,11 @@ class EditorNotifier extends StateNotifier { await step(() => loadMemoryEvents(heroId, limit: EditorPageSize.detail)); } await step( - () => _prefetchAllPages((offset) async { + () => _prefetchAllPages(superseded, (offset) async { final page = await loadProgressionQuests( offset: offset, limit: EditorPageSize.fullList, + path: path, ); return (total: page.total, count: page.quests.length); }), @@ -765,7 +772,7 @@ class EditorNotifier extends StateNotifier { await step(loadGlossary); await step(loadProgressionTutorials); await step( - () => _prefetchAllPages((offset) async { + () => _prefetchAllPages(superseded, (offset) async { final page = await loadStoryState( includeUnset: true, offset: offset, @@ -3016,12 +3023,17 @@ class EditorNotifier extends StateNotifier { } } + /// [path] lets a multi-page caller pin every page to the save its walk began + /// against, so a selection change midway cannot make a later offset — derived + /// from the previous file's total — query a different file. Defaults to the + /// current selection, which is what the panel asks against. Future loadProgressionQuests({ String query = '', int offset = 0, int limit = 100, String? state, String? group, + String? path, }) async { String? error; final data = await _queryProgression({ @@ -3031,7 +3043,7 @@ class EditorNotifier extends StateNotifier { 'limit': limit, if (state != null && state.isNotEmpty) 'state': state, if (group != null && group.isNotEmpty) 'group': group, - }, onError: (message) => error = message); + }, path: path, onError: (message) => error = message); if (data == null) return ProgressionQuestPage(error: error); return ProgressionQuestPage.fromJson(data); } @@ -3363,11 +3375,18 @@ class EditorNotifier extends StateNotifier { /// page's `total` and item count. The offsets mirror the panels' arithmetic — /// items collected so far — because a different offset warms a request they /// never make. A save inside the clamp costs exactly one request, as before. + /// + /// [superseded] is checked before every page, not just before the walk: the + /// offsets and total belong to the file the walk began against, so once + /// something else takes over, every further page would occupy the core queue + /// ahead of the user's own request to warm an offset nothing will ask for. + /// Each page must also be pinned to that file for the same reason. Future _prefetchAllPages( + bool Function() superseded, Future<({int total, int count})> Function(int offset) page, ) async { var offset = 0; - while (true) { + while (!superseded()) { final result = await page(offset); offset += result.count; // An empty page also covers the failure case, where the loader reports a @@ -3388,14 +3407,21 @@ class EditorNotifier extends StateNotifier { /// stay cached, so it clears the memo slot the future was stored in. The /// background warm-up passes false — it has no slot to clear, and clearing the /// memo behind a real load in flight would be wrong. + /// + /// [superseded] likewise belongs to the warm-up: it abandons the walk when + /// something else takes the editor, rather than keeping the core queue busy + /// ahead of the user's own request. A real load passes none — a panel that + /// asked for the roster needs all of it, not a prefix. Future _fetchAllNpcActors( String? pinnedPath, { required bool dropMemoOnError, + bool Function()? superseded, }) async { final npcs = []; var offset = 0; var total = 0; while (true) { + if (superseded?.call() ?? false) break; final page = await loadNpcActors( offset: offset, limit: 1000, diff --git a/apps/save-editor/test/features/editor/ui/quests_detail_journal_test.dart b/apps/save-editor/test/features/editor/ui/quests_detail_journal_test.dart index 9399e38f..bdb45270 100644 --- a/apps/save-editor/test/features/editor/ui/quests_detail_journal_test.dart +++ b/apps/save-editor/test/features/editor/ui/quests_detail_journal_test.dart @@ -84,6 +84,7 @@ class _QuestJournalNotifier extends EditorNotifier { String? group, int offset = 0, int limit = 100, + String? path, }) async => ProgressionQuestPage( quests: _quests, total: _quests.length, diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index bfeb812b..2945120c 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter_test/flutter_test.dart'; import 'package:goresave/features/editor/domain/core_service.dart'; import 'package:goresave/features/editor/domain/editor_notifier.dart'; @@ -22,6 +24,10 @@ class _RecordingCore implements GoresaveCoreService { /// panel pages through. Absent sections answer with an empty payload. final Map pagedSectionTotals; + /// Called as each request is recorded, so a test can disturb the editor from + /// inside a walk rather than having to time it from outside. + void Function(String command, Map payload)? onRequest; + _RecordingCore({ this.delay = Duration.zero, this.slowCommand, @@ -50,6 +56,7 @@ class _RecordingCore implements GoresaveCoreService { Map payload = const {}, }) async { requests.add((command: command, payload: Map.from(payload))); + onRequest?.call(command, payload); final wait = command == slowCommand ? _slowDelay : delay; if (wait > Duration.zero) await Future.delayed(wait); switch (command) { @@ -320,6 +327,49 @@ void main() { expect(offsetsFor('story'), [0]); }); + test('a paged walk stops when something else takes the editor', () async { + // The offsets and total a walk carries belong to the file it began against. + // Once something else takes over, every further page would sit in the core + // queue ahead of the user's own request to warm an offset nothing will ask + // for — and, unpinned, would ask it of a different file. + // Twenty pages' worth, so a walk that ignores supersede is unmistakable. + final core = _RecordingCore( + pagedSectionTotals: const {'quests': 20000}, + ); + final notifier = await _loadedEditor(core); + + // Disturb the editor from INSIDE the walk, on its first page — timing it + // from outside would land before the walk even starts. + var disturbed = false; + core.onRequest = (command, payload) { + if (disturbed) return; + if (command != 'query_progression' || payload['section'] != 'quests') { + return; + } + disturbed = true; + unawaited(notifier.refreshBackups()); + }; + + notifier.prefetchTabData(); + await _settledPrefetch(notifier); + core.onRequest = null; + + final offsets = [ + for (final request in core.requests) + if (request.command == 'query_progression' && + request.payload['section'] == 'quests') + request.payload['offset'] as int, + ]; + // The superseded walk must abandon its remaining pages. The restart then + // walks all twenty, so the count lands near twenty rather than near forty. + expect(offsets.length, lessThan(30), reason: 'a superseded walk kept paging'); + // Every page it did ask for names the file the walk began against. + for (final request in core.requests) { + if (request.command != 'query_progression') continue; + expect(request.payload['path'], r'C:\tmp\saves\G1R-001.sav'); + } + }); + test('prefetch runs once per inspection', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); From fc2b97927b91894124e661cc700441cf579b4279 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 16:38:00 +0200 Subject: [PATCH 13/16] fix(save): keep the typed search linear, and bound the cache by what it holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from Codex on the PR. The sibling-uniqueness check was my own regression. Replacing the per-list name map with a scan per property was right for the handful of entries a property list normally has, and quadratic for a list that has many. Measured on one wide list, scanning every time against counting once: 1,000 siblings 2.4 ms -> 0.15 ms 5,000 siblings 52.9 ms -> 0.46 ms 20,000 siblings 753.1 ms -> 3.0 ms 50,000 siblings 6.1 s -> 7.5 ms Keep the scan below a threshold, where it beats building a map the walk would throw away, and count once above it. A test pins that both sides agree on which names are unique — a disagreement would either hide editable properties or, worse, offer a duplicated name as editable and let a write resolve to the wrong one. The response cache counted only responses against its byte budget while each entry also holds its request verbatim, and a request carries caller-supplied text with no bound of its own. Count everything an entry keeps alive. Typed-search output over a real save is unchanged. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/lib.rs | 18 +++++- crates/gore-save/src/properties.rs | 97 ++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 14 deletions(-) diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 8ba1929a..0935c6ae 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -5517,6 +5517,20 @@ struct CachedResponseEntry { response: String, } +impl CachedResponseEntry { + /// Everything this entry keeps alive, not just the answer. The request is + /// held verbatim as part of the key, and a request carries caller-supplied + /// text — a property-browser query, say — with no bound of its own. Counting + /// only responses would let a handful of large requests hold many times the + /// budget below. + fn footprint(&self) -> usize { + self.key.request.len() + + self.key.fingerprint.len() + + self.path.as_os_str().len() + + self.response.len() + } +} + /// Bounded so a long session cannot grow without limit. A whole save's worth of /// editor queries is around fifteen entries and well under a megabyte, so these /// hold several saves at once — switching back and forth stays free — while @@ -5599,10 +5613,10 @@ fn store_cached_response(key: ResponseCacheKey, response: &str) { }); // Oldest first: within one save every entry is wanted, so evicting by age // drops the save the user has moved away from rather than the current one. - let mut bytes: usize = guard.iter().map(|entry| entry.response.len()).sum(); + let mut bytes: usize = guard.iter().map(CachedResponseEntry::footprint).sum(); while guard.len() > RESPONSE_CACHE_MAX_ENTRIES || bytes > RESPONSE_CACHE_MAX_BYTES { let evicted = guard.remove(0); - bytes -= evicted.response.len(); + bytes -= evicted.footprint(); } } diff --git a/crates/gore-save/src/properties.rs b/crates/gore-save/src/properties.rs index cde1a782..c3eab79f 100644 --- a/crates/gore-save/src/properties.rs +++ b/crates/gore-save/src/properties.rs @@ -823,20 +823,58 @@ fn is_scalar(value: &PropertyValue) -> bool { ) } -/// Whether `name` occurs exactly once in `props`. Property lists are almost -/// always short, so a linear scan beats building a whole `HashMap` per list — -/// and the walk builds one per list, once per node in the tree. -fn occurs_once(props: &[Property], name: &str) -> bool { - let mut seen = 0usize; - for property in props { - if property.name == name { - seen += 1; - if seen > 1 { - return false; +/// Answers "does this name occur exactly once among its siblings?" for every +/// property in one list. +/// +/// A path segment is only addressable when its name is unique among its +/// siblings, so the walk asks this once per property, for every list in the +/// tree. Both shapes matter: +/// +/// * Property lists are almost always a handful of entries, where scanning the +/// list beats building a `HashMap` the walk would then throw away — and it +/// builds one per node, so that allocation is not free. +/// * A list with many distinct names would make that scan quadratic, and a +/// single long list is enough to stall a whole search. Past a threshold the +/// names are counted once and shared. +enum SiblingNames<'a> { + /// Short list: scanned on demand, nothing allocated. + Scan, + Counted(HashMap<&'a str, usize>), +} + +impl<'a> SiblingNames<'a> { + /// Above this, counting once and sharing beats rescanning per property. + /// Below it, a scan is a few comparisons against a cache-hot slice. + const COUNT_ABOVE: usize = 32; + + fn of(props: &'a [Property]) -> Self { + if props.len() <= Self::COUNT_ABOVE { + return Self::Scan; + } + let mut counts = HashMap::<&str, usize>::with_capacity(props.len()); + for property in props { + *counts.entry(property.name.as_str()).or_default() += 1; + } + Self::Counted(counts) + } + + fn occurs_once(&self, props: &[Property], name: &str) -> bool { + match self { + Self::Counted(counts) => counts.get(name).copied() == Some(1), + Self::Scan => { + let mut seen = 0usize; + for property in props { + if property.name == name { + seen += 1; + if seen > 1 { + return false; + } + } + } + seen == 1 } } } - seen == 1 } fn walk_search<'a>( @@ -845,9 +883,10 @@ fn walk_search<'a>( ancestors_addressable: bool, ctx: &mut SearchCtx, ) { + let siblings = SiblingNames::of(props); for p in props { let mark = path.push(Cow::Borrowed(p.name.as_str()), true); - let addressable = ancestors_addressable && occurs_once(props, &p.name); + let addressable = ancestors_addressable && siblings.occurs_once(props, &p.name); // Leaf value? if is_scalar(&p.value) { @@ -4356,6 +4395,40 @@ mod tests { assert_eq!(hits[0].display, "ΟΣΤΟΥΝ"); } + /// A path segment is addressable only when its name is unique among its + /// siblings, and the walk asks that for every property in every list. Both + /// sides of the size threshold must give the same answer, or a long list + /// would quietly report its properties as uneditable — or worse, report a + /// duplicated name as editable and let a write resolve to the wrong one. + #[test] + fn sibling_uniqueness_agrees_across_the_size_threshold() { + for count in [ + 3usize, + SiblingNames::COUNT_ABOVE, + SiblingNames::COUNT_ABOVE + 1, + 200, + ] { + let mut props = Vec::new(); + for index in 0..count { + props.extend_from_slice(&int_property(&format!("m_Unique{index}"), 1)); + } + // One name appearing twice, whatever the list length. + props.extend_from_slice(&int_property("m_Twice", 1)); + props.extend_from_slice(&int_property("m_Twice", 2)); + let payload = root("/Script/Test.Save", &props); + let parsed = parse_private_root(&payload).unwrap(); + + let (hits, total) = search_properties(&parsed, "m_", 0, 10000); + assert_eq!(total, count + 2, "list of {count} lost properties"); + + let unique = hits.iter().find(|h| h.display == "m_Unique0").unwrap(); + assert!(unique.editable, "unique name not addressable at {count}"); + for hit in hits.iter().filter(|h| h.display == "m_Twice") { + assert!(!hit.editable, "duplicated name addressable at {count}"); + } + } + } + #[test] fn search_marks_strings_editable() { // root class string is not a property; build a payload with a StrProperty From df30bdcb61123865088682c6d6ebcfa795b5c8c0 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 16:59:07 +0200 Subject: [PATCH 14/16] fix(save): cap how many backup files a listing holds at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR. The backup listings spread their work across one worker per logical CPU, and each worker holds a whole file while it hashes and inspects it — so peak memory was cores x file size, with nothing bounding either. The severity does not carry over from the report's example: this path passes `include_private: false`, so it never decompresses, and a G1R save is a couple of megabytes on disk rather than the 80 MiB assumed there. Against the folder measured here — 147 backups, largest 2.6 MB — 24 workers meant about 62 MB, not gigabytes. The bound is still worth having, and costs almost nothing, because the work is disk-bound and the curve flattens early (serial 130 ms): 2 workers 99 ms 8 workers 60 ms 4 workers 70 ms 12 workers 62 ms Cap at eight. Past there nothing is left to win, and every further reader is another whole file held in memory for it. `par_map` takes the cap from its caller, since the right bound follows from what a worker holds rather than from how many cores are idle. Co-Authored-By: Claude Opus 5 --- crates/gore-save/src/lib.rs | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 0935c6ae..3ae46427 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -1940,6 +1940,24 @@ fn rename_backup(save_path: &Path, backup_path: &Path, name: &str) -> Result Result, CoreError> { let parent = path.parent().unwrap_or_else(|| Path::new(".")); if !parent.exists() { @@ -1988,7 +2006,7 @@ pub fn list_save_backups(path: &Path) -> Result, CoreError> // listing sits in the load path — so read them side by side rather than one // after another. Order is preserved, and a read error still aborts the whole // listing exactly as a serial loop would. - for item in par_map(candidates, |candidate| { + for item in par_map(candidates, BACKUP_READ_WORKERS, |candidate| { describe_save_backup(candidate, &prefix, &names) }) { backups.push(item?); @@ -2094,7 +2112,7 @@ fn list_persistent_data_list_backups_for_save( // Read and parse the profile backups side by side, as the save backups above // are: each one is read whole, hashed whole, and strictly profile-parsed. - for item in par_map(candidates, |candidate| { + for item in par_map(candidates, BACKUP_READ_WORKERS, |candidate| { describe_profile_backup(candidate, &prefix, &names, slot) }) { backups.push(item?); @@ -4835,9 +4853,15 @@ fn join(handle: std::thread::ScopedJoinHandle<'_, T>) -> T { /// worth splitting — reading and hashing a folder full of save backups, say. /// /// Items are handed out in contiguous chunks, one chunk per thread, bounded by -/// the machine's parallelism so a folder with hundreds of entries does not spawn -/// hundreds of threads. -fn par_map(items: Vec, work: impl Fn(T) -> R + Sync) -> Vec +/// both [`max_workers`](par_map) and the machine's parallelism, so a folder with +/// hundreds of entries does not spawn hundreds of threads. +/// +/// `max_workers` is the caller's, because the right bound depends on what a +/// worker holds rather than on how many cores are idle. A worker that keeps a +/// whole file in memory sets the peak footprint at workers × file size, and +/// splitting disk-bound work past a handful of readers buys nothing to pay for +/// that. +fn par_map(items: Vec, max_workers: usize, work: impl Fn(T) -> R + Sync) -> Vec where T: Send, R: Send, @@ -4845,6 +4869,7 @@ where let threads = std::thread::available_parallelism() .map(|value| value.get()) .unwrap_or(4) + .min(max_workers) .min(items.len()); if threads <= 1 { return items.into_iter().map(work).collect(); From 83cd26e6bc9a783f006709268e1094ab6cedab47 Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 17:17:53 +0200 Subject: [PATCH 15/16] fix(save-editor): make the core hold the save the user came back to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on the PR. The decoded payload and parsed tree live in caches that hold ONE save each, and `inspect_save` seeds them on the way past — except when its own response comes from the response cache. That is exactly what happens on returning to a save opened earlier: the inspection lands in milliseconds while those caches still hold whichever save was opened in between, and the warm-up that follows is all cache hits, so nothing puts it right. The first read that needs the tree then pays for it in front of the user. Measured on a real save, opening A, switching to B and returning to A: the first NPC detail after returning 1.3 s Per-NPC panels cannot be warmed one by one — a save holds ~1500 NPCs — so what has to be warmed is the tree itself. `warm_save` asks the core to make a save the one it holds, and the background warm-up calls it first, ahead of every step that benefits from it. The same read then costs 4.7 ms; the 1.3 s happens while the user is still reading the Overview tab. On a normally loaded save the call is a cache hit and costs a file read and a hash. Deliberately not response-cached: a stored "warmed" would skip the seeding that is the whole point of the call. A test pins that, and another pins that warming moves the reparse off the following read — both fail without the fix. Both are timing tests, because the property IS timing: every path here returns the same answer. They compare two measurements from the same run rather than a fixed budget. The file's tests now also take a mutex, since the caches they exercise are process-global and were displacing each other under the default parallel test run. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 21 +++- .../test/prefetch_tab_data_test.dart | 16 +++ crates/gore-save/src/lib.rs | 23 ++++ crates/gore-save/tests/response_cache.rs | 105 ++++++++++++++++++ 4 files changed, 163 insertions(+), 2 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index 88c8de97..0d32da8f 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -726,8 +726,18 @@ class EditorNotifier extends StateNotifier { } } - // Ordered by how soon the user can reach the data: the Overview tab is - // already on screen, Characters is one click away, then World, then the + // First, because everything that reads private data depends on it and the + // per-NPC panels cannot be warmed one by one — there are far too many. + // Loading a save normally leaves the core holding its decoded payload and + // parsed tree, but returning to a save opened earlier does not: the + // inspection comes back from the core's cache while the tree it holds still + // belongs to whichever save was opened in between, and the first NPC the + // user clicks would pay the decode and parse. Cheap when the core already + // holds this save, which is the usual case. + await step(() => _warmPrivateTree(path)); + + // Then, ordered by how soon the user can reach the data: the Overview tab + // is already on screen, Characters is one click away, then World, then the // property browser. await step(loadGameTime); // Also settles the hero GlobalId that the player's Events sub-tab needs. @@ -3362,6 +3372,13 @@ class EditorNotifier extends StateNotifier { return future; } + /// Ask the core to make [path]'s decoded payload and parsed tree the ones it + /// holds. Returns nothing: the point is the state it leaves behind, which + /// every later private read shares. + Future _warmPrivateTree(String path) async { + await _execute('warm_save', payload: {'path': path}); + } + /// Warm every page a full-list panel will ask for. /// /// The quest and story panels fetch their section whole and filter it in the diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index 2945120c..ea87364f 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -199,6 +199,22 @@ void main() { ); }); + test('prefetch makes the core hold this save before anything else', () async { + // Everything reading private data shares the core's single decoded payload + // and parsed tree, and the per-NPC panels are far too numerous to warm one + // by one. Returning to a save opened earlier serves its inspection from the + // core's cache without reseeding those, so the warm-up has to say so + // explicitly — and first, since every step after it benefits. + final core = _RecordingCore(); + final notifier = await _loadedEditor(core); + + notifier.prefetchTabData(); + await _settledPrefetch(notifier); + + expect(core.commands.first, 'warm_save'); + expect(core.payloadFor('warm_save'), {'path': r'C:\tmp\saves\G1R-001.sav'}); + }); + test('prefetch asks for the page sizes the panels ask for', () async { final core = _RecordingCore(); final notifier = await _loadedEditor(core); diff --git a/crates/gore-save/src/lib.rs b/crates/gore-save/src/lib.rs index 3ae46427..b4a89c1f 100644 --- a/crates/gore-save/src/lib.rs +++ b/crates/gore-save/src/lib.rs @@ -461,6 +461,29 @@ fn execute_json_inner(input: &str) -> Result { "activeProfileId": summary.active_profile_id, })) } + // Make this save the one the decoded-payload and parsed-root caches + // hold, decoding and parsing it if they hold another. + // + // Those caches keep a single save each — a decoded payload and its tree + // run to hundreds of megabytes, so holding two is not free. Everything + // that reads private data shares them, and `inspect_save` normally + // seeds them on the way past. It does not when its own response comes + // from the response cache, which is exactly what happens on returning to + // a save opened earlier: the inspection is served in milliseconds while + // the caches still hold whichever save was opened in between, and the + // first read that needs the tree — a per-NPC detail, which is too + // numerous to warm one by one — pays the decode and parse in front of + // the user. Calling this during the background warm-up moves that cost + // off the click. Cheap when the caches already hold this save. + // + // Deliberately NOT response-cached: a cached "ready" would skip the very + // seeding that is the point of the call. + "warm_save" => { + let path = required_path(&payload)?; + let kraken_backend = codec_backend::KrakenBackend::default(); + decode_private_root_cached(&path, &kraken_backend)?; + Ok(json!({ "warmed": true })) + } "inspect_save" => { let path = required_path(&payload)?; let include_private = payload diff --git a/crates/gore-save/tests/response_cache.rs b/crates/gore-save/tests/response_cache.rs index 3d6ab754..d87277ec 100644 --- a/crates/gore-save/tests/response_cache.rs +++ b/crates/gore-save/tests/response_cache.rs @@ -4,6 +4,16 @@ //! cargo test --release -p gore-save --test response_cache -- --nocapture use serde_json::{Value, json}; +/// The caches under test are process-global and hold ONE save each, so two of +/// these tests running at once displace each other's state — harmless for the +/// content assertions, fatal for the timing ones. Every test takes this first, +/// which keeps the file correct whatever `--test-threads` is set to. +static ONE_AT_A_TIME: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +fn serially() -> std::sync::MutexGuard<'static, ()> { + ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner()) +} + fn exec(req: Value) -> Value { let resp: Value = serde_json::from_str(&gore_save::execute_json(&req.to_string())).unwrap(); assert_eq!(resp["ok"], json!(true), "request failed: {resp}"); @@ -62,6 +72,7 @@ fn first_hero_attribute(path: &str) -> (Vec, f64) { /// the cache is a memo, not an approximation. #[test] fn repeated_reads_return_the_same_answer() { + let _serial = serially(); let Some((_dir, path)) = temp_copy("G1R-cache-repeat.sav") else { return; }; @@ -90,6 +101,7 @@ fn repeated_reads_return_the_same_answer() { /// next read must reflect the new bytes rather than the memo of the old ones. #[test] fn a_write_is_never_served_a_stale_read() { + let _serial = serially(); let Some((_dir, path)) = temp_copy("G1R-cache-write.sav") else { return; }; @@ -126,6 +138,7 @@ fn a_write_is_never_served_a_stale_read() { /// it. #[test] fn an_external_replacement_is_not_served_from_cache() { + let _serial = serially(); let Some((_dir, path)) = temp_copy("G1R-cache-external.sav") else { return; }; @@ -159,6 +172,96 @@ fn an_external_replacement_is_not_served_from_cache() { ); } +/// Returning to a save opened earlier gets its inspection from the response +/// cache, which means nothing reseeds the single-save decode and parse caches — +/// they still hold whichever save was opened in between. `warm_save` exists so +/// the background warm-up can put that right before the user clicks something +/// the response cache cannot answer, such as a per-NPC detail. +/// +/// This is a timing test, because the property IS timing: both paths return the +/// same answer. It compares the two against each other rather than against a +/// fixed budget, so it does not depend on how fast the machine is; the gap it +/// guards was measured at roughly 250x. +#[test] +fn warming_a_returned_to_save_moves_the_reparse_off_the_next_read() { + let _serial = serially(); + let Some((_dir_a, a)) = temp_copy("G1R-warm-a.sav") else { + return; + }; + let Some((_dir_b, b)) = temp_copy("G1R-warm-b.sav") else { + return; + }; + + // A query whose answer is NOT in the response cache each time it is asked, + // so it has to reach the parsed tree — as a freshly opened NPC panel does. + let mut probe = 0; + let mut read_needing_the_tree = |path: &str| { + probe += 1; + let started = std::time::Instant::now(); + exec(json!({ + "command": "search_typed_properties", + "payload": { + "path": path, + "query": format!("GameTime {probe}"), + "offset": 0, + "limit": 10, + }, + })); + started.elapsed() + }; + + // A, away to B, back to A. The inspection comes back cached; the caches hold B. + let _ = inspect(&a); + let _ = read_needing_the_tree(&a); + let _ = inspect(&b); + let _ = read_needing_the_tree(&b); + let _ = inspect(&a); + let cold = read_needing_the_tree(&a); + + // Same again, with the warm-up step the prefetch performs. + let _ = inspect(&b); + let _ = read_needing_the_tree(&b); + let _ = inspect(&a); + exec(json!({ "command": "warm_save", "payload": { "path": a } })); + let warmed = read_needing_the_tree(&a); + + assert!( + warmed * 4 < cold, + "warming did not move the reparse off the read: {warmed:?} against {cold:?}", + ); +} + +/// `warm_save` must never be answered from the response cache: a stored "warmed" +/// would skip the seeding that is the entire point of the call. +#[test] +fn warming_is_never_answered_from_the_cache() { + let _serial = serially(); + let Some((_dir_a, a)) = temp_copy("G1R-warm-cache-a.sav") else { + return; + }; + let Some((_dir_b, b)) = temp_copy("G1R-warm-cache-b.sav") else { + return; + }; + let warm = |path: &str| { + let started = std::time::Instant::now(); + exec(json!({ "command": "warm_save", "payload": { "path": path } })); + started.elapsed() + }; + + warm(&a); + // Already this save: nothing to do beyond reading and hashing the file. + let repeat = warm(&a); + // Displace it, then ask again for the SAME request as the first call. A + // cached answer would return just as fast as the repeat above did. + warm(&b); + let after_displacement = warm(&a); + + assert!( + repeat * 4 < after_displacement, + "warm_save was served from cache: {repeat:?} against {after_displacement:?}", + ); +} + /// `private.npc.position` reports the recorded placement undo, which lives in a /// sidecar next to the save rather than inside it. The sidecar can change while /// the save bytes stay exactly as they were — restoring a backup puts back @@ -166,6 +269,7 @@ fn an_external_replacement_is_not_served_from_cache() { /// key has to cover it. #[test] fn a_changed_placement_note_is_not_served_from_cache() { + let _serial = serially(); let Some((_dir, path)) = temp_copy("G1R-cache-placement.sav") else { return; }; @@ -235,6 +339,7 @@ fn a_changed_placement_note_is_not_served_from_cache() { /// removing a backup changes the answer while the save file is untouched. #[test] fn directory_listings_are_not_cached() { + let _serial = serially(); let Some((dir, path)) = temp_copy("G1R-cache-backups.sav") else { return; }; From f7b77e5a5fbd2cad2db54e948f9e41768078a7fb Mon Sep 17 00:00:00 2001 From: Daniel Hoer Date: Wed, 12 Aug 2026 17:25:27 +0200 Subject: [PATCH 16/16] fix(save-editor): rebuild the core-held tree after the tabs, not before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor on the PR, against the previous commit. Putting the tree rebuild first made it the one thing standing between the user and a cached tab: it runs on the shared core queue, and on a returned-to save it holds that queue for over a second while every step behind it — and every Characters or World click in that window — is an answer already sitting in the cache. Move it to the end. The case where the rebuild costs anything is precisely the case where all the tab queries are cached, so they cost milliseconds ahead of it; on a normally loaded save the call is a cache hit wherever it sits. The tabs the user can click stay instant either way, and the tree is rebuilt behind them, in time for the first NPC they open. Co-Authored-By: Claude Opus 5 --- .../editor/domain/editor_notifier.dart | 26 ++++++++++--------- .../test/prefetch_tab_data_test.dart | 13 ++++++---- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart index 0d32da8f..f275c0e4 100644 --- a/apps/save-editor/lib/features/editor/domain/editor_notifier.dart +++ b/apps/save-editor/lib/features/editor/domain/editor_notifier.dart @@ -726,18 +726,8 @@ class EditorNotifier extends StateNotifier { } } - // First, because everything that reads private data depends on it and the - // per-NPC panels cannot be warmed one by one — there are far too many. - // Loading a save normally leaves the core holding its decoded payload and - // parsed tree, but returning to a save opened earlier does not: the - // inspection comes back from the core's cache while the tree it holds still - // belongs to whichever save was opened in between, and the first NPC the - // user clicks would pay the decode and parse. Cheap when the core already - // holds this save, which is the usual case. - await step(() => _warmPrivateTree(path)); - - // Then, ordered by how soon the user can reach the data: the Overview tab - // is already on screen, Characters is one click away, then World, then the + // Ordered by how soon the user can reach the data: the Overview tab is + // already on screen, Characters is one click away, then World, then the // property browser. await step(loadGameTime); // Also settles the hero GlobalId that the player's Events sub-tab needs. @@ -804,6 +794,18 @@ class EditorNotifier extends StateNotifier { // (see `_prefetchAllPages` for why the two full-list sections above walk // their pages instead of warming the first one.) + // Last, and deliberately so. Everything reading private data shares the + // core's single decoded payload and parsed tree, and the per-NPC panels are + // far too numerous to warm one by one — so the tree itself has to be warmed. + // Loading a save normally leaves the core holding it already, making this a + // few milliseconds; the case that costs is returning to a save opened + // earlier, where the core holds whichever save came in between. But that is + // exactly the case where every step above is a cached answer, and this one + // would hold the queue for a second in front of them. So warm the tabs the + // user can click first, and rebuild the tree behind them, in time for the + // first NPC they open. + await step(() => _warmPrivateTree(path)); + // Only a run that warmed everything retires this inspection. Anything less // leaves the marker unset so the next state change picks the sequence up // again — otherwise a warm-up interrupted by, say, a backup rename would diff --git a/apps/save-editor/test/prefetch_tab_data_test.dart b/apps/save-editor/test/prefetch_tab_data_test.dart index ea87364f..b88b96f6 100644 --- a/apps/save-editor/test/prefetch_tab_data_test.dart +++ b/apps/save-editor/test/prefetch_tab_data_test.dart @@ -199,19 +199,22 @@ void main() { ); }); - test('prefetch makes the core hold this save before anything else', () async { + test('prefetch rebuilds the core-held tree after the tabs, not before', () async { // Everything reading private data shares the core's single decoded payload // and parsed tree, and the per-NPC panels are far too numerous to warm one - // by one. Returning to a save opened earlier serves its inspection from the - // core's cache without reseeding those, so the warm-up has to say so - // explicitly — and first, since every step after it benefits. + // by one — so the warm-up asks for the tree explicitly. It has to come + // LAST: the case where rebuilding it is expensive (returning to a save + // opened earlier) is exactly the case where every tab query is already a + // cached answer, and putting the rebuild first would hold the core queue in + // front of a click that should return in milliseconds. final core = _RecordingCore(); final notifier = await _loadedEditor(core); notifier.prefetchTabData(); await _settledPrefetch(notifier); - expect(core.commands.first, 'warm_save'); + expect(core.commands.last, 'warm_save'); + expect(core.commands.where((c) => c == 'warm_save'), hasLength(1)); expect(core.payloadFor('warm_save'), {'path': r'C:\tmp\saves\G1R-001.sav'}); });