From 7c1a0192d22d3033c46e0c3346a00f714f5ba02e Mon Sep 17 00:00:00 2001
From: asvitkine <alexei.svitkine@gmail.com>
Date: Sat, 5 Aug 2023 13:00:56 -0400
Subject: [PATCH] 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.
---
 .../ai/pro/util/ProOddsCalculator.java        |  3 +--
 .../odds/calculator/BattleCalculator.java     | 20 +++++++++++++++----
 .../ConcurrentBattleCalculator.java           |  4 ++++
 3 files changed, 21 insertions(+), 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
index 4f345e70518..a51fa2582ca 100644
--- 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/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculator.java b/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculator.java
index d829aca6d30..46e4837e731 100644
--- a/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculator.java
+++ b/game-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/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/ConcurrentBattleCalculator.java b/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/ConcurrentBattleCalculator.java
index 5df4983509c..1a6e7dee71e 100644
--- a/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/ConcurrentBattleCalculator.java
+++ b/game-app/game-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,