Memory leaks

[GerdP]

What steps will reproduce the problem?

1. Download OSM data
2. Press Ctrl+H to see history of a selected object
3. Close all data layers
4. Use VisualVM to create a heapdump after a GC

What is the expected result?

No instances of VersionInfoPanel

What happens instead?

Some instances of this class, also various other instances of josm.gui.history.*

Please provide any additional information below. Attach a screenshot if possible.

Build-Date:2018-11-26 18:47:05
Revision:14455
Is-Local-Build:true

Identification: JOSM/1.5 (14455 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 4871 MB / 5461 MB (4694 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080

Plugins:
+ FastDraw (34510)
+ apache-commons (34506)
+ buildings_tools (34724)
+ download_along (34503)
+ editgpx (34737)
+ ejml (34389)
+ geotools (34513)
+ gpxfilter (34506)
+ jaxb (34506)
+ jts (34524)
+ measurement (34529)
+ o5m (34405)
+ opendata (34698)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ utilsplugin2 (34506)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[GerdP]

I tried to find out what's wrong but my understanding of dialog components is too small. GC doesn't remove the HistoryBrowserDialog instance once the dialog is closed.

[Don-vip]

To understand memory leaks you need to use the "GC root" feature of VisualVM. For a given class instance it will show you the root objects that keep a chain of references to this object, preventing it to be garbage collected.

In this case we have 8 instances of VersionInfoPanel, with two different chains:

In both cases TaginfoAction is involved. JosmActions are natively Destroyable and will release all resources (including listeners) when their destroy method is called. In this case we need to make sure that HistoryBrowserDialog.dispose() effectively calls the destroy method of all inner actions.

[GerdP]

OK, thanks for the hint. I think there is also a problem with the Dataset class. My understanding is that JOSM should keep only one instance when no layer is displayed. Sometimes I see a lot more and
ds in org.openstreetmap.josm.gui.tagging.ac.AutoCompletionManager seems to be involved.

[Don-vip]

Yes it's easier to add memory leaks than remove them, I'm sure we still have plenty of them. You can investigate the Dataset/AutoCompletionManager case, I am only working on the history stuff right now.

[Don-vip]

In 14463/josm:

fix #17040 - fix memory leaks when calling history dialog

[Don-vip]

In 14465/josm:

see #17040 - fix PMD warning

[GerdP]

Wow, quite a big change. No wonder that I wasn't able to fix that ;)
The good thing is that I cannot reproduce the Dataset/AutoCompletionManager leak with r14465 :)

[stoecker]

Replying to GerdP:

Wow, quite a big change. No wonder that I wasn't able to fix that ;)

Also a nice one. I like it when improving the code is done by removing code without loosing functionality. The old code thus had unneeded complexity.

[GerdP]

I still see a leak where SideButton and NotesDialog are involved. Should I open a new ticket for that?

[stoecker]

Replying to GerdP:

I still see a leak where SideButton and NotesDialog are involved. Should I open a new ticket for that?

Yes. For each. Or simply fix them without ticket :-)

[GerdP]

I'll give it another try later. It seems that the ToggleDialogs are never destroyed. I tried to change that and got problems with the LayerManager.
I wonder if we have some experimental code active. I see that MainPanel.reAddListerners() is called although there is this comment

   /**
     * Re-adds the layer listeners. Never call this in production, only needed for testing.
     */

[stoecker]

In case you don't know: The wiki has an Annotation feature:

browser/josm/trunk/src/org/openstreetmap/josm/gui/MainPanel.java

That's extremely helpful to find when (and why) certain lines have been added or modified.

[GerdP]

Yes, blame is very useful. I use TortoiseSVN for that. Anyhow, I still have no idea what's wrong :(
I simply have no idea how it should look when everything is correct. Maybe you can try to fix the problem with the ToggleDialogs as well and I'll try to learn from your changes?

[GerdP]

I've added a small patch that seems to fix one leak.

[Don-vip]

Good catch, I didn't check for other TaginfoAction outside of history browser.

[Don-vip]

We can keep this ticket open until we release 18.12 or we manage to fix all memory leaks :D

[GerdP]

17040-work2.patch fixes more problems, esp. mutliple MapView instances which kept refs to all the ToggleDialogs.
I guess this can be done better, the patch just points out some problems.

[GerdP]

17040-work3.patch also fixes a leak in NotesDialog. I think I am starting to undertstand how to use the results in VisualVM to find out where the problems are. I just don't know for sure where to fix them. I'll continue searching tomorrow.

[GerdP]

With 17040-work4.patch two more leaks reg. RelationListEditor and SaveLayersDialog are fixed.

[GerdP]

now also clear lists containing TestError Instances, they prevent GC of class Dataset

[GerdP]

