bigdataviewer / bigdataviewer-core

ImgLib2-based viewer for registered SPIM stacks and more
BSD 2-Clause "Simplified" License
33 stars 35 forks source link

Fix Big Data Viewer Garbage Collection #107

Closed maarzt closed 4 years ago

maarzt commented 4 years ago

This PR fixes three reasons that prevented BDV to be properly garbage collected.

  1. Using the method pack() of JFrame or JDialog will cause the window be refrenced (and prevent) garbage collection until dispose() is called. This is fixed by introducing MemoryFixedDialog. The dialog uses "DISPOSE_ON_CLOSE", and avoid's calling pack() as long as the dialog is invisible.
  2. Remove unnecessary reference from PainterThread to ViewerPanel.
  3. BdvDefaultCards, introduce weak reference when listening for keyboard focus.

Code that triggers the OutOfMemoryException (fixed by this PR):

for ( int i = 0; i < 60; i++ )
{
    System.out.println( i );
    BdvFunctions.showOverlay( new BdvOverlay()
    {
        private final byte[] oneGigaByte = new byte[ 1 << 30 ];

        @Override
        protected void draw( Graphics2D g )
        {

        }
    }, "" ).close();
}
tpietzsch commented 4 years ago

Thank you!

  1. Remove unnecessary reference from PainterThread to ViewerPanel.
  2. BdvDefaultCards, introduce weak reference when listening for keyboard focus.

Those make a lot of sense.

Regarding

  1. Using the method pack() of JFrame or JDialog will cause the window be refrenced (and prevent) garbage collection until dispose() is called. This is fixed by introducing MemoryFixedDialog. The dialog uses "DISPOSE_ON_CLOSE", and avoid's calling pack() as long as the dialog is invisible.

I cannot confirm the pack()/dispose() theory.

I still kept the MemoryFixedDialog (renamed DelayedPackDialog) because I like the idea. But I removed the part where the setDefaultCloseOperation is changed. (I also kept your changes, where the dialogs (e.g. HelpDialog) set the default close option. It seems best to just leave it at the default value.

I tried your test and DISPOSE_ON_CLOSE vs HIDE_ON_CLOSE did not make any difference for me. To make sure, I use this variant which actually makes some dialog visible

for ( int i = 0; i < 60; i++ )
{
    System.out.println( i );
    final BdvOverlaySource handle = BdvFunctions.showOverlay( new BdvOverlay()
    {
        private final byte[] oneGigaByte = new byte[ 1 << 30 ];

        @Override
        protected void draw( Graphics2D g )
        {
        }
    }, "" );
    BigDataViewer bdv = ( ( BdvHandleFrame ) handle.getBdvHandle() ).getBigDataViewer();
    bdv.brightnessDialog.setVisible( true );
    bdv.activeSourcesDialog.setVisible( true );
    bdv.helpDialog.setVisible( true );
    Thread.sleep( 100 );
    handle.close();
    System.gc();
}

What makes the difference between out-of-memory or not is the System.gc(); (or alternatively Thread.sleep(); for a sufficiently long time). Could you confirm on Linux?

maarzt commented 4 years ago

Garbage collection works better now with #99 merged. Only the fix for BdvDefaultCards is needed, to get the garbage collection working properly in Labkit. MemoryFixedDialog or DelayedPackDialog don't seem to be required anymore.

tpietzsch commented 4 years ago

I still left it in, it makes sense independent of memory leaks. Thanks!