Modify

Opened 13 months ago

Last modified 11 months ago

#19885 assigned defect

memory leak with "temporary" objects in validator and actions

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: Longterm
Component: Core Version:
Keywords: Cc:

Description

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);

Attachments (3)

19885-wip.patch (19.5 KB) - added by GerdP 13 months ago.
19885-rel-editor.patch (2.0 KB) - added by GerdP 12 months ago.
RFC: let RelationEditor clean up
19885-splitway.patch (5.9 KB) - added by GerdP 11 months ago.
control flow is really a mess, both in SplitWayAction and in SplitWayCommand

Download all attachments as: .zip

Change History (50)

comment:1 Changed 13 months ago by 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));

comment:2 Changed 13 months ago by GerdP

Milestone: 20.10
Owner: changed from team to GerdP
Status: newassigned

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

Changed 13 months ago by GerdP

Attachment: 19885-wip.patch added

comment:4 Changed 13 months ago by 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.

comment:5 Changed 13 months ago by 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.

comment:6 Changed 13 months ago by 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

comment:7 Changed 13 months ago by 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

comment:8 Changed 13 months ago by 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.

comment:9 Changed 13 months ago by 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

comment:10 Changed 13 months ago by 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

comment:11 Changed 13 months ago by 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.

comment:12 Changed 13 months ago by 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...

comment:14 Changed 13 months ago by 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

comment:15 in reply to:  14 Changed 13 months ago by 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?

comment:16 Changed 13 months ago by 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.

comment:17 Changed 13 months ago by 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".

comment:18 Changed 13 months ago by GerdP

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

comment:19 Changed 13 months ago by GerdP

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

comment:20 Changed 13 months ago by 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

comment:21 Changed 13 months ago by stoecker

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

comment:22 Changed 13 months ago by GerdP

Milestone: 20.10Longterm

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.

comment:23 Changed 13 months ago by 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.

comment:24 Changed 13 months ago by GerdP

In 17200/josm:

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

  • use ChangeNodesCommand

Simplifies code and avoids another memory leak

comment:25 Changed 12 months ago by 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.

comment:26 Changed 12 months ago by 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.

comment:27 Changed 12 months ago by GerdP

In 17215/josm:

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

comment:28 Changed 12 months ago by 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

comment:29 Changed 12 months ago by GerdP

In 17220/josm:

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

  • replace ChangeCommand by ChangeMembersCommand

comment:30 Changed 12 months ago by 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

comment:31 Changed 12 months ago by 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.

comment:32 Changed 12 months ago by 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

comment:33 Changed 12 months ago by 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.

comment:34 Changed 12 months ago by 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

comment:35 Changed 12 months ago by 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.

comment:36 Changed 12 months ago by 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".

comment:37 Changed 12 months ago by GerdP

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

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

     
    3131import javax.swing.JOptionPane;
    3232import javax.swing.JPanel;
    3333import javax.swing.JToggleButton;
    34 import javax.swing.SwingUtilities;
    3534
    3635import org.openstreetmap.josm.actions.AdaptableAction;
    3736import org.openstreetmap.josm.actions.CreateMultipolygonAction;
     
    433432            if (r.isMultipolygon() && r != calculated) {
    434433                r.setMembers(RelationSorter.sortMembersByConnectivity(r.getMembers()));
    435434            }
    436             SwingUtilities.invokeLater(() -> RelationEditor.getEditor(
     435            GuiHelper.executeByMainWorkerInEDT(() -> RelationEditor.getEditor(
    437436                    MainApplication.getLayerManager().getEditLayer(), r, members).setVisible(true));
     437            MainApplication.worker.execute(() -> {
     438                // Ugly hack to prevent memory leak, see #19885
     439                // by now, relation editor has created a copy of r.
     440                r.setMembers(null);
     441            });
    438442        }
    439443        if (!primitives.isEmpty()) {
    440444            DataSet ds = primitives.iterator().next().getDataSet();

Changed 12 months ago by GerdP

Attachment: 19885-rel-editor.patch added

RFC: let RelationEditor clean up

comment:38 Changed 12 months ago by GerdP

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

comment:39 Changed 11 months ago by 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

comment:40 Changed 11 months ago by 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

comment:41 Changed 11 months ago by 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

Changed 11 months ago by GerdP

Attachment: 19885-splitway.patch added

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

comment:42 Changed 11 months ago by 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

comment:43 Changed 11 months ago by 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

comment:44 Changed 11 months ago by 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

comment:45 Changed 11 months ago by GerdP

In 35668/osm:

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

  • simplify code, use ChangeMembersCommand instead of ChangeCommand

comment:46 Changed 11 months ago by GerdP

In 35671/osm:

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

  • simplify code, use ChangeMembersCommand instead of ChangeCommand

comment:47 Changed 11 months ago by GerdP

In 17397/josm:

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

  • always call way.setNodes(null)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain GerdP.
as The resolution will be set.
to The owner will be changed from GerdP to the specified user.
The owner will change to GerdP
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


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

 
Note: See TracTickets for help on using tickets.