perf(engine): eliminate O(N^2) mana sweep in legality probes on go-wide boards - #4479
Conversation
…de boards
Declaring attackers (and every priority decision) was slow on big mana-token
boards (turn-40 Cryptolith-Rite squirrel state: 759 battlefield objects, 701
squirrels each granted {T}: Add). Profiling showed the cost was NOT the combat
AI or the attacker scan (both O(N), clone-free) but the generic legal-actions
path:
- derive_display_state's board-global mana sweep called is_blocked_by_cant_be
_activated + is_blocked_by_cant_activate_during per mana source, each a full
battlefield_active_statics scan => O(N^2) (~1.1M static iters per sweep).
- SimulationFilter clones the full state and runs apply (which finalizes display
state, i.e. the sweep) per candidate => ~2821 sweeps for one declare decision.
Fix A: hoist a ManaActivationGates existence gate once per sweep (mirrors
combat::CombatStaticGates) and skip the per-source scans when no such static
exists. O(N^2) -> O(N).
Fix B: SimulationFilter only reads .is_ok() on a discarded clone, so apply via
PublicFinalizeMode::DeferredDisplay (apply_as_current_for_legality) -- display
derivation is irrelevant to legality and is skipped entirely.
Measured (debug): generic declare path 921s -> 144s; per-priority decision
491ms -> 191ms (sim filter 270ms -> 30ms); mana sweeps in the filter path
2821 -> 0. Adds declare_attackers_bench dev tool.
Guards: apply_for_legality_skips_display_mana_sweep (Fix B),
can_activate_mana_ability_now_respects_cant_activate_during_via_gate (Fix A
gate=true correctness).
There was a problem hiding this comment.
Code Review
This pull request optimizes the performance of the board-global mana-availability sweep on go-wide boards by hoisting activation-prohibition static checks into a pre-computed ManaActivationGates struct and introducing a legality-only simulation path (apply_as_current_for_legality) that skips display state derivation. It also adds a benchmark tool to profile declare-attackers performance. The review feedback highlights a violation of Repository Style Guide Rule R2, recommending the replacement of bool fields in ManaActivationGates with a typed GateStatus enum for better type safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #[derive(Debug, Clone, Copy)] | ||
| pub struct ManaActivationGates { | ||
| has_cant_be_activated: bool, | ||
| has_cant_activate_during: bool, | ||
| } | ||
|
|
||
| impl ManaActivationGates { | ||
| /// One `game_functioning_statics` sweep computing both presence flags. | ||
| pub fn compute(state: &GameState) -> Self { | ||
| let mut gates = ManaActivationGates { | ||
| has_cant_be_activated: false, | ||
| has_cant_activate_during: false, | ||
| }; | ||
| for (_, def) in super::functioning_abilities::game_functioning_statics(state) { | ||
| match def.mode { | ||
| crate::types::statics::StaticMode::CantBeActivated { .. } => { | ||
| gates.has_cant_be_activated = true | ||
| } | ||
| crate::types::statics::StaticMode::CantActivateDuring { .. } => { | ||
| gates.has_cant_activate_during = true | ||
| } | ||
| _ => {} | ||
| } | ||
| if gates.has_cant_be_activated && gates.has_cant_activate_during { | ||
| break; | ||
| } | ||
| } | ||
| gates | ||
| } | ||
| } |
There was a problem hiding this comment.
According to Repository Style Guide R2, bool fields should not be used on structs to represent state or design space. Instead, we should parameterize with a typed enum (e.g., GateStatus) to carry the same information with more meaning and type safety.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum GateStatus {
Allowed,
Prohibited,
}
#[derive(Debug, Clone, Copy)]
pub struct ManaActivationGates {
cant_be_activated: GateStatus,
cant_activate_during: GateStatus,
}
impl ManaActivationGates {
/// One game_functioning_statics sweep computing both presence flags.
pub fn compute(state: &GameState) -> Self {
let mut gates = ManaActivationGates {
cant_be_activated: GateStatus::Allowed,
cant_activate_during: GateStatus::Allowed,
};
for (_, def) in super::functioning_abilities::game_functioning_statics(state) {
match def.mode {
crate::types::statics::StaticMode::CantBeActivated { .. } => {
gates.cant_be_activated = GateStatus::Prohibited;
}
crate::types::statics::StaticMode::CantActivateDuring { .. } => {
gates.cant_activate_during = GateStatus::Prohibited;
}
_ => {}
}
if gates.cant_be_activated == GateStatus::Prohibited && gates.cant_activate_during == GateStatus::Prohibited {
break;
}
}
gates
}
}References
- R2. No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums like ControllerRef, Comparator, PlayerScope, Option, and dedicated discriminated unions instead. (link)
| if gates.has_cant_be_activated | ||
| && super::casting::is_blocked_by_cant_be_activated(state, player, source_id, ability_def) | ||
| { | ||
| return false; | ||
| } | ||
| // CR 602.5 + CR 117.1b: CantActivateDuring blocks activation this turn. | ||
| if super::casting::is_blocked_by_cant_activate_during(state, player, ability_def) { | ||
| if gates.has_cant_activate_during | ||
| && super::casting::is_blocked_by_cant_activate_during(state, player, ability_def) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Update the gated checks to use the new GateStatus enum instead of the bool fields, adhering to Repository Style Guide R2.
| if gates.has_cant_be_activated | |
| && super::casting::is_blocked_by_cant_be_activated(state, player, source_id, ability_def) | |
| { | |
| return false; | |
| } | |
| // CR 602.5 + CR 117.1b: CantActivateDuring blocks activation this turn. | |
| if super::casting::is_blocked_by_cant_activate_during(state, player, ability_def) { | |
| if gates.has_cant_activate_during | |
| && super::casting::is_blocked_by_cant_activate_during(state, player, ability_def) | |
| { | |
| return false; | |
| } | |
| if gates.cant_be_activated == GateStatus::Prohibited | |
| && super::casting::is_blocked_by_cant_be_activated(state, player, source_id, ability_def) | |
| { | |
| return false; | |
| } | |
| // CR 602.5 + CR 117.1b: CantActivateDuring blocks activation this turn. | |
| if gates.cant_activate_during == GateStatus::Prohibited | |
| && super::casting::is_blocked_by_cant_activate_during(state, player, ability_def) | |
| { | |
| return false; | |
| } |
References
- R2. No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums like ControllerRef, Comparator, PlayerScope, Option, and dedicated discriminated unions instead. (link)
Declaring attackers (and every priority decision) was slow on big mana-token
boards (turn-40 Cryptolith-Rite squirrel state: 759 battlefield objects, 701
squirrels each granted {T}: Add). Profiling showed the cost was NOT the combat
AI or the attacker scan (both O(N), clone-free) but the generic legal-actions
path:
_activated + is_blocked_by_cant_activate_during per mana source, each a full
battlefield_active_statics scan => O(N^2) (~1.1M static iters per sweep).
state, i.e. the sweep) per candidate => ~2821 sweeps for one declare decision.
Fix A: hoist a ManaActivationGates existence gate once per sweep (mirrors
combat::CombatStaticGates) and skip the per-source scans when no such static
exists. O(N^2) -> O(N).
Fix B: SimulationFilter only reads .is_ok() on a discarded clone, so apply via
PublicFinalizeMode::DeferredDisplay (apply_as_current_for_legality) -- display
derivation is irrelevant to legality and is skipped entirely.
Measured (debug): generic declare path 921s -> 144s; per-priority decision
491ms -> 191ms (sim filter 270ms -> 30ms); mana sweeps in the filter path
2821 -> 0. Adds declare_attackers_bench dev tool.
Guards: apply_for_legality_skips_display_mana_sweep (Fix B),
can_activate_mana_ability_now_respects_cant_activate_during_via_gate (Fix A
gate=true correctness).