From e6ae2e0f67abec8bcabe0644c0081326afa05d99 Mon Sep 17 00:00:00 2001 From: grom Date: Wed, 21 Oct 2026 17:46:55 -0600 Subject: [PATCH] ui: a scroll names the child it moved instead of giving up on the graph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SetVerticalScroll` and `SetHorizontalScroll` called `Model::touch_layout` on the line before `reanchor_child`. That gave up naming for the whole graph and then *erased* the precise touch that followed: `reanchor_child` re-anchors the scroll child through `write_anchor`, which reads an offsets-only change (same point, same target) and names the child node — but `touch_layout_node` will not un-say an imprecise touch, so the give-up won on every call. Naming the child is the whole truth here. A scroll offset reaches the layout only as that one anchor: anchor targets are ids fixed at `SetPoint`, so no edge moves and the roster is unchanged, and the clip a scroll child draws within is the ScrollFrame's resolved rect rather than the offset — `clip::scroll_clip_sources` rebuilds it per call and caches nothing. Measured on grom's 2026-10-21 17:08 session, which was a map-and-panel run. These two lines were **472 of the run's 806 whole-graph derivations**, the largest single source by a factor of two, and the cost lands on the Lua that reads a frame edge afterwards rather than on the pass's own `resolve` phase: | | n | fps | UI pass | `frame:GetLeft` | |---|---|---|---|---| | windows with >= 10 scroll derivations | 12 | 68.5 (min 53) | 5.93 ms/f | 2.18 ms/f | | windows with no derivations at all | 11 | 92.4 (min 77) | 1.62 ms/f | 0.05 ms/f | `r(scroll derivations, GetLeft us/f) = +0.907` over 57 in-play windows, against `+0.014` for every other derivation site. `[ui-verbs]` has `frame:GetLeft` at 94-376 us a call in those windows and 0.6-1.5 us in the quiet ones, on ~14 calls a frame, because stock FrameXML reads frame edges constantly and each read past the clean epoch re-derived. That is why a scrolling panel — the world map, the quest log, a profession list — was the expensive thing on screen. The test scrolls both axes on the shipped interface and asserts zero derivations, with a positive control so it cannot pass on a dead counter. Put one `touch_layout` back and it reads exactly 10 for 10 scrolls. `WOW_LAYOUT_VERIFY=1` over the real 92-addon interface gives `frames 13379 -> 13379, regions 29597 -> 29645` — identical to the recorded pre-existing baseline, so this narrowing introduced nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/ui_script/resolve_bench.rs | 76 +++++++++++++++++++ crates/benilla-ui/src/script/scrollframe.rs | 24 +++++- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/crates/benilla-app/src/ui_script/resolve_bench.rs b/crates/benilla-app/src/ui_script/resolve_bench.rs index 5dda4de7..38a609ca 100644 --- a/crates/benilla-app/src/ui_script/resolve_bench.rs +++ b/crates/benilla-app/src/ui_script/resolve_bench.rs @@ -383,6 +383,82 @@ fn a_region_moving_every_frame_costs_no_graph_derivation_on_the_shipped_ui() { ); } +/// Scrolling a panel costs zero derivations: the offset reaches the layout only as the scroll +/// child's anchor, which `write_anchor` names, so the two setters owe no conservative touch of +/// their own. +/// +/// This is the regression test for the largest single derivation source yet measured on a live +/// session — `SetVerticalScroll`/`SetHorizontalScroll` called `Model::touch_layout` on the line +/// before `reanchor_child`, which gave up naming the whole graph and then swallowed the precise +/// touch that followed. 472 of 806 derivations in grom's 2026-10-21 17:08 run, and the reason a +/// scrolling panel dragged `frame:GetLeft` from ~1 us a call to 94-376 us. +/// +/// Both axes, because they were byte-clones of each other and a fix to one is easy to forget. +#[test] +fn scrolling_a_panel_costs_no_graph_derivation_on_the_shipped_ui() { + benilla_formats::wow_data_or_skip!(); + let mut s = settled_default_ui(); + s.run( + r#" + BenchScroll = CreateFrame("ScrollFrame", "BenchScroll", UIParent) + BenchScroll:SetPoint("CENTER", 0, 0) + BenchScroll:SetWidth(300); BenchScroll:SetHeight(200) + BenchScrollChild = CreateFrame("Frame", "BenchScrollChild", BenchScroll) + BenchScrollChild:SetWidth(300); BenchScrollChild:SetHeight(900) + BenchScroll:SetScrollChild(BenchScrollChild) + -- A row anchored inside the child, so the scroll really does move something that resolves. + BenchScrollRow = BenchScrollChild:CreateFontString(nil, "OVERLAY", "GameFontNormal") + BenchScrollRow:SetPoint("TOPLEFT", BenchScrollChild, "TOPLEFT", 4, -4) + BenchScrollRow:SetText("row") + bench_scroll_n = 0 + function bench_scroll_frame() + bench_scroll_n = bench_scroll_n + 1 + BenchScroll:SetVerticalScroll(bench_scroll_n * 7) + BenchScroll:SetHorizontalScroll(bench_scroll_n * 2) + -- The read stock FrameXML does dozens of times a frame, and what forces the resolve. + local _ = BenchScrollRow:GetLeft() + end + "#, + ) + .unwrap(); + + // Positive control: the births above move the roster, so settling them must derive. Without + // this the assertion below passes just as well on a counter that never moves. + let born_at = s.layout_derivations(); + for _ in 0..6 { + s.run("bench_scroll_frame()").unwrap(); + app_frame_ticked(&mut s, 1.0 / 144.0); + } + assert!( + s.layout_derivations() > born_at, + "creating the ScrollFrame, its child and a FontString must derive the graph — a birth \ + moves the roster. Reading zero here means `layout_derivations` never moves and the \ + assertion below is vacuous." + ); + + let derives_before = s.layout_derivations(); + let walks_before = s.layout_gate_walks(); + for _ in 0..10 { + s.run("bench_scroll_frame()").unwrap(); + app_frame_ticked(&mut s, 1.0 / 144.0); + } + let derives = s.layout_derivations() - derives_before; + let walks = s.layout_gate_walks() - walks_before; + assert!( + walks >= 10, + "the frames must still be reaching the gate at all — {walks} walks over 10 frames means \ + this test stopped exercising the path it is guarding" + ); + assert_eq!( + derives, 0, + "10 frames of scrolling derived the layout graph {derives} times, i.e. about one per \ + scroll. A scroll offset moves exactly one thing — the scroll child's anchor offsets, \ + same point and same target — so `write_anchor` names the child and nothing else needs \ + saying. A derivation here means a setter has gone back to the conservative \ + `Model::touch_layout`, which also erases the precise touch that follows it." + ); +} + /// A tooltip content change solves a tooltip-sized scope on the full shipped UI, gated on counts, /// not milliseconds, which machine state skews. #[test] diff --git a/crates/benilla-ui/src/script/scrollframe.rs b/crates/benilla-ui/src/script/scrollframe.rs index 6a408dae..e6183efb 100644 --- a/crates/benilla-ui/src/script/scrollframe.rs +++ b/crates/benilla-ui/src/script/scrollframe.rs @@ -152,6 +152,26 @@ fn subtree_font_strings(model: &Model, f: FrameHandle) -> Vec mlua::Result<()> { let h = frame_handle_of(lua, this)?; let mut model = lua.app_data_mut::().expect("model app_data"); @@ -250,7 +270,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { if !changed { return Ok(()); } - lua.app_data_mut::().expect("model").touch_layout(); + // No `touch_layout` here: see [`reanchor_child`], which names the one node that moved. reanchor_child(lua, &this)?; fire_scroll(lua, &this, "OnVerticalScroll", px) })?, @@ -283,7 +303,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { if !changed { return Ok(()); } - lua.app_data_mut::().expect("model").touch_layout(); + // No `touch_layout` here either: see [`reanchor_child`]. reanchor_child(lua, &this)?; fire_scroll(lua, &this, "OnHorizontalScroll", px) })?,