Card: copy getChangedCardTypes

[Hanmac]

Closes #6579

@Jetz72 can you do a Benchmark to test if it doesn't slowdown?

maybe CardState should cache getTypeWithChanges?

[Jetz72]

@Jetz72 can you do a Benchmark to test if it doesn't slowdown?

I'll state for the record that I was using the term "benchmark" loosely; I just slapped together some tests running out of GameSimulationTest that spit out differences in timestamps. Have yet to find a reasonably priced profiler on Linux or set up any sort of clean testing environment. But regardless, I tweaked what I had before to see what happens here.

I put 12 copies of Arcane Adaptation into play with different creature types chosen, then used a big creature and Fungal Sprouting to create a bunch of tokens. Then I wrath'd the board. Results:

18 tokens, master

Create time: 5.429042587S
Destroy time: 1.04462223S

Create time: 5.243296811S
Destroy time: 1.029953409S

Create time: 5.433600201S
Destroy time: 1.062678831S

18 tokens, cardChangedTypesCopy

Create time: 5.324745105S
Destroy time: 1.019810928S

Create time: 5.180989957S
Destroy time: 1.038830566S

Create time: 5.242260314S
Destroy time: 1.103055503S

100 tokens, master

Create time: 5M19.732451132S
Destroy time: 37.454955669S

100 tokens, cardChangedTypesCopy

Create time: 5M14.637408959S
Destroy time: 36.746079087S

I don't want to discount random variance as an influence on this takeaway, but I also don't want to spend 20 minutes running the 100 tokens test again and again just to double check. Looks like the impact of this change is at worst negligible and at best a tiny improvement. Maybe stripping out the extra overhead in getType is helping?

Could probably tip the scales back and test that theory if I added more Arcane Adaptations and ran it for longer. But either way, whatever sluggishness type-changing causes, I don't think this is a significant impact on it. Caching might be beneficial here, but a proper profiler would be better for determining that. Some other quick tests for reference:

18 tokens, 0 Arcane Adaptations

Create time: 0.169727729S
Destroy time: 0.037481884S

100 tokens, 0 Arcane Adaptations

Create time: 0.526035786S
Destroy time: 0.184369256S

18 tokens, 1 Arcane Adaptation

Create time: 0.506178578S
Destroy time: 0.110224161S

100 Tokens, 1 Arcane Adaptation

Create time: 13.458784444S
Destroy time: 1.768149243S

18 tokens, 50 Arcane Adaptations

Create time: 2M36.176879002S
Destroy time: 30.060126531S

18 tokens, 50 Arcane Adaptations all set to the same creature type

Create time: 48.411102085S
Destroy time: 9.239191711S

18 tokens, 50 Fervors granting them all Haste

Create time: 9.717805667S
Destroy time: 1.665164339S

18 tokens, 50 Grizzly Bears

Create time: 0.16821668S
Destroy time: 0.050768615S

[Hanmac]

@tool4ever do you think we can merge that for now and do more caching later?

[tool4ever]

java.util.ConcurrentModificationException: null at java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1209) at java.util.TreeMap$EntryIterator.next(TreeMap.java:1245) at java.util.TreeMap$EntryIterator.next(TreeMap.java:1240) at com.google.common.collect.StandardTable$CellIterator.next(StandardTable.java:255) at com.google.common.collect.StandardTable$CellIterator.next(StandardTable.java:242) at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:52) at com.google.common.collect.Iterators$ConcatenatedIterator.next(Iterators.java:1433) at com.google.common.collect.Iterators$1.next(Iterators.java:145) at forge.card.CardType.getTypeWithChanges(CardType.java:592) at forge.game.card.Card.getType(Card.java:5149) at forge.game.card.Card.getType(Card.java:5142) at forge.game.card.Card.isValid(Card.java:6964) at forge.game.GameObject.isValid(GameObject.java:27) at forge.game.card.CardPredicates.lambda$restriction$19(CardPredicates.java:138) at forge.game.card.CardPredicates$$ExternalSyntheticLambda46.apply(D8$$SyntheticClass:0) at com.google.common.collect.Iterators$5.computeNext(Iterators.java:673) at com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:145) at com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:140) at forge.util.collect.FCollection.addAll(FCollection.java:349) at forge.util.collect.FCollection.<init>(FCollection.java:115) at forge.game.card.CardCollection.<init>(CardCollection.java:115) at forge.game.card.CardLists.filter(CardLists.java:314) at forge.game.card.CardLists.getValidCards(CardLists.java:181) at forge.game.staticability.StaticAbilityContinuous.getAffectedCards(StaticAbilityContinuous.java:1076) at forge.game.staticability.StaticAbilityContinuous.applyContinuousAbility(StaticAbilityContinuous.java:71) at forge.game.staticability.StaticAbility.applyContinuousAbilityBefore(StaticAbility.java:275) at forge.game.GameAction.checkStaticAbilities(GameAction.java:1143) at forge.game.GameAction.checkStateEffects(GameAction.java:1264) at forge.game.phase.PhaseHandler.checkStateBasedEffects(PhaseHandler.java:1194) at forge.game.phase.PhaseHandler.startFirstTurn(PhaseHandler.java:1065)

Imo you might just be suppressing the real cause which could just lead to futher problems somewhere else...
I mean nothing should be allowed to modify the types this deep in engine logic? 🤔

[kevlahnota]

can you just acquire lock in the treemap ie Tables.synchronizedTable(TreeBasedTable.create())?

[Hanmac]

@tool4ever do you have a reproduce able case I can test it?

And should all Tables in Card be Synced?

[Hanmac]

can you just acquire lock in the treemap ie Tables.synchronizedTable(TreeBasedTable.create())?

That shouldn't be, but i try it.

@tool4ever can you test again, now with synchronizedTable ?

[tool4ever]

Uh sorry, I didn't mean it was crashing from your change 😅
Rather I was just trying to understand what this was all about

[Hanmac]

@kevlahnota your opinion? Should the Tables still be synced?

[kevlahnota]

@kevlahnota your opinion? Should the Tables still be synced?

hmmm I was under the impression that those are accessed and modified while iteration occurs. Sync can lock but still could produce concurrent modification because the table is not really thread safe.

[Hanmac]

@tehdiplomat what do you think?

should i revert the Sync Table stuff, or should it keep it?

the error that @tool4ever posted should be fixed with ImmutableList.copyOf

[tehdiplomat]

I don't mind merging this soon, but can we wait until after I finish the release. I thought it worked last night, but i think with the changes to the installer stuff the artifacts didn't seem to get uploaded last night, so i might need another day or two to get that resolved.

[kevlahnota]

@kevlahnota your opinion? Should the Tables still be synced?

I think just remove the sync, we can see if it still the issue on sentry

[Hanmac]

@kevlahnota @tehdiplomat sync removed for this