Memory leak - PActivityScheduler keeps processed activities in reference

[GoogleCodeExporter]

Piccolo's PActivityScheduler keeps activities in reference even after they are 
processed. This also means that any state that activities reference (nodes, 
e.g.) will also stay in reference and not get garbage collected.

This example demonstrates the problem: 
http://github.com/atdixon/piccolo2d-examples/blob/master/src/main/java/atdixon/p
iccolo/example/ActivityMemoryLeakExample.java

This applies to Piccolo2d/Java, trunk and 1.3.

User group thread: 
http://groups.google.com/group/piccolo2d-users/browse_thread/thread/6e906b3afa5f
d749

Original issue reported on code.google.com by atdi...@gmail.com on 3 Aug 2010 at 11:31

• Blocked on: #195

[GoogleCodeExporter]

Attaching patch that fixes 185.

Original comment by atdi...@gmail.com on 3 Aug 2010 at 11:34

Attachments:

• Piccolo2D_Java_185.patch

[GoogleCodeExporter]

Thank you for the patch.  Would it be ok to include your example in the 
Piccolo2D source tree?  The standard Piccolo2D license header

http://code.google.com/p/piccolo2d/source/browse/piccolo2d.java/trunk/license-pi
ccolo.txt

would need to be added.  Best yet would be if you might include that as an 
attachment to this issue along with a patch to the <contributors> section of 
the Piccolo2D parent pom

http://code.google.com/p/piccolo2d/source/browse/piccolo2d.java/trunk/parent/pom
.xml

to mark your contribution.

I could then apply the patch to trunk and with minor changes (remove java 1.5+ 
syntax) to the release-1.3 branch.

Original comment by heue...@gmail.com on 4 Aug 2010 at 8:47

• Changed state: Accepted
• Added labels: Type-Defect, Priority-High, Effort-Medium, OpSys-All, Toolkit-Piccolo2D.Java, Component-Core, Milestone-1.3.1

[GoogleCodeExporter]

Patch attached which includes both:

- parent/pom.xml (contributors update)
- 
examples/src/main/java/org/piccolo2d/examples/ActivityMemoryLeakBugExample.java

What do you think of putting "bug" examples in separate package (e.g., 
org.piccolo2d.examples.bugs)?

Original comment by atdi...@gmail.com on 5 Aug 2010 at 2:54

Attachments:

• ActivityMemoryLeakBugExample.patch

[GoogleCodeExporter]

Applied to trunk

$ svn commit -m "Issue 185 ; applying patch from atdixon with minor changes, 
list of activities is cleared after activities are processed" .
Sending        
core/src/main/java/org/piccolo2d/activities/PActivityScheduler.java
Adding         
examples/src/main/java/org/piccolo2d/examples/ActivityMemoryLeakBugExample.java
Sending        parent/pom.xml
Transmitting file data ...
Committed revision 1038.

and release-1.3 branch

$ svn commit -m "Issue 185 ; applying patch from atdixon with minor changes, 
list of activities is cleared after activities are processed" .
Sending        
core/src/main/java/edu/umd/cs/piccolo/activities/PActivityScheduler.java
Adding         
examples/src/main/java/edu/umd/cs/piccolo/examples/ActivityMemoryLeakBugExample.
java
Sending        parent/pom.xml
Transmitting file data ...
Committed revision 1039.

Thanks again for your patch, please verify.

Created a new issue 186 for the org.piccolo2d.examples.bugs package idea.

Original comment by heue...@gmail.com on 6 Aug 2010 at 3:56

• Changed state: Fixed

[GoogleCodeExporter]

Verified in trunk and release-1.3. Thanks!

Original comment by atdi...@gmail.com on 8 Aug 2010 at 2:54

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 8 Aug 2010 at 2:56

• Changed state: Verified

[GoogleCodeExporter]

The patch renders the first call of processingActivities.clear(); redundant (in 
the same procedure the patch was applied to). I think the first line thus 
should be removed to keep the code clear. Furthermore the example uses piccolo 
from a thread other than the event dispatch thread. This possibly does not 
affect the issue demonstrated by the example, but is not the right way to go as 
piccolo is not thread-safe.

Original comment by nls...@gmail.com on 8 Aug 2010 at 9:42

Attachments:

• issue185fix.patch

[GoogleCodeExporter]

Thanks, nlskrg, I agree. I code read your patch and it looks correct to me.

Original comment by atdi...@gmail.com on 8 Aug 2010 at 7:01

[GoogleCodeExporter]

Reopening per comments 7 & 8.

Original comment by heue...@gmail.com on 9 Aug 2010 at 3:56

• Changed state: Started

[GoogleCodeExporter]

Is there any reason that ArrayList processingActivities needs to exist at all?  
Cannot processActivities iterate in reverse over the activities in activities 
directly?

Original comment by heue...@gmail.com on 24 Aug 2010 at 2:35

[GoogleCodeExporter]

The ArrayList activities may be modified while iterating over 
processingActivities, e.g., a finished activity is removed. I think, this isn't 
a problem when iterating in reverse, since it is always the last element that 
is removed. However, possibly other modification may occur triggered by 
activity delegates. Possibly such modifications should be supported by this 
temporary copy of the ArrayList activities?

Original comment by nls...@gmail.com on 24 Aug 2010 at 9:21

[GoogleCodeExporter]

Applied patch on trunk.

$ svn commit -m "Issue 185 ; applying patch issue185fix.patch" .
Committed revision 1085.

Original comment by heue...@gmail.com on 21 Dec 2010 at 6:06

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 21 Dec 2010 at 6:15

[GoogleCodeExporter]

Applied patch on release-1.3 branch.

$ svn commit -m "Issue 185 ; applying patch issue185fix.patch" .
Committed revision 1087.

Original comment by heue...@gmail.com on 21 Dec 2010 at 7:54

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by atdi...@gmail.com on 2 Apr 2011 at 2:35

• Changed state: Verified