Cache per-card trait lists instead of rebuilding them on every access#11366
Open
liamiak wants to merge 5 commits into
Open
Cache per-card trait lists instead of rebuilding them on every access#11366liamiak wants to merge 5 commits into
liamiak wants to merge 5 commits into
Conversation
getReplacementEffects / getStaticAbilities / getTriggers rebuilt their list from the layer system on every call. Instrumenting one AI-vs-AI game on a 510 card deck showed 18.0M, 11.2M and 2.7M calls respectively, and in over 99.9% of them the rebuilt list was identical to the previous one for that card state. Cache the result per CardState and drop it when an input changes, mirroring the existing cachedKeywords / updateKeywordsCache pattern. Inputs are: the three lists' own mutators, bulk copy (copyFrom / addAbilitiesFrom), changed card traits in layers 3 and 6, the keyword cache, type changes (which drive the Saga, planeswalker and battle ETB replacements, Adventure/Omen and basic land mana), counters (Shield and Stun add replacement effects), and setStates. Two subtleties worth calling out: - The Original state merges LeftSplit/RightSplit's raw lists into its own, per CR 712.3 (a split card has its halves' combined characteristics outside the stack), so mutating a split state must invalidate the whole card. - updateTypes() only refreshes the current state, so updateTypeCache() drops every state's cache rather than relying on a non-current state's cache happening to agree with its equally stale type. This also covers the clone and rollback paths, which reach it through updateChangedText(). rulesHost=true/false yield different lists and cache separately. The LKI/preList path is safe by construction: ReplacementHandler swaps in the LKI or lastState card object and queries that, which is a separate CardState with its own cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FCollection kept a HashSet beside its list purely to reject duplicate adds and answer contains() in O(1). Trait rebuilds (getReplacementEffects / getStaticAbilities / getTriggers, plus every FCollection copy they make) create tens of millions of these per game, almost all holding one or two elements and never queried by value - so the set was pure overhead: an extra allocation per collection and a hash insert per element. Build the set on demand instead. Until something needs value semantics at scale (asSet, or growth past a small threshold) uniqueness is enforced by scanning the list, which is cheaper than the set for the tiny sizes that dominate. Also copy straight across the backing list when constructing from another FCollection (the source already guarantees uniqueness) and skip iterating empty collections in addAll. Verified against a fixed 5-seed AI-vs-AI match on a 510-card deck: every game's end state is byte-identical to before, and total match time drops substantially (the set churn was the single largest allocation source in the profile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
getReplacementEffects / getStaticAbilities / getTriggers rebuild their lists
from the layer system on every call, millions of times per game, and for the
overwhelming majority of cards every input to that rebuild is empty:
- hasRemoveIntrinsic() built a values() view and ran an anyMatch stream even
when the changedCardTypes table is empty (which it is for all but a handful
of cards). Short-circuit on the empty table and drop the stream.
- getChangedCardTraitsList() assembled a Guava concat view over two usually-
empty table value collections plus a freshly allocated singleton. When both
tables are empty - the common case - return a cached singleton list holding
just the layer-4 land-trait wrapper instead.
- KeywordCollection's apply{SpellAbility,Trigger,ReplacementEffect,
StaticAbility} iterated the multimap's nested value view even with no
keywords. Bail early when the map is empty.
No behavioural change; the same fixed-seed matches produce identical end
states.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ReplacementEffect, SpellAbility, StaticAbility and Trigger all define equality on their int id, but hashed via Objects.hash(SomeClass.class, id), which allocates a two element varargs array and boxes the id on every call. These land in hash sets constantly during trait rebuilds and stack processing. Replace that with an id offset from a per-class base, and take the base from the class name rather than the Class object. Class.hashCode() is an identity hash, so it varies between JVM runs, which left the hashCode of every trait and ability - and hence the iteration order of any hash collection keyed on them - different from one run to the next. Same distribution, no allocation, no boxing, and reproducible across runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The risk with caching the trait lists is a missed invalidation: the cache would hand back a stale list and the game would quietly play by the wrong rules. That is hard to find from the outside, because a stale cache is perfectly deterministic - replaying the same seed reproduces the wrong result. Add -Dforge.verifyTraitCache=true: on every cache hit, rebuild the list anyway and compare. A mismatch throws immediately, naming the card, the state and which list diverged, instead of surfacing later as an unexplained board difference. Also add -Dforge.disableTraitCache=true as an escape hatch. Both read into static finals, so with the flags off the JIT folds the checks away. The getters are split into a cached wrapper plus a build* method so the validation path can re-run exactly the original construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Contributor
|
i will try to cherry pick some changes tomorrow |
Contributor
|
i need to check if and if the Stream can be prebuilt, even if the underlying structures change 🤔 |
tool4ever
reviewed
Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
CardState.getReplacementEffects(),getStaticAbilities()andgetTriggers()rebuild their list from the layer system on every call. Instrumenting one AI-vs-AI game on a 510-card deck:getReplacementEffectsgetStaticAbilitiesgetTriggersThat is ~32M rebuilds per game, of which ~45K produced anything new. This caches the result per
CardStateand drops it when an input changes — the same shape as the existingcachedKeywords/updateKeywordsCachepair.Measurements
Seeded AI-vs-AI matches, master vs this branch measured back to back in one session:
Absolute times move with machine load, and the ratio moves with it too (a busier machine penalises the heavier arm more) — a quieter session measured −35%/−21%/−68%. Direction and rough magnitude are stable, and the win scales with board size, which matches the mechanism: what is being removed is per-card work.
Correctness
This is the part worth scrutinising, since a stale list means playing by the wrong rules. Invalidation covers the three lists' own mutators and bulk copy (
copyFrom/addAbilitiesFrom), changed card traits in layers 3 and 6, the keyword cache, type changes, counters (Shield and Stun add replacement effects), andsetStates.rulesHost=true/falseyield different lists and so cache separately. The LKI/preListpath is safe by construction:ReplacementHandlerswaps in the LKI orlastStatecard object and queries that, which is a separateCardStatewith its own cache.@Jetz72's question about game state copying and rollback on #11360 prompted me to go over those paths properly, and it turned up something worth hardening:
updateTypes()only refreshes the current state, so a non-current state could keep a cached list while its type moved on.updateTypeCache()now drops every state's cache rather than relying on a non-current state's cache happening to agree with its equally stale type. That also covers the clone and rollback paths, which reach it throughupdateChangedText().Validation
A stale cache is deterministic — replaying a seed reproduces the wrong result — so comparing end states alone is weak.
-Dforge.verifyTraitCache=truerebuilds and compares on every cache hit, throwing with the card, state and list named the moment a stale list would have been used.-Dforge.disableTraitCache=trueis an escape hatch. Both arestatic final, so the JIT folds them out when off.🤖 Implemented with the assistance of Claude Code (Opus).