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"); +}