From a8b6fc8ad658b4c4b2b6fab4d6985f12ab0d225b Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 19 Oct 2026 22:05:01 -0600 Subject: [PATCH] perf(layout): the resolve's seed pass walked the graph, not the change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each solve chooses which nodes to solve and which clean rects to seed it with. It did that by asking **every id in the graph** — 18 203 frames and 42 284 regions on the player's interface — "is one of your dependents dirty?", for a dirty closure whose steady-state median is 2 frames and 4 regions. And nothing was measuring it. `[layout-pre]`'s last lap ends at the change gate; `[layout-prof]`'s `frame_us`/`region_us` start at the round loop. This pass sits between them, so neither line charged it — which is how a `frame:GetLeft` that measures 17.5–18.4 us a call in a live session (six a frame, through stock `MouseIsOver`'s four edge reads) could force a resolve the profiler called 3 us. A `seedsel_us` column now closes that gap, and it stays: a phase between two timers is a phase nobody owns. Measured on the player's own 80-addon interface, last-quarter solves: median p75 p90 mean max before 458 498 607 488 882 after 0 3 9 38 3788 `solved` and `swept` come out identical to the digit (median 2/4, mean 156.3/408.3, max 18183/41415): the same work is selected. Before, 202 of 549 solves cost over 400 us and the dearest of them had dirty sets of 0–13 nodes, which is the pathology itself; after, 29 of 549 exceed it and every one is solving 14 000–16 600 frames at 0.48 us a node, against a `frame_us` of 10 170 us on the same pass. The cost moved from the size of the graph to the size of the change, which was the point; the higher max is that, not a regression. **What it is not: an fps fix.** The bench's per-frame totals do not move — 1.294 -> 1.293 ms/frame grouped, `vm-tick` 0.667 -> 0.676 (noise, single runs). The one column that shifts is `resolve`, 0.011 -> 0.006 ms/frame. The 458 us is per *solve*, and a steady frame does a fraction of one; the live bound is the whole of `frame:GetLeft`, 105-110 us/f, against a 7.6-9.5 ms frame. So this removes real waste and buys about 1%, which is worth having and is not what grom asked for. Chasing it at all was a misread of mine: `GetLeft` is 54% of the *binding* bill, and the binding bill is ~200 us of a ~9000 us frame. The same set, by the same relation read the other way round: a clean node is seeded iff some dirty node reads it, and a node's dependencies are exactly the three `scope.edge` arguments in the roster walk — its anchors' targets, plus a region's owner frame, whose scale it reads. `LayoutScope::dirty_ids` carries the dirty set as a list, written wherever `dirty` turns true so the two cannot disagree, and `seed_dep` enumerates the dependencies. A `full_scope` pass skips the walk: everything is dirty, so there are no seeds to find. Order is not a property here, which is what makes the rewrite safe. Both rosters are built from `HashMap` iteration (`frame_to_id`, `region_data`), so no pass ever had a canonical order, and the region sweep is a fixpoint whose round cap covers any chain — order moves `rounds`, not rects. Only the seed lists are sorted, to drop duplicates a shared dependency would otherwise re-`set_external` every round. `verify_selection` is the falsifier: under `WOW_LAYOUT_VERIFY` it recomputes all four lists the old way and requires the same sets. That is a tighter question than `resolve_layout`'s rect comparison — it isolates this pass, so a selection bug cannot hide behind an unrelated red, and an unrelated red cannot be pinned on the selection. Verified non-vacuous by dropping the region-owner dependency and watching it fail (`0 vs 1 entries`). Note for whoever runs it: `WOW_LAYOUT_VERIFY=1` over `tick_bench` is **red at HEAD**, independently of this change — an incremental pass ends with 48 fewer region rects than a full derivation of the same frame (`regions 29597 -> 29645`), byte-identical with and without this commit. That is a missing `touch_layout()` somewhere, not a selection bug, and it had never been run before because `tick_bench` is `--ignored` and the flag is in no gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-ui/src/script/layout.rs | 210 +++++++++++++++++++++---- 1 file changed, 178 insertions(+), 32 deletions(-) diff --git a/crates/benilla-ui/src/script/layout.rs b/crates/benilla-ui/src/script/layout.rs index 12986224..6c8af115 100644 --- a/crates/benilla-ui/src/script/layout.rs +++ b/crates/benilla-ui/src/script/layout.rs @@ -164,6 +164,96 @@ pub(super) const FONTSTRING_MIN_SPAN: f32 = 1.0; /// Marks a [`LayoutScope::node_of`] index as one into `regions` rather than `plan`. const REGION_TAG: u32 = 0x8000_0000; +/// Seed `x`'s cached rect for this pass if `x` is clean: a dirty node reads it and the round loop +/// will not recompute it. A dirty `x`, or one with no roster slot, needs nothing — the first is +/// solved and the second constrains nothing (see [`LayoutScope::edge`]). +fn seed_dep(scope: &LayoutScope, x: u32, seed_frames: &mut Vec, seed_regions: &mut Vec) { + if scope.dirty.get(x as usize).copied().unwrap_or(false) { + return; + } + let Some(&m) = scope.node_of.get(x as usize) else { + return; + }; + if m == u32::MAX { + return; + } + if m & REGION_TAG != 0 { + seed_regions.push(m & !REGION_TAG); + } else { + seed_frames.push(m); + } +} + +/// `WOW_LAYOUT_VERIFY`: recompute the four selection lists the way the pass used to — ask **every** +/// id whether one of its dependents is dirty — and require the same sets. +/// +/// `resolve_layout`'s own verify compares *rects* after a full re-derivation, which catches this +/// pass getting the sets wrong but also everything else that can make a rect differ. This isolates +/// the question, so a selection bug cannot hide behind an unrelated red and an unrelated red cannot +/// be blamed on the selection. Sets, not sequences: see the pass's own comment on why order here +/// was never canonical. +fn verify_selection( + scope: &LayoutScope, + solve_frames: &[u32], + sweep_regions: &[u32], + seed_frames: &[u32], + seed_regions: &[u32], +) { + let read_by_dirty = |id: usize| -> bool { + let mut e = scope.dep_head[id]; + while e != u32::MAX { + if scope.dirty[scope.dep_to[e as usize] as usize] { + return true; + } + e = scope.dep_next[e as usize]; + } + false + }; + let (mut want_solve, mut want_sweep) = (Vec::new(), Vec::new()); + let (mut want_sf, mut want_sr) = (Vec::new(), Vec::new()); + for id in 0..scope.node_of.len() { + let n = scope.node_of[id]; + if n == u32::MAX { + continue; + } + let (idx, is_region, dirty) = (n & !REGION_TAG, n & REGION_TAG != 0, scope.dirty[id]); + if is_region { + if dirty { + want_sweep.push(idx); + want_sr.push(idx); + } else if read_by_dirty(id) { + want_sr.push(idx); + } + } else if dirty { + want_solve.push(idx); + } else if read_by_dirty(id) { + want_sf.push(idx); + } + } + let sorted = |v: &[u32]| { + let mut v = v.to_vec(); + v.sort_unstable(); + v.dedup(); + v + }; + for (what, got, want) in [ + ("solve_frames", sorted(solve_frames), sorted(&want_solve)), + ("sweep_regions", sorted(sweep_regions), sorted(&want_sweep)), + ("seed_frames", sorted(seed_frames), sorted(&want_sf)), + ("seed_regions", sorted(seed_regions), sorted(&want_sr)), + ] { + assert!( + got == want, + "WOW_LAYOUT_VERIFY: walking the dirty set chose a different {what} than walking the \ + graph — {} vs {} entries. A node's dependencies are its anchors' targets plus a \ + region's owner; if `LayoutScope::edge` gained a fourth source, the selection pass \ + needs it too.", + got.len(), + want.len(), + ); + } +} + /// Per-node layout scope, so a resolve touches only what moved. Indexed densely by layout id, as /// frames and regions share one counter (`Model::next_id`); nothing allocates on a steady frame. #[derive(Default)] @@ -186,6 +276,10 @@ pub(crate) struct LayoutScope { dep_free: Vec, /// Per id: in the dirty closure. dirty: Vec, + /// The same set as a list, so a pass can walk the dirty nodes instead of every id. Written + /// wherever [`Self::dirty`] turns true, so the two cannot disagree; no duplicates, because + /// every writer goes through the false→true edge. + dirty_ids: Vec, /// The closure's worklist. stack: Vec, /// The cached frame roster, `(handle, id)`, kept across resolves. @@ -219,6 +313,7 @@ impl LayoutScope { self.dep_head.resize(n, u32::MAX); self.dirty.clear(); self.dirty.resize(n, false); + self.dirty_ids.clear(); self.dep_to.clear(); self.dep_next.clear(); self.dep_free.clear(); @@ -232,6 +327,7 @@ impl LayoutScope { fn begin_incremental(&mut self) { self.dirty.clear(); self.dirty.resize(self.node_of.len(), false); + self.dirty_ids.clear(); self.stack.clear(); self.staged.clear(); } @@ -314,6 +410,7 @@ impl LayoutScope { if !*d { *d = true; self.stack.push(id); + self.dirty_ids.push(id); } } } @@ -332,9 +429,12 @@ impl LayoutScope { /// Mark every node dirty, for what a per-node diff cannot see (a death, a screen move). fn dirty_all(&mut self) { + self.dirty_ids.clear(); for (id, &n) in self.node_of.iter().enumerate() { if n != u32::MAX { self.dirty[id] = true; + #[allow(clippy::cast_possible_truncation)] + self.dirty_ids.push(id as u32); } } } @@ -683,10 +783,10 @@ impl UiScript { if full_scope { scope.dirty_all(); } else { - for (id, &n) in scope.node_of.iter().enumerate() { - if n != u32::MAX && scope.now[id] != scope.last[id] { - scope.stack.push(id as u32); - scope.dirty[id] = true; + for id in 0..scope.node_of.len() { + if scope.node_of[id] != u32::MAX && scope.now[id] != scope.last[id] { + #[allow(clippy::cast_possible_truncation)] + scope.seed(id as u32); } } scope.close(); @@ -698,38 +798,83 @@ impl UiScript { let mut sweep_regions: Vec = Vec::new(); let mut seed_frames: Vec = Vec::new(); let mut seed_regions: Vec = Vec::new(); - // Whether a node in the closure reads `id`; a rect nothing dirty reads is not seeded. - let read_by_dirty = |scope: &LayoutScope, id: u32| -> bool { - let mut e = scope.dep_head[id as usize]; - while e != u32::MAX { - if scope.dirty[scope.dep_to[e as usize] as usize] { - return true; + // `[layout-pre]` ends at the gate and `frame_us`/`region_us` start at the round loop, so + // the walk between them — every id in the graph, not every *dirty* id — was in neither + // line. That gap is why an 18 us `GetLeft` looked like a 3 us resolve. + let t_seedsel = layout_prof_enabled().then(std::time::Instant::now); + // Walk out from the dirty set rather than across the graph. + // + // This used to ask **every id** "is one of your dependents dirty?" — 18 203 frames and + // 42 284 regions on the player's interface — for a dirty closure whose steady-state median + // is 2 frames and 4 regions. It also sat between `[layout-pre]`'s last lap and the round + // loop's first, so neither report charged it, which is how a `GetLeft` that measured + // 17.5–18.4 us a call live (`WOW_UI_VERBS`, six a frame through stock `MouseIsOver`) could + // force a resolve the profiler called 3 us. + // + // The four lists come out the same **sets**, by the same relation read the other way + // round: a clean node is seeded iff some dirty node reads it, and a node's dependencies + // are exactly the three `scope.edge` arguments in the roster walk — its anchors' targets, + // plus a region's owner frame, whose scale it reads. `WOW_LAYOUT_VERIFY=1` re-derives the + // whole graph and compares every rect, which is what would catch an enumeration that + // drifted from `edge`'s. + // + // Sets, not sequences: both rosters are built from `HashMap` iteration (`frame_to_id` for + // frames, `region_data` for regions), so no pass here ever had a canonical order, and the + // region sweep is a fixpoint whose round cap covers any chain — order moves `rounds`, not + // rects. Only the seed lists are sorted, to drop the duplicates a shared dependency would + // otherwise re-`set_external` on every round. + // A full-scope pass has no seeds to find: every node is dirty, so every `seed_dep` would + // return early. Skip the walk rather than make 60 000 calls that do nothing. + let seeds_possible = !full_scope; + for &did in &scope.dirty_ids { + let n = match scope.node_of.get(did as usize) { + Some(&n) if n != u32::MAX => n, + _ => continue, + }; + let idx = n & !REGION_TAG; + if n & REGION_TAG != 0 { + sweep_regions.push(idx); + // The frame solve runs before the sweep, so a dirty region is seeded as well. + seed_regions.push(idx); + if !seeds_possible { + continue; } - e = scope.dep_next[e as usize]; - } - false - }; - for id in 0..scope.node_of.len() { - let n = scope.node_of[id]; - if n == u32::MAX { - continue; - } - let (idx, is_region, dirty) = (n & !REGION_TAG, n & REGION_TAG != 0, scope.dirty[id]); - #[allow(clippy::cast_possible_truncation)] - let id32 = id as u32; - if is_region { - if dirty { - sweep_regions.push(idx); - seed_regions.push(idx); - } else if read_by_dirty(scope, id32) { - seed_regions.push(idx); + let row = scope.regions[idx as usize]; + if let Some(d) = region_data.get(&row.rh) { + for a in &d.anchors { + seed_dep(scope, a.relative_to, &mut seed_frames, &mut seed_regions); + } } - } else if dirty { + if let Some(&owner_id) = frame_to_id.get(&row.owner) { + seed_dep(scope, owner_id, &mut seed_frames, &mut seed_regions); + } + } else { solve_frames.push(idx); - } else if read_by_dirty(scope, id32) { - seed_frames.push(idx); + if !seeds_possible { + continue; + } + let (h, _) = scope.plan[idx as usize]; + if let Some(input) = layout_inputs.get(&h) { + for a in &input.anchors { + seed_dep(scope, a.relative_to, &mut seed_frames, &mut seed_regions); + } + } } } + seed_frames.sort_unstable(); + seed_frames.dedup(); + seed_regions.sort_unstable(); + seed_regions.dedup(); + let seedsel_us = t_seedsel.map_or(0, |t| t.elapsed().as_micros()); + if layout_verify_enabled() { + verify_selection( + scope, + &solve_frames, + &sweep_regions, + &seed_frames, + &seed_regions, + ); + } // This solve's width, beside `layout_solves` (how often) and `layout_rounds` (how deep). *layout_last_scope = (solve_frames.len(), sweep_regions.len()); let round_cap = scope.plan.len() + region_data.len() + 2; @@ -892,7 +1037,8 @@ impl UiScript { // `anchored` is the sweep's roster, `solved` and `swept` this pass's scope. eprintln!( "[layout-prof] rounds={} frames={} regions_total={} anchored={} \ - solved={} swept={} scope={} regions_swept={} frame_us={} region_us={}", + solved={} swept={} scope={} regions_swept={} seedsel_us={seedsel_us} \ + frame_us={} region_us={}", round + 1, scope.plan.len(), region_data.len(),