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 a95e81e4..86b5cb5b 100644 --- a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs +++ b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs @@ -798,6 +798,11 @@ fn tick_bench() { // Every event `ui_quest`, `ui_quest_log` and `ui_quest_share` fire, plus the two the // reference fires beside them on a turn-in, in the order a turn-in produces them. for event in [ + "WORLD_MAP_UPDATE", + "GOSSIP_SHOW", + "GOSSIP_CLOSED", + "TRAINER_SHOW", + "TRADE_SKILL_SHOW", "QUEST_DETAIL", "QUEST_PROGRESS", "QUEST_COMPLETE", @@ -849,17 +854,26 @@ fn tick_bench() { // Measured 2026-10-21 on grom's own 92-addon interface, three fires each: // // us/fire worst event - // 5068 14759 QUEST_DETAIL <- HCQuest.lua:139 took 14,097 us of ONE fire - // 764 1324 QUEST_LOG_UPDATE - // 662 1564 QUEST_FINISHED + // 7539 22094 QUEST_DETAIL <- HCQuest.lua:139 took 16,498 us of ONE fire + // 1047 2759 QUEST_FINISHED + // 834 1284 QUEST_LOG_UPDATE + // 455 905 TRADE_SKILL_SHOW + // 228 545 GOSSIP_SHOW + // - - WORLD_MAP_UPDATE, GOSSIP_CLOSED, TRAINER_SHOW: below the top rows // - // **Read `us/fire` against `worst` before believing either.** QUEST_DETAIL's 14.8 ms is + // **Read `us/fire` against `worst` before believing either.** QUEST_DETAIL's 22.1 ms is // one fire of three; the other two cost ~0.1 ms, so it is HCQuest building something once - // and keeping it, not a cost a player pays per quest. With that read correctly, **no quest - // event costs more than ~1.5 ms recurring here** — which means this bench does not - // reproduce the 57-73 ms `feed_quest` spikes of the live session, and says so rather than - // offering a number that looks like an answer. The bench's quest log is empty and pfQuest - // has no real route to recompute; the live `[ui-events]` row is what will attribute it. + // and keeping it, not a cost a player pays per quest. With that read correctly, **no event + // here costs more than ~1.5 ms recurring**. + // + // The five non-quest events are the ones grom's 2026-10-21 `[ui-events]` named live, at + // 45-72 ms a fire and 12-38 ms for `WORLD_MAP_UPDATE`. They cost ~0.2-0.5 ms here, and the + // gap is the point: this harness has no NPC in front of it, no gossip options, no trainer + // list, no open world map and an empty quest log, so the handlers that spend the live + // milliseconds find nothing to do and return. **This bench does not reproduce a + // panel-show or map-repaint cost**, and it says so rather than offering a number that + // looks like an answer. Fire these to catch a regression in what the dispatch itself + // costs; read the live `[ui-events]` row for what the fan-out costs a player. } // ── What one addon bug costs a frame ──────────────────────────────────────────────────── diff --git a/crates/benilla-app/src/game_plugins.rs b/crates/benilla-app/src/game_plugins.rs index 759dcbd1..25cf8036 100644 --- a/crates/benilla-app/src/game_plugins.rs +++ b/crates/benilla-app/src/game_plugins.rs @@ -1371,8 +1371,6 @@ pub(crate) mod schedule_tests { "`MSG_TALENT_WIPE_CONFIRM` answers the trainer's gossip option"), ("ui_taxi/mod.rs", "feed_taxi", Because::PlayerRoundTrip, "both packets follow talking to a flight master"), - ("ui_tradeskill.rs", "drain_trade_skill", Because::FilledByVm, - "every consumed queue is Lua's; the cast-event continuation is inert until a Lua `DoTradeSkill` latched a count"), ("ui_trainer/mod.rs", "feed_trainer", Because::PlayerRoundTrip, "a refusal answers a Train click and a list answers a trainer gossip click; the edges ride `VmMemo`s"), ]; diff --git a/crates/benilla-app/src/ui_script/verb_event_gate.rs b/crates/benilla-app/src/ui_script/verb_event_gate.rs index 87bb3860..e20a7cc9 100644 --- a/crates/benilla-app/src/ui_script/verb_event_gate.rs +++ b/crates/benilla-app/src/ui_script/verb_event_gate.rs @@ -49,27 +49,6 @@ const ELSEWHERE: &[(&str, &str, &str)] = &[ "QUEST_LOG_UPDATE", "benilla-app/src/ui_quest_log.rs", ), - // `trade_skill_touched`, set by all four verbs, drained by `take_trade_skill_touched`. - ( - "CollapseTradeSkillSubClass", - "TRADE_SKILL_UPDATE", - "benilla-app/src/ui_tradeskill.rs", - ), - ( - "ExpandTradeSkillSubClass", - "TRADE_SKILL_UPDATE", - "benilla-app/src/ui_tradeskill.rs", - ), - ( - "SetTradeSkillInvSlotFilter", - "TRADE_SKILL_UPDATE", - "benilla-app/src/ui_tradeskill.rs", - ), - ( - "SetTradeSkillSubClassFilter", - "TRADE_SKILL_UPDATE", - "benilla-app/src/ui_tradeskill.rs", - ), // `repeat_count` moved (`repeat_changed`). ( "DoTradeSkill", diff --git a/crates/benilla-app/src/ui_tradeskill.rs b/crates/benilla-app/src/ui_tradeskill.rs index ac5ae0bf..38bda86c 100644 --- a/crates/benilla-app/src/ui_tradeskill.rs +++ b/crates/benilla-app/src/ui_tradeskill.rs @@ -478,12 +478,12 @@ fn drain_trade_skill( } } - // A filter or expand/collapse fires `TRADE_SKILL_UPDATE` from inside the reference's C call - // (`0x4fd710`, `0x4fd750`); ours is the touched flag, answered the same frame. - let touched = script.take_trade_skill_touched(); - if touched && open.line.is_some() { - script.fire_event("TRADE_SKILL_UPDATE", vec![]); - } + // A filter or expand/collapse fires `TRADE_SKILL_UPDATE` from inside its own call, as the + // reference's C helper does (`benilla_ui::script::tradeskill::api::signal_trade_skill_update`). + // It used to be a flag drained here and answered the next frame, which is a different event: + // the addon guard that swallows the re-entrant signal had cleared by then, so the library + // rescanned, its rescan called the verb, and the two ran each other at one fire a frame for as + // long as the window was open. if script.take_trade_skill_close() { debug!("ui_tradeskill: client-side close (no packet)"); diff --git a/crates/benilla-app/tests/tradeskill_frame.rs b/crates/benilla-app/tests/tradeskill_frame.rs index c13dee1a..e941ddae 100644 --- a/crates/benilla-app/tests/tradeskill_frame.rs +++ b/crates/benilla-app/tests/tradeskill_frame.rs @@ -92,12 +92,9 @@ fn state() -> TradeSkillState { } } -/// What the app does after a list mutator: drain the touched flag and fire `TRADE_SKILL_UPDATE`. -fn pump(script: &mut UiScript) { - if script.take_trade_skill_touched() { - script.fire_event("TRADE_SKILL_UPDATE", vec![]); - } -} +/// Nothing: a list mutator fires `TRADE_SKILL_UPDATE` inside its own call now, as the reference's +/// helper does. Kept at the call sites so each one still reads as "and then the window repaints". +fn pump(_script: &mut UiScript) {} #[test] fn collapse_all_tab_and_filter_dropdowns_work_end_to_end() { diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 6707b9fb..5be506d0 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -974,9 +974,6 @@ pub(crate) struct Model { pub(crate) trade_skill_last_line: u32, /// The selected recipe's spell id (`0xbde044`), which carries it across reopen and re-push. pub(crate) trade_skill_selected_spell: u32, - /// A filter or subclass collapse changed: `TRADE_SKILL_UPDATE` (`0x13a`) is owed this frame, as - /// the client fires it inside the call (`0x4fd710`/`0x4fd730`/`0x4fd750`, `0x4fd180`). - pub(crate) trade_skill_touched: bool, /// The open craft window's recipes. pub(crate) craft: Option, @@ -1878,7 +1875,6 @@ impl Model { trade_skill_invslot_mask: u32::MAX, trade_skill_last_line: 0, trade_skill_selected_spell: 0, - trade_skill_touched: false, craft: None, craft_dos: Vec::new(), craft_selection: 0, diff --git a/crates/benilla-ui/src/script/tradeskill/api.rs b/crates/benilla-ui/src/script/tradeskill/api.rs index 194aa18e..6e3499ef 100644 --- a/crates/benilla-ui/src/script/tradeskill/api.rs +++ b/crates/benilla-ui/src/script/tradeskill/api.rs @@ -19,6 +19,27 @@ fn opt_str(lua: &Lua, s: Option<&String>) -> mlua::Result { }) } +/// `TRADE_SKILL_UPDATE`, dispatched **now**, as the one helper all four list mutators tail-call +/// does (`0x4fd710`, `0x4fd730`, `0x4fd750` -> `reference/1.12-verb-events.tsv`). +/// +/// Inline is the whole point, not a detail. These four verbs are called *from* a +/// `TRADE_SKILL_UPDATE` handler — a profession library reads the list by expanding every header, +/// walking it and folding back what was folded — so the signal is re-entrant by construction, and +/// every addon that does it carries its own guard against that: an in-progress flag, or a scan +/// counter. Those guards are written against an inline signal, and only an inline signal lands +/// inside the window they cover. +/// +/// Queued for the next frame instead, the signal arrives after the guard has cleared, the library +/// scans again, its scan calls these verbs again, and the two sides keep each other alive at one +/// fire a frame for as long as the window is open. grom's 2026-10-21 session had exactly that: +/// `[ui-events]` reporting `TRADE_SKILL_UPDATE · 1.01 fires/f` across 37 consecutive windows — +/// three unbroken minutes — at 0.58-0.84 ms a frame in handlers over ~900 calls a window, four +/// addons rebuilding their recipe lists every frame for nothing. It is the 118-second and +/// 61-second stretches at 55-70 fps in a session whose median was 86. +fn signal_trade_skill_update(lua: &Lua) { + crate::script::tick::fire_event_into(lua, "TRADE_SKILL_UPDATE", Vec::new()); +} + /// Register the tradeskill globals. pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { let g = lua.globals(); @@ -177,18 +198,22 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { g.set( "ExpandTradeSkillSubClass", lua.create_function(|lua, id: usize| { - let mut model = lua.app_data_mut::().expect("model app_data"); - set_collapsed(&mut model, id, false); - model.trade_skill_touched = true; + { + let mut model = lua.app_data_mut::().expect("model app_data"); + set_collapsed(&mut model, id, false); + } + signal_trade_skill_update(lua); Ok(()) })?, )?; g.set( "CollapseTradeSkillSubClass", lua.create_function(|lua, id: usize| { - let mut model = lua.app_data_mut::().expect("model app_data"); - set_collapsed(&mut model, id, true); - model.trade_skill_touched = true; + { + let mut model = lua.app_data_mut::().expect("model app_data"); + set_collapsed(&mut model, id, true); + } + signal_trade_skill_update(lua); Ok(()) })?, )?; @@ -332,8 +357,8 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { // The subclass and inv-slot filters (Blizzard_TradeSkillUI.lua:314-414): index i is 1-based // into `GetTradeSkillSubClasses` (`0x4ffc70` bounds-checks it) or `GetTradeSkillInvSlots`, and // 0 is all. `Set*(i, 1, 1)` shows only i, as every menu click does; `Set*(i, 1)` shows i and - // `Set*(i, 0)` hides it. Each set fires TRADE_SKILL_UPDATE through the touched flag - // (`0x4fd710`), and `Get*(0)` asks whether everything is shown. The reference's per-header + // `Set*(i, 0)` hides it. Each set fires TRADE_SKILL_UPDATE inline (`0x4fd710`, see + // [`signal_trade_skill_update`]), and `Get*(0)` asks whether everything is shown. The reference's per-header // subclass flag (`+0xc`, mask `0x84dd60`) is kept here as hidden group keys. g.set( "GetTradeSkillSubClassFilter", @@ -361,33 +386,35 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { "SetTradeSkillSubClassFilter", lua.create_function( |lua, (index, on, exclusive): (usize, Option, Option)| { - let mut model = lua.app_data_mut::().expect("model app_data"); - let Some(t) = model.trade_skill.as_ref() else { - return Ok(()); - }; - let keys: Vec<(u32, u32)> = build_groups(&t.recipes) - .into_iter() - .map(|g| g.key) - .collect(); - let on = on.unwrap_or(1) != 0; - let exclusive = exclusive.unwrap_or(0) != 0; - match index.checked_sub(1) { - None => model.trade_skill_subclass_hidden.clear(), - Some(n) => { - let Some(&key) = keys.get(n) else { - return Ok(()); - }; - if on && exclusive { - model.trade_skill_subclass_hidden = - keys.iter().copied().filter(|&k| k != key).collect(); - } else if on { - model.trade_skill_subclass_hidden.remove(&key); - } else { - model.trade_skill_subclass_hidden.insert(key); + { + let mut model = lua.app_data_mut::().expect("model app_data"); + let Some(t) = model.trade_skill.as_ref() else { + return Ok(()); + }; + let keys: Vec<(u32, u32)> = build_groups(&t.recipes) + .into_iter() + .map(|g| g.key) + .collect(); + let on = on.unwrap_or(1) != 0; + let exclusive = exclusive.unwrap_or(0) != 0; + match index.checked_sub(1) { + None => model.trade_skill_subclass_hidden.clear(), + Some(n) => { + let Some(&key) = keys.get(n) else { + return Ok(()); + }; + if on && exclusive { + model.trade_skill_subclass_hidden = + keys.iter().copied().filter(|&k| k != key).collect(); + } else if on { + model.trade_skill_subclass_hidden.remove(&key); + } else { + model.trade_skill_subclass_hidden.insert(key); + } } } } - model.trade_skill_touched = true; + signal_trade_skill_update(lua); Ok(()) }, )?, @@ -436,30 +463,32 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { "SetTradeSkillInvSlotFilter", lua.create_function( |lua, (index, on, exclusive): (usize, Option, Option)| { - let mut model = lua.app_data_mut::().expect("model app_data"); - if model.trade_skill.is_none() { - return Ok(()); - } - let bits = present_inv_slots(&model); - let on = on.unwrap_or(1) != 0; - let exclusive = exclusive.unwrap_or(0) != 0; - // The mask each path writes, as `0x4fd730` is called. - match index.checked_sub(1) { - None => model.trade_skill_invslot_mask = u32::MAX, - Some(n) => { - let Some(&bit) = bits.get(n) else { - return Ok(()); - }; - if on && exclusive { - model.trade_skill_invslot_mask = 1 << bit; - } else if on { - model.trade_skill_invslot_mask |= 1 << bit; - } else { - model.trade_skill_invslot_mask &= !(1 << bit); + { + let mut model = lua.app_data_mut::().expect("model app_data"); + if model.trade_skill.is_none() { + return Ok(()); + } + let bits = present_inv_slots(&model); + let on = on.unwrap_or(1) != 0; + let exclusive = exclusive.unwrap_or(0) != 0; + // The mask each path writes, as `0x4fd730` is called. + match index.checked_sub(1) { + None => model.trade_skill_invslot_mask = u32::MAX, + Some(n) => { + let Some(&bit) = bits.get(n) else { + return Ok(()); + }; + if on && exclusive { + model.trade_skill_invslot_mask = 1 << bit; + } else if on { + model.trade_skill_invslot_mask |= 1 << bit; + } else { + model.trade_skill_invslot_mask &= !(1 << bit); + } } } } - model.trade_skill_touched = true; + signal_trade_skill_update(lua); Ok(()) }, )?, diff --git a/crates/benilla-ui/src/script/tradeskill/mod.rs b/crates/benilla-ui/src/script/tradeskill/mod.rs index 81f04d9a..f357c8d8 100644 --- a/crates/benilla-ui/src/script/tradeskill/mod.rs +++ b/crates/benilla-ui/src/script/tradeskill/mod.rs @@ -169,12 +169,4 @@ impl super::UiScript { pub fn take_trade_skill_close(&mut self) -> bool { std::mem::take(&mut self.model_mut().trade_skill_close) } - - /// Whether a filter or fold changed since the last drain. The reference re-sorts and fires - /// `TRADE_SKILL_UPDATE` (event `0x13a`) inside the call (`0x4fd710`, `0x4fd730`, `0x4fd750`); - /// the app fires it the same frame, and the stock filter and collapse-all clicks repaint only - /// off that event. - pub fn take_trade_skill_touched(&mut self) -> bool { - std::mem::take(&mut self.model_mut().trade_skill_touched) - } } diff --git a/crates/benilla-ui/src/script/tradeskill/tests.rs b/crates/benilla-ui/src/script/tradeskill/tests.rs index 0a9ab929..e3fe91fc 100644 --- a/crates/benilla-ui/src/script/tradeskill/tests.rs +++ b/crates/benilla-ui/src/script/tradeskill/tests.rs @@ -721,25 +721,90 @@ fn one_hand_weapon_spans_both_hand_slots() { ); } -/// Each filter or fold raises the flag the app answers with TRADE_SKILL_UPDATE (`0x4fd710`, -/// `0x4fd750`). +/// Each filter or fold fires TRADE_SKILL_UPDATE **inside its own call** (`0x4fd710`, `0x4fd730`, +/// `0x4fd750`), not on a flag for the next frame: `fired` is read with the verb's `run` still on +/// the stack, so a deferred signal fails this. #[test] -fn filter_and_fold_mutators_raise_the_touched_flag() { +fn each_filter_and_fold_mutator_fires_trade_skill_update_inline() { let mut s = UiScript::new().unwrap(); s.set_trade_skill(Some(state())); - assert!(!s.take_trade_skill_touched()); + s.run( + r#" + fired = 0 + local f = CreateFrame("Frame") + f:RegisterEvent("TRADE_SKILL_UPDATE") + f:SetScript("OnEvent", function() fired = fired + 1 end) + "#, + ) + .unwrap(); + assert_eq!(s.eval::("return fired").unwrap(), 0); - s.run("SetTradeSkillSubClassFilter(2, 1, 1)").unwrap(); - assert!(s.take_trade_skill_touched()); - assert!(!s.take_trade_skill_touched(), "drained"); + for (n, verb) in [ + "SetTradeSkillSubClassFilter(2, 1, 1)", + "SetTradeSkillInvSlotFilter(0, 1, 1)", + "CollapseTradeSkillSubClass(0)", + "ExpandTradeSkillSubClass(0)", + ] + .iter() + .enumerate() + { + s.run(verb).unwrap(); + assert_eq!( + s.eval::("return fired").unwrap(), + n as i64 + 1, + "{verb} did not fire TRADE_SKILL_UPDATE in its own call" + ); + } +} - s.run("SetTradeSkillInvSlotFilter(0, 1, 1)").unwrap(); - assert!(s.take_trade_skill_touched()); +/// The loop this inlining exists to prevent, in the shape the live one had. +/// +/// A profession library reads the recipe list from its own `TRADE_SKILL_UPDATE` handler by +/// unfolding every header and folding back what was folded, and guards the re-entrant signal that +/// makes with an in-progress flag (`LibCraftingProfessions-1.0.lua:555`, `:829`). Fired inline the +/// signal lands inside that flag's window and is swallowed, so one real update is one scan. Queued +/// for the next frame it lands after the flag has cleared: the library scans again, the scan calls +/// the verb again, and neither side ever stops — `1.01 fires/f` for as long as the window is open. +#[test] +fn a_librarys_own_reentrancy_guard_ends_the_cascade_in_one_frame() { + let mut s = UiScript::new().unwrap(); + s.set_trade_skill(Some(state())); + s.run( + r#" + scans = 0 + local scanning = false + local f = CreateFrame("Frame") + f:RegisterEvent("TRADE_SKILL_UPDATE") + f:SetScript("OnEvent", function() + if scanning then return end + scanning = true + scans = scans + 1 + -- What the library's scan does: unfold everything, read, fold back. + ExpandTradeSkillSubClass(0) + CollapseTradeSkillSubClass(1) + SetTradeSkillSubClassFilter(0, 1, 1) + scanning = false + end) + "#, + ) + .unwrap(); - s.run("CollapseTradeSkillSubClass(0)").unwrap(); - assert!(s.take_trade_skill_touched()); - s.run("ExpandTradeSkillSubClass(0)").unwrap(); - assert!(s.take_trade_skill_touched()); + s.fire_event("TRADE_SKILL_UPDATE", vec![]); + assert_eq!( + s.eval::("return scans").unwrap(), + 1, + "the re-entrant signals should be swallowed by the guard, not counted" + ); + + // And nothing is owed to a later frame: ticking does not start another scan. + for _ in 0..10 { + s.tick(1.0 / 60.0); + } + assert_eq!( + s.eval::("return scans").unwrap(), + 1, + "one real update must be one scan, not one a frame for as long as the window is open" + ); } /// ItemLevel (`record+0x14`) sorts between tier and name.