From 18ba55ae272a2b1d0b1bda259e60481a944ba8c1 Mon Sep 17 00:00:00 2001 From: Myrd Date: Fri, 30 Dec 2016 19:34:07 +0000 Subject: [PATCH] Attempt to fix memory leak with TrackableTypes. The problem was that TrackableTypes were all global (held in static variables), but kept around state (objLookup maps) that shouldn't be global - e.g. that could contain per game data. Additionally, it seems this state wasn't always getting cleared correctly and thus would cause leaking of memory over multiple games - which would especially be a problem for Simulated AI code which created temporary games for simulation. This change attempts to fix the issue by moving the storage of objLookup maps to the Tracker object, which corresponds to a single Game. This way, there is no cross-contamination between different games and the state is properly cleaned up when a Game goes away. --- .../java/forge/trackable/TrackableObject.java | 4 ++++ .../java/forge/trackable/TrackableTypes.java | 17 +++++++++-------- .../src/main/java/forge/trackable/Tracker.java | 16 ++++++++++++++++ .../main/java/forge/match/AbstractGuiGame.java | 4 ---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/forge-game/src/main/java/forge/trackable/TrackableObject.java b/forge-game/src/main/java/forge/trackable/TrackableObject.java index 2a2540aa568..3358f014e80 100644 --- a/forge-game/src/main/java/forge/trackable/TrackableObject.java +++ b/forge-game/src/main/java/forge/trackable/TrackableObject.java @@ -30,6 +30,10 @@ public abstract class TrackableObject implements IIdentifiable, Serializable { return id; } + public final Tracker getTracker() { + return tracker; + } + @Override public int hashCode() { return id; diff --git a/forge-game/src/main/java/forge/trackable/TrackableTypes.java b/forge-game/src/main/java/forge/trackable/TrackableTypes.java index 59b7436fdbc..4c1b4ec9eb7 100644 --- a/forge-game/src/main/java/forge/trackable/TrackableTypes.java +++ b/forge-game/src/main/java/forge/trackable/TrackableTypes.java @@ -37,13 +37,16 @@ public class TrackableTypes { } public static abstract class TrackableObjectType extends TrackableType { - private final HashMap objLookup = new HashMap(); - private TrackableObjectType() { } + protected HashMap getObjLookup(T obj) { + return obj.getTracker().getObjLookupForType(this); + } + public T lookup(T from) { if (from == null) { return null; } + HashMap objLookup = getObjLookup(from); T to = objLookup.get(from.getId()); if (to == null) { objLookup.put(from.getId(), from); @@ -52,12 +55,9 @@ public class TrackableTypes { return to; } - public void clearLookupDictionary() { - objLookup.clear(); - } - @Override protected void updateObjLookup(T newObj) { + HashMap objLookup = getObjLookup(newObj); if (newObj != null && !objLookup.containsKey(newObj.getId())) { objLookup.put(newObj.getId(), newObj); newObj.updateObjLookup(); @@ -68,6 +68,7 @@ public class TrackableTypes { protected void copyChangedProps(TrackableObject from, TrackableObject to, TrackableProperty prop) { T newObj = from.get(prop); if (newObj != null) { + HashMap objLookup = getObjLookup(newObj); T existingObj = objLookup.get(newObj.getId()); if (existingObj != null) { //if object exists already, update its changed properties existingObj.copyChangedProps(newObj); @@ -107,14 +108,14 @@ public class TrackableTypes { for (int i = 0; i < newCollection.size(); i++) { T newObj = newCollection.get(i); if (newObj != null) { - T existingObj = itemType.objLookup.get(newObj.getId()); + T existingObj = itemType.getObjLookup(newObj).get(newObj.getId()); if (existingObj != null) { //if object exists already, update its changed properties existingObj.copyChangedProps(newObj); newCollection.remove(i); newCollection.add(i, existingObj); } else { //if object is new, cache in object lookup - itemType.objLookup.put(newObj.getId(), newObj); + itemType.getObjLookup(newObj).put(newObj.getId(), newObj); } } } diff --git a/forge-game/src/main/java/forge/trackable/Tracker.java b/forge-game/src/main/java/forge/trackable/Tracker.java index 3e7750ba623..8fdc3e529b4 100644 --- a/forge-game/src/main/java/forge/trackable/Tracker.java +++ b/forge-game/src/main/java/forge/trackable/Tracker.java @@ -1,12 +1,16 @@ package forge.trackable; +import java.util.HashMap; import java.util.List; import com.google.common.collect.Lists; +import forge.trackable.TrackableTypes.TrackableType; + public class Tracker { private int freezeCounter = 0; private final List delayedPropChanges = Lists.newArrayList(); + private final HashMap, Object> objLookups = new HashMap<>(); public final boolean isFrozen() { return freezeCounter > 0; @@ -16,6 +20,18 @@ public class Tracker { freezeCounter++; } + // Note: objLookups exist on the tracker and not on the TrackableType because + // TrackableType is global and Tracker is per game. + public HashMap getObjLookupForType(TrackableType type) { + @SuppressWarnings("unchecked") + HashMap result = (HashMap) objLookups.get(type); + if (result == null) { + result = new HashMap<>(); + objLookups.put(type, result); + } + return result; + } + public void unfreeze() { if (!isFrozen() || --freezeCounter > 0 || delayedPropChanges.isEmpty()) { return; diff --git a/forge-gui/src/main/java/forge/match/AbstractGuiGame.java b/forge-gui/src/main/java/forge/match/AbstractGuiGame.java index 9554c5045ea..5f44e0d8de4 100644 --- a/forge-gui/src/main/java/forge/match/AbstractGuiGame.java +++ b/forge-gui/src/main/java/forge/match/AbstractGuiGame.java @@ -72,10 +72,6 @@ public abstract class AbstractGuiGame implements IGuiGame, IMayViewCards { public void setGameView(final GameView gameView0) { if (gameView == null || gameView0 == null) { if (gameView0 != null) { - //ensure lookup dictionaries are reset before each game - TrackableTypes.CardViewType.clearLookupDictionary(); - TrackableTypes.PlayerViewType.clearLookupDictionary(); - TrackableTypes.StackItemViewType.clearLookupDictionary(); gameView0.updateObjLookup(); } gameView = gameView0;