JAL-3597 additional references nulled, bug in test corrected
authorgmungoc <g.m.carstairs@dundee.ac.uk>
Tue, 14 Jul 2020 08:43:50 +0000 (09:43 +0100)
committerBen Soares <b.soares@dundee.ac.uk>
Thu, 13 Aug 2020 22:07:46 +0000 (23:07 +0100)
src/jalview/gui/PCAPanel.java
src/jalview/gui/TreePanel.java
src/jalview/renderer/AnnotationRenderer.java
src/jalview/viewmodel/AlignmentViewport.java
test/jalview/gui/FreeUpMemoryTest.java

index 4f660a2..3512e42 100644 (file)
@@ -141,6 +141,12 @@ public class PCAPanel extends GPCAPanel
   protected void close_actionPerformed()
   {
     setPcaModel(null);
+    if (this.rc != null)
+    {
+      this.rc.sequencePoints = null;
+      this.rc.setAxisEndPoints(null);
+      this.rc = null;
+    }
   }
 
   @Override
index ca4f84e..6c9c892 100755 (executable)
@@ -159,6 +159,7 @@ public class TreePanel extends GTreePanel
         {
           getViewport().removePropertyChangeListener(listener);
         }
+        releaseReferences();
       }
     });
 
@@ -168,6 +169,17 @@ public class TreePanel extends GTreePanel
   }
 
   /**
+   * Ensure any potentially large object references are nulled
+   */
+  public void releaseReferences()
+  {
+    this.tree = null;
+    this.treeCanvas.tree = null;
+    this.treeCanvas.nodeHash = null;
+    this.treeCanvas.nameHash = null;
+  }
+
+  /**
    * @return
    */
   protected PropertyChangeListener addAlignmentListener()
index adca17e..671bf6b 100644 (file)
@@ -150,6 +150,7 @@ public class AnnotationRenderer
    */
   public void dispose()
   {
+    hiddenColumns = null;
     hconsensus = null;
     complementConsensus = null;
     hStrucConsensus = null;
index 8dcd1b3..063d4ab 100644 (file)
@@ -959,6 +959,7 @@ public abstract class AlignmentViewport
     ranges = null;
     currentTree = null;
     selectionGroup = null;
+    colSel = null;
     setAlignment(null);
   }
 
index 24697c0..277c27e 100644 (file)
@@ -22,6 +22,69 @@ import org.testng.annotations.Test;
 
 import junit.extensions.PA;
 
