search

Card-Forge / forge

An unofficial rules engine for the world's greatest card game.
https://card-forge.github.io/forge/
GNU General Public License v3.0
1.02k stars 571 forks source link

Card: copy getChangedCardTypes #6580

Open Hanmac opened 6 days ago

Hanmac commented 6 days ago

Closes #6579

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

maybe CardState should cache getTypeWithChanges?

Jetz72 commented 6 days ago

@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 commented 6 days ago

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

tool4ever commented 6 days ago

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? :thinking:

kevlahnota commented 5 days ago

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

Hanmac commented 5 days ago

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

And should all Tables in Card be Synced?

Hanmac commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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

kevlahnota commented 5 days ago

@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 commented 2 days ago

@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 commented 2 days ago

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 commented 11 hours ago

@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 commented 9 hours ago

@kevlahnota @tehdiplomat sync removed for this