Modify

Opened 3 weeks ago

Last modified 9 days 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 (1)

19885-wip.patch (19.5 KB) - added by GerdP 3 weeks ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 3 weeks 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 3 weeks ago by GerdP

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

comment:3 Changed 3 weeks 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 3 weeks ago by GerdP

Attachment: 19885-wip.patch added

comment:4 Changed 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 2 weeks 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 2 weeks 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 2 weeks 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 2 weeks 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 2 weeks 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 2 weeks ago by GerdP

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

comment:19 Changed 2 weeks ago by GerdP

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

comment:20 Changed 2 weeks 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 2 weeks ago by stoecker

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

comment:22 Changed 2 weeks 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 2 weeks 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 2 weeks 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 13 days 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 days 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 days 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 11 days 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 11 days ago by GerdP

In 17220/josm:

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

  • replace ChangeCommand by ChangeMembersCommand

comment:30 Changed 11 days 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 10 days 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 9 days 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 9 days 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 9 days 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 9 days 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.

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.