Cache the per-card trigger and static ability lists

[liamiak]

CardState.getTriggers() and getStaticAbilities() rebuild their result from the layer system on every call — the changed-traits list, then the unhidden keywords — and the rebuilt list is identical to the previous one well over 99% of the time. This caches each list on the state and drops it whenever an input changes.

Scope. This previously cached replacement effects too. That is gone, along with the counter plumbing that existed only to serve it: the shield/stun/finality block in updateReplacementEffects is the only place counters reach a trait list, and per #11631 those extras are better moved out of getReplacementEffects than cached around. Triggers and statics have no such dependency — getHiddenStaticAbilities() already keeps the counter-derived statics out of the cached list.

Invalidation points: changed card traits (both tables), keyword cache updates, type changes (via hasRemoveIntrinsic), trigger/static mutation, state replacement, and card copies.

Measured.

• Correctness: every cache hit rebuilds the list and compares it against the cached one — 4M+ verified hits across the suite, zero mismatches. A deliberately injected disagreement was run first to confirm the check actually fires, and the LeftSplit/RightSplit merge path was checked separately since the suite covers it thinnest.
• Speed: A/B in one JVM behind a static flag, interleaved — 1.242x on an ordinary board, 1.094x on a dense one. The smaller figure is share-of-runtime; combat evaluation dominates a heavy board. Both are AITest setups over six turns rather than full games. (An earlier comment on this PR quoted 1.155x / 1.042x, measured before the flag also gated the new invalidation calls, so that baseline was not master.)
• Suite 357/0/6, checkstyle clean.

Known limit. The cache fields are unsynchronised and CardView reads getTriggers() from view code. That is the same exposure as the existing cachedKeywords field on CardState, so not a new risk class, but untested.

Written with Claude Code; the measurements and the differential check are mine to defend.

[tool4ever]

Some interesting ideas here and the code doesn't look too messy...
Also probably superior to the similar part from #11314 since it wouldn't be limited to AI 🤔

Maybe @Hanmac wants to think about the caching logic?

I'm more into the FCollection ideas, will see if they cause any problems 👍

[Hanmac]

i will try to cherry pick some changes tomorrow
(like the collection Logic for FCollection),
and the small changes for LandChanges

[Hanmac]

i need to check if getChangedCardTraitsList might be better if it would return a Stream<> instead of Iterable

