From f6d3b491c392914981b65fee21df51b530ee80f7 Mon Sep 17 00:00:00 2001 From: gmungoc Date: Tue, 14 Jul 2020 09:43:50 +0100 Subject: [PATCH] JAL-3597 additional references nulled, bug in test corrected --- src/jalview/gui/PCAPanel.java | 6 ++ src/jalview/gui/TreePanel.java | 12 +++ src/jalview/renderer/AnnotationRenderer.java | 1 + src/jalview/viewmodel/AlignmentViewport.java | 1 + test/jalview/gui/FreeUpMemoryTest.java | 107 ++++++++++++++++++-------- 5 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/jalview/gui/PCAPanel.java b/src/jalview/gui/PCAPanel.java index 4f660a2..3512e42 100644 --- a/src/jalview/gui/PCAPanel.java +++ b/src/jalview/gui/PCAPanel.java @@ -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 diff --git a/src/jalview/gui/TreePanel.java b/src/jalview/gui/TreePanel.java index ca4f84e..6c9c892 100755 --- a/src/jalview/gui/TreePanel.java +++ b/src/jalview/gui/TreePanel.java @@ -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() diff --git a/src/jalview/renderer/AnnotationRenderer.java b/src/jalview/renderer/AnnotationRenderer.java index adca17e..671bf6b 100644 --- a/src/jalview/renderer/AnnotationRenderer.java +++ b/src/jalview/renderer/AnnotationRenderer.java @@ -150,6 +150,7 @@ public class AnnotationRenderer */ public void dispose() { + hiddenColumns = null; hconsensus = null; complementConsensus = null; hStrucConsensus = null; diff --git a/src/jalview/viewmodel/AlignmentViewport.java b/src/jalview/viewmodel/AlignmentViewport.java index 8dcd1b3..063d4ab 100644 --- a/src/jalview/viewmodel/AlignmentViewport.java +++ b/src/jalview/viewmodel/AlignmentViewport.java @@ -959,6 +959,7 @@ public abstract class AlignmentViewport ranges = null; currentTree = null; selectionGroup = null; + colSel = null; setAlignment(null); } diff --git a/test/jalview/gui/FreeUpMemoryTest.java b/test/jalview/gui/FreeUpMemoryTest.java index 24697c0..277c27e 100644 --- a/test/jalview/gui/FreeUpMemoryTest.java +++ b/test/jalview/gui/FreeUpMemoryTest.java @@ -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. + * + * If the test fails, this means that reference(s) to large object(s) have + * failed to be garbage collected. In this case: + * + *

+ *

Fixing memory leaks

+ *

+ * 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: + *

+ * 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. - * - * 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; } } -- 1.7.10.2