From 0c862838da5899fafbc080ab1c0f60d176be00f4 Mon Sep 17 00:00:00 2001 From: grom Date: Tue, 20 Oct 2026 20:57:15 -0600 Subject: [PATCH] ui: a SetScale names its subtree instead of giving up on the graph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[ui-phases]`'s new write-site tally answered the combat-fps question on its first armed session: 463 whole-graph layout derivations across 34 windows, and **232 of them** at `object/frame_state.rs:427` — the `touch_layout()` inside `SetScale`. Splitting the per-window count by site and regressing against resolve time over 43 in-world windows, resolve correlates with `SetScale` derivations at r = +0.996 and with every other derivation at r = -0.041. The four costliest windows of the run are 36-64 `SetScale` derivations each and almost nothing else. That also corrects the flat "~15.5 ms per derivation" of the round before: a mint-triggered derivation costs ~1.1 ms, a `SetScale`-triggered one 12-13 ms. The difference is that `SetScale` gave up *two* ledgers, not one. The caller is pfUI's action-button press animation (`pfUI/modules/actionbar.lua:182`, `:207`, `:233`, `:259`): four styles that all run `this:SetScale(this:GetScale() + fade)` on `OnUpdate` for the ~60 frames a button's fade lasts. Pressing an ability starts it, which is why a settled interface never reproduced this and a fight always did. Both give-ups had precise versions already, written for `SetParent`: - `touch_layout()` -> `touch_layout_subtree_scale(h)`. A scale write moves the subtree's `effective_scale` and nothing else: anchor targets are ids fixed at `SetPoint`, so no edge moves and the roster is unchanged. - `touch_measure_all()` -> the same walk now names the subtree's regions on the measure ledger. This was the bigger half. `measure_dirty = None` makes the next `fontstrings_needing_measure` walk all ~43,000 regions of a real interface to issue, typically, zero requests; `stale_measure_key` reads only a region's own data plus *its owner frame's* `effective_scale`, so the subtree's regions are exactly the ones that can go stale. Measured on the player's own 92-addon interface, 43,195 regions, one `SetScale` per frame for 240 frames (`tick_bench`'s new `[scale]` row): before changing resolve 0.601 ms/f · 0.996 derivations/f (239/240) after changing resolve 0.005 ms/f · 0.000 derivations/f ( 0/240) control same resolve 0.005 ms/f · 0.000 derivations/f ( 0/240) `changing` is now indistinguishable from a control that writes the value it already holds. The harness understates the live cost (its measure answers are a 6px-per-char stub), so the live saving is to be confirmed in a play-test, not predicted from that row. Correctness is left to `WOW_LAYOUT_VERIFY`, which `cfg!(test)` forces on for all 1975 benilla-ui tests: it re-derives the whole graph after every `resolve()` and panics on any rect the incremental pass got wrong, and runs the full measure roster beside the ledger drain and panics on any stale region the ledger missed. So the two new tests assert only that the cheap path was taken, on a graph three frames deep with a FontString on the leaf. The real-interface verify run fails identically to its documented pre-existing red (`frames 13379 -> 13379, regions 29597 -> 29645`), so this change adds nothing to it. Also repairs an assert message in `layout_gate.rs` whose line continuations had been eaten by a heredoc, leaving runs of spaces mid-sentence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/addon_harness/tick_bench_tests.rs | 103 +++++++++++++++ crates/benilla-ui/src/script/model.rs | 26 +++- .../src/script/object/frame_state.rs | 12 +- .../src/script/tests/layout_gate.rs | 125 +++++++++++++++++- 4 files changed, 257 insertions(+), 9 deletions(-) diff --git a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs index 3034af52..bdf365b2 100644 --- a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs +++ b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs @@ -670,6 +670,109 @@ fn tick_bench() { // `[ui-phases]` site tally is what names the real one. } + // ── What one `SetScale` a frame costs ─────────────────────────────────────────────────── + // + // This is the one the live site tally named. `[ui-phases]`'s per-write-site breakdown put + // **232 of 463** whole-graph derivations in grom's 2026-10-20 session at + // `object/frame_state.rs:427` — the `touch_layout()` inside `SetScale` — and the four + // costliest windows of the run were 36 to 64 of them each, nothing else. + // + // The caller is pfUI's action-button press animation + // (`pfUI/modules/actionbar.lua:182`, `:207`, `:233`, `:259`): while a button's animation is + // live its `OnUpdate` runs `this:SetScale(this:GetScale() + fade)` **every frame** for the + // ~60 frames the alpha takes to reach zero. Every one of those calls changes the scale, so + // every one gives up naming, so every one makes the next resolve derive all ~44,000 regions. + // Pressing an ability is what starts it, which is why it is a combat problem. + // + // Two windows, identical but for the value written: `changing` moves the scale every frame, + // `same` writes the value it already holds, so `set_scale`'s `changed` guard is false and no + // layout write happens at all. The difference is the derivation and nothing else — same + // frame, same graph, same binding, same call count. + { + s.run( + "if not BenillaScaleProbe then \ + BenillaScaleProbe = CreateFrame(\"Frame\", \"BenillaScaleProbe\", UIParent) \ + BenillaScaleProbe:SetWidth(36) \ + BenillaScaleProbe:SetHeight(36) \ + BenillaScaleProbe:SetPoint(\"CENTER\", UIParent, \"CENTER\", 0, 0) \ + local t = BenillaScaleProbe:CreateTexture(nil, \"OVERLAY\") \ + t:SetAllPoints(BenillaScaleProbe) \ + end", + ) + .expect("probe frame"); + let mut row = |label: &str, changing: bool| { + // Settle: the probe's own mint has to be spent before the window, or it pays for it. + for _ in 0..30 { + feed_units(&mut s, Company::Grouped); + s.sync_nameplates(GEOMETRY, &plates); + s.tick(1.0 / 60.0); + answer_measures(&mut s); + s.resolve(); + } + let _ = s.take_errors(); + let _ = s.take_layout_derive_sites(); + let derives = s.layout_derivations(); + let (mut resolve_ms, mut worst_ms) = (0.0, 0.0_f64); + let mut measures = 0usize; + for frame in 0..FRAMES { + feed_units(&mut s, Company::Grouped); + s.sync_nameplates(GEOMETRY, &plates); + // pfUI's own step is `30/GetFramerate()*0.05`; a twenty-frame ramp keeps the + // value in the range a button animation actually walks through. + let scale = if changing { + 1.0 + 0.01 * f64::from(frame % 20) + } else { + 1.0 + }; + s.run(&format!("BenillaScaleProbe:SetScale({scale})")) + .expect("set scale"); + s.tick(1.0 / 60.0); + let b = Instant::now(); + measures += answer_measures(&mut s); + s.resolve(); + let this = ms(b); + resolve_ms += this; + worst_ms = worst_ms.max(this); + } + let derives = s.layout_derivations() - derives; + println!( + "[scale] {label:<8} resolve {:.3} ms/f (worst frame {worst_ms:.1} ms) · \ + {:.3} derivations/f ({derives} in {FRAMES} frames) · {:.0} measures/f{}", + resolve_ms / per, + f64::from(u32::try_from(derives).unwrap_or(u32::MAX)) / per, + per_derive(u64::try_from(measures).unwrap_or(u64::MAX)) / per, + if derives > 0 { + format!(" → {:.2} ms each", resolve_ms / per_derive(derives)) + } else { + String::new() + }, + ); + for (site, n) in s.take_layout_derive_sites().iter().take(4) { + println!("[scale] {label:<8} {n:5} from {site}"); + } + }; + row("same", false); + row("changing", true); + // Measured 2026-10-20 on the player's own 92-addon interface, 43,195 regions: + // + // before changing resolve 0.601 ms/f (worst 1.2 ms) · 0.996 derivations/f (239/240) + // after changing resolve 0.005 ms/f (worst 0.0 ms) · 0.000 derivations/f ( 0/240) + // control same resolve 0.005 ms/f (worst 0.0 ms) · 0.000 derivations/f ( 0/240) + // + // 239 of 240 frames derived the whole graph, and the site tally named + // `object/frame_state.rs:427` for every one of them. The fix was two narrowings at that + // one site, both of which already existed for `SetParent`'s sake: name the subtree on the + // layout ledger (`Model::touch_layout_subtree_scale`) instead of giving up, and name the + // subtree's regions on the measure ledger instead of dropping it. The second mattered + // more than the first here — a dropped measure ledger makes the next + // `fontstrings_needing_measure` walk all 43,195 regions to issue zero requests, which is + // why `measures/f` reads 0 in both rows while the cost was 120x apart. + // + // `changing` is now indistinguishable from `same`, which is the point: the window still + // does the same 240 `SetScale` calls and still moves the rects, it just no longer pays + // for the whole interface to do it. + } + // ── What one addon bug costs a frame ──────────────────────────────────────────────────── // // mlua's protected call installs a message handler that, for every stack level, searches the diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 95a76781..6707b9fb 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -2103,9 +2103,17 @@ impl Model { } } - /// A `SetParent` under `root`: only the subtree's scale moves, as anchor targets are ids fixed - /// at `SetPoint`. Addon pools reparent hundreds of frames a redraw, so this stays precise. - pub(crate) fn touch_layout_reparent(&mut self, root: FrameHandle) { + /// A write that moved the **effective scale** of `root`'s subtree and nothing else: a + /// `SetParent`, or a `SetScale` on `root` itself. Anchor targets are ids fixed at `SetPoint`, + /// so no edge moves and the roster is unchanged — only each node's scale input. + /// + /// Both callers are hot and for the same reason: addon pools reparent hundreds of frames a + /// redraw, and pfUI's action-button press animation walks a button overlay's scale a step + /// every frame for the ~60 frames its fade lasts. Giving up naming for either one costs a + /// whole-graph derivation per call — 232 of the 463 in one measured session, and the four + /// costliest five-second windows of that run were almost nothing else. See the `[scale]` row + /// in `benilla-app`'s `tick_bench`. + pub(crate) fn touch_layout_subtree_scale(&mut self, root: FrameHandle) { let mut subtree = vec![root]; let mut at = 0; while at < subtree.len() { @@ -2119,11 +2127,23 @@ impl Model { let Model { arena, layout_inputs, + measure_dirty, .. } = self; for &h in &subtree { if let Some(f) = arena.frame(h) { layout_inputs.entry(h).or_default().scale = f.effective_scale; + // The owner's effective scale is in every one of its FontStrings' measure keys + // (`layout::stale_measure_key`), and it is the *only* thing outside a region's own + // data that key reads — so the subtree's regions are exactly the ones that can go + // stale, and the ledger does not have to be dropped for them. A `None` ledger + // makes the next `fontstrings_needing_measure` walk all 43,000 regions of a real + // interface to issue, typically, no requests at all. The walk is kept honest by + // the verify build, which runs it beside the drain and panics on any stale region + // the ledger missed. + if let Some(list) = measure_dirty.as_mut() { + list.extend(f.regions.iter().copied()); + } } } for h in subtree { diff --git a/crates/benilla-ui/src/script/object/frame_state.rs b/crates/benilla-ui/src/script/object/frame_state.rs index 6c88bf31..79864066 100644 --- a/crates/benilla-ui/src/script/object/frame_state.rs +++ b/crates/benilla-ui/src/script/object/frame_state.rs @@ -361,8 +361,7 @@ pub(super) fn install(lua: &Lua, m: &Table) -> mlua::Result<()> { let shown = model.arena.reparent_finish(h, new_parent, was_visible); // A reparent moves the subtree's effective scale, a layout input and part of every // descendant FontString's measure key. - model.touch_layout_reparent(h); - model.touch_measure_all(); + model.touch_layout_subtree_scale(h); shown }; event::fire_visibility_changes(lua, shown); @@ -424,9 +423,12 @@ pub(super) fn install(lua: &Lua, m: &Table) -> mlua::Result<()> { let changed = model.arena.frame(h).is_some_and(|f| f.scale != scale); model.arena.set_scale(h, scale); if changed { - model.touch_layout(); - // The owner's effective scale is in every descendant FontString's measure key. - model.touch_measure_all(); + // Only the subtree's scale moved, which is the same invalidation a `SetParent` + // makes, so name the subtree rather than giving up on the graph. This used to be + // a bare `touch_layout()`, and an addon that walks a frame's scale every frame — + // pfUI's button press animation does, for ~60 frames a keypress — bought a + // whole-graph derivation per frame with it. + model.touch_layout_subtree_scale(h); } Ok(()) })?, diff --git a/crates/benilla-ui/src/script/tests/layout_gate.rs b/crates/benilla-ui/src/script/tests/layout_gate.rs index 57c6cb7e..6ce78f7e 100644 --- a/crates/benilla-ui/src/script/tests/layout_gate.rs +++ b/crates/benilla-ui/src/script/tests/layout_gate.rs @@ -562,6 +562,129 @@ fn a_derivation_names_the_write_site_and_not_the_bookkeeper() { mixed.iter().map(|(site, _)| site.as_str()).collect(); assert!( distinct.len() >= 2, - "two different writes should name two different sites, got {distinct:?} — every row on one line means a `#[track_caller]` is missing from the naming chain and the tally is reporting its own bookkeeper" + "two different writes should name two different sites, got {distinct:?} — every row on \ + one line means a `#[track_caller]` is missing from the naming chain and the tally \ + is reporting its own bookkeeper" ); } + +/// How many times the graph has been derived from scratch. +fn derives(s: &UiScript) -> u64 { + s.lua() + .app_data_ref::() + .expect("model app_data") + .layout_derives +} + +/// A `SetScale` names its subtree instead of giving up on the whole graph. +/// +/// It used to give up. `[ui-phases]`'s per-write-site tally put **232 of the 463** whole-graph +/// derivations in grom's 2026-10-20 session on `SetScale`'s own `touch_layout()`, and the four +/// costliest five-second windows of that run were 36 to 64 of them and almost nothing else. The +/// caller is pfUI's action-button press animation, which walks a button overlay's scale one step +/// per frame for the ~60 frames its fade lasts (`pfUI/modules/actionbar.lua:182`) — so pressing an +/// ability bought a derivation of all ~44,000 regions every frame until it faded out. +/// +/// A scale write is the same invalidation a `SetParent` makes: the subtree's `effective_scale` +/// moves, no anchor edge moves and the roster is unchanged, so `touch_layout_subtree_scale` names +/// it. The graph here is three frames deep with a FontString on the deepest, because the derive +/// syncs *every* live frame's scale into `layout_inputs` and a region reads its owner frame's — a +/// narrowing that stopped at the first child would resolve this correctly and prove nothing. +/// +/// That the rects are still **right** is not asserted here: `WOW_LAYOUT_VERIFY` is forced on in +/// this crate's tests and re-derives the whole graph after every `resolve()` below, panicking on +/// any rect the incremental pass got wrong. This test owns the other half — that the cheap path +/// was taken at all. +#[test] +fn scaling_a_frame_names_its_subtree_and_does_not_derive_the_graph() { + let mut s = script(); + s.set_screen_size(800.0, 600.0); + setup(&s); + s.run( + r#" + grand = CreateFrame("Frame", "Grand", child) + grand:SetWidth(10); grand:SetHeight(10) + grand:SetPoint("TOPLEFT", child, "BOTTOMLEFT", 0, 0) + label = grand:CreateFontString("Label", "ARTWORK") + label:SetPoint("TOPLEFT", grand, "TOPLEFT", 2, -2) + "#, + ) + .expect("deep setup"); + s.resolve(); + s.resolve(); // settle: a ledger is live, and the gate is closed + let _ = s.take_layout_derive_sites(); // the first take is what arms the tally + let (was_epoch, was_solves, was_derives) = (epoch(&s), solves(&s), derives(&s)); + + s.run("Parent:SetScale(1.5)").expect("scale the parent"); + s.resolve(); + + assert_eq!( + derives(&s), + was_derives, + "a scale write should name its subtree, not derive the graph" + ); + assert!( + s.take_layout_derive_sites().is_empty(), + "and the site tally should have nothing to report — a row here names the line that gave up" + ); + // Not vacuous: the write has to have registered and a resolve has to have run, or a change + // gate that simply dropped the write would pass the two assertions above. + assert!( + epoch(&s) > was_epoch, + "the scale write must still open the gate" + ); + assert!( + solves(&s) > was_solves, + "and the fixpoint must still run: nothing was skipped, only named" + ); + assert!( + (s.eval::("return Grand:GetEffectiveScale()") + .expect("effective scale") + - 1.5) + .abs() + < 1e-6, + "the scale must have reached the subtree's leaves" + ); + + // A second step, the shape the animation actually has: one write per frame, each one changing + // the value, every one of them staying off the derive path. + for step in 1..=5 { + let was = derives(&s); + s.run(&format!( + "Parent:SetScale({})", + 1.5 + f64::from(step) * 0.01 + )) + .expect("step the scale"); + s.resolve(); + assert_eq!( + derives(&s), + was, + "step {step} of a scale animation derived the graph" + ); + } +} + +/// Writing the scale a frame already has is not a layout write at all. +/// +/// `set_scale`'s `changed` guard is what makes the hot path cheap for the many addons that +/// re-apply a configured scale on every `OnUpdate` without reading it first. The `[scale] same` +/// row in `benilla-app`'s `tick_bench` is the same claim measured on the player's own interface. +#[test] +fn rewriting_the_same_scale_touches_nothing() { + let mut s = script(); + s.set_screen_size(800.0, 600.0); + setup(&s); + s.run("Parent:SetScale(1.25)").expect("scale once"); + s.resolve(); + s.resolve(); // settle + let (was_epoch, was_solves, was_derives) = (epoch(&s), solves(&s), derives(&s)); + + for _ in 0..10 { + s.run("Parent:SetScale(1.25)").expect("rewrite the scale"); + s.resolve(); + } + + assert_eq!(epoch(&s), was_epoch, "an unchanged scale is not a write"); + assert_eq!(solves(&s), was_solves, "so no resolve should have run"); + assert_eq!(derives(&s), was_derives, "and nothing should have derived"); +}