fix more leaks reg. Dataset

[stoecker]

A small note. The fact that you have commit permissions means that we trust you. No need to ask for review in fixing bugs. Only necessary if you don't trust your own ideas.

If you make an error we can always blame you later :-)

No really, we don't want broken nightly releases, but there is no reason to fear it in case you are not 100% sure...

[GerdP]

Well, I don't trust some of my solutions. Esp. the one reg. NavigatableComponent.
I did not yet try to run the unit tests. I am creating the patches on my couch using a rather slow Laptop ;-)

[GerdP]

In 14470/josm:

see #17040 Fix various memory leaks
Not sure if this will break Unit tests. Many of them don't work on my PC with a clean copy.

[GerdP]

Need help: Some tests fail but I have no idea why:
https://josm.openstreetmap.de/jenkins/job/JOSM/lastCompletedBuild/testReport/

[GerdP]

In 14471/josm:

see #17040 Move call of UndoRedoHandler.removeCommandQueueListener() to the action that adds itself

[Don-vip]

Replying to GerdP:

Some tests fail but I have no idea why:

See #16973. This is unrelated to your changes but a side effect of the server migration.

[GerdP]

Replying to Don-vip:

Replying to GerdP:

Some tests fail but I have no idea why:

See #16973. This is unrelated to your changes but a side effect of the server migration.

You are right, but I was aware of #16973. These were other erros, but but they are gone again with r14471.

[GerdP]

In 14472/josm:

see #17040 call removeZoomChangeListener in destroy()

[GerdP]

In 14473/josm:

see #17040 don't keep refs to OSM primitives in validation tests because this can prevent GC of Dataset instance

[GerdP]

In 14475/josm:

see #17040 Use PopupMenuHandler to reset all primitiveActions

[GerdP]

In 14477/josm:

see #17040 fix checkstyle issues

[GerdP]

In 14478/josm:

see #17040 make MapSlider Destroyable

[GerdP]

@Don-vip: Why isn't Dataset.clear() called as in this patch?

[GerdP]

Sorry, I've asked that before and now found #13394. I think there is no need to keep a big memory leak when VisualVM also shows a small one. Without the Dataset.clear() call I sometimes find Dataset instances without any GC root. Maybe GC fails because each primitve has a ref back to Dataset?

[GerdP]

In 14479/josm:

see #17040 Clean refs to Dataset in AutoCompletionManager when OsmDataLayer is removed

[GerdP]

I found one more leak but I have no idea how to fix that. I used copy+paste to create a copy of a leisure=pitch way, later I used upload and closed the data layer. A heapdump shows as GC root for the Dataset instance.

[Don-vip]

In 14483/josm:

see #17040 - fix SonarQube issues

[Don-vip]

In 14500/josm:

see #17040 - fix memory leak after primitives are copied to clipboard

[stoecker]

I'd say #17064 is a regression due to this ticket.

[GerdP]

Yes, working on it.

[GerdP]

I wonder why DrawAction.finishDrawing() is called so rarely. It is not even called when the edit layer is closed. Where would be the best place to change that?

[GerdP]

Please review

[GerdP]

Please review

[GerdP]

Please ignore 17040-properties.patch. Causes NPE.

[GerdP]

Fixed NPE (sel was reset too early)

[GerdP]

Optimization: Reduce memory footprint and number of String instances für svg graphics

[Don-vip]

resetSelection would be a better name. For the svgSalamander patch, can you please create a pull request as well to ​https://github.com/blackears/svgSalamander ?

[GerdP]

OK, thanks for review. I've never created a pull request. What do I have to do?

[Don-vip]

For simpler ones concerning only one file you can just use the ​edit button of Github and follow the wizards.

[GerdP]

OK, I'll try. Besides that I wonder if we can forget the svg xml data once the image was rendered. Would that be possible?
Or do we need it again for other resolutions?

[GerdP]

In 14509/josm:

see #17040 implement resetSelection() to forget recently selected primitives

[Don-vip]

The XML is kept in memory?! Where?

[GerdP]

Well, not the xml, but class StyleAttribute stores all kinds of properties as strings. That means we have thousands of String instances with value "0" or "1". My small patch catches many but not all of them.

[Klumbumbus]

Replying to GerdP:

forget recently selected primitives

I don't undertstand the code, but the commit message sounds a bit as it might break wiki:/Help/Action/UndoSelection

[GerdP]

In 14510/josm:

see #17040 call DrawAction.finishDrawing() when selection is empty or destroy() is called.

[GerdP]

It resets a list in the TagEditor.

[GerdP]

Replying to Klumbumbus:

Replying to GerdP:

forget recently selected primitives

I don't undertstand the code, but the commit message sounds a bit as it might break wiki:/Help/Action/UndoSelection

