Modify

Opened 4 years ago

Closed 4 years ago

#12355 closed enhancement (fixed)

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

Reported by: michael2402 Owned by: michael2402
Priority: normal Milestone: 16.02
Component: Core Version:
Keywords: performance Cc:

Description

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
`

Attachments (1)

patch-add-tag-map.patch (11.7 KB) - added by michael2402 4 years ago.

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by michael2402

Attachment: patch-add-tag-map.patch added

comment:1 Changed 4 years ago by Don-vip

Keywords: performance added

comment:2 Changed 4 years ago by bastiK

Milestone: 16.02

comment:3 Changed 4 years ago by stoecker

Owner: changed from team to michael2402
Status: newneedinfo

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?

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

Last edited 4 years ago by michael2402 (previous) (diff)

comment:5 in reply to:  description Changed 4 years ago by bastiK

+1 for the patch.

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

comment:6 Changed 4 years ago by 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.

comment:7 in reply to:  6 Changed 4 years ago by 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?

comment:8 Changed 4 years ago by stoecker

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

comment:9 Changed 4 years ago by bastiK

Resolution: fixed
Status: needinfoclosed

In 9649/josm:

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

comment:10 Changed 4 years ago by bastiK

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

comment:11 Changed 4 years ago by Don-vip

Resolution: fixed
Status: closedreopened

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

comment:12 Changed 4 years ago by 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.

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

comment:14 Changed 4 years ago by Don-vip

In 9656/josm:

see #12355 - fix NPE (regression from r9649)

comment:15 Changed 4 years ago by michael2402

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

comment:16 Changed 4 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain michael2402.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.