[Patch] fix some memory leaks

[GerdP]

I've noticed that JOSM gets slower when you open+close a large input file
a few times.
I found two memory leaks:
1) Dataset.clear() is not called anywhere
2) The RelationsListDialog doesn't overwrite destroy() but allocates
a list which refers to relations. Therefore data is not freed even if
all layers are closed.

The attached patch fixes this.
I hope I found the right places for this.

[Don-vip]

@michael: do you remember having removed a call to Dataset.clear()?

[GerdP]

I now think that Dataset.clear() is not needed. The reference to OsmDataLayer is removed,
so the only problem here is the RelationListDialog.

[michael2402]

The change in LayerManager will prevent the layer from being used multiple times.
We should find out where the references to the DataSet are kept and remove those references. Then GC can clean the data set.

[GerdP]

Replying to michael2402:

The change in LayerManager will prevent the layer from being used multiple times.
We should find out where the references to the DataSet are kept and remove those references. Then GC can clean the data set.

Yes, the reference to DataSet is kept in each OSM primitive. The RelationListDialog keeps a list of
Relation references. So I think that part of the patch is good.

[Don-vip]

In 10861/josm:

fix #13394 - fix some memory leaks (patch by Gerd Petermann)

[GerdP]

Hmm, I think we need a call to Dataset.clear() somewhere when a OsmDataLayer is destroyed, means, when I close the corresponding "Data Layer".
I also think that this means that no object in that layer is selected, so before calling Dataset.clear() it would be good to "unselect" all objects. Why? Some plugins keep references to primitives, esp. utilsplugin2.
Without a call to Dataset.clear() each primitive will have a reference to the Dataset instance which references all primitives and thus GC fails to free anything.

[michael2402]

Replying to Gerd Petermann <gpetermann_muenchen@…>:

Hmm, I think we need a call to Dataset.clear() somewhere when a OsmDataLayer is destroyed, means, when I close the corresponding "Data Layer".
I also think that this means that no object in that layer is selected, so before calling Dataset.clear() it would be good to "unselect" all objects. Why? Some plugins keep references to primitives, esp. utilsplugin2.

Why are those plugins keeping references to primitives? They should either:

• Only store the ID or tags or whatever they need to keep track of
• Listen to layer removals/active layer changes if they want to sore extra data to a layer, then clean their data structures once that layer is removed.

If they still have object references, it means that they have a memory leak. Although probably small, this will sum up, too. And in many cases, it is a hint that there is a more severe issue somewhere.

Without a call to Dataset.clear() each primitive will have a reference to the Dataset instance which references all primitives and thus GC fails to free anything.

If you need primitives after a layer is deleted, you can:

• Detach them from the DataSet (not recommended)
• Create a PrimitiveData for them. This is meant to store data about primitives independent from the data set.
• Use a weak reference if they absolutely need to have a reference (not perfect either, but works for simple node in way/... tests)

[GerdP]

I also thought that way but found no docu what a plugin should do or must not do and how it should react
on events. On the other hand the Dataset.clear() method should not do any harm and will reduce the leak.
Why do you want to avoid it?

[michael2402]

My main reason for avoiding it is that it helps us track down bugs in plugins.

For the same reason we are enforcing strict listener checks. We could add a mechanism that detects that situation (do a forced GC and see if the reference was really deleted). That way, we would force plugin authors to care for it. It is easier to find the references to such objects if there are a lot of them.

I traced several bugs in the last time that were related to not having cleaned up correctly. Listeners that were registered twice, unregistered but still required, data that has complex cyclic dependencies and more. This is why I am not a big fan of trying to work around bugs plugin authors introduce - best ist JOSM displays an error dialog. This will annoy a few users for a short time until it is fixed, but then we won't have that much unpredictable behavior if we change something in JOSM internally (like the layer being allowed to exist after it was removed from the main map view, or existing without even being added to a map view).

[Don-vip]

Michael's got a point :)
Gerd, do you know what parts of utilsplugin2 keep references to osm primitives?

[GerdP]

I'll try to produce a list or a patch.
BTW: I use VisualVM to find these memory leaks. If you know a better free tool, please let me know.

[GerdP]

List of parts in utilsplugin2 which may keep references to primitives:

org.openstreetmap.josm.plugins.utilsplugin2.actions.TagBufferAction in field selectionBuf
org.openstreetmap.josm.plugins.utilsplugin2.actions.TagSourceAction in field selectionBuf
org.openstreetmap.josm.plugins.utilsplugin2.command.ChangeRelationMemberCommand in fields relation, oldMember, and newMember
org.openstreetmap.josm.plugins.utilsplugin2.multitagger.MultiTagDialog in field currentSelection
org.openstreetmap.josm.plugins.utilsplugin2.multitagger.MultiTaggerTableModel in fields list , also List<Command> cmds looks suspicous
org.openstreetmap.josm.plugins.utilsplugin2.selection.AdjacentNodesAction in field activeWays
org.openstreetmap.josm.plugins.utilsplugin2.selection.SelectBoundaryAction in field lastUsedStartingWay
org.openstreetmap.josm.plugins.utilsplugin2.selection.UndoSelectionAction maybe in field lastSel (action seems not to work, will report details in separate ticket)