Spatial cache for OsmQuestController, OsmNoteQuestController and MapDataController

[Helium314]

A first version for the OsmQuestController cache discussed in #4079.
I left the comments to clarify my motivations.

Main benefits

• much faster get for quests in view (called on opening and solving quests)
• much faster getAllVisibleInBBox when tiles are cached, otherwise marginally slower
  This translates to a ~30% improvement of QuestPinsManager.onNewTilesRect, as creating and updating pins is rather slow.

What is missing:

• unit tests
• decide cache size
• clear / trim cache on low memory
• maybe bugs?

[westnordost]

Reading vertically through the code:

• given that there should also be a cache for the MapData, the Cache class should be generic (just like Android's LruCache is). At least in regard what is in the tiles that are grouped by TilePos
• it is a very bad idea to let the cache class extend LinkedHashMap. This exposes all the interface of the LinkedHashMap to the outside and that class is not threadsafe. If you are not well versed in writing threadsafe code, better not stride far from the reference implementation (LruCache)
• there is no place for deferring updates to the cache to the background, there is no place for coroutines in the cache class (i.e. it does not belong there). It does not make sense at all, actually. The cache must always be up-to-date, only the updates to the database may be deferred (because the cache is always up to date). It is however the job of the *Controller class to coordinate this, not the cache
• the cache class is doing far too much. It is not the job of the cache to translate between bbox and tilepos forth and back. It should really be just a get/put store with e.g. a create-lambda passed in the constructor

[Helium314]

So:

• move the map to an private val
• move most code to the controller
• with those changes I don't see much use for a create-lambda, as this would have to query the db for each tile separately (no bbox - tilepos translation!), and that was the first thing I tried and found too slow (not surprising)

[westnordost]

with those changes I don't see much use for a create-lambda, as this would have to query the db for each tile separately (no bbox - tilepos translation!

Hmm right. I guess there are two options:

• cache has TilesRect (not TilePos) on interface
• leave BoundingBox on interface

For the former case, the code in the controller would look like this:

fun get(bbox) {
  val tilesRect = bbox.asTileRect(zoom=16)
  val data = cache.get(tilesRect)
  return data.filter { it in bbox }
}

This means that the cache first returns all the data from all the tiles that intersect the bbox, the controller filters it out afterwards. I reckon this may be a little unperformant, especially if the bboxes queried do not align with the tiles (thinking about map data cache here).

For the latter, the code above would basically be in the cache. Though, it can already not add the element within each cached tile to the result set and have some additional logic that would not even check for each element if it is inside the bbox or not for tiles that are completely contained within the bbox. Hence, even faster.

So overall, it looks like your original solution to which to use on the interface (bbox or tiles) is more performant. Plus, thinking about that there is the same cache class for all the controllers that manage geospatial data (quests, map data, notes), it is certainly less code because the above logic would not have to be repeated in each controller.

[Helium314]

fun get(bbox) {
  val tilesRect = bbox.asTileRect(zoom=16)
  val data = cache.get(tilesRect)
  return data.filter { it in bbox }
}

This means that the cache first returns all the data from all the tiles that intersect the bbox, the controller filters it out afterwards. I reckon this may be a little unperformant, especially if the bboxes queried do not align with the tiles (thinking about map data cache here).

For the latter, the code above would basically be in the cache. Though, it can already not add the element within each cached tile to the result set and have some additional logic that would not even check for each element if it is inside the bbox or not for tiles that are completely contained within the bbox. Hence, even faster.

The cache could return a Map<TilePos, Data> instead of a Set<Data>, which would allow the get(bbox) to avoid filter { it in bbox } for fully contained tiles.
Or maybe a lambda for combining data and another one for filtering by bbox could be given to the cache... but then again the same functions could also be called in get(bbox), which would give a bit more flexibility (e.g. combining the Map<OsmQuestKey, Quest> actually doesn't need to create another map, a flattened list of values is sufficient).

A problem I see with a generic cache with lambdas is that the cache for OsmQuestController needs a lambda (bbox, questTypeNames) -> T for decent performance, but a generic cache would only have (bbox) -> T.

So overall, it looks like your original solution to which to use on the interface (bbox or tiles) is more performant. Plus, thinking about that there is the same cache class for all the controllers that manage geospatial data (quests, map data, notes), it is certainly less code because the above logic would not have to be repeated in each controller.

Actually I'm not sure how much really can be done here in a generic way, the logic for combining and filtering data will still need to be in the controller, either in get(bbox), or in the lambda.

[westnordost]

A problem I see with a generic cache with lambdas is that the cache for OsmQuestController needs a lambda (bbox, questTypeNames) -> T for decent performance, but a generic cache would only have (bbox) -> T.

IMO that's preliminary optimization. The controller can just cache.get(...).filter { it.name in questTypeNames }. Later, one can think about subclassing (or rather: wrapping) the generic cache if this really makes a big difference. (Or have the spatial cache in VisibleQuestsSource instead and let the OsmQuestController just have a cache for the get(id) query.

Actually I'm not sure how much really can be done here in a generic way, the logic for combining and filtering data will still need to be in the controller, either in get(bbox), or in the lambda.

I don't understand, why would the combining (of the data in possibly several tiles) and filtering (the elements whether they are contained in the bbox) need to be done in the controller for the latter case, i.e. the SpatialCache.get(bbox)?

[Helium314]

A problem I see with a generic cache with lambdas is that the cache for OsmQuestController needs a lambda (bbox, questTypeNames) -> T for decent performance, but a generic cache would only have (bbox) -> T.

IMO that's preliminary optimization. The controller can just cache.get(...).filter { it.name in questTypeNames }. Later, one can think about subclassing (or rather: wrapping) the generic cache if this really makes a big difference. (Or have the spatial cache in VisibleQuestsSource instead and let the OsmQuestController just have a cache for the get(id) query.

I actually went from caching all quests to caching only visible quests because it makes a considerable difference when fetching from DB.
So I guess the cache needs to move to VisibleQuestsSource then.

A get(id) cache could then be a simple LRU cache (containing a fixed maximum number of quests) that could also use the quests returned by getAllVisibleInBBox.

[westnordost]

I actually went from caching all quests to caching only visible quests because it makes a considerable difference when fetching from DB.

I don't understand why though. I guess it must be about the extra quests returned that are not visible. Are you sure that you create a HashSet for the collection questTypes passed into getAllVisibleInBBox? If it is just a list, it is no wonder why it is slow (100+ iterations per retrieved quest).

Also, if this is still slow, there there is another solution than moving the cache to VisibleQuestsSource:

1. memorize with what questTypes getAllVisibleInBBox has been called last
2. if the questTypes on the next call differ, clear the cache

[Helium314]

I guess it must be about the extra quests returned that are not visible.

That's what I concluded. The slow part is getAllVisibleInBBoxFromDB, which is exactly the same as getAllVisibleInBBox without this PR.
When I tried the cache with getting all quests, I simply passed null instead of a quest type list/set.

Also, if this is still slow, there there is another solution than moving the cache to VisibleQuestsSource:

1. memorize with what `questTypes` `getAllVisibleInBBox` has been called last
2. if the `questTypes` on the next call differ, clear the cache

Right, I didn't think of simply moving this from cache to the controller.

[Helium314]

Some semi-related comments:
I can only use the position in bbox you suggested if I change

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/util/math/SphericalEarthMath.kt

Line 468 in c20190b

fun BoundingBox.contains(pos: LatLon): Boolean {

to

operator fun BoundingBox.contains(pos: LatLon): Boolean {

---

OsmQuest.key is used frequently. Does this create a new OsmQuestKey every time?

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/data/osm/osmquests/OsmQuest.kt

Line 20 in c20190b

override val key: OsmQuestKey get() = OsmQuestKey(elementType, elementId, questTypeName)

And if yes, would it make sense to reduce calls e.g. by making OsmQuest.key by lazy?

I general I find measuring performance easy enough, but for things that may or may not waste memory I have

• little background knowledge
• no estimation / feeling for how much memory something uses
• no way of measuring e.g. memory used by a map and by its contents

Is there some way Android Studio can help on the last point?

[Helium314]

I did some comparison between the current cache(s) for MapDataController and my old attempt of the one cache (LinkedHashMap<TilePos, Map<ElementKey, Pair<Element, ElementGeometry>>>), focusing on getMutableMapDataWithGeometry.
There is not much difference in terms of performance, except that the one cache is roughly twice as fast when retrieving entire tiles (maybe because it already contains "everything" for that tile).

There is some difference in memory usage, judging by java memory part of the Android Studio memory profiler. Loading 8 tiles into cache increases java from ~26 to 32 for one cache, while for the cache in this PR it goes up to 44.
That seems like a pretty large difference, did I do something completely wrong? Either in "measuring" or with the caches.

[westnordost]

OsmQuest.key is used frequently. Does this create a new OsmQuestKey every time?

yes

And if yes, would it make sense to reduce calls e.g. by making OsmQuest.key by lazy?

only if it is the same OsmQuest on which key is called frequently. Currently, quests are being fetched anew from the database all the time. Also, if key is kind of guaranteed to be called at least once, it could also be just instead of an indirection added via lazy.

override val key = OsmQuestKey(elementType, elementId, questTypeName)

Is there some way Android Studio can help on the last point?

Yes, it's quite powerful, too: https://developer.android.com/studio/profile/memory-profiler
To measure the size of the cache at one point, you could implement a button or something that will trigger all the caches being cleared (i.e. emulate onLowMemory system call) and see how the memory use changes after that.

[Helium314]

only if it is the same OsmQuest on which key is called frequently. Currently, quests are being fetched anew from the database all the time. Also, if key is kind of guaranteed to be called at least once, it could also be just instead of an indirection added via lazy.

Currently the questCache holds all quests by key, and the spatialCache holds all quest positions by key.
But if the quest is fetched directly from DB (get(questKey) when not cached), or created outside the cached area or for hidden quest types (on upload), the key is not needed. So I think lazy would be most suitable.

Yes, it's quite powerful, too: https://developer.android.com/studio/profile/memory-profiler To measure the size of the cache at one point, you could implement a button or something that will trigger all the caches being cleared (i.e. emulate onLowMemory system call) and see how the memory use changes after that.

Ok, thanks!
So I can conclude the size of the cache is just the difference before and after clearing caches?

[westnordost]

So I can conclude the size of the cache is just the difference before and after clearing caches?

Probably. I am not sure how quickly the garbage collector will collect it. IIRC there is a call you can make to force garbage collecting now (IMO useful only for debugging).

[westnordost]

I've now reviewed the SpatialClass class only so far. Looks quite good already, I only found one conceptual issue. I will review the rest later.

[westnordost]

I re-read the things you clarified in code and added some comments etc myself, see the git change history

[westnordost]

(Please also note the TODOs I added in the code, requesting you to write an explanatory comment why it is done like that, in case you missed them)

[westnordost]

Thanks for adding the explanatory comments!

[westnordost]

So, this is done except responding to my remark regarding caching nulls. Also, I am a bit concernerd that you didn't even have to change any tests when you changed this behavior. Test coverage of the MapDataCache could probably be improved. But in any case, I guess we can add more tests later, too, it's not a blocker. I plan to release an alpha for v48.0 latest on the 30ths of September, so this can go in there.

[westnordost]

So, ready to merge, right? I suppose even if you planned to add some more tests for MapDataCache, it might as well be a separate PR

[Helium314]

Yes, it's ready.
Do you have any specific tests in mind?

[westnordost]

I skimmed through the implementation and compared to the tests that exist so far. Note that the implementation is different for ways and relations, thus putting ways and relations into one bucket in the tests mean that one of the two execution paths are not covered

Striked points are what I already found in the tests (updated Sept 29, 14:02):

Missing tests

• "is in spatial cache" logic in line 121ff for ways and line 157ff for relations, each. Should probably be covered in several tests due to the complexity

• get by bbox when all is cached

• get by bbox when some is not cached

• get by bbox when all is not cached

• (get by bbox trims data)

Current tests

• update puts node if tile is cached

• update does not put node if tile is not cached

• update puts way

• update puts relation

• update puts way geometry

• update puts relation geometry

• update deletes nodes

• update deletes ways

• update deletes relations

• get / fetch node when node not in spatial cache

• get / fetch node when node in spatial cache

• get / fetch way

• get / fetch relation

• get / fetch node geometry

• get / fetch way geometry

• get / fetch relation geometry

• get / fetch ways by node ids

• get / fetch ways by node ids

• get / fetch ways by node ids is is cached for nodes inside bbox after updating with bbox

• get / fetch relations by member

• clear clears everything

• trim trims

• ways by node id does return just replaced way that now does contain the node but before it didnt

• relations by member does return just replaced relation that now does contain the node but before it didnt

• ways by node id does not return just replaced way that does not contain that node anymore

• relations by member does not return just replaced relation that does not contain that member anymore

• ways by node id does not return just deleted way

• relations by member does not return just deleted relation

• get multiple elements, all are cached

• get multiple elements, some are cached

• get multiple elements, none are cached

• get multiple nodes, all are cached

• get multiple nodes, some are cached

• get multiple nodes, none are cached

• get multiple geometries, all are cached

• get multiple geometries, some are cached

• get multiple geometries, none are cached

[Helium314]

I updated/clarified and added a few of the tests...

[westnordost]

Very cool! I updated the above list

[Helium314]

now done:

ways by node id does return just replaced way that now does contain the node but before it didnt

relations by member does return just replaced relation that now does contain the node but before it didnt

ways by node id does not return just deleted way

relations by member does not return just deleted relation

get multiple elements, all are cached

get multiple elements, some are cached

get multiple elements, none are cached

get multiple nodes, all are cached

get multiple nodes, some are cached

get multiple nodes, none are cached

get multiple geometries, all are cached

get multiple geometries, some are cached

get multiple geometries, none are cached

[westnordost]

Awesome!

[Helium314]

Added some more tests covering the "is in spatial cache" logic

And a test each for

• ways by node id does not return just replaced way that does not contain that node anymore
• relations by member does not return just replaced relation that does not contain that member anymore

The rest will come later with a PR...

[westnordost]

Perfect! Man, this PR is so massive, both in effort invested and in the gain provided! I'm glad that it can go live finally in v48 :-)

[westnordost]

It's also by far the most commented/reviewed one, and, the 1000ths closed pull request 🥳