memory leak with "temporary" objects in validator and actions

[GerdP]

I found some places where ways or relations are cloned for temporary use. This is likely to cause a memory leak since the clone might refer to child objects and those child objects will also refer to the clone unless the clone is correctlly cleaned up. Pattern:

// create full clone
Way wnew = new Way(w);
// do something with the clone
..
// make sure that the clone is cleaned up if no longer needed
wnew.setNodes(null);

[GerdP]

A lot of these clones could be avoided. They are often created to produce a ChangeCommand:

        Way newWay = new Way(w);
        newWay.setNodes(newNodes);
        cmds.add(new ChangeCommand(w, newWay));

Instead one may create a ChangeNodesCommand. This requires less code, is faster and probably requires less memory:

        cmds.add(new ChangeNodesCommand(w, newNodes));

[GerdP]

Ouch! The relation editor also creates clones and doesn't care about cleanup. It would be quite difficult to fix this problem in all the code in core and all plugins. Sometimes commands are created but not executed because user cancels the operation.
Maybe it is possible to write a "garbage collector" routine which would remove those parent objects which don't belong to the current dataset and which are not used in any relation editor window or for any command in the UndoRedoHandler.
The routine would be rather simple, we just have to make sure that is not executed while one of the cloned parents is still in use.

[GerdP]

Patch fixes various memory leaks and implements a new consistency check which helps to find more.
Worst case so far: When you move the mouse so that the virtual middle node of a way segment is highlighted this will create (multiple) new parent ways.

[GerdP]

I think there is really no way to avoid those leaks while a complex action like SplitWayAction is computing the effects of a split on all the ways and parent relations. We have to ignore those minor leaks where a fix would require more code.

[GerdP]

In 17110/josm:

see #19885: memory leak with "temporary" objects in validator and actions
first bunch of changes

• use ChangeNodesCommand instead of ChangeCommand
• call DataSet.clearSelectionHistory() in Dataset.clear()
• remove fake parent way in AbstractMapRenderer

[GerdP]

In 17138/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• use ChangeNodesCommand instead of ChangeCommand
• remove fake parent way in AlignInCircleAction

[GerdP]

In 17140/josm:

see #19885: memory leak with "temporary" objects in validator and actions
Reduce number of memory leaks in RelationEditor when nothing is changed.

[GerdP]

In 17141/josm:

see #19885: memory leak with "temporary" objects in validator and actions
next bunch of changes:

• use ChangeNodesCommand instead of ChangeCommand
• further simplifications

[GerdP]

In 35574/osm:

see #19885: memory leak with "temporary" objects in validator and actions

• use ChangeNodesCommand instead of ChangeCommand
• simplify code to copy tags from address node

[GerdP]

In 17143/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• use ChangeNodesCommand instead of ChangeCommand
• further simplifications

This one was a bit more complex. I hope I didn't mess up any special use case.

[GerdP]

In 17163/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• use ChangeNodesCommand instead of ChangeCommand
• use ChangePropertyCommand instead of ChangeCommand
• remove unused parameter

Just a small step forward...

[simon04]

org.openstreetmap.josm.actions.JoinAreasActionTest.testExamples fails → https://josm.openstreetmap.de/jenkins/job/JOSM/jdk=JDK8/lastCompletedBuild/testReport/org.openstreetmap.josm.actions/JoinAreasActionTest/

[GerdP]

In 17167/josm:

see #19885: memory leak with "temporary" objects in validator and actions
revert changes in JoinAreasAction made in r16048. Can't use ChangePropertyCommand to remove all tags

[stoecker]

Replying to GerdP:

In 17167/josm:

see #19885: memory leak with "temporary" objects in validator and actions
revert changes in JoinAreasAction made in r16048. Can't use ChangePropertyCommand to remove all tags

Hmm, wouldn't it be better to fix ChangePropertyCommand so it can be used for that purpose?

[GerdP]

