Improve battle calc logic for copying collections. (#11858)

* Improve logic for copying collections. Instead of copying the bombardingUnits, only copy when needed for serialization. Add some comments. * Fix word in comment. * Remove unused import.
triplea-game · Aug 5, 2023 · 7c1a019 · 7c1a019
1 parent aa61694
commit 7c1a019
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 6 deletions.
diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProOddsCalculator.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProOddsCalculator.java
@@ -14,7 +14,6 @@
 import games.strategy.triplea.odds.calculator.AggregateResults;
 import games.strategy.triplea.odds.calculator.IBattleCalculator;
 import games.strategy.triplea.util.TuvUtils;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import org.triplea.java.collections.CollectionUtils;
@@ -253,7 +252,7 @@ private ProBattleResult callBattleCalc(
             t,
             attackingUnits,
             defendingUnits,
-            new ArrayList<>(bombardingUnits),
+            bombardingUnits,
             TerritoryEffectHelper.getEffects(t),
             retreatWhenOnlyAirLeft,
             runCount);

diff --git a/...-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculator.java b/...-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculator.java
@@ -14,6 +14,8 @@
 import games.strategy.triplea.delegate.battle.BattleTracker;
 import games.strategy.triplea.delegate.battle.MustFightBattle;
 import games.strategy.triplea.util.TuvCostsCalculator;
+import java.io.Serializable;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
@@ -68,13 +70,13 @@ public AggregateResults calculate(
               : gameData.getPlayerList().getPlayerId(defender.getName());
       final Territory location2 = gameData.getMap().getTerritory(location.getName());
       final Collection<Unit> attackingUnits =
-          GameDataUtils.translateIntoOtherGameData(attacking, gameData);
+          translateCollectionIntoOtherGameData(attacking, gameData);
       final Collection<Unit> defendingUnits =
-          GameDataUtils.translateIntoOtherGameData(defending, gameData);
+          translateCollectionIntoOtherGameData(defending, gameData);
       final Collection<Unit> bombardingUnits =
-          GameDataUtils.translateIntoOtherGameData(bombarding, gameData);
+          translateCollectionIntoOtherGameData(bombarding, gameData);
       final Collection<TerritoryEffect> territoryEffects2 =
-          GameDataUtils.translateIntoOtherGameData(territoryEffects, gameData);
+          translateCollectionIntoOtherGameData(territoryEffects, gameData);
       gameData.performChange(ChangeFactory.removeUnits(location2, location2.getUnits()));
       gameData.performChange(
           ChangeFactory.addUnits(location2, mergeUnitCollections(attackingUnits, defendingUnits)));
@@ -125,6 +127,16 @@ public AggregateResults calculate(
     }
   }
 
+  private <T> Collection<T> translateCollectionIntoOtherGameData(
+      Collection<T> collection, GameData otherData) {
+    // translateIntoOtherGameData() uses serialization, so if the collection is not serializable,
+    // copy it into one that is. In particular, HashMap.keySet() and similar are not serializable.
+    if (!(collection instanceof Serializable)) {
+      collection = new ArrayList<>(collection);
+    }
+    return GameDataUtils.translateIntoOtherGameData(collection, otherData);
+  }
+
   private Collection<Unit> mergeUnitCollections(Collection<Unit> c1, Collection<Unit> c2) {
     var combined = new HashSet<>(c1);
     combined.addAll(c2);

diff --git a/...core/src/main/java/games/strategy/triplea/odds/calculator/ConcurrentBattleCalculator.java b/...core/src/main/java/games/strategy/triplea/odds/calculator/ConcurrentBattleCalculator.java
@@ -181,6 +181,10 @@ public AggregateResults calculate(
               workers.parallelStream()
                   .map(
                       worker ->
+                          // Note: Although we're running in parallel, the data passed in does not
+                          // get modified, so no copies are necessary. Also, the outer calculate()
+                          // call is synchronous, so there's no problem if the caller later modifies
+                          // the collections that were provided.
                           worker.calculate(
                               attacker,
                               defender,