Memory leaks due to TrackableTypes?

[Myrd]

So I've been working on improving the Simulated AI system I started some time back.

One property of this system is it currently creates many, many temporary games as part of its simulation, concurrently with the actual real game that's being played. I've noticed that this results in ever-increasing memory growth and tracked this down to apparent memory leaks as a result of the TrackableTypes system.

I'm not very familiar with the purpose of this system, but as far as I understand, what's happening is as follows:

- There are some static TrackableTypes objects - for example TrackableTypes.StackItemViewType - which means any other objects transitively referenced by these will not be garbage collected.
- It appears these end up referencing a lot of view objects (e.g. StackItemView) over time. Each of which, by virtue of being a TrackableObject holds a "props" object which is an EnumMap that's backed by a 129-entry array.

So, I have a a few concerns / questions about the above:

1. Does it even makes sense to have the static global TrackableTypes? Given that it's possible to have multiple Game objects concurrently (e.g. at the very least in the simulated AI mode behind the scenes), it seems any indexing into these by GameObject ids would not be correct - since each game has a distinct id namespace and there can be multiple objects with the same id if they're in different games. However, maybe I'm misunderstanding something here? (As I've not noticed any behaviour problems because of this.). If my concerns are valid, then a solution would be to have TrackableTypes.StackItemViewType and other similar things that refer to game objects to hang off the Game object instead of being statics. Then, their contents could be garbage collected when the Game is no longer referenced.

2. What's responsible for clearing entries out of those collections? If moving them to be per-Game doesn't make sense, then maybe I just need to make the simulation code forcefully clear them somehow?

3. We should probably switch the TrackableObject's props map to something more memory efficient - as it seems wasteful to have a 129-slot array allocated when most of the time something like 10 properties are set (from inspection). Maybe there's some good map classes in some of the project's dependencies that already solve this problem.

[Hanmac]

I never fully understand what the TrackableObjects are for, but good that you are fighting against memory usage. I noticed that after a few games Forge is getting slower. That might be part of it.

PS: what I also wanted to ask you is that I did some refactor with Splice and something with Order SpellAbility. Maybe there need to be some Simulation part for that too?

[Agetian]

There is indeed something leaking memory over time, I thought it might be the image cache but I was wrong, you may be right about TrackableObjects. The more games you play, the slower things get, and eventually after a long match with many objects the game can crash with an OutOfMemory error (especially common to see on Android version of Forge since mobile devices are generally slower and have less memory than modern PCs).

I have to say I don't fully understand the TrackableObjects system, if I remember right it might have been implemented by Dan (drdev) unless I'm confusing something. I don't remember what exactly the purpose was, but maybe to keep track of object data for view updates?.. :/ At any rate, you can try PMing Dan and asking him if he's familiar with this system (he responded to me recently even though he's too busy at the moment to contribute actively), hopefully he can shed some light on the details of what this does and how it works.

Thank you very much for your ongoing effort with the simulated AI, by the way! And if you can make Forge more memory-efficient and stop leaking memory over time, it would be great as well!

EDIT: Looks like AbstractGuiBase::setGameView is making use of this class and clears up at least some of the elements:

Code: Select all

    @Override
    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;
            return;
        }

        //if game view set to another instance without being first cleared,
        //update existing game view object instead of overwriting it
        gameView.copyChangedProps(gameView0);
    }


- Agetian

[Myrd]

All right, I sent a PM to drdev and asked him to take a look at this post.

PS: what I also wanted to ask you is that I did some refactor with Splice and something with Order SpellAbility. Maybe there need to be some Simulation part for that too?

Possibly. Truth is, there is a lot of improvements that can still be done to the Simulation code. Anything it doesn't handle explicitly falls back the the regular AI code, which might be OK in the mean time.

