From: Ben Soares Date: Thu, 3 Sep 2020 15:27:44 +0000 (+0100) Subject: Merge branch 'develop' of https://source.jalview.org/git/jalview into develop X-Git-Tag: Develop-2_11_2_0-d20201215~24^2~15 X-Git-Url: http://source.jalview.org/gitweb/?a=commitdiff_plain;h=f8691b979a068d0992608a51de66271f299dbb6b;hp=7c269dc87e6fda52db3d73d07bc5f3e879e1f681;p=jalview.git Merge branch 'develop' of https://source.jalview.org/git/jalview into develop --- diff --git a/help/help/html/keys.html b/help/help/html/keys.html index 1a5fc18..29b6813 100755 --- a/help/help/html/keys.html +++ b/help/help/html/keys.html @@ -78,34 +78,24 @@ Redo the last sequence edit undone. - Up Arrow - Normal - Moves selected sequence(s) up the alignment - - - Down Arrow - Normal - Moves selected sequence(s) down the alignment. - - - Left Arrow - Normal - Slides selected sequence(s) left. Press Alt key to slide - in cursor mode - - - Right Arrow - Normal - Slides selected sequence(s) right. Press Alt key to slide - in cursor mode - - Cursor Keys
(Arrow Keys) Cursor Move cursor around alignment + Cursor Keys
(Arrow Keys) + + Normal
+ (+Alt in Cursor) + Moves selected sequence(s) up, down, left, or right + according to the direction pressed.
+
+ Alt+Arrow key to move selection or sequence under cursor + in cursor mode. + + + Page Up Both Scroll up the alignment view diff --git a/src/jalview/ext/ensembl/EnsemblRestClient.java b/src/jalview/ext/ensembl/EnsemblRestClient.java index 771980c..59c568b 100644 --- a/src/jalview/ext/ensembl/EnsemblRestClient.java +++ b/src/jalview/ext/ensembl/EnsemblRestClient.java @@ -69,9 +69,9 @@ abstract class EnsemblRestClient extends EnsemblSequenceFetcher * @see https://github.com/Ensembl/ensembl-rest/wiki/Change-log * @see http://rest.ensembl.org/info/rest?content-type=application/json */ - private static final String LATEST_ENSEMBLGENOMES_REST_VERSION = "10.0"; + private static final String LATEST_ENSEMBLGENOMES_REST_VERSION = "13.0"; - private static final String LATEST_ENSEMBL_REST_VERSION = "10.0"; + private static final String LATEST_ENSEMBL_REST_VERSION = "13.0"; private static final String REST_CHANGE_LOG = "https://github.com/Ensembl/ensembl-rest/wiki/Change-log"; diff --git a/src/jalview/ext/so/SequenceOntology.java b/src/jalview/ext/so/SequenceOntology.java index 0d631e6..138aa58 100644 --- a/src/jalview/ext/so/SequenceOntology.java +++ b/src/jalview/ext/so/SequenceOntology.java @@ -20,8 +20,6 @@ */ package jalview.ext.so; -import jalview.io.gff.SequenceOntologyI; - import java.io.BufferedInputStream; import java.io.BufferedReader; import java.io.IOException; @@ -44,6 +42,9 @@ import org.biojava.nbio.ontology.Triple; import org.biojava.nbio.ontology.io.OboParser; import org.biojava.nbio.ontology.utils.Annotation; +import jalview.bin.Cache; +import jalview.io.gff.SequenceOntologyI; + /** * A wrapper class that parses the Sequence Ontology and exposes useful access * methods. This version uses the BioJava parser. @@ -114,7 +115,7 @@ public class SequenceOntology implements SequenceOntologyI } } long elapsed = System.currentTimeMillis() - now; - System.out.println("Loaded Sequence Ontology from " + zipFile + " (" + Cache.log.info("Loaded Sequence Ontology from " + zipFile + " (" + elapsed + "ms)"); } catch (Exception e) { @@ -183,19 +184,19 @@ public class SequenceOntology implements SequenceOntologyI boolean oldTermIsObsolete = isObsolete(replaced); if (newTermIsObsolete && !oldTermIsObsolete) { - System.err.println("Ignoring " + term.getName() + Cache.log.debug("Ignoring " + term.getName() + " as obsolete and duplicated by " + replaced.getName()); term = replaced; } else if (!newTermIsObsolete && oldTermIsObsolete) { - System.err.println("Ignoring " + replaced.getName() + Cache.log.debug("Ignoring " + replaced.getName() + " as obsolete and duplicated by " + term.getName()); } else { - System.err.println("Warning: " + term.getName() + Cache.log.debug("Warning: " + term.getName() + " has replaced " + replaced.getName() + " for lookup of '" + description + "'"); } @@ -336,7 +337,7 @@ public class SequenceOntology implements SequenceOntologyI { if (!termsNotFound.contains(term)) { - System.err.println("SO term " + term + " invalid"); + Cache.log.error("SO term " + term + " invalid"); termsNotFound.add(term); } } diff --git a/src/jalview/gui/AlignFrame.java b/src/jalview/gui/AlignFrame.java index 910ab63..8e7820f 100644 --- a/src/jalview/gui/AlignFrame.java +++ b/src/jalview/gui/AlignFrame.java @@ -1782,10 +1782,10 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, } /** - * DOCUMENT ME! + * Calls AlignmentI.moveSelectedSequencesByOne with current sequence selection or the sequence under cursor in keyboard mode * * @param up - * DOCUMENT ME! + * or down (if !up) */ public void moveSelectedSequences(boolean up) { @@ -1793,8 +1793,23 @@ public class AlignFrame extends GAlignFrame implements DropTargetListener, if (sg == null) { - return; + if (viewport.cursorMode) + { + sg = new SequenceGroup(); + sg.addSequence(viewport.getAlignment() + .getSequenceAt(alignPanel.getSeqPanel().seqCanvas.cursorY),false); + } else { + return; + } + } + + if (sg.getSize() < 1) + { + return; } + + // TODO: JAL-3733 - add an event to the undo buffer for this ! + viewport.getAlignment().moveSelectedSequencesByOne(sg, viewport.getHiddenRepSequences(), up); alignPanel.paintAlignment(true, false); diff --git a/src/jalview/gui/PCAPanel.java b/src/jalview/gui/PCAPanel.java index 96a8b0d..6c76c3e 100644 --- a/src/jalview/gui/PCAPanel.java +++ b/src/jalview/gui/PCAPanel.java @@ -140,6 +140,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/TreeCanvas.java b/src/jalview/gui/TreeCanvas.java index 29ba52b..40cf080 100755 --- a/src/jalview/gui/TreeCanvas.java +++ b/src/jalview/gui/TreeCanvas.java @@ -20,19 +20,6 @@ */ package jalview.gui; -import jalview.analysis.Conservation; -import jalview.analysis.TreeModel; -import jalview.api.AlignViewportI; -import jalview.datamodel.Sequence; -import jalview.datamodel.SequenceGroup; -import jalview.datamodel.SequenceI; -import jalview.datamodel.SequenceNode; -import jalview.gui.JalviewColourChooser.ColourChooserListener; -import jalview.schemes.ColourSchemeI; -import jalview.structure.SelectionSource; -import jalview.util.Format; -import jalview.util.MessageManager; - import java.awt.Color; import java.awt.Dimension; import java.awt.Font; @@ -49,9 +36,10 @@ import java.awt.print.PageFormat; import java.awt.print.Printable; import java.awt.print.PrinterException; import java.awt.print.PrinterJob; -import java.util.Enumeration; import java.util.Hashtable; import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.Vector; import javax.swing.JPanel; @@ -59,6 +47,19 @@ import javax.swing.JScrollPane; import javax.swing.SwingUtilities; import javax.swing.ToolTipManager; +import jalview.analysis.Conservation; +import jalview.analysis.TreeModel; +import jalview.api.AlignViewportI; +import jalview.datamodel.Sequence; +import jalview.datamodel.SequenceGroup; +import jalview.datamodel.SequenceI; +import jalview.datamodel.SequenceNode; +import jalview.gui.JalviewColourChooser.ColourChooserListener; +import jalview.schemes.ColourSchemeI; +import jalview.structure.SelectionSource; +import jalview.util.Format; +import jalview.util.MessageManager; + /** * DOCUMENT ME! * @@ -103,9 +104,9 @@ public class TreeCanvas extends JPanel implements MouseListener, Runnable, int labelLength = -1; - Hashtable nameHash = new Hashtable(); + Map nameHash = new Hashtable<>(); - Hashtable nodeHash = new Hashtable(); + Map nodeHash = new Hashtable<>(); SequenceNode highlightNode; @@ -378,31 +379,25 @@ public class TreeCanvas extends JPanel implements MouseListener, Runnable, */ public Object findElement(int x, int y) { - Enumeration keys = nameHash.keys(); - - while (keys.hasMoreElements()) + for (Entry entry : nameHash.entrySet()) { - Object ob = keys.nextElement(); - Rectangle rect = (Rectangle) nameHash.get(ob); + Rectangle rect = entry.getValue(); if ((x >= rect.x) && (x <= (rect.x + rect.width)) && (y >= rect.y) && (y <= (rect.y + rect.height))) { - return ob; + return entry.getKey(); } } - keys = nodeHash.keys(); - - while (keys.hasMoreElements()) + for (Entry entry : nodeHash.entrySet()) { - Object ob = keys.nextElement(); - Rectangle rect = (Rectangle) nodeHash.get(ob); + Rectangle rect = entry.getValue(); if ((x >= rect.x) && (x <= (rect.x + rect.width)) && (y >= rect.y) && (y <= (rect.y + rect.height))) { - return ob; + return entry.getKey(); } } @@ -464,9 +459,8 @@ public class TreeCanvas extends JPanel implements MouseListener, Runnable, if ((node.left() == null) && (node.right() == null)) { double height = node.height; - double dist = node.dist; - - int xstart = (int) ((height - dist) * wscale) + offx; +// double dist = node.dist; +// int xstart = (int) ((height - dist) * wscale) + offx; int xend = (int) (height * wscale) + offx; int ypos = (int) (node.ycount * chunk) + offy; @@ -651,13 +645,14 @@ public class TreeCanvas extends JPanel implements MouseListener, Runnable, { fm = g.getFontMetrics(font); - if (nameHash.size() == 0) + int nameCount = nameHash.size(); + if (nameCount == 0) { repaint(); } if (fitToWindow || (!fitToWindow && (scrollPane - .getHeight() > ((fm.getHeight() * nameHash.size()) + offy)))) + .getHeight() > ((fm.getHeight() * nameCount) + offy)))) { draw(g, scrollPane.getWidth(), scrollPane.getHeight()); setPreferredSize(null); @@ -665,8 +660,8 @@ public class TreeCanvas extends JPanel implements MouseListener, Runnable, else { setPreferredSize(new Dimension(scrollPane.getWidth(), - fm.getHeight() * nameHash.size())); - draw(g, scrollPane.getWidth(), fm.getHeight() * nameHash.size()); + fm.getHeight() * nameCount)); + draw(g, scrollPane.getWidth(), fm.getHeight() * nameCount); } scrollPane.revalidate(); diff --git a/src/jalview/gui/TreePanel.java b/src/jalview/gui/TreePanel.java index 37b4dac..2d8e729 100755 --- a/src/jalview/gui/TreePanel.java +++ b/src/jalview/gui/TreePanel.java @@ -159,6 +159,7 @@ public class TreePanel extends GTreePanel { av.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 c61606e..fa2900d 100644 --- a/src/jalview/renderer/AnnotationRenderer.java +++ b/src/jalview/renderer/AnnotationRenderer.java @@ -149,6 +149,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 4f399f0..0d26db1 100644 --- a/src/jalview/viewmodel/AlignmentViewport.java +++ b/src/jalview/viewmodel/AlignmentViewport.java @@ -963,6 +963,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..3a5a9e4 100644 --- a/test/jalview/gui/FreeUpMemoryTest.java +++ b/test/jalview/gui/FreeUpMemoryTest.java @@ -1,17 +1,8 @@ package jalview.gui; -import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; -import jalview.analysis.AlignmentGenerator; -import jalview.bin.Cache; -import jalview.bin.Jalview; -import jalview.datamodel.AlignmentI; -import jalview.datamodel.SequenceGroup; -import jalview.io.DataSourceType; -import jalview.io.FileLoader; - import java.awt.event.MouseEvent; import java.io.File; import java.io.IOException; @@ -20,20 +11,96 @@ import java.io.PrintStream; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import jalview.analysis.AlignmentGenerator; +import jalview.bin.Cache; +import jalview.bin.Jalview; +import jalview.datamodel.AlignmentI; +import jalview.datamodel.SequenceGroup; +import jalview.io.DataSourceType; +import jalview.io.FileLoader; import junit.extensions.PA; +/** + * Provides a simple test that memory is released when all windows are closed. + *
    + *
  • generates a reasonably large alignment and loads it
    • + *
  • performs various operations on the alignment
    • + *
  • closes all windows
    • + *
  • requests garbage collection
    • + *
  • asserts that the remaining memory footprint (heap usage) is 'not large' + *
    • + *
