Memory leaks due to TrackableTypes?

[Myrd]

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

Apologies once again. Committed a fix.

[Agetian]

Thanks, Myrd! No worries, Forge is quite difficult to update without breaking stuff :/

- Agetian

[Myrd]

Okay, I've attempted to fix the leak in r32893 (and some follow-up fixes in the next two commits).

What I did is I moved the objLookup maps from being held by TrackableTypes (which were global) to instead be managed by the Tracker object, which is per-game. This way, any state saves in these maps will be cleaned up when a Game goes away and we no longer need to explicitly make calls to clearLookupDictionary() when switching games.

Let me know if you notice any problems with the above!

(This still doesn't solve the other point about inefficient memory use of the EnumMaps on the TrackableObject, though.)

[Hanmac]

hey about that objLookup while looking at "HashMap<TrackableType<?>, Object>" where Object is another HashMap<Integer, T>, wouldn't a Table<TrackableType<?>, Integer, T> be better?
i didn't checked it yet if thats possible of it would need another base class/interface.

i think the lookup of it would look the same, like we can get the Map<Integer, T> if we want, but also a Map<TrackableType<?>, T> if we might want that too.
even a shortcut with get(TrackableType<?>, Integer) would be possible.

Edit: i tested with ? and T and got to the solution that:
"Table<TrackableType<?>, Integer, Object>" would work.
Now i need to see if the methods would look better.

PS: Myrd did you checkout my test cases for Damage yet?
i have some more where i test against wither and Melira, Sylvok Outcast but i don't know of o should do it in the same test case or if i should create different onces.

[Myrd]

hey about that objLookup while looking at "HashMap<TrackableType<?>, Object>" where Object is another HashMap<Integer, T>, wouldn't a Table<TrackableType<?>, Integer, T> be better?
i didn't checked it yet if thats possible of it would need another base class/interface.

i think the lookup of it would look the same, like we can get the Map<Integer, T> if we want, but also a Map<TrackableType<?>, T> if we might want that too.
even a shortcut with get(TrackableType<?>, Integer) would be possible.

Edit: i tested with ? and T and got to the solution that:
"Table<TrackableType<?>, Integer, Object>" would work.
Now i need to see if the methods would look better.

I don't have an opinion either way - so if you think it's worth it to get a cleaner end result, by all means go for it! My goal was to fix the memory leaks which either version does.

PS: Myrd did you checkout my test cases for Damage yet?
i have some more where i test against wither and Melira, Sylvok Outcast but i don't know of o should do it in the same test case or if i should create different onces.

Yeah, I saw them. I don't have any objection in using the simulation test for this - sure one could argue that it's unrelated, but the side benefit is it's also adding extra test coverage for the simulation code - which I'm certainly not against! So it seems if it's easy to keep adding tests there then by all means we can keep doing this. If the test class grows too large at some point, we can consider splitting it up, but I don't think it will be much of an issue for a while...

[Agetian]

A little heads-up: at the moment, line 228 in SpellAbilityPicketTest.java shows an error (this line in particular):

Code: Select all

List<String> choices = picker.getPlan().getDecisions().get(0).choices;


Maybe the relevant code was changed but the test wasn't updated or something?..

- Agetian

[Myrd]

A little heads-up: at the moment, line 228 in SpellAbilityPicketTest.java shows an error (this line in particular):

Code: Select all

List<String> choices = picker.getPlan().getDecisions().get(0).choices;


Maybe the relevant code was changed but the test wasn't updated or something?..

- Agetian

What's the error? That line should be correct - and I don't see an error on my end. It used to be that the field was called "choice" and was a String, but "choices" is the new name and it's now a List<String> in Plan.Decision. Is that not the case for you?

[Agetian]

Ah, nevermind, looks like it was just NetBeans being stupid or something, it showed an error even though the class was indeed correct. I reset the cache and it's not showing an error anymore.

- Agetian

[mcrawford620]

I don't know if this is related, but I just updated to r33133 (first time in a couple of months) and I can't build. In SpellAbilityChoicesIterator.java, I get an error about AllowRepeatModesIterator -- it doesn't implement the remove() method of Iterator. So it won't compile.

Is there something else I need to do?

[Agetian]

I don't know if this is related, but I just updated to r33133 (first time in a couple of months) and I can't build. In SpellAbilityChoicesIterator.java, I get an error about AllowRepeatModesIterator -- it doesn't implement the remove() method of Iterator. So it won't compile.

Is there something else I need to do?

This is strange, can you try rebuilding with dependencies (first try "Clean and Build", then "Build with Dependencies") to make sure everything is up to date? (not sure which IDE you're using, this is in NetBeans terms, but I'm sure other IDEs have similar functionality). I'm not getting any compile errors there, so possibly it's just something not fully up to date on your end or something... I had an error like that recently that I reported and then it turned out I had to re-update, clean the project and rebuild it with dependencies and the error went away :/

- Agetian

[mcrawford620]

OK, I found the equivalent for IDEA, which I think is Invalidating the Caches and restarting. And I still get the same error.

The error seems pretty straightforward -- there's an internal class that claims to implement java.util.Iterator, but it does not implement the remove() method. So I don't see how it could compile. It's almost like some compilation settings are wrong, like maybe that directory is supposed to be ignored or something? Can you open up the file in NetBeans and see if it highlights it with an error?

I can go back to, say, r32900 and compile just fine. But when SpellAbilityChoicesIterator was broken out into a separate class, it breaks the compilation.

[Agetian]

Hmm, I definitely see your point, but I just went there and it's not showing any errors there :/ Some warnings about non-final fields, but that's about it.

- Agetian

[friarsol]

I don't know if this is related, but I just updated to r33133 (first time in a couple of months) and I can't build. In SpellAbilityChoicesIterator.java, I get an error about AllowRepeatModesIterator -- it doesn't implement the remove() method of Iterator. So it won't compile.

Is there something else I need to do?

The same thing happened to me. Can someone on Java 8 add the line and see if things still work? We probably should commit that method definiteion.

[Agetian]

Looks like Java 7 needs the implementation while Java 8 provides a default implementation, which is why things compile on Java 8 but do not compile on Java 7. I committed a simple default stub for the method in r33159-33160, please let me know if things start to compile correctly on Java 7 now.

- Agetian

[mcrawford620]

Yay, thanks, it works now.