Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13394 closed enhancement (fixed)

[Patch] fix some memory leaks

Reported by: GerdP Owned by: team
Priority: normal Milestone: 16.08
Component: Core Version:
Keywords: template_report Cc: michael2402

Description

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.

Attachments (1)

fix_memory_leaks.patch (1.6 KB) - added by GerdP 3 years ago.

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by GerdP

Attachment: fix_memory_leaks.patch added

comment:1 Changed 3 years ago by Don-vip

Milestone: 16.08

comment:2 Changed 3 years ago by Don-vip

Cc: michael2402 added

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

comment:3 Changed 3 years ago by GerdP

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

comment:4 Changed 3 years ago by 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.

comment:5 in reply to:  4 Changed 3 years ago by 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.

comment:6 Changed 3 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 10861/josm:

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

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by Don-vip

Resolution: fixed
Status: closedreopened

comment:9 in reply to:  7 Changed 3 years ago by 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)
Last edited 3 years ago by michael2402 (previous) (diff)

comment:10 Changed 3 years ago by 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?

comment:11 Changed 3 years ago by 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).

comment:12 Changed 3 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

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

comment:13 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by 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)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.