WIll have a look tomorrow. I first thought that it works similar to ChangeNodesCommand which also doesn't allow an empty list, but a way without nodes really makes no sense while a way without tags can be valid.
I also think that a ChangeRelationMembersCommand would help.

[GerdP]

Hmm, wouldn't it be better to fix ChangePropertyCommand so it can be used for that purpose?

The command is used by varios plugins and the current code does nothing when the map is empty. Up to now the command works more like a "AddOrReplacePropertyCommand". I'd rather add a "RemoveAllPropertiesCommand".

[GerdP]

See #19924 for a regression. Have to check the other changes for this ticket now...

[GerdP]

Good news: ExtrudeAction was the only one where I replaced Way.remove() by List.remove()

[GerdP]

In 17187/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• let modifyWay() return the list of nodes instead of a newly created copy of the way

Simplifies code and avoids another memory leak

[stoecker]

Simply dropping 6 lines to make the code better is surely forbidden somewhere in programming guidelines. 😂

[GerdP]

This will take longer. Detected issues so far:

• Actions create a ChangeCommand instead of a ChangeNodesCommand or RemoveNodesCommand
• Actions create a ChangeNodesCommand instead of a RemoveNodesCommand, e.g. if a single node is removed from a way with 1000 nodes.
• For relations we need commands to change the member list or remove members, similar to those with way nodes
• New ways or relations should not be added with AddCommand, instead some kind of CreateParentCommand could do the same without the need to create the complete Way or Relation before.
• Similar problems with Commands that change properties (tags)

I think about one or more factory methods like these to reduce memory needed for the undo/redo stacks.

Command c = ChangeNodesCommand.createBestCommand(way, newNodes);
Command c = ChangeMembersCommand.createBestCommand(rel, newMember}}}
Command c = ChangePropertyCommand.createBestCommand(rel, tag}}}

Another issue is in the UndoRedoHandler:
If undo is used and not followed by redo the command stack simply removes some Commands from the stack. These commands are probably never GCed as they might still be refered to by child objects in the DataSet. Same is true for commands that are removed because limit for the stack size is reached.

[GerdP]

In 17199/josm:

see #19885: memory leak with "temporary" objects in validator and actions
Implement new ChangeMembersCommand. To be used instead of ChangeCommand when only the members of a relation are changed to be changed.

[GerdP]

In 17200/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• use ChangeNodesCommand

Simplifies code and avoids another memory leak

[GerdP]

In 17205/josm:

see #19885: memory leak with "temporary" objects in validator and actions
Start to use ChangeMembersCommand instead of ChangeCommand in those places where the cloned relation was only created for the ChangeCommand and not referenced elsewhere.

[GerdP]

In 17214/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• unlink cloned relation if not used

With current code method removeTagsFromWaysIfNeeded()is never called for old relations.

[GerdP]

In 17215/josm:

see #19885: memory leak with "temporary" objects in validator and actions
use new ChangeMembersCommand in PropertiesDialog

[GerdP]

In 17219/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• unlink members from new relation when multipolygon is not valid

[GerdP]

In 17220/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• replace ChangeCommand by ChangeMembersCommand

[GerdP]

In 17221/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• no need to clone relation, replace ChangeCommand by ChangeMembersCommand

Simplifies code and fixes memory leak

[GerdP]

In 17226/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• initialize cloneMap as Collections.emptyMap() to reduce memory footprint for Commamds

The original code allocated a HashMap for all commands, only some require it. Typically, the map contains 0 or 1 entries, so use Utils.toUnmodifiable() to optimize further.

[GerdP]

In 17241/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• replace ChangeCommand by ChangePropertyCommand to avoid cloning primitives

Fixes another memory leak and simplifies code

[GerdP]

In 17242/josm:

see #19885: memory leak with "temporary" objects in validator and actions
Allow empty member list. Unlike with ways we allow empty relations.

[GerdP]

In 17243/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• replace ChangeCommand by ChangePropertyCommand to avoid cloning primitives
• also move the call of super.endTest() to the end of the method endTest(), else reported timings make not much sense

