Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple maintenance() calls possibly impacting performance #1128

Closed
pasniak opened this issue Aug 5, 2018 · 12 comments
Closed

Multiple maintenance() calls possibly impacting performance #1128

pasniak opened this issue Aug 5, 2018 · 12 comments

Comments

@pasniak
Copy link
Contributor

pasniak commented Aug 5, 2018

Why, when scrolling around I get multiple maintenance() calls which seem to slow down the display (see screenshots)? Maybe the should be compacted into periodic call?

    public void maintenance() {
        this.garbageCollection();
        this.mPreCache.fill();
    }

Issue Type

[x] Question
[ ] Bug
[ ] Improvement
[ ] Build system related
[x] Performance
[ ] Documentation

Description and/or steps/code to reproduce the problem

screenshot from 2018-08-05 09-57-41

screenshot from 2018-08-05 10-02-37

@pasniak
Copy link
Contributor Author

pasniak commented Aug 5, 2018

We probably can close this. My long[] mTileIndices (in MapTileList) is 301 elems long and the loop in contains is slow in Debug mode. Is is fast in Release.

@monsieurtanuki
Copy link
Collaborator

@pasniak Looking back at the code there might be a couple of possible (but light) improvements.
Let me see...

@monsieurtanuki
Copy link
Collaborator

First thing: be sure that you pulled the latest version of master. I recently removed a duplicated code execution regarding protectDisplayedTilesForCache in DefaultOverlayManager.onDraw.

A solution would be to increase the capacity using MapTileCache.ensureCapacity (up to 301 in your case?). With a bigger capacity, we don't have to check which tiles we can remove from the cache (with a time-consuming O(n) contains method) because we have enough space (cf. MapTileCache.garbageCollection).