+ * If the test fails, this means that reference(s) to large object(s) have + * failed to be garbage collected. In this case: + *
    + *
  • set a breakpoint just before the test assertion in + * {@code checkUsedMemory}
    • + *
  • if the test fails intermittently, make this breakpoint conditional on + * {@code usedMemory > expectedMax}
    • + *
  • run the test to this point (and check that it is about to fail i.e. + * {@code usedMemory > expectedMax})
    • + *
  • use visualvm to obtain a heap + * dump from the suspended process (and kill the test or let it fail)
    • + *
  • inspect the heap dump using visualvm for large objects and their + * referers
    • + *
  • Tips:
    • + *
      + *
    • Perform GC from the Monitor view in visualvm before requesting the heap + * dump - test failure might be simply a delay to GC
      • + *
    • 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
      • + *
    • 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
      • + *
    + *
+ *

+ *

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

    + *
  • 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.
    • + *
  • 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()}.
    • + *
  • 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
    • + *
+ * 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; + /* + * maximum retained heap usage (in MB) for a passing test + */ + private static int MAX_RESIDUAL_HEAP = 45; + /** * Configure (read-only) Jalview property settings for test */ @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,98 +109,79 @@ public class FreeUpMemoryTest Cache.applicationProperties.setProperty("SHOW_IDENTITY", True); } - /** - * A simple test that memory is released when all windows are closed. - *
    - *
  • generates a reasonably large alignment and loads it
    • - *
  • performs various operations on the alignment
    • - *
  • closes all windows
    • - *
  • requests garbage collection
    • - *
  • asserts that the remaining memory footprint (heap usage) is 'not large' - *
    • - *
- * 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 { 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); - checkUsedMemory(expectedMin); + checkUsedMemory(MAX_RESIDUAL_HEAP); } - private static long getUsedMemory() + /** + * Returns the current total used memory (available memory - free memory), + * rounded down to the nearest MB + * + * @return + */ + private static int getUsedMemory() { - long availableMemory = Runtime.getRuntime().totalMemory() / ONE_MB; - long freeMemory = Runtime.getRuntime().freeMemory() / ONE_MB; + long availableMemory = Runtime.getRuntime().totalMemory(); + long freeMemory = Runtime.getRuntime().freeMemory(); long usedMemory = availableMemory - freeMemory; - return usedMemory; + + return (int) (usedMemory / ONE_MB); } + /** * Requests garbage collection and then checks whether remaining memory in use * is less than the expected value (in Megabytes) * * @param expectedMax */ - protected void checkUsedMemory(long expectedMax) + protected void checkUsedMemory(int expectedMax) { /* - * request garbage collection and wait for it to run; + * request garbage collection and wait for it to run (up to 3 times); * NB there is no guarantee when, or whether, it will do so - * wait time depends on JRE/processor, generous allowance here */ - System.gc(); - waitFor(1500); - - /* - * a second gc() call should not be necessary - but it is! - * the test passes with it, and fails without it - */ - System.gc(); - waitFor(1500); - - /* - * check used memory is 'reasonably low' - */ - long usedMemory = getUsedMemory(); - /* - * sanity check - fails if any frame was added after - * closeAll_actionPerformed - */ - assertEquals(Desktop.instance.getAllFrames().length, 0); + long usedMemory = 0L; + Long minUsedMemory = null; + int gcCount = 0; + while (gcCount < 3) + { + gcCount++; + System.gc(); + waitFor(1500); + usedMemory = getUsedMemory(); + if (minUsedMemory == null || usedMemory < minUsedMemory) + { + minUsedMemory = usedMemory; + } + if (usedMemory < expectedMax) + { + break; + } + } /* - * if this assertion fails - * - set a breakpoint here - * - run jvisualvm to inspect a heap dump of Jalview - * - identify large objects in the heap and their referers + * if this assertion fails (reproducibly!) + * - set a breakpoint here, conditional on (usedMemory > expectedMax) + * - run VisualVM to inspect the heap usage, and run GC from VisualVM to check + * it is not simply delayed garbage collection causing the test failure + * - take a heap dump and 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"); - assertTrue(usedMemory < expectedMax, String.format( + System.out.println("(Minimum) Used memory after " + gcCount + + " call(s) to gc() = " + minUsedMemory + "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 +241,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 +285,7 @@ public class FreeUpMemoryTest int width = 100000; int height = 100; ag.generate(width, height, 0, 10, 15); + ps.close(); return f; } }