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 95808877..7d27e9e7 100644 --- a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs +++ b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs @@ -1242,11 +1242,26 @@ fn hover_frame(s: &mut UiScript, company: Company, plates: &[PlateFeed], rebuild /// Two shapes, because the guess was that they would differ: a crowded pull re-derives the layout /// graph every frame anyway (four plates a frame are strangers), so a rebuild's own dirtying /// looked like it would be partly already paid, where a player standing in a city has a settled -/// graph and would pay for the whole resolve. **They do not differ.** Measured on grom's -/// interface the rebuild costs 6.63 ms/frame under forty churning plates and 6.78 ms/frame with -/// none up, against whole-frame baselines of 1.84 ms and 0.85 ms — so the cost is the plate's own -/// line walk and the hooks hanging off it, flat, and not the graph it dirties. Both windows are -/// kept because that is the finding. +/// graph and would pay for the whole resolve. They did not differ — 6.63 ms/frame under forty +/// churning plates and 6.78 ms with none up, against whole-frame baselines of 1.84 and 0.85 — and +/// I read that as the cost being the plate's own line walk and the hooks hanging off it, flat, +/// rather than the graph it dirties. +/// +/// **That reading was wrong, and `WOW_UI_VERBS` said so in one run.** The rebuilt frame's cost was +/// one row: `frame:GetLeft`, **3 calls a frame at 2188 us each — 6.56 of the 6.6 ms**, inside +/// `UIParent:OnUpdate`. `GetLeft` forces a resolve, and the resolve was a whole-graph derivation +/// because each of OctoPawn's two `OnShow` frame mints gave up naming the graph; the two shapes +/// agreed because *both* were paying the full derivation, not because neither was. +/// +/// Fixed by appending a just-minted frame to the cached graph as a leaf +/// (`LayoutScope::append_frame_leaf`, with `layout_gate::minting_a_frame_appends_a_leaf_rather +/// _than_deriving_the_graph` for why that is sound). Now: **the rebuild costs -0.11 ms/frame +/// under forty plates and +0.14 ms with none up**, from 6.63 and 6.78, and the rebuilt windows +/// report no derivation sites at all. Whole-frame totals 1.60 and 0.91 ms. +/// +/// The lesson for the next one of these: the two-shape comparison was a fair experiment that +/// happened to be blind to its own answer. A per-verb profile over the same window would have +/// named it immediately, and it is one env var. /// /// The live asset probes are installed and report 0.001 ms/frame, and the plate re-measures no /// text (0.1 requests a frame, the same held or rebuilt): neither the patch chain nor the font @@ -1266,9 +1281,8 @@ fn hover_frame(s: &mut UiScript, company: Company, plates: &[PlateFeed], rebuild /// the number created (137 against 480 here), so Lua reclaims most of them; it is a cost per /// raise, not an unbounded leak. /// -/// Kept as instrumentation rather than acted on: the lever would be letting a brand-new leaf node -/// join the graph without a full derive, which is a layout-engine change and wants its own -/// measurement. +/// Acted on: that lever — letting a brand-new leaf node join the graph without a full derive — is +/// what this bench's numbers above are now measuring the absence of. #[test] #[ignore = "bench — needs WOW_DATA and the player's AddOns; run explicitly with --release"] fn a_mouseover_plate_rebuilt_every_frame() { diff --git a/crates/benilla-ui/src/script/layout.rs b/crates/benilla-ui/src/script/layout.rs index 6c8af115..39fd99a2 100644 --- a/crates/benilla-ui/src/script/layout.rs +++ b/crates/benilla-ui/src/script/layout.rs @@ -89,7 +89,7 @@ const NO_NODE: u64 = 0; /// A frame's per-node hash; the full derive and the incremental pass must agree bit for bit. #[inline] -fn frame_node_hash(input: &LayoutInput) -> u64 { +pub(crate) fn frame_node_hash(input: &LayoutInput) -> u64 { let mut node = InputFingerprint::default(); node.input(input); node.finish() @@ -446,15 +446,82 @@ impl LayoutScope { self.last_screen = Some(screen); } - /// [`Self::commit`] for an incremental pass: only the re-hashed slots. `last_count` and - /// `last_screen` stay, since a structural change always takes the full path. + /// [`Self::commit`] for an incremental pass: only the re-hashed slots. `last_screen` stays, + /// since a screen move always takes the full path. + /// + /// `last_count` moves with the slots that go from "no hash" to one, which is how + /// [`Self::append_frame_leaf`] keeps the next full pass's node-death test honest: that test is + /// `matched != last_count` over the ids whose `last` is not [`NO_NODE`], so a node introduced + /// on this path has to be counted here or the pass after it reads a death that never happened + /// and takes full scope for nothing. fn commit_incremental(&mut self) { for &(id, h) in &self.staged { - self.last[id as usize] = h; + let slot = &mut self.last[id as usize]; + if *slot == NO_NODE && h != NO_NODE { + self.last_count += 1; + } + *slot = h; } self.staged.clear(); } + /// Append a brand-new frame to the cached graph as a **leaf**, so a frame minted between two + /// resolves does not cost a whole-graph derivation. `false` means the caller must give up + /// naming after all. + /// + /// Sound only for a frame whose id was *just* minted, which is the one caller + /// ([`Model::frame_id`]): **no anchor anywhere can name an id that did not exist a moment + /// ago**, so nothing in the cached graph reads this node and no existing edge has to move. + /// What it reads is its own anchors' targets, and `targets` carries them, because a mint is + /// not always a creation — `frame_id` mints on the first *fetch*, which for a frame built in + /// Rust (a nameplate, a chat bubble) is the first time Lua asks for its wrapper, long after + /// its anchors were written. + /// + /// `last` is left at [`NO_NODE`] so the incremental seed sees the hash move and seeds the + /// node; `commit_incremental` then counts it. + /// + /// The payoff is per tooltip raise, and it is most of one: an addon that owns a one-shot timer + /// by minting `CreateFrame("Frame")` — the 1.12 way to write `C_Timer.After`, and what + /// OctoPawn does twice on every `GameTooltip` `OnShow` — dirtied the whole graph each time, + /// and `UIParent:OnUpdate`'s three `GetLeft` calls then each paid a full derivation: 2.19 ms + /// a call, 6.56 ms of a 6.6 ms rebuilt-hover frame on grom's interface. + pub(crate) fn append_frame_leaf( + &mut self, + h: FrameHandle, + id: u32, + hash: u64, + targets: &[u32], + ) -> bool { + // No cached graph to append to: the next resolve derives in full anyway. + if self.last.is_empty() || self.node_of.is_empty() { + return false; + } + let i = id as usize; + if i >= self.node_of.len() { + self.last.resize(i + 1, NO_NODE); + self.now.resize(i + 1, NO_NODE); + self.node_of.resize(i + 1, u32::MAX); + self.dep_head.resize(i + 1, u32::MAX); + self.dirty.resize(i + 1, false); + } + // Already a node, or a slot `last` remembers: not a fresh leaf, so this is not our case. + if self.node_of[i] != u32::MAX || self.last[i] != NO_NODE { + return false; + } + #[allow(clippy::cast_possible_truncation)] + { + self.node_of[i] = self.plan.len() as u32; + } + self.plan.push((h, id)); + self.now[i] = hash; + // What this node reads. An edge to an id with no slot is dropped by `edge` itself, which + // is right: an id that is not a node can never turn dirty, so it constrains nothing. + for &to in targets { + self.edge(id, to); + } + true + } + /// Forget the memory, so the next resolve derives the graph in full scope. pub(crate) fn invalidate(&mut self) { self.last_count = 0; diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 9c33baa2..6757e184 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -2387,8 +2387,61 @@ impl Model { self.next_id += 1; self.frame_to_id.insert(h, id); self.id_to_frame.insert(id, h); - // Every creation path mints here, so a new frame always opens the layout gate. - self.touch_layout(); + // Every creation path mints here, so a new frame always opens the layout gate — but as a + // **leaf**, which is cheap, rather than by giving up naming, which costs a whole-graph + // derivation. A frame whose id was minted this instant has no anchors of its own yet and + // cannot be the target of anybody else's, so it reads nothing and nothing reads it; the + // cached roster, edges and hashes all stand with it appended. + // + // What that is worth: an addon owning a one-shot timer by minting `CreateFrame("Frame")` + // (the 1.12 way to write `C_Timer.After`) used to dirty the whole graph per mint, and + // `UIParent:OnUpdate`'s `GetLeft` calls then each paid a derivation — 6.56 ms of a 6.6 ms + // rebuilt-hover frame on grom's interface, three calls at 2.19 ms each. + // + // The scale and clamp a derivation would have synced are written here for the same reason: + // only a full derivation reads them off the arena, so an appended node would otherwise + // carry a default 1.0 scale under a scaled parent until the next one. + let (scale, clamp) = self + .arena + .frame(h) + .map(|f| (f.effective_scale, f.clamped_to_screen)) + .unwrap_or((1.0, false)); + let (extent_x, extent_y) = (self.screen.right, self.screen.top); + let input = self.layout_inputs.entry(h).or_default(); + input.scale = scale; + input.clamp = clamp; + if clamp { + input.extent_x = extent_x; + input.extent_y = extent_y; + } + // Refuse the append when the frame already carries regions, because a region is a graph + // node **without needing a minted id**: the full derivation walks `region_data` and mints + // region ids inside that walk, where a frame is a node only once its id exists. So a + // frame whose regions were built before its id was fetched has rows the cached roster + // cannot gain by appending one frame, and only a derivation can add them. + // + // It costs nothing where it matters: the mint that was expensive is the bare + // `CreateFrame("Frame")` an addon uses to own a one-shot `OnUpdate`, which has no regions + // at all. A plate, a bubble or anything off a template keeps the full-derivation path. + // + // (This guard is not what `WOW_LAYOUT_VERIFY`'s 48-region shortfall over the real + // interface is about. That red is pre-existing and documented — reproduced identically + // with this whole change stashed, `frames 13544 -> 13544, regions 29481 -> 29529` either + // way — and adding the guard moved neither number.) + let has_regions = self.arena.frame(h).is_some_and(|f| !f.regions.is_empty()); + let hash = crate::script::layout::frame_node_hash(input); + // A mint is not always a creation: `frame_id` mints on the first *fetch* too, and a frame + // built in Rust (a nameplate, a chat bubble) is first fetched when Lua asks for its + // wrapper, with its anchors long written. So the append is handed what this node reads. + let targets: Vec = input.anchors.iter().map(|a| a.relative_to).collect(); + if self.layout_touched.is_some() + && !has_regions + && self.layout_scope.append_frame_leaf(h, id, hash, &targets) + { + self.touch_layout_node(id); + } else { + self.touch_layout(); + } id } diff --git a/crates/benilla-ui/src/script/tests/layout_gate.rs b/crates/benilla-ui/src/script/tests/layout_gate.rs index 6ce78f7e..73c12d06 100644 --- a/crates/benilla-ui/src/script/tests/layout_gate.rs +++ b/crates/benilla-ui/src/script/tests/layout_gate.rs @@ -512,10 +512,15 @@ fn a_derivation_names_the_write_site_and_not_the_bookkeeper() { s.take_layout_derive_sites().is_empty(), "unarmed, the tally has nothing to say" ); + // A **region** mint, which still gives up naming: a new region can join the resolve's + // external set. Creating a *frame* no longer does — a just-minted frame is appended to the + // cached graph as a leaf (`LayoutScope::append_frame_leaf`), which is the whole point of that + // change — so a frame mint is no longer a write that can exercise this tally. s.run( r#" f = CreateFrame("Frame", "Named", nil); f:SetWidth(10); f:SetHeight(10) f:SetPoint("TOPLEFT", nil, "TOPLEFT", 0, 0) + art = f:CreateTexture(nil, "ARTWORK"); art:SetAllPoints() "#, ) .expect("setup"); @@ -523,7 +528,7 @@ fn a_derivation_names_the_write_site_and_not_the_bookkeeper() { let rows = s.take_layout_derive_sites(); assert!( !rows.is_empty(), - "creating a frame gives up naming — if this is empty the tally is not being written, and \ + "a region mint gives up naming — if this is empty the tally is not being written, and \ every reading taken from it is a false negative" ); for (site, n) in &rows { @@ -548,10 +553,13 @@ fn a_derivation_names_the_write_site_and_not_the_bookkeeper() { // `#[track_caller]` anywhere in the chain does not empty the tally — it collapses every row // onto `give_up_naming`'s own line, so the tally looks healthy and says nothing. Two triggers // that give up naming from two different lines must therefore read as two rows: minting a - // frame id and minting a region id are different writes and the report has to tell them apart. + // region id and a region's first anchor — which joins it to the graph — are different writes + // and the report has to tell them apart. (Minting a *frame* id used to be one of the pair; it + // appends a leaf now and names it, which is why the frame step below is only scaffolding.) for step in [ r#"g = CreateFrame("Frame", "Another", nil)"#, r#"fs = Another:CreateFontString(nil, "ARTWORK")"#, + r#"fs:SetPoint("TOPLEFT", nil, "TOPLEFT", 1, -1)"#, ] { s.run(step).expect("trigger"); s.resolve(); @@ -576,6 +584,115 @@ fn derives(s: &UiScript) -> u64 { .layout_derives } +/// **Minting a frame appends a leaf instead of deriving the whole graph**, and the rects it leaves +/// behind are the ones a full derivation would have produced. +/// +/// This was the tooltip-raise cost, nearly whole. An addon that wants a one-shot delay in 1.12 +/// writes `CreateFrame("Frame")` and hangs an `OnUpdate` on it — there is no `C_Timer.After` — and +/// OctoPawn does exactly that twice on every `GameTooltip` `OnShow` +/// (`Scoring/ScoringCore.lua:540`, `UI/Paperdoll/Pdmo.lua:101`). Each mint gave up naming, so +/// `UIParent:OnUpdate`'s three `GetLeft` calls each paid a derivation of all ~19,000 frames and +/// ~44,000 regions: **2.19 ms a call, 6.56 ms of a 6.6 ms frame** on grom's interface +/// (`benilla-app`'s `tick_bench`, `[ui-verbs]` `frame:GetLeft`). After this it is 0.14 ms, and the +/// rebuilt window reports no derivations at all. +/// +/// Why a leaf is sound: the id was minted this instant, so **no anchor anywhere can name it** — +/// nothing in the cached graph reads the new node and no cached edge has to move. What it reads is +/// its own anchors' targets, which the append is handed, because a mint is not always a creation: +/// `frame_id` mints on the first *fetch* too, and a frame built in Rust (a nameplate, a chat +/// bubble) is first fetched when Lua asks for its wrapper, with its anchors long written. That +/// case is the second half of this test. +#[test] +fn minting_a_frame_appends_a_leaf_rather_than_deriving_the_graph() { + let mut s = script(); + s.run( + r#"root = CreateFrame("Frame", "Root", nil) + root:SetWidth(100) root:SetHeight(50) + root:SetPoint("TOPLEFT", nil, "TOPLEFT", 10, -10)"#, + ) + .expect("a graph to append to"); + s.resolve(); + s.resolve(); + let before = derives(&s); + + // A bare mint, the `C_Timer.After` shape: no anchors, no art, just a frame to hang an + // `OnUpdate` on. + s.run(r#"timer = CreateFrame("Frame")"#).expect("mint"); + s.resolve(); + assert_eq!( + derives(&s), + before, + "minting a frame must not derive the graph from scratch" + ); + // And the graph it was appended to still answers, which is what the append must not break. + assert_eq!( + s.eval::("return Root:GetLeft()").expect("root left"), + 10.0 + ); + + // A mint that is a *fetch*: a frame whose anchors were written before Lua ever asked for its + // id. The anchor is to `Root`, so the new node reads a node already in the graph and the + // append has to carry that edge — without it, moving `Root` would leave this one behind. + let anchored = { + let mut model = s.lua().app_data_mut::().expect("model app_data"); + let h = model + .arena + .create(crate::widget::FrameKind::Frame, Some("Late".into()), None); + let root = model.arena.lookup("Root").expect("Root"); + let root_id = model.frame_id(root); + let input = model.layout_inputs.entry(h).or_default(); + input.width = 20.0; + input.height = 20.0; + input.anchors = vec![crate::layout::Anchor::new( + crate::layout::Point::TopLeft, + root_id, + crate::layout::Point::TopRight, + 0.0, + 0.0, + )]; + h + }; + s.resolve(); + s.resolve(); + let before = derives(&s); + // The first fetch of its id, which is the mint: an append with one edge. + let late_id = { + let mut model = s.lua().app_data_mut::().expect("model app_data"); + model.frame_id(anchored) + }; + assert!(late_id > 0); + s.resolve(); + assert_eq!( + derives(&s), + before, + "a first fetch of an already-anchored frame's id must append, not derive" + ); + // Read off the model, not `_G`: a frame built in the arena is never published under a name, + // which is exactly the kind this half of the test is about. + let left_of = |s: &UiScript, h| { + s.lua() + .app_data_ref::() + .expect("model app_data") + .resolved + .get(&h) + .map(|r| r.left) + }; + assert_eq!( + left_of(&s, anchored), + Some(110.0), + "the appended node resolved against its anchor target: Root's left 10 + width 100" + ); + // The edge is live: moving the target must move the appended node too. + s.run(r#"Root:SetPoint("TOPLEFT", nil, "TOPLEFT", 30, -10)"#) + .expect("move Root"); + s.resolve(); + assert_eq!( + left_of(&s, anchored), + Some(130.0), + "without the appended edge this would still read 110" + ); +} + /// 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