+/**
+ * Provides a simple test that memory is released when all windows are closed.
+ * <ul>
+ * <li>generates a reasonably large alignment and loads it</li>
+ * <li>performs various operations on the alignment</li>
+ * <li>closes all windows</li>
+ * <li>requests garbage collection</li>
+ * <li>asserts that the remaining memory footprint (heap usage) is 'not large'
+ * </li>
+ * </ul>
+ * If the test fails, this means that reference(s) to large object(s) have
+ * failed to be garbage collected. In this case:
+ * <ul>
+ * <li>set a breakpoint just before the test assertion in
+ * {@code checkUsedMemory}</li>
+ * <li>if the test fails intermittently, make this breakpoint conditional on
+ * {@code usedMemory > expectedMax}</li>
+ * <li>run the test to this point (and check that it is about to fail i.e.
+ * {@code usedMemory > expectedMax})</li>
+ * <li>use <a href="https://visualvm.github.io/">visualvm</a> to obtain a heap
+ * dump from the suspended process (and kill the test or let it fail)</li>
+ * <li>inspect the heap dump using visualvm for large objects and their
+ * referers</li>
+ * <li>Tips:</li>
+ * <ul>
+ * <li>Perform GC from the Monitor view in visualvm before requesting the heap
+ * dump</li>
+ * <li>View 'Objects' and filter classes to {@code jalview}. Sort columns by
+ * Count, or Size, and look for anything suspicious. For example, if the object
+ * count for {@code Sequence} is non-zero (it shouldn't be), pick any instance,
+ * and follow the chain of {@code references} to find which class(es) still hold
+ * references to sequence objects</li>
+ * <li>If this chain is impracticably long, re-run the test with a smaller
+ * alignment (set width=100, height=10 in {@code generateAlignment()}), to
+ * capture a heap which is qualitatively the same, but much smaller, so easier
+ * to analyse; note this requires an unconditional breakpoint</li>
+ * </ul>
+ * </ul>
+ * <p>
+ * <h2>Fixing memory leaks</h2>
+ * <p>
+ * Experience shows that often a reference is retained (directly or indirectly)
+ * by a Swing (or related) component (for example a {@code MouseListener} or
+ * {@code ActionListener}). There are two possible approaches to fixing:
+ * <ul>
+ * <li>Purist: ensure that all listeners and similar objects are removed when no
+ * longer needed. May be difficult, to achieve and to maintain as code
+ * changes.</li>
+ * <li>Pragmatic: null references to potentially large objects from Jalview
+ * application classes when no longer needed, typically when a panel is closed.
+ * This ensures that even if the JVM keeps a reference to a panel or viewport,
+ * it does not retain a large heap footprint. This is the approach taken in, for
+ * example, {@code AlignmentPanel.closePanel()} and
+ * {@code AnnotationPanel.dispose()}.</li>
+ * <li>Adjust code if necessary; for example an {@code ActionListener} should
+ * act on {@code av.getAlignment()} and not directly on {@code alignment}, as
+ * the latter pattern could leave persistent references to the alignment</li>
+ * </ul>
+ * Add code to 'null unused large object references' until the test passes. For
+ * a final sanity check, capture the heap dump for a passing test, and satisfy
+ * yourself that only 'small' or 'harmless' {@code jalview} object instances
+ * (such as enums or singletons) are left in the heap.
+ */
 public class FreeUpMemoryTest
 {
   private static final int ONE_MB = 1000 * 1000;
@@ -32,8 +95,9 @@ public class FreeUpMemoryTest
   @BeforeClass(alwaysRun = true)
   public void setUp()
   {
-    Jalview.main(new String[] { "-nonews", "-props",
-        "test/jalview/testProps.jvprops" });
+    Jalview.main(
+            new String[]
+            { "-nonews", "-props", "test/jalview/testProps.jvprops" });
     String True = Boolean.TRUE.toString();
     Cache.applicationProperties.setProperty("SHOW_ANNOTATIONS", True);
     Cache.applicationProperties.setProperty("SHOW_QUALITY", True);
@@ -42,39 +106,14 @@ public class FreeUpMemoryTest
     Cache.applicationProperties.setProperty("SHOW_IDENTITY", True);
   }
 
-  /**
-   * A simple test that memory is released when all windows are closed.
-   * <ul>
-   * <li>generates a reasonably large alignment and loads it</li>
-   * <li>performs various operations on the alignment</li>
-   * <li>closes all windows</li>
-   * <li>requests garbage collection</li>
-   * <li>asserts that the remaining memory footprint (heap usage) is 'not large'
-   * </li>
-   * </ul>
-   * If the test fails, this suggests that a reference to some large object
-   * (perhaps the alignment data, or some annotation / Tree / PCA data) has
-   * failed to be garbage collected. If this is the case, the heap will need to
-   * be inspected manually (suggest using jvisualvm) in order to track down
-   * where large objects are still referenced. The code (for example
-   * AlignmentViewport.dispose()) should then be updated to ensure references to
-   * large objects are set to null when they are no longer required.
-   * 
-   * @throws IOException
-   */
   @Test(groups = "Memory")
   public void testFreeMemoryOnClose() throws IOException
   {
+    long expectedMin = 37L;
+
     File f = generateAlignment();
     f.deleteOnExit();
 
-    long expectedMin = 35L;
-    long usedMemoryAtStart=getUsedMemory();
-    if (usedMemoryAtStart>expectedMin)
-    {
-      System.err.println("used memory before test is "+usedMemoryAtStart+" > "+expectedMin+"MB .. adjusting minimum.");
-      expectedMin = usedMemoryAtStart;
-    }
     doStuffInJalview(f);
 
     Desktop.instance.closeAll_actionPerformed(null);
@@ -89,6 +128,7 @@ public class FreeUpMemoryTest
     long usedMemory = availableMemory - freeMemory;
     return usedMemory;
   }
+
   /**
    * Requests garbage collection and then checks whether remaining memory in use
    * is less than the expected value (in Megabytes)
@@ -129,11 +169,11 @@ public class FreeUpMemoryTest
      * - identify large objects in the heap and their referers
      * - fix code as necessary to null the references on close
      */
-    System.out.println("Used memory after gc = " + usedMemory + "MB");
+    System.out.println("Used memory after gc = " + usedMemory
+            + "MB (should be <" + expectedMax + ")");
     assertTrue(usedMemory < expectedMax, String.format(
             "Used memory %d should be less than %d (Recommend running test manually to verify)",
-            usedMemory,
-            expectedMax));
+            usedMemory, expectedMax));
   }
 
   /**
@@ -193,7 +233,7 @@ public class FreeUpMemoryTest
      * wait until Tree and PCA have been computed
      */
     while (af.viewport.getCurrentTree() == null
-            && dialog.getPcaPanel().isWorking())
+            || dialog.getPcaPanel().isWorking())
     {
       waitFor(10);
     }
@@ -237,6 +277,7 @@ public class FreeUpMemoryTest
     int width = 100000;
     int height = 100;
     ag.generate(width, height, 0, 10, 15);
+    ps.close();
     return f;
   }
 }