Skip to content

Cache the per-card trigger and static ability lists #11366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
liamiak wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cache the per-card trigger and static ability lists #11366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions forge-game/src/main/java/forge/game/card/Card.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ public CardStateName getCurrentStateName() {

// use by CopyPermanent
public void setStates(Map<CardStateName, CardState> map) {
invalidateTraitCaches();
states.clear();
states.putAll(map);
}
Expand Down Expand Up @@ -4192,6 +4193,10 @@ public final boolean removeChangedCardTypes(final long timestamp, final long sta
}

public final void updateTypeCache() {
// updateTypes() only refreshes the current state, so drop every state's cached traits here:
// clone and rollback paths reach this via updateChangedText(), and relying on a non-current
// state's cache happening to match its equally stale type would be far too subtle.
invalidateTraitCaches();
this.getCurrentState().updateTypes();
}

Expand Down Expand Up @@ -4921,13 +4926,15 @@ public final Table<Long, Long, CardTraitChanges> getChangedCardTraitsByText() {
return changedCardTraitsByText;
}
public final void setChangedCardTraitsByText(Table<Long, Long, CardTraitChanges> changes) {
invalidateTraitCaches();
changedCardTraitsByText.clear();
for (Table.Cell<Long, Long, CardTraitChanges> e : changes.cellSet()) {
changedCardTraitsByText.put(e.getRowKey(), e.getColumnKey(), e.getValue().copy(this, true));
}
}
public final void addChangedCardTraitsByText(Collection<SpellAbility> spells,
Collection<Trigger> trigger, Collection<ReplacementEffect> replacements, Collection<StaticAbility> statics, long timestamp, long staticId) {
invalidateTraitCaches();
changedCardTraitsByText.put(timestamp, staticId, new CardTraitChanges(
spells, trigger, replacements, statics, e -> true
));
Expand All @@ -4952,6 +4959,7 @@ public final ICardTraitChanges addChangedCardTraits(Collection<SpellAbility> spe
return addChangedCardTraits(result, timestamp, staticId, updateView);
}
public final ICardTraitChanges addChangedCardTraits(ICardTraitChanges changes, long timestamp, long staticId, boolean updateView) {
invalidateTraitCaches();
changedCardTraits.put(timestamp, staticId, changes);
if (updateView) {
updateAbilityTextForView();
Expand All @@ -4960,9 +4968,11 @@ public final ICardTraitChanges addChangedCardTraits(ICardTraitChanges changes, l
}

public final boolean removeChangedCardTraits(long timestamp, long staticId) {
invalidateTraitCaches();
return changedCardTraits.remove(timestamp, staticId) != null;
}
public final boolean removeChangedCardTraitsByText(long timestamp, long staticId) {
invalidateTraitCaches();
return changedCardTraitsByText.remove(timestamp, staticId) != null;
}

Expand All @@ -4982,13 +4992,15 @@ public final Table<Long, Long, ICardTraitChanges> getChangedCardTraits() {
}

public final void setChangedCardTraits(Table<Long, Long, ICardTraitChanges> changes) {
invalidateTraitCaches();
changedCardTraits.clear();
for (Table.Cell<Long, Long, ICardTraitChanges> e : changes.cellSet()) {
changedCardTraits.put(e.getRowKey(), e.getColumnKey(), e.getValue().copy(this, true));
}
}

public boolean clearChangedCardTraits() {
invalidateTraitCaches();
boolean changed = false;
if (!changedCardTraitsByText.isEmpty()) {
changed = true;
Expand Down Expand Up @@ -5221,7 +5233,19 @@ public final KeywordCollection getUnhiddenKeywords(CardState state) {
public final void updateKeywordsCache() {
updateKeywordsCache(getCurrentState());
}
/**
* Drop the cached trait lists (triggers / static abilities) on every state of this card.
* Called from each place an input to those lists can change.
*/
public final void invalidateTraitCaches() {
for (CardState st : states.values()) {
st.invalidateTraitCache();
}
}

public final void updateKeywordsCache(final CardState state) {
// Keywords contribute triggers and statics, so the trait lists must go too.
state.invalidateTraitCache();
KeywordCollection keywords = new KeywordCollection();

// Layer 1
Expand Down
38 changes: 38 additions & 0 deletions forge-game/src/main/java/forge/game/card/CardState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,24 @@ public class CardState implements GameObject, IHasSVars, ITranslatable {
// wrapped in a List so it can be reused directly
private List<LandTraitChanges> landTraitChanges = List.of(new LandTraitChanges(this));

// Trait caches. getStaticAbilities/getTriggers rebuild their list from the layer system on
// every call - millions of times per game - and the result is identical to the previous one
// over 99.9% of the time. Cache it and drop the cache whenever any input changes; see
// Card.invalidateTraitCaches for the invalidation points.
private FCollectionView<StaticAbility> cachedStaticAbilities;
private FCollectionView<Trigger> cachedTraitTriggers;

/**
* Drop every cached trait list; they are rebuilt lazily on next access.
* Note: mutating a split state's raw trait lists must invalidate the whole card, because the
* Original state merges LeftSplit/RightSplit's lists into its own - hence those mutators call
* Card.invalidateTraitCaches() rather than this.
*/
final void invalidateTraitCache() {
cachedStaticAbilities = null;
cachedTraitTriggers = null;
}

public CardState(Card card, CardStateName name) {
this(card.getView().createAlternateState(name), card);
}
Expand Down Expand Up @@ -156,6 +174,9 @@ public CardTypeView getTypeWithChanges() {

public void updateTypes() {
this.changedType = getType().getTypeWithChanges(card.getChangedCardTypes());
// LandTraitChanges clears both lists when hasRemoveIntrinsic() is set, and that reads
// changedCardTypes - which every type mutation funnels through here to refresh.
invalidateTraitCache();
}
public void updateTypesForView() {
view.updateType(this);
Expand Down Expand Up @@ -686,6 +707,9 @@ public final boolean addSpellAbility(final SpellAbility a) {
}

public final FCollectionView<Trigger> getTriggers() {
if (cachedTraitTriggers != null) {
return cachedTraitTriggers;
}
FCollection<Trigger> result = new FCollection<>(triggers);
if (getStateName().equals(CardStateName.Original)) {
if (getCard().hasState(CardStateName.LeftSplit))
Expand All @@ -694,6 +718,7 @@ public final FCollectionView<Trigger> getTriggers() {
result.addAll(getCard().getState(CardStateName.RightSplit).triggers);
}
card.updateTriggers(result, this);
cachedTraitTriggers = result;
return result;
}

Expand All @@ -711,10 +736,14 @@ public final boolean hasTrigger(final int id) {
}

public final boolean addTrigger(final Trigger t) {
card.invalidateTraitCaches();
return triggers.add(t);
}

public final FCollectionView<StaticAbility> getStaticAbilities() {
if (cachedStaticAbilities != null) {
return cachedStaticAbilities;
}
FCollection<StaticAbility> result = new FCollection<>(staticAbilities);
if (getStateName().equals(CardStateName.Original)) {
if (getCard().hasState(CardStateName.LeftSplit))
Expand All @@ -723,12 +752,15 @@ public final FCollectionView<StaticAbility> getStaticAbilities() {
result.addAll(getCard().getState(CardStateName.RightSplit).staticAbilities);
}
card.updateStaticAbilities(result, this);
cachedStaticAbilities = result;
return result;
}
public final boolean addStaticAbility(StaticAbility stab) {
card.invalidateTraitCaches();
return staticAbilities.add(stab);
}
public final boolean removeStaticAbility(StaticAbility stab) {
card.invalidateTraitCaches();
return staticAbilities.remove(stab);
}

Expand Down Expand Up @@ -963,6 +995,10 @@ public final void copyFrom(final CardState source, final boolean lki, final Card
this.landManaAbilities.put(e.getKey(), e.getValue().copy(card, true));
}
}

// after the copy, not before: the trait copies above read back through this card, so an
// earlier drop could be refilled from a half-copied state
card.invalidateTraitCaches();
}

public final void addAbilitiesFrom(final CardState source, final boolean lki) {
Expand Down Expand Up @@ -993,6 +1029,8 @@ public final void addAbilitiesFrom(final CardState source, final boolean lki) {
staticAbilities.add(sa.copy(card, lki));
}
}

card.invalidateTraitCaches();
}

public CardState copy(final Card host, CardStateName name, final boolean lki) {
Expand Down