Fix #1747: skip GameObject clone on owner-zone filter fast path - #1748
Conversation
…hase-rs#1747) `matches_target_filter_in_owner_zone` cloned the full GameObject on every call just to override `controller := owner` for owner-scoped (hand / library / graveyard) filter matching — allocating `name`, the `counters` HashMap, and several Vecs per object. This is hot on library scans for tutors/search effects (Diabolic Tutor, Cultivate), where the clone runs once per scanned card. When `controller == owner` — the overwhelmingly common case for objects in owner zones, where control-change effects almost never apply — the override is a no-op, so the clone is pure waste. Add a fast path that calls `filter_inner_for_object` against the borrowed object directly, skipping the clone. Behavior is identical: the override only changes the result when `controller != owner`, which still takes the clone path. Adds a regression test exercising both paths (CR 109.5 / CR 400.3 owner scoping preserved: a control-changed card in an owner zone still counts as its owner's). This is bottleneck #1 of the six in the report; the remaining items (Arc<GameEvent> triggers, DifferentNameFrom memoization, layer-flush escalation, SBA single-scan, NameMatchesAnyPermanent battlefield-only iteration) are higher-risk refactors that warrant profiling/benchmarks before landing and are deferred to follow-ups. Closes phase-rs#1747
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization in matches_target_filter_in_owner_zone by skipping the GameObject clone when the controller equals the owner, and adds a regression test to verify this behavior. The review feedback identifies a critical issue where the LKI cache lookup in effective_controller can bypass this owner-scoping override in non-battlefield zones, and suggests expanding the regression test to cover this LKI scenario.
| // Vecs per call, which is hot on library scans for tutors/search effects). | ||
| // Behavior is identical: the override only changes the result when | ||
| // `controller != owner`. | ||
| if obj.controller == obj.owner { |
There was a problem hiding this comment.
[HIGH] LKI cache lookup in effective_controller defeats the owner-scoping override in matches_target_filter_in_owner_zone. Evidence: crates/engine/src/game/filter.rs:687.
Why it matters: When an object has an LKI entry in state.lki_cache (e.g., it recently died or changed zones), effective_controller will return lki.controller (the opponent's controller) instead of the overridden obj.controller (the owner), which violates CR 109.5 / CR 400.3 by letting control-change LKI exclude the owner in hand, library, and graveyard zones.
Suggested fix: Modify effective_controller (around line 601) to bypass the LKI cache lookup for hand, library, and graveyard zones:
fn effective_controller(state: &GameState, obj: &GameObject, object_id: ObjectId) -> PlayerId {
if !matches!(obj.zone, Zone::Battlefield | Zone::Stack | Zone::Hand | Zone::Library | Zone::Graveyard) {
if let Some(lki) = state.lki_cache.get(&object_id) {
return lki.controller;
}
}
obj.controller
}| // Slow path: control-change the card to P1 (owner stays P0). Owner- | ||
| // scoping must still treat it as P0's card via the clone+override. | ||
| state.objects.get_mut(&card).unwrap().controller = PlayerId(1); | ||
| assert_ne!( | ||
| state.objects[&card].controller, | ||
| state.objects[&card].owner, | ||
| "precondition: slow path requires controller != owner" | ||
| ); | ||
| assert!( | ||
| super::matches_target_filter_in_owner_zone(&state, card, &your_card, &ctx_p0), | ||
| "owner-scoping: a P1-controlled, P0-owned card in an owner zone is still P0's" | ||
| ); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] The new regression test does not cover the LKI cache scenario where the bug actually manifests. Evidence: crates/engine/src/game/filter.rs:3882.
Why it matters: Without an LKI cache entry in the test, the interaction between matches_target_filter_in_owner_zone and effective_controller is not exercised, leaving the correctness of owner-scoping unverified when LKI is present.
Suggested fix: Add an LKI cache entry with a different controller to the test to ensure owner-scoping is preserved even when LKI exists.
- bind CardId(state.next_object_id) before the &mut state borrow in create_object (E0502: cannot read state while mutably borrowed) - keep both assert_eq!/assert_ne! comparison operands on one line per rustfmt
|
Pushed maintainer review fixes on top of current What changed:
Local verification:
|
|
Re-pushed after #1745 landed so this branch is current with |
matthewevans
left a comment
There was a problem hiding this comment.
Implementation review is clean. The owner-zone fast path now avoids full GameObject clones without allowing stale LKI controller data to override owner-scoped matching; the existing LKI look-back behavior remains intact, targeted local tests passed, and CI is green on head 5729cd5.
Summary
Closes #1747 — Engine performance degrades severely on token-storm and complex-board turns.
The report identifies six bottlenecks. This PR lands the first and lowest-risk one — a provably behavior-identical win — and documents the rest as profiling-gated follow-ups (they're invasive refactors of correctness-critical hot paths that warrant benchmarks before landing).
Change — bottleneck #1:
GameObjectclone on owner-zone filter matchingmatches_target_filter_in_owner_zone(filter.rs:680) cloned the entireGameObjecton every call just to override one field (controller := owner) for owner-scoped (hand / library / graveyard) filter evaluation — allocating thenameString, thecountersHashMap, and several Vecs per object. This runs once per scanned card on library scans for tutors/search effects (Diabolic Tutor, Cultivate).Fix: when
controller == owner— the overwhelmingly common case for objects in owner zones, where control-change effects almost never apply — the override is a no-op, so match against the borrowed object directly and skip the clone entirely. The clone path is retained for the rarecontroller != ownercase.Behavior is identical: the override only changes the result when
controller != owner. A regression test exercises both paths and confirms CR 109.5 / CR 400.3 owner-scoping is preserved (a control-changed card in an owner zone still counts as its owner's).Deferred follow-ups (the other five bottlenecks)
These are higher-risk refactors of hot, correctness-critical paths (triggers, layers, SBA) that should be validated with profiling/benchmarks before landing — out of scope for this minimal, safe PR:
Arc<GameEvent>for trigger collection (triggers.rs:952+) — changes event storage type across the codebase.DifferentNameFromcaller-sideHashSetmemoization (filter.rs:2820) — the O(n²) is the per-outer-object rebuild; fixing it needs caller changes.layers.rs:1300) — layer-system correctness.sba.rs:56) — SBA fixpoint correctness.NameMatchesAnyPermanentbattlefield-only iteration (filter.rs:2492) — straightforward but reflows a wide match block; trivially safe, batched with the rest.Notes
🤖 Generated with Claude Code