[josm-dev] Jumbo Patch v3 (or so)

[Gabriel Ebner]

On Mon, Dec 17, 2007 at 12:15:01AM -0800, Dave Hansen wrote:
> So, I actually implemented the read-only view of the Way.nodes list.
> Works like a charm!  (Instead of blathering about it in an email on
> proper encapsulation :P)
> 
> http://dev.openstreetmap.org/~daveh/josm/003/
> 
> This removes a pretty serious chunk of the original diff.  It shrunk by
> at least 10k or so, and I can probably do even more pruning.

Maybe I didn't get my point across clearly, but I think that there are
concrete problems introduced by the very design of adding the reverse-lookup
table into the nodes:



1) Extending ways[1] will create duplicate entries in Node.wayList.
DrawAction creates a copy of the way using "new Way(w)" and then appends the
node to the copy.  This will cause duplicate entries in Node.wayList since we
add the way to the reverse-lookup table regardless of whether it is in the
dataset or not.

[1] The same applies for deleting nodes, merging nodes, joining nodes to ways,
reversing ways and combining ways.

2) Deleting ways will leave spurious entries in Node.wayList.  Since we don't
remove ways from Node.wayList upon executing a DeleteCommand, Node.wayList
will contain references to the way even after it's been deleted.

3) Undoing the addition of a way will leave spurious entries in Node.wayList.
When adding a way we add it to Node.wayList, but we don't remove it from there
when we undo it.

4) All of the above will cause memory leaks.  Whenever a way in memory uses a
given node, Node.wayList will hold a reference to the way.  Since we don't
take care of the reverse-lookup table in ChangeCommand, DeleteCommand, we are
in effect preventing the GC from freeing the way after clearing the undo list
after upload.



Point 1 cannot be solved without removing ChangeCommand and adding all sorts
of new Command subclasses like ReplaceTagsCommand, ReplaceNodesCommand, and so
on.  This would entail modifying nearly all of the actions, and
double-checking that we don't create any ways without disposing[2] them
correctly.

Point 2 and 3 could be solved by modifying the DeleteCommand to remove the
backreferences.

Solving point 4 would mean explicitly disposing of commands, both when
discarding the redo list and when clearing the undo list.

It is probable that this change introduces further possible leaks in
MergeVisitor because we don't dispose the ways from the dataset to be merged
and also further possible bugs because we call cloneFrom() in MergeVisitor and
Node.cloneFrom copies the backreferences, which might point into the wrong
data set.

[2] The backreferences are created automagically when creating a new way
object or adding nodes to it.  This means that creating a way is no longer
side-effect free, and therefore you'll have to explicitly call dispose() to
undo the side-effects if you no longer need the object.

> Gabriel, I also did some serious work to DuplicatedNodes.  I took your
> ideas to heart, and implemented something that actually creates a map
> from LatLon->[Types]->[Nodes].  So, it's a hash of LatLons, which
> indexes into a list of the types of nodes at that LatLon, then the
> nodes.

Okay, that looks better now.  Though we could probably still save some sixty
lines by calling MergeNodes.mergeNodes(sel, nodes) in fixError instead of
doing the merging ourselves.

  Gabriel.