perf(layout): the resolve's seed pass walked the graph, not the change
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn
This commit is contained in:
Claude Opus 5
@@ -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<u32>, seed_regions: &mut Vec<u32>) {
|
||||
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<u32>,
|
||||
/// Per id: in the dirty closure.
|
||||
dirty: Vec<bool>,
|
||||
/// 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<u32>,
|
||||
/// The closure's worklist.
|
||||
stack: Vec<u32>,
|
||||
/// 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<u32> = Vec::new();
|
||||
let mut seed_frames: Vec<u32> = Vec::new();
|
||||
let mut seed_regions: Vec<u32> = 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(),
|
||||
|
||||
Reference in New Issue
Block a user