[Patch] Add a new TagMap as Set<String, String>

[michael2402]

This improves performance of the getKeys() method by adding a new Map that uses the same array format as AbstractPrimitive does. When changing keys, this method is often called.

I also added a setKeys that simply copies that array from that map.

I would propose merging this with AbstractPrimitive: We can have a common parent object (or even let AbstractPrimitive simply implement Map<String, String>). This would make the code much cleaner. WHen doing this, I suggest renaming things and deprecating the old names:

• getTags() returns a copy of the tags map
• setKeys -> setTags
• visitKeys -> visitTags (only used once as far as I know)
• removeAll -> clear
• isKeyTrue -> isTagTrue (same meaning as hasTag(key, <anything true>))
• getNumKeys -> size
• (hasKeys -> !isEmpty)
• hasKey -> containsKey

Those are the performance impacts of this patch only. This patch does not (yet) change the AbstractPrimitve to use the new implementation as default.

OLD:
`
TIMER OsmPrimitive#getKeys() with tag/node ratio 0.05: 59ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 0.3: 8ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 3.0: 28ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 20.0: 71ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 200.0: 390ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 0.05: 18ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 0.3: 17ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 3.0: 31ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 20.0: 74ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 200.0: 429ms
`

NEW:
`
TIMER OsmPrimitive#getKeys() with tag/node ratio 0.05: 54ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 0.3: 13ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 3.0: 8ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 20.0: 5ms
TIMER OsmPrimitive#getKeys() with tag/node ratio 200.0: 5ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 0.05: 29ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 0.3: 21ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 3.0: 19ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 20.0: 26ms
TIMER OsmPrimitive#getKeys().get(key) with tag/node ratio 200.0: 37ms
`

OsmReaderPerformanceTest (parse large OSM file)

OLD:
`
TIMER load compressed (.osm.bz2) 4 times: 8754ms
TIMER load .osm-file 4 times: 4006ms
`

NEW:
`
TIMER load compressed (.osm.bz2) 4 times: 8602ms
TIMER load .osm-file 4 times: 3905ms
`

[stoecker]

This probably affects memory footprint of JOSM by modifying a core class like the primitives. Can you give numbers how exactly the result is? Does it take more/less memory or does it stay the same?

What does it mean e.g. when 1 million ways/nodes are loaded which have tags assigned/not assigned?

[michael2402]

Memory footprint when simply using it:

Less. But only slightly. The set is always used temporarely during data updates / dataset loading. JOSM uses a Map<String, String> e.g. to pass on the tags to listeners. The tag storage inside the OsmPrimitive is independend from this.

Memory footprint when letting AbstractPrimitive inherit from it (not in this patch):

No change, since we simply move the tags array one level up in the hirarchy but the total field count stays the same. This just makes the code cleaner and re-useable.

[bastiK]

+1 for the patch.

Btw., visitTags is for higher performance (no intermediate HashMap), but it is a bit inconvenient, so rarely used.

[michael2402]

visitTags is good when doing some map-reduce-operations. Only used by a patch I made this summer to the graphics system to reduce object allocations. Even if we use it to copy tags between two OsmPrimitives it would not increase performance as much as simply copying the tags array.

[bastiK]

Replying to michael2402:

visitTags is good when doing some map-reduce-operations. Only used by a patch I made this summer to the graphics system to reduce object allocations. Even if we use it to copy tags between two OsmPrimitives it would not increase performance as much as simply copying the tags array.

Right, why am I telling you that... :)

@stoecker: Do you sign off on this?

[stoecker]

If there are no negative memory impacts I have no opinion. I assume it's a good thing to implement the patch :-)

[bastiK]

In 9649/josm:

applied #12355 - add a new TagMap as Set<String, String> (patch by michael2402)

[bastiK]

There have been changes in the meantime, [9536] in particular, but it should be okay.

[Don-vip]

This change breaks 58 tests and the taginfo target:
https://josm.openstreetmap.de/jenkins/job/JOSM/jdk=JDK7/829/

[Don-vip]

also MultiFetchServerObjectReaderTest hanged and I needed to kill -3 + kill -9 the junit process, but I don't know if it's due to this change. I added a timeout in r9655 to avoid the same problem again.

[Don-vip]

I can't download OSM data with current JOSM. did you forget something in the patch?

Caused by: java.lang.NullPointerException
	at org.openstreetmap.josm.data.osm.AbstractPrimitive.setKeys(AbstractPrimitive.java:446)
	at org.openstreetmap.josm.data.osm.OsmPrimitive.setKeys(OsmPrimitive.java:924)
	at org.openstreetmap.josm.data.osm.OsmPrimitive.load(OsmPrimitive.java:1261)
	at org.openstreetmap.josm.data.osm.Node.load(Node.java:260)
	at org.openstreetmap.josm.io.OsmReader.parseNode(OsmReader.java:216)
	at org.openstreetmap.josm.io.OsmReader.parseOsm(OsmReader.java:158)
	at org.openstreetmap.josm.io.OsmReader.parseRoot(OsmReader.java:120)
	at org.openstreetmap.josm.io.OsmReader.parse(OsmReader.java:106)
	at org.openstreetmap.josm.io.OsmReader.doParseDataSet(OsmReader.java:621)
	... 12 more

[Don-vip]

In 9656/josm:

see #12355 - fix NPE (regression from r9649)

[michael2402]

Simply calling setKeys(null) will use the TagMap implementation. Sorry for that, I did not think about it.