I just tried with r14460. It doesn't seem to work, or I don't understand what it should do.

[Klumbumbus]

Replying to GerdP:

It doesn't seem to work

When you select one or more object(s) and unselect them then use UndoSelection, you get back the previous selection. However for me this works only once and not consecutively like written on the wiki page. Maybe skyper remembers if this feature used to behave differently in the past (as he added the wiki text 8 years ago).

[Don-vip]

Replying to GerdP:

Well, not the xml, but class StyleAttribute stores all kinds of properties as strings. That means we have thousands of String instances with value "0" or "1". My small patch catches many but not all of them.

These strings are probably needed, I don't know the svgSalamander code by heart and it's not really state of the art/easy to understand... We keep the SVGDiagram in ImageResource so that we can request images of different sizes after having loaded the SVG.

ImageResource are optionnally kept in actions:

    /**
     * Set both icons of an Action
     * @param a The action for the icons
     * @param addresource Adds an resource named "ImageResource" if <code>true</code>
     * @since 10369
     */
    public void attachImageIcon(AbstractAction a, boolean addresource) {
        attachImageIcon(a);
        if (addresource) {
            a.putValue("ImageResource", this);
        }
    }

It seems this feature is only used in SideButton constructor:

    /**
     * Constructs a new {@code SideButton}.
     * @param action action used to specify the new button
     * an icon must be provided with {@link ImageResource#attachImageIcon(AbstractAction, boolean)}
     * @throws IllegalArgumentException if no icon provided
     * @since 744
     */
    public SideButton(Action action) {
        super(action);
        ImageResource icon = (ImageResource) action.getValue("ImageResource");
        if (icon != null) {
            setIcon(icon.getImageIconBounded(
                ImageProvider.ImageSizes.SIDEBUTTON.getImageDimension()));
        } else {
            throw new IllegalArgumentException("No icon provided");
        }

Maybe we actually store some ImageResource instances for nothing by calling too often attachImageIcon(this, true) for places where we could maybe use simply attachImageIcon(this). Hard to tell, it needs to be checked case by case...

[GerdP]

Replying to Klumbumbus:

Replying to GerdP:

It doesn't seem to work

When you select one or more object(s) and unselect them then use UndoSelection, you get back the previous selection. However for me this works only once and not consecutively like written on the wiki page. Maybe skyper remembers if this feature used to behave differently in the past (as he added the wiki text 8 years ago).

I think the problem is quite old. If I got that right we have to many selectionChanged events for one user action.
This also breaks the Shift+R (#14862).
Example: When you select an existing object this causes one SelectionChangeEvent for MousePressed and another for MouseReleased.

[GerdP]

Forget comment:56, I was wrong. I've attached an incomplete patch to #14862.

[GerdP]

In 14537/josm:

see #17040 call Dataset.clear() in OsmDataLayer.destroy()

Without this call sometimes the complete Dataset instance is not GCed, esp. when dDataset contains a large number of objects. The call to clear() eases work of GC.

[Don-vip]

In 14542/josm:

see #17040 - fix unit tests

[GerdP]

In 14545/josm:

see #17040 - destroy ChangesetInfoAction and UserInfoAction

[GerdP]

Please review, I am always unsure whether to use destroy() or dispose(). Removes two listeners added for each ChangesetManager (Ctrl+Alt+C)

[Don-vip]

When dispose() is available (in Window subclasses) it should be used. When it is not, you can implement Destroyable.

[GerdP]

My rule is this:

• if I can override dispose() I should do that, next
• if I can override destroy() I thould do that, next
• implement Destroyable

Do you agree?

[Don-vip]

Yes :)

[GerdP]

In 14546/josm:

see #17040 implement Destroyable in ChangesetDetailPanel and use it

Removes listeners added with each use of ChangesetManager (Ctrl+Alt+C)

[GerdP]

In 14547/josm:

see #17040 revert r14546 Sometimes tries to remove listener twice

[GerdP]

In 14548/josm:

see #17040 - destroy AddImageryLayerAction

Up to now they were never destroyed, and one is created for each available entry in ImageryMenu whenever you open the menu.

[GerdP]

Please review ChangesetDetail-v2.patch. I found no reason for the addComponentListener and it caused trouble with the previous patch.
Maybe it is needed for something special that I never use?

[Don-vip]

The changeset window is non modal so it seems the code ensures the buttons are enabled only when the panel is displayed and there's a layer.

I don't think many people use the changeset manager, so it's probably fine to remove the listeners only on destroy (i.e when the window is closed) instead of hide/show (i.e when a tab is changed).

[GerdP]

OK, I'll work with this patch for a while. With this last patch it looks quite good now, Dataset is no longer kept after "normal" edit sessions and listeners are removed. I'll play now with more complex cases like conflict dialog and undo/redo actions. I guess that some plugins also need a closer look.

[GerdP]

In 14555/josm:

see #17040 commit ChangesetDetail-v2.patch

[GerdP]

In 14556/josm:

see #17040 more cleanup in OsmDataLayer.destroy()

• remove DatasetListeners previously added
• clear recentRelations
• don't call Dataset.clear() here because there may still be active listeners

• call DataSet.clear in LayerManager after all listeners are removed

Hope this doesn't break unit tests, can't test on my machine.

[GerdP]

Can anybody explain why it is not okay to use Dataset.clear() for a locked Dataset, please? I see no good reason for that.

[GerdP]

Please review, I've removed a SwingUtilities.invokeLater() here to fix a leak. It was introduced with r3346 but for me it seems to work also without.

[GerdP]

In 14589/josm:

#see #17040 implement Destroyable in ToggleDialog.TitleBar and use it in ToggleDialog.destroy()

[GerdP]

I tried to commit 17040-reltoolbox.patch but it seems I have no right to do that. Message would be

see #17040 call removeActiveLayerChangeListener() in destroy()

[Don-vip]

The folder is named reltoolbox, no?

[GerdP]

yes, why?
I see, the patch doesn't contain the plugin directory.

[GerdP]

Seems the commit problem reg. 17040-reltoolbox.patch is on my side. I've now committed the patch on my PC, yesterday I tried with the laptop.
One question: I've changed ToggleDialog during the last days and this is a super class in some plugins. Should I compile those and commit only the josm/dist changes each time when I change ToggleDialog? My changes might have broken something in those plugins.

[GerdP]

In 14598/josm:

see #17040 kitofx.patch: use String.intern() for deduplication of Strings that appear in svg graphics

[Don-vip]

Replying to GerdP:

My changes might have broken something in those plugins.

It seems fine. The plugin integration test is OK and we have no bug reports.

[GerdP]

Would the integration tests recognize a pointer which is set to null in the destroy() methods?
Or do they only check if plugins can be compiled?

[Don-vip]

No, it checks binary compatibility only. To detect NPE you can take a look to sonar.

[GerdP]

sonar shows a lot of interesting information but I found no hint that would help me in my case. I've now used some simple searches to find out if any of the plugins uses any of the fields that are set to null and found none, so I'm crossing fingers ;-)