[GerdP]

Still some places to fix in core, not talking about plugins.

Some actions really need special care. I think we have error prone data flow like this:

  newRel = new Relation(old);
  // change newRel
  ...
  cmds.add(new ChangeCommand(old, newRel));
  ...
  // change newRel again
  newRel.put(key_xyz, "new value");
  ...
  UndoRedoHandler.add(cmds); // executes the ChangeCommand with the latest status of newRel

When I replace the ChangeCommand by a ChangeMembersCommand I get of course different results.

[GerdP]

I am making progress with the complex commands (split, join, combine). On my local system I see only a few dead links with core as long as I don't start to undo/redo things. Undoing an AddCommand for a way with 10 nodes removes the way from the dataset but the links from the nodes to way stay intact. It would be cleaner to have a SequenceCommand with an AddCommand with an empty way and a ChangeNodesCommand that adds the nodes, but this combination requires more memory.
Open brain teasers are the creation of relations via presets and "Update multipolygon".

[GerdP]

The leak produced by the creation of relations might be fixed like this:

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPreset.java

@@ -31 +31 @@
 import javax.swing.JOptionPane;
 import javax.swing.JPanel;
 import javax.swing.JToggleButton;
-import javax.swing.SwingUtilities;
 
 import org.openstreetmap.josm.actions.AdaptableAction;
 import org.openstreetmap.josm.actions.CreateMultipolygonAction;
@@ -433 +432 @@
             if (r.isMultipolygon() && r != calculated) {
                 r.setMembers(RelationSorter.sortMembersByConnectivity(r.getMembers()));
             }
-            SwingUtilities.invokeLater(() -> RelationEditor.getEditor(
+            GuiHelper.executeByMainWorkerInEDT(() -> RelationEditor.getEditor(
                     MainApplication.getLayerManager().getEditLayer(), r, members).setVisible(true));
+            MainApplication.worker.execute(() -> {
+                // Ugly hack to prevent memory leak, see #19885
+                // by now, relation editor has created a copy of r.
+                r.setMembers(null);
+            });
         }
         if (!primitives.isEmpty()) {
             DataSet ds = primitives.iterator().next().getDataSet();

[GerdP]

RFC: let RelationEditor clean up

[GerdP]

Forget the ugly patch in comment:37. 19885-rel-editor.patch is a more general approach. Less ugly, but still quite confusing.

[GerdP]

In 17357/josm:

see #19885: memory leak with "temporary" objects in validator and actions
Minor changes:

• add hint regarding memory leak in javadoc to copy constructors for Way and Relation
• decrease memory footprint of incomplete ways

[GerdP]

In 17358/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• (hopefully) fix memory leaks in complex actions
• handle complex cases with presets and RelationEditor

I hope these changes don't break plugins which extend or overwrite RelationEditor

[GerdP]

In 17359/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• revert changes made in r17358 in SplitWayCommand , they break a unit test and I do not yet see why

[GerdP]

control flow is really a mess, both in SplitWayAction and in SplitWayCommand

[GerdP]

In 17362/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• fix memory leaks in SplitWayAction and SplitWayCommand
• fix (not yet used) counter of modified relations

[GerdP]

In 17365/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• revert fix of counter of relations, it is used in a popup and should in fact count all relations

[GerdP]

In 17367/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• simplify code in SplitWayCommand, no need to create a way when we only need to know the nodes

[GerdP]

In 35668/osm:

see #19885: memory leak with "temporary" objects in validator and actions

• simplify code, use ChangeMembersCommand instead of ChangeCommand

[GerdP]

In 35671/osm:

see #19885: memory leak with "temporary" objects in validator and actions

• simplify code, use ChangeMembersCommand instead of ChangeCommand

[GerdP]

In 17397/josm:

see #19885: memory leak with "temporary" objects in validator and actions

• always call way.setNodes(null)

[stoecker]

Closing this rather generic ticket. Remaining cases should be handled in individual tickets.