Splice isn't a focus for me right now for simulation, but I suspect if there is something lacking there right now, it would be the extra decision making around what to splice together. Conceptually, you'd want the Simulation AI to evaluate the different splice decisions separately and consider them against each other and other actions. Which means extra tracking of them and adding them to the decision tree. Once added, then the rest of the Simulation AI system should handle these correctly - assuming it can detect from its game state scoring code that splicing is better than not. (At the very least, it would be an extra card in hand and likely less tapped lands, which should be true - but also it would be able to tell that it may be able to cast it multiple times in a sequence, etc.)

[Myrd]

3. We should probably switch the TrackableObject's props map to something more memory efficient - as it seems wasteful to have a 129-slot array allocated when most of the time something like 10 properties are set (from inspection). Maybe there's some good map classes in some of the project's dependencies that already solve this problem.

So I experimented in switching to use THashMap from trove4j, which has better memory use. Indeed, I was able to cut down memory use when using this, however I also noticed performance of simulation AI code decreased quite a bit (~10% slower) - probably due to resizing of the underlying maps' storage (whereas an EnumMap never needs to resize since its backing is always the number of enums). So I'm thinking the best thing would be to probably split TrackableProperty into different enums (each of which having much fewer entries and thus an efficient EnumMap). Logically it is already split up that way - i.e. cards have one set of properties and players have another - so it's pretty inefficient that every TrackableObject allocates space for every possible property - even those it can't possibly have.

If we don't want to split the enum into multiple ones (due to how the code is structured), one possibility is for each trackable type to define an intermediate mapping - so if a type supports 10 properties, the TrackableObject can use an Object[10] array for its map and then map from indexes into this array to indexes into the enum when needed.

.... But the above still doesn't raise the more important concern of the design of this overall system and whether it's leaking memory.

[Agetian]

I have temporarily reverted r32802 (which created the FaceDown characteristic on the card lazily) because it had caused several glitches to appear - namely, the visualization of cards in their Transformed, Flip and Meld alternate states was broken; not sure if the functional side of things was affected in any way. Please feel free to revert my revert (r32808) and update it when possible, Maybe more tests may need to be made with other cards that request an alternate or facedown state (Morph, Manifest, etc.) btw, if possible.

- Agetian

[Hanmac]

The problem is hasAlternateState because the check is not that easy anymore when they dont have face down as default anymore.

[Myrd]

Sorry about that. I've now relanded it with an appropriate fix for hasAlternateState() with the new set up. In my limited testing it seems to work fine (checked morph and DFC cards) - but let me know if you see any other glitches.

[Agetian]

Thanks a lot for following this through, Myrd! I'll give it a try and I'll let you know if I spot any odd behavior.

By the way, I've been playing around with the simulation AI lately and I have to say it's shaping up to be a really impressive achievement! I have noticed some interesting and unexpected plays by the AI in this mode. One thing that might need improving in the future (not sure if this is planned) is evaluation of a spell or an ability with the opponent's creatures (potential attackers/blockers) in mind. For example, right now the AI will happily Outlast all its creatures as long as it has enough mana (and enough creatures with Outlast), even if it runs into a lethal attack from the opponent next turn (maybe this has to do with weighting the abilities that require creatures to tap somehow differently depending on what the player has on the battlefield or something like that, I'm not sure :/ ). But all things considered, great job so far! Keep it up and best of luck!

Oh, and currently the newly implemented Dash test fails at the line which asserts that score2 should be greater than score.

- Agetian

[Myrd]

By the way, I've been playing around with the simulation AI lately and I have to say it's shaping up to be a really impressive achievement! I have noticed some interesting and unexpected plays by the AI in this mode. One thing that might need improving in the future (not sure if this is planned) is evaluation of a spell or an ability with the opponent's creatures (potential attackers/blockers) in mind. For example, right now the AI will happily Outlast all its creatures as long as it has enough mana (and enough creatures with Outlast), even if it runs into a lethal attack from the opponent next turn (maybe this has to do with weighting the abilities that require creatures to tap somehow differently depending on what the player has on the battlefield or something like that, I'm not sure :/ ). But all things considered, great job so far! Keep it up and best of luck!

