Skip to content

Skip a name lookup that cannot change the result - #11975

Open
MostCromulent wants to merge 2 commits into
Card-Forge:masterfrom
MostCromulent:getname-guard
Open

MostCromulent wants to merge 2 commits into
Card-Forge:masterfrom
MostCromulent:getname-guard

Conversation

@MostCromulent

Copy link
Copy Markdown
Contributor

Summary

Card.getName walks a table of name-changing effects on every call, and that table is empty for almost every card in almost every game. Returning the printed name directly when it is empty leaves every result unchanged and makes AI-vs-AI games on token decks faster.

Where this fits

Token decks spend 83.5% of their time in AI combat prediction, and the recent merges (#11946, #11951, #11956, #11961, #11963, #11967) removed much of the engine work underneath it. Card.getName is now 7.4% of profiler samples on mirrored Goblin token decks, 98% of it reached from Card.staticReplaceDamage and Player.staticReplaceDamage. The same guard measured −1.1% with overlapping ranges before those merges; the work that was hiding it is gone.

Both methods walk the battlefield and compare c.getName() against roughly fifteen hardcoded names, so a card matching none of them pays the empty walk fifteen times per damage event. changedCardNames is a TreeBasedTable: values() builds a view and begins iterating before finding nothing, while isEmpty() is one check on the backing map.

Implementation

Four lines in Card.getName(CardState): return state.getName() when changedCardNames is empty. The existing loop never assigned anything in that case, so the returned value is identical, and the added check is constant time.

Measurements

AI-vs-AI games, four repetitions per arm, arms alternated and the order reversed between reps.

workload before after
two-player token decks 41,474 ms 38,245 ms −7.8%
four-player commander 93,536 ms 94,637 ms inside variance
ordinary constructed 6,224 ms 6,387 ms inside variance

On token decks the slowest run of the new code beat the fastest run of the old. Commander and ordinary constructed replay byte-identically on both arms, every game giving the same log hash across all four repetitions, so their differences are the same work timed twice and both sit well inside the run-to-run variance of those workloads.


🤖 Generated with Claude Code

getName walks a table of name-changing effects on every call, and that
table is empty for almost every card in almost every game. The table is a
sorted structure, so asking for its contents builds a view and starts an
iteration before finding nothing, while asking whether it is empty is a
single check.

The damage replacement code makes this expensive. Card.staticReplaceDamage
and Player.staticReplaceDamage each walk the whole battlefield and compare
a card's name against about fifteen hardcoded names in turn, so a card
matching none of them pays the empty walk fifteen times per damage event.

AI-vs-AI games on token decks run 7.8 percent faster. Ordinary constructed
and four-player commander play byte-identical games and show no change
outside measurement noise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread forge-game/src/main/java/forge/game/card/Card.java Outdated
@MostCromulent

Copy link
Copy Markdown
Contributor Author

Profiling the suggested approach: a shared empty table for the whole family of fields, materialised on first write.

Why the family is worth changing

  • 45,000 Card instances are constructed per six AI-vs-AI token games, 15,000 for ordinary constructed.
  • Card holds 23 TreeBasedTable fields, all created eagerly, so that is roughly a million empty tables per six-game batch.
  • Of the first 200,000 remove/clear calls against those tables, 24 found anything to do.

Measured

Token decks, three repetitions per arm in one session. The prediction RNG is frozen per call and the parallel attacker evaluation serialised, so both arms replay identically and the comparison is like-for-like; neither of those is part of the change.

mean vs master
master 21,436 ms
shared empty table 20,278 ms −5.4%

Log hashes are identical between arms, so AI decisions are unchanged.

The getName guard currently in this PR measures a further −3.1% on top, but that sits below the run-to-run floor for the workload and cannot be resolved at this sample size. The general change is the better justification, so the guard is dropped rather than kept alongside it. That changes this PR's scope and title from the single accessor to the whole family of fields.

Three ways to implement it

call sites changed allocation per card if a write site is missed
A — shared empty TreeBasedTable 35, put/putAll only; remove/clear on an empty mutable table are already no-ops none silent write into the shared instance
B — ForwardingTable wrapper none; only the 23 declarations one small wrapper per field, about 80% of the allocation still removed impossible to miss
C — shared ImmutableTable.of() 78 none immediate UnsupportedOperationException

A is closest to the original suggestion and keeps allocation at zero. B has the smallest review surface and cannot be reintroduced by a later contributor, at the cost of one small object per field. C is what was measured above; it has the largest diff of the three.

A preference on which shape to take would be useful before the implementation is finalised.

@tool4ever

tool4ever commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

B seems safer with clearer architecture imo

A Card declares twenty-three TreeBasedTable fields for the continuous
effects that can change its name, types, colours, keywords, traits, mana
cost, SVars and power and toughness. All twenty-three are built when the
card is, and for most cards most of them stay empty for the whole game.
Six AI-vs-AI games on token decks construct about 45,000 cards, so that
is roughly a million empty tables built and discarded.

The fields now start as a small wrapper around a shared immutable empty
table and swap in a real TreeBasedTable the first time something is put
into one. Reads, removes and clears on an untouched table answer from the
empty one without building anything. No call site changes, and the fields
stay final.

This replaces the getName guard from the previous commit, which skipped
one of these empty walks at one accessor. Starting the table empty removes
the walk everywhere instead.

AI-vs-AI games, four repetitions per arm, arms alternated and the order
reversed between reps: token decks 44,604 ms to 42,229 ms, 5.3 percent
faster. Ordinary constructed and four-player commander replay
byte-identically on both arms, every game giving the same log hash across
all four repetitions, so their differences are the same work timed twice
and both sit inside the run-to-run variance of those workloads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tool4ever
tool4ever requested a review from Hanmac September 20, 2026 13:10

@tool4ever tool4ever left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

@Hanmac

Hanmac commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

i'm unsure about this new LazyTable

the whole TreeTable probably needs to be changed later anyway when we finally update all the effects into being static abilities for Dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants