Store tangram geometry in pin

[Helium314]

Currently a tangram Point is created for each pin every time PinsMapComponent.set is called.

By storing the tangram geometry in the pin, this can be avoided, which improves performance of PinsMapComponent.set by roughly 30%, and reduces need for garbage collection.
Actually the change is only used when using QuestPinsManager.updateQuestPins (mostly after answering / hiding a quest), other calls to PinsMapComponent.set always use new Pins. Still it's an improvement, as solving quests happens quite often.

(a similar change is possible for StyleableOverlayMapComponent)

[westnordost]

This sounds good, the implementation is also good.

Two ideas for further improvement:

1. The constructor of com.mapzen.tangram.geometry.Point expects a java.util.Map but then the only thing it does with that is to iterate through its entrySet() to put it into an array. Implementing a ArrayMap that implements Map but just consists of a List of key-value pairs would take up less space in memory (probably doesn't really matter) and remove the necessity of calculating the hashCode for every element inserted into the map. I dimly remember that something similar already came up in the past - ... maybe it was not worth it or there was no discernible gain at all?
2. As the size of the map is known at construction time, it could be passed into the constructor to avoid that the map needs to grow when the items are inserted.

[Helium314]

I dimly remember that something similar already came up in the past - ... maybe it was not worth it or there was no discernible gain at all?

I remember that too. There was no measureable performance improvement, and the memory improvement is probably not worth it: using this info we may save somthing like 8 bytes per entry, and 16 bytes per map. For a high density area with 10k pins in view, this would be ~720 kB.
We could likely save more memory (and improve performance, and have something that remains after migration to MapLibre) by not creating pins that are hidden by others (same position lower priority quest).

But if it's really simple to do... (I didn't look at it)

As the size of the map is known at construction time, it could be passed into the constructor to avoid that the map needs to grow when the items are inserted.

Though for quests there are 7 entries, and default initial capacity is 16. Does unused capacity use memory?

[westnordost]

Though for quests there are 7 entries, and default initial capacity is 16. Does unused capacity use memory?

Yes, of course. But the more important thing is that it doesn't grow.

[westnordost]

We could likely save more memory (and improve performance, and have something that remains after migration to MapLibre) by not creating pins that are hidden by others (same position lower priority quest).

This would help performance in areas with high density of quests, but would require putting the quests/quest pins into a HashMap<LatLon, Quest(Pin)> first.

I think, probably not worth it because there are not that many that are actually on top of each other. They only seem that way because they appear after one another, e.g. width after lane, housenumber after building, roof shape after levels, lit after sidewalk (or other way round?), ...

[Helium314]

a similar change is possible for StyleableOverlayMapComponent

I was about to start a PR when I noticed that this is might need further changes, as the road colors depend on theme.
So I'll test whether it works anyway (not sure what exactly happens on light/dark switch), and if there are issues I think it's too much work.

[westnordost]

You could not set it in the lazy function and let StyleableOverlayMapComponent::set supply it if style.stroke == null && (style.strokeLeft != null || style.strokeRight != null)

[westnordost]

Ah nvm, not that easy as can't access the props in the tangram geometry once ist has been constructed

[westnordost]

I think the only possibility would be to not have the com.mapzen.tangram.geometry.Geometry directly as a lazy value but let StyleableOverlayMapComponent::set call a function on StyledElement, passing the resources (or directly the two color strings) and StyledElement caching the result of that function "manually".

[Helium314]

I added some logging and waited for an automatic theme switch. StyleableOverlayManager is destroyed on auto-switch, so the StyledElements are created again after the switch anyway.
When switching manually, one has to enter the settings. And when doing so, onStop is called and thus StyledElements are cleared.

[westnordost]

Sounds like this could be done then

[Helium314]

I dimly remember that something similar already came up in the past - ... maybe it was not worth it or there was no discernible gain at all?

Found it: https://github.com/streetcomplete/StreetComplete/pull/4152/files
For a dense area with several thousand quests this saves only 200 kB.

What I found to save a lot more is string interning when loading elements / tags from database: 4 MB saved for 38k elements, at the cost of ~5% slower loading.

[westnordost]

Hmm... yeah, that sounds like it make sense. On the other hand, memory is usually in ample supply so that memory-optimization at the cost of CPU is rarely worth it.
As far as I can gather, interning or not interning the strings should also not make any performance difference on accessing the HashMap, except if collisions happen massively.

Also, I am a bit skeptic if it is a good idea to intern every trash value that can be found in the OSM database. Maybe just the keys?

Anyway, if you are looking into interning, the first thing that comes to my mind is to intern the string elements (keys, values, but probably not the regex stuff) of the filter syntax during parsing.

[Helium314]

Some more detailed results (averaged over a few runs, memory use after GC):

• Current state
  • fetch nodes: 11.4 s
  • fetch DataWithGeometry: 23.6
  • memory use: 39.4 MB
• Creating HashMaps with with deserialized tags (this is necessary for actually using interning)
  • fetch nodes: 10.9 s
  • fetch DataWithGeometry: 22.8
  • memory use: 38.4 MB
• Intern keys only
  • fetch nodes: 11.9 s
  • fetch DataWithGeometry: 24.1
  • memory use of java: 36.4 MB
• Intern keys and values
  • fetch nodes: 12.6 s
  • fetch DataWithGeometry: 25.6
  • memory use of java: 34.8 MB

I have no clue why creating an additional HashMaps is actually faster than just keeping the map returned by decodeFromString...
But anyway, I don't have a strong opinion on what to do here, or whether to do anything.

[westnordost]

I think it is not really worth it.

[Helium314]

Alright, then let's leave it as it is