and if the Stream can be prebuilt, even if the underlying structures change 🤔
(That probably doesn't work)

[liamiak]

Rebased onto master and cut down to just the caching. @Hanmac @tool4ever - thanks for taking the
FCollection and LandTraitChanges pieces, that turned out to make this much easier to reason about.

Three of the five commits are gone:

• FCollection is in via Cherry-pick FCollection from #11366 #11431. The readObject I had was redundant - set is transient, so it
  is already null after deserialization - dropping it was correct.
• "Skip redundant work in the per-card trait rebuild" is superseded by LandTraitChanges: small improvements #11389. Master already has the
  hasRemoveIntrinsic early-out, the landTraitChanges list-wrapping and the
  getChangedCardTraitsList fast path, so against current master that commit added only comments.
• The trait/ability hashCode change and the cache validation mode I dropped myself. Measuring them
  separately, the hashCodes are worth 0.6%, which is inside run-to-run noise, and they are a
  different idea from caching - happy to raise them on their own if you think they are worth having.
  The validation mode would have been the first Boolean.getBoolean("forge.…") switch in forge-game
  or forge-core, and adding a configuration mechanism as a side effect of a perf fix seemed like the
  wrong trade.

What is left is only the caching, +81 lines across two files, and it carries the whole speedup:

	in-game time	wall clock
master	35 877 ms	46.7 s
this PR	16 773 ms	27.3 s

sim -d "Big 240531" "Big 240531" -n 3 -s 12345, so both sides play the same games - identical turn
counts, winners and match scores - which is also the check that the cache is not changing behaviour.
Twice per build, under 1% variance. 336 tests, 0 failures.

@Hanmac you asked earlier whether getChangedCardTraitsList would be better returning a Stream<>,
and whether it could be prebuilt even as the underlying structures change. That method is yours now
after #11389, so I have left it alone here - but the seeded-sim setup above is a reliable way to
measure it if you want a number on it.

[liamiak]

Rebased onto master, plus a fix for a hole in the invalidation set.

clearCounters() emptied the counters field directly rather than going through either setCounters, so it was the one counter path that never dropped the caches — a card kept its shield and stun replacement effects after the counters were gone. GameAction.changeZone hits that path whenever a permanent leaves the battlefield.

Rather than add a third invalidation call, the field is now private to GameEntity, which owns the writes and calls a new onCountersChanged() that Card overrides once. getCounters() handed out the live multiset, which was the remaining way past the hook, so it now returns a cached unmodifiable view — and the two callers that stored that reference now copy instead. One of them was sharing a single multiset between a real card and the LKI copy the AI evaluates pumps against. Regression test included.

That also corrects something I claimed earlier: that the seeded equivalence run covered what a validation mode would prove. It does not. Identical games show the two builds agree with each other, not that a cached list still agrees with a rebuilt one, and that is the distinction this bug fell through.

On the overlap with #10507 — they come at the same three getters from opposite ends. That one early-exits when the list would be empty, so it cannot go stale, but it only helps cards that have nothing. This one caches the built list, so it helps every card, at the price of an invalidation obligation on each path that mutates an input. Both carry a completeness obligation — theirs is that the predicates keep mirroring every source — so it is more a question of which shape is easier to maintain. They also collide textually in the same methods, so probably one or the other rather than both.

Full suite green, checkstyle clean.

[tool4ever]

these should not need caching if we have a PR add them differently first: #11631 (review)

[Hanmac]

@tool4ever part of this #11409 (comment)
I might cache them with and without stuff like shield counters

[liamiak]

That's fair, and going through it the objection lands on one of the four caches rather than all of them.

Only updateReplacementEffects reads counters, and only in its rulesHost branch — shield/stun/finality. updateTriggers and updateStaticAbilities read getChangedCardTraitsList and getUnhiddenKeywords, nothing else. Statics already solved this the way you're describing: the counter-derived ones live in counterTypeKeywordStatic, served by getHiddenStaticAbilities() and added on demand at GameAction:1109, so they never enter the cached list.

So I've dropped the replacement cache and everything that existed to serve it. The counter-routing commit goes entirely and Card.setCounters is untouched again. What's left is triggers and statics, +62/-0 in two files.

@Hanmac — on the Plain/AsRulesHost pair, I did build that, and it's exactly the part that drags counters in. If the rules-host extras move to getReplacementList() on demand as tool4ever suggests, it collapses to one list needing no counter invalidation at all, so I'd rather that land in #11631 than cache around it here.

Verification: every cache hit rebuilds the list and compares it against the cached one — 4M+ verified hits across the suite, zero mismatches, with a deliberately injected disagreement first to prove the check actually fires, plus a targeted check on the LeftSplit/RightSplit merge path since that is what the suite covers thinnest.

Timing, corrected since I first posted this: 1.242x on an ordinary board, 1.094x on a dense one, A/B in one JVM behind a static flag. The figures I originally gave (1.155x / 1.042x) used a disabled arm that still ran the new invalidation calls, so it was not master. The flag now gates those too, so the disabled arm matches master. Both boards are AITest setups over six turns rather than full games. Suite 357/0/6.

Two things worth flagging rather than leaving for you to find:

• copyFrom and addAbilitiesFrom dropped the cache before copying the traits in, and those copies read back through the host card — so a read mid-copy could refill from a half-copied state. Nothing in the suite hits it, but the invalidation now sits after the mutations.
• The cache fields are unsynchronised and CardView reads getTriggers() from view code. That is the same exposure as the existing cachedKeywords field on CardState, so not a new risk class, but I have not tested it and would not claim it is safe.

Also correcting my last point above: I said I had found a counters aliasing bug in getPumpedCreature and would send it separately. Having looked properly that is weaker than I made it sound. The GameState half is deliberate — the comment there says the reference is kept on purpose — and the ComputerUtilCard half is real but latent: all four callers only read the copy, and I found no path that mutates its counters. So I am not filing it.

[liamiak]

Pushed a much smaller version of this. The replacement-effect cache is gone, along with the counter plumbing that only existed to serve it — it is now triggers and statics only, +62/-0 in two files.

The reasoning is in this thread, which GitHub has collapsed as outdated since the line it was anchored to no longer exists. In short: getHiddenStaticAbilities() already keeps the counter-derived statics out of the cached list, so triggers and statics never had the dependency the replacement path does — which is why dropping that one cache also drops every reason to touch setCounters.

@tool4ever @Hanmac

[liamiak]

@tool4ever — late answer to your note about the overlap with #10507 (the thread is collapsed as outdated, so putting this here).

I ported MostCromulent's fast path onto this branch behind a second flag, so both could be measured in one JVM with both flags off equal to master.

They aren't alternatives. The fast path fires only when a card has no triggers/statics of its own, no split states, no trait-granting keyword and no overlays — i.e. when the list would be empty. This PR only pays off in the opposite case, when there is something to build. The one place they meet is that empty population, where MostCromulent's returns a shared empty and mine returns a cached one; the fast path is cheaper there, since it skips the cache write and the invalidation bookkeeping entirely.

By call count the empty population dominates — 2,272,336 fast-path hits over ten turns across two boards.

Correctness: I ran the fast path against a full rebuild on every skip. 2,272,336 checks, zero cases where it returned empty and a rebuild would not have. The canUse*FastPath predicates mirror the sources correctly as far as my harness can see.

Timing, ordinary board (30 interleaved reps, medians): master 538ms · this PR 439ms (1.227x) · #10507 495ms (1.158x) · both 450ms (1.195x).

Of the four arms, this PR alone was the fastest. On their own it was the larger of the two by 1.227x against 1.158x — an ordering that also held across the first nine reps before the box started drifting, though at ~6% apart I would not lean on the margin. Running both measured no better than this PR on its own (450ms against 439ms), a gap inside the run's noise, so read that as no gain rather than a regression. It is what the mechanism predicts on a light board: the fast path takes the empty-list calls the cache would have served cheaply, while the cache still pays its invalidation bookkeeping.

Heavy board: no usable number. Within-arm spread was 5231-8195ms against differences of a few percent, so that 2x2 is inconclusive and I'm not going to claim additivity I can't show. That is the board where I would expect them to diverge, since it is where the fast path stops firing — but expecting is not measuring.