Speed up AI turns and reduce memory and save game sizes. (#10442)

* Speed up AI turns and reduce memory and save game sizes.

Speed up AI turns and reduce memory and save game sizes, by not instantiating empty collections for many different properties of attachments.

Standardize handling with helpers for getters, which return unmodifiable views of the underlying collections. Introduce an unmodifiable empty IntegerMap and the ability to return an unmodifiable view of an IntegerMap, for consistency with other collection methods.

Annotate null fields with `@Nullable` which lets IDEs help identify warnings when used without null checks directly.s.

Author: asvitkine

game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java

@@ -4,10 +4,21 @@
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.Iterables;
+import games.strategy.engine.data.gameparser.GameParseException;
 import games.strategy.triplea.Constants;
 import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
+import javax.annotation.Nullable;
+import lombok.Getter;
+import lombok.Setter;
+import org.triplea.java.collections.IntegerMap;
 
 /**
  * Contains some utility methods that subclasses can use to make writing attachments easier. FYI:
@@ -23,8 +34,8 @@ public abstract class DefaultAttachment extends GameDataComponent implements IAt
   private static final long serialVersionUID = -1985116207387301730L;
   private static final Splitter COLON_SPLITTER = Splitter.on(':');
 
-  private Attachable attachedTo;
-  private String name;
+  @Getter @Setter private Attachable attachedTo;
+  @Getter private String name;
 
   protected DefaultAttachment(
       final String name, final Attachable attachable, final GameData gameData) {
@@ -103,29 +114,13 @@ public String getRawPropertyString(final String property) {
     return getProperty(property).map(MutableProperty::getValue).map(Object::toString).orElse(null);
   }
 
-  @Override
-  public Attachable getAttachedTo() {
-    return attachedTo;
-  }
-
-  @Override
-  public void setAttachedTo(final Attachable attachable) {
-    attachedTo = attachable;
-  }
-
-  @Override
-  public String getName() {
-    return name;
-  }
-
   @Override
   public void setName(final String name) {
     this.name =
         Optional.ofNullable(name)
             // replace-all to automatically correct legacy (1.8) attachment spelling
             .map(attachmentName -> attachmentName.replaceAll("ttatch", "ttach"))
             .orElse(null);
-    ;
   }
 
   /**
@@ -156,4 +151,69 @@ public final boolean equals(final Object obj) {
             Objects.toString(attachedTo, null), Objects.toString(other.attachedTo, null))
         && (Objects.equals(name, other.name) || this.toString().equals(other.toString()));
   }
+
+  protected Territory getTerritoryOrThrow(String name) throws GameParseException {
+    return Optional.ofNullable(getData().getMap().getTerritory(name))
+        .orElseThrow(() -> new GameParseException("No territory named: " + name + thisErrorMsg()));
+  }
+
+  protected List<GamePlayer> parsePlayerList(final String value, List<GamePlayer> existingList)
+      throws GameParseException {
+    for (final String name : splitOnColon(value)) {
+      if (existingList == null) {
+        existingList = new ArrayList<>();
+      }
+      existingList.add(getPlayerOrThrow(name));
+    }
+    return existingList;
+  }
+
+  protected GamePlayer getPlayerOrThrow(String name) throws GameParseException {
+    return Optional.ofNullable(getData().getPlayerList().getPlayerId(name))
+        .orElseThrow(() -> new GameParseException("No player named: " + name + thisErrorMsg()));
+  }
+
+  protected Set<UnitType> parseUnitTypes(String context, String value, Set<UnitType> existingSet)
+      throws GameParseException {
+    for (final String u : splitOnColon(value)) {
+      if (existingSet == null) {
+        existingSet = new HashSet<>();
+      }
+      existingSet.add(getUnitTypeOrThrow(u));
+    }
+    return existingSet;
+  }
+
+  public UnitType getUnitTypeOrThrow(String unitType) throws GameParseException {
+    return Optional.ofNullable(getData().getUnitTypeList().getUnitType(unitType))
+        .orElseThrow(() -> new GameParseException("No unit type: " + unitType + thisErrorMsg()));
+  }
+
+  protected static <T> List<T> getListProperty(@Nullable List<T> value) {
+    if (value == null) {
+      return List.of();
+    }
+    return Collections.unmodifiableList(value);
+  }
+
+  protected static <K, V> Map<K, V> getMapProperty(@Nullable Map<K, V> value) {
+    if (value == null) {
+      return Map.of();
+    }
+    return Collections.unmodifiableMap(value);
+  }
+
+  protected static <T> Set<T> getSetProperty(@Nullable Set<T> value) {
+    if (value == null) {
+      return Set.of();
+    }
+    return Collections.unmodifiableSet(value);
+  }
+
+  protected static <T> IntegerMap<T> getIntegerMapProperty(@Nullable IntegerMap<T> value) {
+    if (value == null) {
+      return IntegerMap.of();
+    }
+    return IntegerMap.unmodifiableViewOf(value);
+  }
 }

game-app/game-core/src/main/java/games/strategy/triplea/attachments/AbstractConditionsAttachment.java

@@ -24,10 +24,12 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * This class is designed to hold common code for holding "conditions". Any attachment that can hold
- * conditions (ie: RulesAttachments), should extend this instead of DefaultAttachment.
+ * conditions (ie: RulesAttachments), should extend this instead of DefaultAttachment. Note: Empty
+ * collection fields default to null to minimize memory use and serialization size.
  */
 public abstract class AbstractConditionsAttachment extends DefaultAttachment implements ICondition {
   public static final String TRIGGER_CHANCE_SUCCESSFUL = "Trigger Rolling is a Success!";
@@ -40,7 +42,7 @@ public abstract class AbstractConditionsAttachment extends DefaultAttachment imp
   private static final Splitter HYPHEN_SPLITTER = Splitter.on('-');
 
   // list of conditions that this condition can contain
-  protected List<RulesAttachment> conditions = new ArrayList<>();
+  protected @Nullable List<RulesAttachment> conditions = null;
   // conditionType modifies the relationship of conditions
   protected String conditionType = AND;
   // will logically negate the entire condition, including contained conditions
@@ -90,11 +92,11 @@ private void setConditions(final List<RulesAttachment> value) {
 
   @Override
   public List<RulesAttachment> getConditions() {
-    return conditions;
+    return getListProperty(conditions);
   }
 
   protected void resetConditions() {
-    conditions = new ArrayList<>();
+    conditions = null;
   }
 
   private void setInvert(final boolean s) {

game-app/game-core/src/main/java/games/strategy/triplea/attachments/AbstractPlayerRulesAttachment.java

@@ -11,6 +11,7 @@
 import games.strategy.triplea.Constants;
 import java.util.Collection;
 import java.util.Map;
+import javax.annotation.Nullable;
 import lombok.extern.slf4j.Slf4j;
 import org.triplea.java.collections.IntegerMap;
 
@@ -24,7 +25,8 @@
  * ownership of territories, or objectiveValue (the money given if the condition is true), would NOT
  * go in This class. <br>
  * Please do not add new things to this class. Any new Player-Rules type of stuff should go in
- * "PlayerAttachment".
+ * "PlayerAttachment". Note: Empty collection fields default to null to minimize memory use and
+ * serialization size.
  */
 @Slf4j
 public abstract class AbstractPlayerRulesAttachment extends AbstractRulesAttachment {
@@ -34,8 +36,8 @@ public abstract class AbstractPlayerRulesAttachment extends AbstractRulesAttachm
   // These variables are related to a "rulesAttachment" that changes certain rules for the attached
   // player. They are
   // not related to conditions at all.
-  protected String movementRestrictionType = null;
-  protected String[] movementRestrictionTerritories = null;
+  protected @Nullable String movementRestrictionType = null;
+  protected @Nullable String[] movementRestrictionTerritories = null;
   // allows placing units in any owned land
   protected boolean placementAnyTerritory = false;
   // allows placing units in any sea by owned land
@@ -51,7 +53,7 @@ public abstract class AbstractPlayerRulesAttachment extends AbstractRulesAttachm
   // negates dominatingFirstRoundAttack
   protected boolean negateDominatingFirstRoundAttack = false;
   // automatically produces 1 unit of a certain
-  protected IntegerMap<UnitType> productionPerXTerritories = new IntegerMap<>();
+  protected @Nullable IntegerMap<UnitType> productionPerXTerritories = null;
   // type per every X territories owned
   // stops the user from placing units in any territory that already contains more than this
   protected int placementPerTerritory = -1;
@@ -117,10 +119,6 @@ public static ICondition getCondition(
   }
 
   private void setMovementRestrictionTerritories(final String value) {
-    if (value == null) {
-      movementRestrictionTerritories = null;
-      return;
-    }
     movementRestrictionTerritories = splitOnColon(value);
     validateNames(movementRestrictionTerritories);
   }
@@ -138,18 +136,14 @@ private void resetMovementRestrictionTerritories() {
   }
 
   private void setMovementRestrictionType(final String value) throws GameParseException {
-    if (value == null) {
-      movementRestrictionType = null;
-      return;
-    }
     if (!(value.equals("disallowed") || value.equals("allowed"))) {
       throw new GameParseException(
           "movementRestrictionType must be allowed or disallowed" + thisErrorMsg());
     }
     movementRestrictionType = value;
   }
 
-  public String getMovementRestrictionType() {
+  public @Nullable String getMovementRestrictionType() {
     return movementRestrictionType;
   }
 
@@ -171,15 +165,15 @@ private void setProductionPerXTerritories(final String value) throws GameParseEx
       unitTypeToProduce = s[1];
     }
     // validate that this unit exists in the xml
-    final UnitType ut = getData().getUnitTypeList().getUnitType(unitTypeToProduce);
-    if (ut == null) {
-      throw new GameParseException("No unit called: " + unitTypeToProduce + thisErrorMsg());
-    }
+    final UnitType ut = getUnitTypeOrThrow(unitTypeToProduce);
     final int n = getInt(s[0]);
     if (n <= 0) {
       throw new GameParseException(
           "productionPerXTerritories must be a positive integer" + thisErrorMsg());
     }
+    if (productionPerXTerritories == null) {
+      productionPerXTerritories = new IntegerMap<>();
+    }
     productionPerXTerritories.put(ut, n);
   }
 
@@ -188,11 +182,11 @@ private void setProductionPerXTerritories(final IntegerMap<UnitType> value) {
   }
 
   public IntegerMap<UnitType> getProductionPerXTerritories() {
-    return productionPerXTerritories;
+    return getIntegerMapProperty(productionPerXTerritories);
   }
 
   private void resetProductionPerXTerritories() {
-    productionPerXTerritories = new IntegerMap<>();
+    productionPerXTerritories = null;
   }
 
   private void setPlacementPerTerritory(final String value) {

game-app/game-core/src/main/java/games/strategy/triplea/attachments/AbstractRulesAttachment.java

@@ -8,22 +8,25 @@
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.GameState;
 import games.strategy.engine.data.MutableProperty;
-import games.strategy.engine.data.PlayerList;
 import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.gameparser.GameParseException;
 import games.strategy.triplea.delegate.Matches;
 import games.strategy.triplea.delegate.OriginalOwnerTracker;
-import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import javax.annotation.Nullable;
 import org.triplea.java.RenameOnNextMajorRelease;
 import org.triplea.java.collections.CollectionUtils;
 
-/** The Purpose of this class is to hold shared and simple methods used by RulesAttachment. */
+/**
+ * The Purpose of this class is to hold shared and simple methods used by RulesAttachment. Note:
+ * Empty collection fields default to null to minimize memory use and serialization size.
+ */
 public abstract class AbstractRulesAttachment extends AbstractConditionsAttachment {
   private static final long serialVersionUID = -6977650137928964759L;
 
@@ -33,17 +36,17 @@ public abstract class AbstractRulesAttachment extends AbstractConditionsAttachme
   // directPresenceTerritories, or any of the other territory lists
   // only used if the attachment begins with "objectiveAttachment"
   // Note: Subclasses should use getPlayers() which take into account getAttachedTo().
-  private List<GamePlayer> players = new ArrayList<>();
+  private @Nullable List<GamePlayer> players = null;
   protected int objectiveValue = 0;
   // only matters for objectiveValue, does not affect the condition
   protected int uses = -1;
   // condition for what turn it is
   @RenameOnNextMajorRelease(newName = "rounds")
-  protected Map<Integer, Integer> turns = null;
+  protected @Nullable Map<Integer, Integer> turns = null;
   // for on/off conditions
   protected boolean switched = true;
   // allows custom GameProperties
-  protected String gameProperty = null;
+  protected @Nullable String gameProperty = null;
   // Determines if we will be counting each for the purposes of objectiveValue
   private boolean countEach = false;
   // Used with the next Territory conditions to determine the number of territories needed to be
@@ -56,26 +59,21 @@ protected AbstractRulesAttachment(
   }
 
   private void setPlayers(final String names) throws GameParseException {
-    final PlayerList pl = getData().getPlayerList();
-    for (final String p : splitOnColon(names)) {
-      final GamePlayer player = pl.getPlayerId(p);
-      if (player == null) {
-        throw new GameParseException("Could not find player. name:" + p + thisErrorMsg());
-      }
-      players.add(player);
-    }
+    players = parsePlayerList(names, players);
   }
 
   private void setPlayers(final List<GamePlayer> value) {
     players = value;
   }
 
   protected List<GamePlayer> getPlayers() {
-    return players.isEmpty() ? new ArrayList<>(List.of((GamePlayer) getAttachedTo())) : players;
+    return players == null
+        ? List.of((GamePlayer) getAttachedTo())
+        : Collections.unmodifiableList(players);
   }
 
   private void resetPlayers() {
-    players = new ArrayList<>();
+    players = null;
   }
 
   @Override
@@ -166,7 +164,7 @@ private void setGameProperty(final String value) {
     gameProperty = value;
   }
 
-  private String getGameProperty() {
+  private @Nullable String getGameProperty() {
     return gameProperty;
   }
 
@@ -179,10 +177,6 @@ private void resetGameProperty() {
   }
 
   private void setRounds(final String rounds) throws GameParseException {
-    if (rounds == null) {
-      turns = null;
-      return;
-    }
     turns = new HashMap<>();
     final String[] s = splitOnColon(rounds);
     if (s.length < 1) {
@@ -217,17 +211,17 @@ private void setRounds(final Map<Integer, Integer> value) {
 
   @VisibleForTesting
   public Map<Integer, Integer> getRounds() {
-    return turns;
+    return getMapProperty(turns);
   }
 
   private void resetRounds() {
     turns = null;
   }
 
-  protected boolean checkTurns(final GameState data) {
+  protected boolean checkRounds(final GameState data) {
     final int turn = data.getSequence().getRound();
-    for (final int t : turns.keySet()) {
-      if (turn >= t && turn <= turns.get(t)) {
+    for (final Map.Entry<Integer, Integer> entry : getRounds().entrySet()) {
+      if (turn >= entry.getKey() && turn <= entry.getValue()) {
         return true;
       }
     }