Another solution would be to use smaller protected tile sets (cf. MapTileProviderBasic's constructor: getTileCache().getProtectedTileComputers()). With smaller sets, it's faster to compute the sets and it's faster to check which tiles the sets contain (cf. MapTileCache.garbageCollection and MapTileCache.shouldKeepTile)

I'm also working on the following idea: adding a message in the log. Something like "You'd have better performances with a tile cache capacity of xxx, or with smaller protected-cache-tile sets".

@pasniak
Copy link
Contributor Author

pasniak commented Aug 6, 2018

I am trying to get my head around the ensureCapacity... (where to set it? do you have an example?)

@monsieurtanuki
Copy link
Collaborator

Something like getTileCache().ensureCapacity(50); in MapTileProviderBasic.

But I'm actually PR'ing something called setAutoEnsureCapacity, in order to automatically upgrade the cache capacity to the level of convenience specified by getTileCache().getProtectedTileComputers().

@pasniak
Copy link
Contributor Author

pasniak commented Aug 6, 2018

👍 the computers are beyond my pay grade...

@monsieurtanuki
Copy link
Collaborator

Just PR'ed #1131

@monsieurtanuki
Copy link
Collaborator

Btw the concept of protected tile is not that hard to understand - I assume.

The idea is to say "OK we currently display those tiles, but there's a good chance we'll soon need tiles for zooms +-1 or tiles at the border".
That explains this code in MapTileProviderBasic:

getTileCache().getProtectedTileComputers().add(new MapTileListZoomComputer(-1)); // zoom -1
getTileCache().getProtectedTileComputers().add(new MapTileListZoomComputer(1)); // zoom +1
getTileCache().getProtectedTileComputers().add(new MapTileListBorderComputer(1, false)); // border of 1 around, and just the border

And what happens is that during the maintenance we do basically 2 things:

  1. ensure that those "protected" tiles are not removed from the memory cache, as we assume they will probably very soon be important when zooming/panning - there's a trade between low memory management and reactivity
  2. asynchronously pre-cache the "protected" tiles into the memory cache, from a limited list of providers (cf. getTileCache().getPreCache().addProvider)

@monsieurtanuki
Copy link
Collaborator

The next thing I would like to fix is the "is that tile in the protected list?" algorithm.
Currently, the map tile lists are just long[], which can only be processed iteratively.
The bigger the list, the slower the contains check. Which happens during the UI thread...
The solution would be to consider a sort of zoom+rectangle container instead of a mere list.
I'll think about it...

@monsieurtanuki
Copy link
Collaborator

The solution would be to consider a sort of zoom+rectangle container instead of a mere list.

Done in #1131
The name is MapTileArea

@monsieurtanuki monsieurtanuki self-assigned this Aug 11, 2018
@monsieurtanuki
Copy link
Collaborator

Just merged #1131.
@pasniak Feel like testing it?

cbalster pushed a commit to cbalster/osmdroid that referenced this issue Oct 28, 2018
Removed class:
* `LRUMapTileCache`: already deprecated class, removed _en route_

Impacted classes:
* `IConfigurationProvider`: impacted the removal of `LRUMapTileCache` in comments; gently refactored
* `MapTileCache`: new `boolean mAutoEnsureCapacity` and its getter; optimized method `garbageCollection` using `mAutoEnsureCapacity`; method `ensureCapacity` now returns a `boolean` ("actually changed the capacity?"); optimized method `shouldKeepTile`
* `MapTilePreCache`: added a little optimization in method `fill`; fixed typos
* `MapTileProviderBasic`: set the new `MapTileCache.mAutoEnsureCapacity` to `true`
cbalster pushed a commit to cbalster/osmdroid that referenced this issue Oct 28, 2018
… MapTileArea

`MapTileArea` now replaces `MapTileList` in most operations, for performance reasons. In `MapTileAreaTest.testPerformances` on a 100 item list: the init phase goes 50 times faster, the lookup phase goes about twice faster for an item in the list, and 10 times faster for an item not in the list.

New classes/interface:
* `MapTileArea`: a zoom/xmin/xmax/ymin/ymax tile area, that replaces the array of tile indices kept in `MapTileList` for performance reasons
* `MapTileAreaTest`: a unit test class for `MapTileArea`
* `MapTileAreaList`: a list of `MapTileArea`
* `MapTileAreaListTest`: a unit test class for `MapTileAreaList`
* `MapTileAreaComputer`: Compute a map tile area from a map tile area source, replacing `MapTileListComputer`
* `MapTileAreaBorderComputer`: Compute a map tile area from a map tile area source - the source with a border
* `MapTileAreaBorderComputerTest`: a unit test class for `MapTileAreaBorderComputer`
* `MapTileAreaZoomComputer`: Compute a map tile area from a map tile area source - the source on another zoom level
* `MapTileAreaZoomComputerTest`: a unit test class for `MapTileAreaZoomComputer`

Deprecated classes/interface:
* `MapTileListComputer`
* `MapTileListBorderComputer`
* `MapTileListBorderComputerTest`
* `MapTileListZoomComputer`
* `MapTileListZoomComputerTest`

Other impacted classes:
* `MapTileCache`: replaced `MapTileList*` with `MapTileArea*`; created `private` method `refreshAdditionalLists` for code readability
* `MapTileCacheTest`: replaced `MapTileList*` with `MapTileArea*`
* `MapTilePreCache`: replaced `MapTileList` with `MapTileAreaList`
* `MapTileProviderBasic`: replaced `MapTileList*` with `MapTileArea*`
* `TilesOverlay`: replaced `MapTileList*` with `MapTileArea*`
cbalster pushed a commit to cbalster/osmdroid that referenced this issue Oct 28, 2018
Impacted class:
* `MapTileAreaTest`: removed a risky performance test
cbalster pushed a commit to cbalster/osmdroid that referenced this issue Oct 28, 2018
@monsieurtanuki
Copy link
Collaborator

@pasniak ping
The fix is available in the new 6.0.3 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants