Memory leak with PSwingRepaintManager

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create and display a PSwingCanvas on a panel in an application.
2. Move to a different part of the application where the panel is no longer
visible and should be garbage collected.

What is the expected output? What do you see instead?
I am expecting the PSwingCanvas and the panel it is on to be eligible for
garbage collection. However, analysis of the Java heap shows the panel and
PSwingCanvas are still reachable from the root set via the
PSwingRepaintManager singleton referenced from the static
PSwingCanvas.pSwingRepaintManager field. (this is more noticable if the
panel also contains a JTable with 70,000 rows!) 

The PSwingRepaintManager is a singleton and it accumulates references to
all PSwingCanvas instances that have ever been painted. This causes the
entire component tree rooted at every PSwingCanvas to not be eligible for
garbage collection. 

The simplest and most reliable way to fix this problem would be to keep
weak references to the PSwingCanvases so the singleton on its own can't
keep them alive.

The following patch implements this suggestion and it has been shown to work.

### Eclipse Workspace Patch 1.0
#P piccolo2d
Index:
extras/src/main/java/edu/umd/cs/piccolox/pswing/PSwingRepaintManager.java
===================================================================

---
extras/src/main/java/edu/umd/cs/piccolox/pswing/PSwingRepaintManager.java
(revision 425)
+++
extras/src/main/java/edu/umd/cs/piccolox/pswing/PSwingRepaintManager.java
(working copy)
@@ -32,8 +32,8 @@

 import javax.swing.*;
 import java.awt.*;
-import java.util.ArrayList;
 import java.util.Vector;
+import java.util.WeakHashMap;

 /**
  * This RepaintManager replaces the default Swing implementation, and is
used to
@@ -70,7 +70,12 @@
  * @author Sam R. Reid
  */
 public class PSwingRepaintManager extends RepaintManager {
-    private ArrayList swingWrappers = new ArrayList();
+   
+   /** 
+    * All keys in this map are weak references to swing wrappers. The values 
+    * are all null! 
+    */
+    private final WeakHashMap swingWrappers = new WeakHashMap();

     // The components that are currently painting
     // This needs to be a vector for thread safety
@@ -131,7 +136,7 @@
         // need to translate the repaint request since the component may
         // be offset inside another component.
         for (Component comp = c; comp != null && comp.isLightweight() &&
!captureRepaint; comp = comp.getParent()) {
-            if (swingWrappers.contains(comp.getParent())) {
+            if (swingWrappers.containsKey(comp.getParent())) {
                 if (comp instanceof JComponent) {
                     captureRepaint = true;
                     capturedComponent = (JComponent) comp;
@@ -197,6 +202,6 @@
     }

     void addPSwingCanvas(PSwingCanvas swingWrapper) {
-        swingWrappers.add(swingWrapper.getSwingWrapper());
+        swingWrappers.put(swingWrapper.getSwingWrapper(), null);
     }
 }
\ No newline at end of file

Original issue reported on code.google.com by jfuerth@gmail.com on 20 Jan 2009 at 10:34

[GoogleCodeExporter]

Thanks for the analysis.

I still need some convincing on your workaround, though.  Using weak references 
is
usually a cover for sloppy programming.  In this case, wrappers should be 
removed
from that ArrayList when their parent components are hidden/closed, if 
possible.  I
will investigate.

Original comment by heue...@gmail.com on 21 Jan 2009 at 3:26

• Changed state: Accepted
• Added labels: Type-Defect, Priority-High, Effort-Medium, Toolkit-Piccolo2D.Java, Component-Extras, Milestone-1.3

[GoogleCodeExporter]

I don't normally use weak references in my own programming, and I'm not terribly
attached to the fix we've submitted. Here's what I can say in its defense:

1. It works :)

2. The swingWrappers list in the PSwingRepaintManager is basically a cache of 
live,
reachable swing wrappers that have been encountered before. Because those 
semantics
(live, reachable) match weak references exactly, no other solution will work 
better.
A correctly implemented alternative solution could be just as effective, but it 
will
involve adding more lines of code to PSwing.

So, you'll get no killer arguments from me. As long as the fix is 100% 
effective, and
released soon, I'll be happy!

Original comment by jfuerth@gmail.com on 11 Feb 2009 at 8:20

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

heuermh has suggested an alternative fix that's impressive in it's elegance.

It seems possible to do away with the swingWrappers entirely, this would 
completely 
eliminate any references and would hence kill the possibly for a memory leak 
from this 
code.

He's pushing off implementing it until he can demonstrate to himself that the 
change is 
necessary.

Original comment by allain.lalonde on 17 Jul 2009 at 7:40

[GoogleCodeExporter]

Committing an example that attempts to demonstrate the leak.  The behaviour 
with or
without the fix Allain alluded to before doesn't appear any different to me.  
Used
memory is not reclaimed when PSwingCanvases are removed, as might be expected.

$ commit -m "Issue 74 ; adding PSwingMemoryLeakExample to examples" .
  Adding
examples/src/main/java/edu/umd/cs/piccolo/examples/pswing/PSwingMemoryLeakExampl
e.java
  Transmitting file data ...
  Committed revision 491.

Original comment by heue...@gmail.com on 17 Jul 2009 at 8:38

[GoogleCodeExporter]

For what it's worth, used memory is not reclaimed when PSwingCanvases are 
removed
with the original patch too.  Perhaps the example is flawed.

Original comment by heue...@gmail.com on 17 Jul 2009 at 8:43

[GoogleCodeExporter]

Although I think the memory leak identified above is a problem.

It's not the only one.  Turns out that the memory leak exists in PCanvas, not 
just 
PSwingCanvas.  By looking through a heap dump, I've found that the culprit is 
the call 
to installInputSources() in PCanvas' constructor.  If you call 
setEnabled(false) before 
removing the canvases from the example, the finalizers get called.  I'll update 
the 
example to confirm.

Original comment by allain.lalonde on 17 Jul 2009 at 11:11

[GoogleCodeExporter]

heuermh's  elegant solution I referred to above is attached.  It basically 
amounts to 
removing the need for tracking PSwingCanvas.SwingWrapper instances in the 
PSwingRepaintManager.  With this patch applied and with the changes made to the 
PCanvas 
in r496, the memory leak demonstration does as its supposed to decrements the 
instances 
of PSwingCanvas.

Original comment by allain.lalonde on 18 Jul 2009 at 12:26

Attachments:

• PSwingRepaintManager-leak.diff

[GoogleCodeExporter]

Committed changes, test passes in Eclipse, doesn't pass on the command line or 
when
being run from Hudson. *Very stange*

Original comment by allain.lalonde on 18 Jul 2009 at 3:27

[GoogleCodeExporter]

PCanvas has a static variable CURRENT_ZCANVAS referencing the canvas created 
last.
This might be an explanation for pCanvasFinalizerCount being one less than 
expected
in PSwingRepaintManagerTest.java.

I am not sure if CURRENT_ZCANVAS is required but i used to set it to null. 
However
this did not solve the memory leak problem completely.

Original comment by nls...@gmail.com on 18 Jul 2009 at 10:02

[GoogleCodeExporter]

Marking as fixed.  Several commits were made on this issue between Allain and 
myself,
so please review svn trunk.

Original comment by heue...@gmail.com on 28 Jul 2009 at 2:31

• Changed state: Fixed

[GoogleCodeExporter]

Verifying that is was  fixed in r501. By not needing to keep a reference, the 
memory
leak disappears.

Original comment by allain.lalonde on 5 Nov 2009 at 3:24

• Changed state: Verified