It is currently 14 Nov 2019, 20:02
   
Text Size

Memory leaks due to TrackableTypes?

Post MTG Forge Related Programming Questions Here

Moderators: timmermac, friarsol, Blacksmith, KrazyTheFox, Agetian, CCGHQ Admins

Re: Memory leaks due to TrackableTypes?

Postby Myrd » 27 Dec 2016, 17:25

Agetian wrote: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.
Myrd
 
Posts: 87
Joined: 24 Nov 2014, 05:58
Has thanked: 4 times
Been thanked: 31 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 27 Dec 2016, 17:32

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

- Agetian
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby Myrd » 30 Dec 2016, 19:36

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.)
Myrd
 
Posts: 87
Joined: 24 Nov 2014, 05:58
Has thanked: 4 times
Been thanked: 31 times

Re: Memory leaks due to TrackableTypes?

Postby Hanmac » 30 Dec 2016, 20:29

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.
Hanmac
 
Posts: 954
Joined: 06 May 2013, 18:44
Has thanked: 229 times
Been thanked: 158 times

Re: Memory leaks due to TrackableTypes?

Postby Myrd » 31 Dec 2016, 17:04

Hanmac wrote: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.

Hanmac wrote: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...
Myrd
 
Posts: 87
Joined: 24 Nov 2014, 05:58
Has thanked: 4 times
Been thanked: 31 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 09 Jan 2017, 06:05

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
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby Myrd » 09 Jan 2017, 14:28

Agetian wrote: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?
Myrd
 
Posts: 87
Joined: 24 Nov 2014, 05:58
Has thanked: 4 times
Been thanked: 31 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 09 Jan 2017, 14:40

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
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby mcrawford620 » 12 Jan 2017, 05:02

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?
mcrawford620
 
Posts: 111
Joined: 25 Jun 2012, 16:59
Has thanked: 55 times
Been thanked: 25 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 12 Jan 2017, 05:18

mcrawford620 wrote: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
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby mcrawford620 » 12 Jan 2017, 05:34

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.
mcrawford620
 
Posts: 111
Joined: 25 Jun 2012, 16:59
Has thanked: 55 times
Been thanked: 25 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 12 Jan 2017, 05:45

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
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby friarsol » 12 Jan 2017, 22:03

mcrawford620 wrote: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.
friarsol
Global Moderator
 
Posts: 7477
Joined: 15 May 2010, 04:20
Has thanked: 241 times
Been thanked: 936 times

Re: Memory leaks due to TrackableTypes?

Postby Agetian » 13 Jan 2017, 04:29

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
Agetian
Programmer
 
Posts: 3359
Joined: 14 Mar 2011, 05:58
Has thanked: 644 times
Been thanked: 499 times

Re: Memory leaks due to TrackableTypes?

Postby mcrawford620 » 14 Jan 2017, 21:47

Yay, thanks, it works now.
mcrawford620
 
Posts: 111
Joined: 25 Jun 2012, 16:59
Has thanked: 55 times
Been thanked: 25 times

PreviousNext

Return to Developer's Corner

Who is online

Users browsing this forum: No registered users and 2 guests


Who is online

In total there are 2 users online :: 0 registered, 0 hidden and 2 guests (based on users active over the past 10 minutes)
Most users ever online was 427 on 11 Nov 2019, 13:00

Users browsing this forum: No registered users and 2 guests

Login Form