Yep, I definitely want to improve this and it should be exactly as you say. I think I had started something like this (see simulateUpcomingCombatThisTurn() in GameStateEvaluator) but it needs quite a bit of work to be really effective. It's not something I'm currently working on - so if you'd like to take a stab at it, all help is appreciated!

Oh, and currently the newly implemented Dash test fails at the line which asserts that score2 should be greater than score.

- Agetian

Interesting - it doesn't fail for me - but I do have some local changes right now, so perhaps something I have changed locally is fixing it? I can investigate in a bit, but first can you make sure it's not related to some local changes in your client?

And by the way, the simulated AI is definitely still a work in progress. Still lots, lots, lots of areas for improvement as well as functionality fixes (e.g. the game copying stuff is definitely not completely perfect and needs improvement as more edge cases are found).

[Agetian]

I tried checking out a completely clean copy of SVN (trunk) and compiling it and still got exactly the same test failure as I had with my working copy. I'll post the full log here:

Code: Select all

Tests run: 30, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 53.391 sec <<< FAILURE! - in TestSuite
testDash on testDash(forge.ai.simulation.GameSimulatorTest)(forge.ai.simulation.GameSimulatorTest)  Time elapsed: 0.039 sec  <<< FAILURE!
java.lang.AssertionError: null
   at forge.ai.simulation.GameSimulatorTest.testDash(GameSimulatorTest.java:617)


Hope it'll help!
And I'll start looking at the simulation code, I'll see if I can find an opportunity to contribute something meaningful to it.

- Agetian

[Hanmac]

@Myrd: I think your changes with SpellAbility copy are a bit wrong.
Specially when you call setHostCard because you call it on the original and not on the copy.

Also it might not even needed. The holt should be updated when the SpellAbility is added to the new card. Also you did call clone.hostcard = newhost before. With that setHostCard should not have any effect (it does because you call it on the wrong object)

[Myrd]

@Myrd: I think your changes with SpellAbility copy are a bit wrong.
Specially when you call setHostCard because you call it on the original and not on the copy.

Also it might not even needed. The holt should be updated when the SpellAbility is added to the new card. Also you did call clone.hostcard = newhost before. With that setHostCard should not have any effect (it does because you call it on the wrong object)

Whoops, thanks for noticing and you're right. However, the reason I had put the line that assigns hostCard before is because the logic below it was adding the SA to the Game of the host card, but that is not correct if the clone will be part of a different Game. So instead, I reverted my changes now and moved that bit (adding it to the Game) to when the SpellAbility is set on a Card.

[Myrd]

I tried checking out a completely clean copy of SVN (trunk) and compiling it and still got exactly the same test failure as I had with my working copy. I'll post the full log here:

Code: Select all

Tests run: 30, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 53.391 sec <<< FAILURE! - in TestSuite
testDash on testDash(forge.ai.simulation.GameSimulatorTest)(forge.ai.simulation.GameSimulatorTest)  Time elapsed: 0.039 sec  <<< FAILURE!
java.lang.AssertionError: null
   at forge.ai.simulation.GameSimulatorTest.testDash(GameSimulatorTest.java:617)


Aha, I figured out what's wrong. Turns out I used assert() - Java language construct - instead of assertTrue() - JUnit API. And I guess for some reason I was running the test in a mode that had assert statements disabled? Not sure. Anyway, I confirmed that it's false for me too and removed the check for now.

[Agetian]

It looks like r32823 is breaking Commander (it becomes impossible to cast the Commander from command zone at all), thus also breaking game modes dependent on it (e.g. Planar Conquest). When you have a minute, can you please take a look what's up?

Original bug report here:
viewtopic.php?f=52&t=6333&p=207265#p207265

- Agetian