Persistent cache 2

[mvglasow]

Includes improvements discussed with Emux and Ludwig:

• Read entire cache dir when initializing a persistent cache
• Reliably enforce cache size for persistent cache (data from previous instances is now included in the count)
• Fixed NPE which occasionally occurred in FileSystemTileCache#containsKey() when persistence was enabled
• Added test case for persistent tile cache
• Added missing compatibility constructors and helper methods for apps migrating from 0.5.0
• Use file timestamp to determine age of map file
• Changed TTL to TimeToLive in identifiers
• Comment for default TTL (86400000 millis = 1 day)
• Changed default Mapnik TTL to 8279s (current OSM setting) and added comment

As mentioned before, in order to enforce cache size (and also to fix the NPE Ludwig found) I now examine the entire cache dir when the cache is initialized and enter anything I find there in the lruCache. This might affect startup performance if the cache dir holds a large number of files.

[devemux86]

Thanks Michael for the report and for keeping the 2nd commit separate (we can check more easily the changes).

• I think at AndroidUtil.createTileCache methods we should have also unchanged ones for compatibility reasons.
  So some additions / modifications could be needed in order to have the old 4 ones and 2 new with all the thread and persistent options (now the methods that are not using the screenRatio are all changed).
• In the file system the tile cache follows the z/x/y pattern.
  (The TileStoreLayer has as default the z/y/x pattern)
• For the startup performance we need to be as fast as possible.
  Maybe using a threaded approach?

[applantation]

I am just looking at it now, will comment later.

On 20 December 2014 at 11:24, Emux notifications@github.com wrote:

Thanks Michael for the report and for keeping the 2nd commit separate (we
can check more easily the changes).

I think at AndroidUtil.createTileCache methods we should have also
unchanged ones for compatibility reasons.
So some additions / modifications could be needed in order to have the
old 4 ones and 2 new with all the thread and persistent options (now the
methods that are not using the screenRatio are all changed).

In the file system the tile cache follows the z/x/y pattern.
(The TileStoreLayer has as default the z/y/x pattern)

For the startup performance we need to be as fast as possible.
Maybe using a threaded approach?

—
Reply to this email directly or view it on GitHub
#557 (comment).

[mvglasow]