[GerdP]

There is one more leak involiving josm.data.preferences.ColorInfo. Each time when I press F12 I see 373 new instances. I did not yet find out where to fix that.

[GerdP]

Merry Christmas!
Please review 17040-pref-dlg.patch. Is it OK to call dispose() from the OK and Cancel action?

[Don-vip]

We usually don't call dispose directly but the javadoc ​doesn't forbid it. Did you measure the time it takes to show again the preferences dialog after that? Some parts are slow to initialize and the performance penalty could maybe be seen as a regression for end users.

[GerdP]

The dialog is created each time when you press F12. The change only removes the memory leak.

[GerdP]

We have DownloadDialog that implements a singleton but PreferenceDialog doesn't. I don't see a performance problem but maybe plugins add something that increases complexity so that the delay is notable?

[Don-vip]

DownloadDialog is a lot faster to load. Preferences dialog loadup time increases with the number of plugin installed, and also it can take time to switch between tabs (as tab initialization is delayed to make the dialog load faster). See #7386 for details.

[Don-vip]

Replying to GerdP:

The dialog is created each time when you press F12. The change only removes the memory leak.

OK sorry I misunderstood the patch.

[GerdP]

In 14601/josm:

see #17040 dispose PreferenceDialog and call removeWindowListener()

[GerdP]

I'd prefer to use setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE); but that would not allow to set the field canceled.
No idea why there is a getter for this field, it seems to be unused. Maybe legacy code?

[Don-vip]

You can use the blame feature to know that.

[GerdP]

OT: simplify PreferenceDialog

[GerdP]

Replying to Don-vip:

You can use the blame feature to know that.

I used that before I wrote my comment;) With r2745 .. r2748 Gubaer implemented OAuth and in those changes the getter was introduced. I found no reason for that in those old sources nor in the current, but it's hard to tell because there may be plugins using it.
17040-pref-dlg-v2.patch shows a possible alternative. Please ignore the changes in GettingStarted, I disabled the Christmas code because it obstructed my measurements and forgot to revert.

[GerdP]

In 14602/josm:

see #17040 fix typo in ChangesetCacheManager.destroyInstance() so that
ChangesetCacheManagerModel.tearDown() is called

[GerdP]

In 14607/josm:

see #17040 call setTransferHandler(null) in destroy()

[GerdP]

These were my last changes for this year. To be continued...

[Don-vip]

We'll open a new ticket then :)