• AndroidUtil.createTileCache: I missed those, will add compatibility methods here too. I'll go through the code once more to make sure there are no other methods that need to be adapted for full API compatibility.
• File system tile cache: I understand that this is merely a documentation issue, right? Out of curiosity – why the z/y/x pattern for TileStoreLayer? The TMS standard, to which the docs refer, specifies z/x/y (see http://wiki.osgeo.org/wiki/Tile_Map_Service_Specification#Tile_Resources), and I see that confimed by briefly glimpsing at some OpenLayers-based web page.
• Startup performance and threading: I can try to reuse more of the standard code, so reading data from the previous instance will happen in a separate thread if the cache is threaded. Maybe I can set up a test case to measure performance (create a tile cache, load m×n tiles at zoom level k as well as all their parent tiles, destroy it, create a new one and measure how long rebuilding takes.)

[devemux86]

My thoughts about the pattern have emerged due to mvglasow@e318562#diff-187ae3feaa74b08b44371bbc1c7288bbR397 where the reading of the cache directory takes place and eventually the construction of the keys.
I'd suggest to follow the default pattern here.

Yes the common practice is z/x/y
http://wiki.openstreetmap.org/wiki/Slippy_map_tilenames
http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
and in TileStoreLayer we have indeed as "default" the z/y/x pattern (something that confused me too first).

[mvglasow]

In reality, I probably should remove the reference to z/x/y from the comment because the code is agnostic to that. If the cache directory is at /path/to/cache/dir and the method finds a tile at /path/to/cache/dir/3/2/1.tile, it will create an entry with key 3/2/1.

At that point it doesn't matter whether the convention is z/x/y or z/y/x or even x/y/z – the code will handle anything gracefully, as long as the path depth is correct (two levels of subdirs from the cache path) and the file has the correct extension. So the following would be rejected:

• /path/to/cache/dir/3/2.tile – incorrect path depth
• /path/to/cache/dir/3/2/1/0.tile – incorrect path depth
• /path/to/cache/dir/3/2/1.png – incorrect extension

[devemux86]

+1

[applantation]

I would prefer
private void readCacheDirectory() {
...
}

to be rewritten with a bit more code reuse and not to have logging on dead
branches.
The code duplication for validFile and valid Directory should go into
separate methods (cleaner, easier to change).

I am just testing, but I only have another few minutes, so I am sending
this off now. More later.

On 20 December 2014 at 13:41, mvglasow notifications@github.com wrote:

AndroidUtil.createTileCache: I missed those, will add compatibility
methods here too. I'll go through the code once more to make sure there are
no other methods that need to be adapted for full API compatibility.

File system tile cache: I understand that this is merely a
documentation issue, right? Out of curiosity – why the z/y/x pattern for
TileStoreLayer? The TMS standard, to which the docs refer, specifies
z/x/y (see
http://wiki.osgeo.org/wiki/Tile_Map_Service_Specification#Tile_Resources),
and I see that confimed by briefly glimpsing at some OpenLayers-based web
page.

Startup performance and threading: I can try to reuse more of the
standard code, so reading data from the previous instance will happen in a
separate thread if the cache is threaded. Maybe I can set up a test case to
measure performance (create a tile cache, load m×n tiles at zoom level k as
well as all their parent tiles, destroy it, create a new one and measure
how long rebuilding takes.)

—
Reply to this email directly or view it on GitHub
#557 (comment).

[ludwigb]

Just found an interesting problem, that totally confused me for quite some
time today.

It is replicable with the Samples app:

1. Start Samples/RenderthemeV4 app and let it create the tiles
2. Go to Settings and see that the options for the RenderthemeStyleMenu
   are there. They are (at the bottom of the Settings menu).
3. Go back to the map and return to main Samples menu.
4. Enter RenderthemeV4 example again. The tiles will now all come from
   the persistent tile cache.
5. Go to Settings and the RenderthemeStyleMenu (at the bottom of
   Settings) is gone.

The problem is that the style menu is only created when the Rendertheme is
being parsed, but when all the tiles come from the cache, the rendertheme
does not get created in the Databaserenderer TileBitmap
executeJob(RendererJob rendererJob) {

I do not really have a good solution right now. I always thought that that
part is a bit messed up, but until now it never showed. At least I now
understand the reason.

Ludwig

On 20 December 2014 at 14:25, Ludwig notifications@github.com wrote:

I would prefer
private void readCacheDirectory() {
...
}

to be rewritten with a bit more code reuse and not to have logging on dead
branches.
The code duplication for validFile and valid Directory should go into
separate methods (cleaner, easier to change).

I am just testing, but I only have another few minutes, so I am sending
this off now. More later.

On 20 December 2014 at 13:41, mvglasow notifications@github.com wrote:

AndroidUtil.createTileCache: I missed those, will add compatibility
methods here too. I'll go through the code once more to make sure there
are

no other methods that need to be adapted for full API compatibility.

File system tile cache: I understand that this is merely a
documentation issue, right? Out of curiosity – why the z/y/x pattern for
TileStoreLayer? The TMS standard, to which the docs refer, specifies
z/x/y (see
http://wiki.osgeo.org/wiki/Tile_Map_Service_Specification#Tile_Resources),

and I see that confimed by briefly glimpsing at some OpenLayers-based
web

page.

Startup performance and threading: I can try to reuse more of the
standard code, so reading data from the previous instance will happen in
a
separate thread if the cache is threaded. Maybe I can set up a test case
to
measure performance (create a tile cache, load m×n tiles at zoom level k
as
well as all their parent tiles, destroy it, create a new one and measure
how long rebuilding takes.)

—
Reply to this email directly or view it on GitHub
#557 (comment).

—
Reply to this email directly or view it on GitHub
#557 (comment).

[mvglasow]

@applantation what do you mean by "dead branches"? Those that don't refer to valid dirs/files? I've dropped that while adding isValidDirectory() and isValidFile() methods.

Since the two new methods allow their file argument to be null, I've also added a null check to isValidCacheDirectory.

About the Samples app: is there anything that needs to be fixed in my code?

[ludwigb]

I am just trying to fix this render theme problem, which I hope will be a
fix that does not affect your patch.

Will look at your changes a bit later.

Ludwig

On 21 December 2014 at 16:29, mvglasow notifications@github.com wrote:

@applantation https://github.com/applantation what do you mean by "dead
branches"? Those that don't refer to valid dirs/files? I've dropped that
while adding isValidDirectory() and isValidFile() methods.

Since the two new methods allow their file argument to be null, I've also
added a null check to isValidCacheDirectory.

About the Samples app: is there anything that needs to be fixed in my code?

—
Reply to this email directly or view it on GitHub
#557 (comment).

[applantation]

I just pushed a fix for the problem I described to dev, it moves the
parsing of the rendertheme into the creation of the TileRendererLayer,
rather than delay it to the point when tiles are rendered. This does not
change any user code. (The only real disadvantage is that this happens now
on the UI thread, but moving the parsing into a separate thread is possible
(via some sort of future) and I will look into that.

It looks like that this does not affect your pull request, but it needs to
be rebased on dev, I did this:

git fetch origin refs/pull/557/head:refs/remotes/pr/557

git checkout pr/557

git rebase dev

Ludwig

On 21 December 2014 at 18:07, ludwigb notifications@github.com wrote:

I am just trying to fix this render theme problem, which I hope will be a
fix that does not affect your patch.

Will look at your changes a bit later.

Ludwig

On 21 December 2014 at 16:29, mvglasow notifications@github.com wrote:

@applantation https://github.com/applantation what do you mean by
"dead
branches"? Those that don't refer to valid dirs/files? I've dropped that
while adding isValidDirectory() and isValidFile() methods.

Since the two new methods allow their file argument to be null, I've
also
added a null check to isValidCacheDirectory.

About the Samples app: is there anything that needs to be fixed in my
code?

—
Reply to this email directly or view it on GitHub
#557 (comment).

—
Reply to this email directly or view it on GitHub
#557 (comment).

[devemux86]

Yes I think I can see a performance hit on the rendering because of the render theme's parsing move.

[ludwigb]

Yes, I think that is when the UI thread gets blocked. Moving that into a
separate thread (but one that will run whether or not a tile gets rendered)
should remove that again.

On 22 December 2014 at 11:24, Emux notifications@github.com wrote:

Yes I think I can see a performance hit on the first rendering because of
the render theme's parsing move.

—
Reply to this email directly or view it on GitHub
#557 (comment).

[mvglasow]

Talking about threads: I looked through the code again, hoping that I could use the existing worker thread for my purposes. However, only creating the list entries one by one could be offloaded into the worker thread if I did it that way. The bulk of the work is to parse the directory tree, though, and that would have remained in the main thread.

To clarify: at this stage I am just feeding key/file mappings into lruCache – no parsing of actual tile data takes place here.

To get a feeling for startup performance when reading data from a previous instance, I set up a test that populates a cache with the area of a city rendered at zoom level 16 and all lower ones, then has another instance re-read the cache. I did some test runs on my machine, and rebuilding a cache of ~4600 tiles took somewhere in the range of 100–150 ms.

Would such a delay be acceptable? If not, I'd probably need a hand with the thread stuff – I'd like to see Ludwig's commit for moving render theme parsing into a separate thread so I could copy from his code.

[applantation]

The general rule for Android is not to have anything long-running or
blocking, e.g. I/O ops, on the UI-thread. What may seem acceptable
behaviour on a fast device, might trigger an ANR on others, which is
something to avoid.

I was just working on a solution with futures for the Rendertheme parsing,
but I now have something else to do, so will not be able to fix this today.

Ludwig

On 22 December 2014 at 13:40, mvglasow notifications@github.com wrote:

Talking about threads: I looked through the code again, hoping that I
could use the existing worker thread for my purposes. However, only
creating the list entries one by one could be offloaded into the worker
thread if I did it that way. The bulk of the work is to parse the directory
tree, though, and that would have remained in the main thread.

To clarify: at this stage I am just feeding key/file mappings into
lruCache – no parsing of actual tile data takes place here.

To get a feeling for startup performance when reading data from a previous
instance, I set up a test that populates a cache with the area of a city
rendered at zoom level 16 and all lower ones, then has another instance
re-read the cache. I did some test runs on my machine, and rebuilding a
cache of ~4600 tiles took somewhere in the range of 100–150 ms.

Would such a delay be acceptable? If not, I'd probably need a hand with
the thread stuff – I'd like to see Ludwig's commit for moving render theme
parsing into a separate thread so I could copy from his code.

—
Reply to this email directly or view it on GitHub
#557 (comment).

[mvglasow]

No problem, I might not get around to doing a lot of coding myself over
the next few days, so I'll just wait.

[mvglasow]

PS: I just pushed the changes I have made so far (excluding the cache rebuild speed test) in a separate commit.

[applantation]

I have just pushed a changes that moves the rendertheme parsing into a
Future (so it does not block the UI thread anymore and runs on a separate
thread in the background).

This seems to be working very well with the persistent cache changes: now
at startup we pull old tiles from the cache while the rendertheme gets
parsed in the background and is ready when the first new tiles need to get
rendered.

So far I have not seen any concurrency issues, but I will continue to test
(minus Christmas break).

This feels like a big improvement in user perceived performance, which is a
very welcome improvement.

Thanks.

Ludwig

On 22 December 2014 at 16:34, mvglasow notifications@github.com wrote:

PS: I just pushed the changes I have made so far (excluding the cache
rebuild speed test) in a separate commit.

—
Reply to this email directly or view it on GitHub
#557 (comment).

[devemux86]

(We should probably move the conversation about the render theme parsing in mailing list, as it involves the whole library)

I integrated the new commit Ludwig but I still see the performance hit at 1st rendering (compared with 0.5), will continue the tests.

[ludwigb]

I have just pushed the changes that Michael proposed and implemented in his
pull-request for a persistent cache feature
#557.
Thanks to Michael for putting up with our many change requests.

On top of Michael's change I have pushed two more changes:

• reading of the tile cache directory in a separate thread to move this
  I/O operation off the UI thread
• integration into Samples app with a setting (see
  Settings->Tilecache->Persistence) whether you want to activate it. Changing
  it to False will purge the cache.

What is currently missing is a purge-cache operation if any of the
Rendertheme Stylemenu options change (then all the tiles will need to be
re-rendered). Might do that later, time permitting.

My impression from limited testing is that particularly for larger-screen
devices (such as tablets) the perceptual performance improves as tiles are
just reloaded from disk when the activity is restarted.

Ludwig

On 23 December 2014 at 13:02, Emux notifications@github.com wrote:

(We should probably move the conversation about the render theme parsing
in mailing list, as it involves the whole library)

I integrated the new commit Ludwig but I still see the performance hit at
1st rendering (compared with 0.5), will continue the tests.

—
Reply to this email directly or view it on GitHub
#557 (comment).

[mvglasow]

Thanks for the merge and the improvements! Glad to finally have the code
in dev!

[devemux86]

I have pushed on dev an addition for complete methods of tile cache creation.

Thanks @mvglasow for the PR.