perf!: reduce work to build and fix bugs in TileLayer, and improve culling of off-screen tiles

[ignatz]

A couple of performance improvements:

• cheaper data structures
• cheaper construction of stale Tiles
• add missing early abort to _reainChildren
• fewer allocations
• and just do not render tiles that are off screen.

[JaffaKetchup]

Just to note with the latest commit and culling tiles off screen (I thought we already did this!):

• Be careful not to break panBuffer and zoomBuffer (I haven't tested this yet)
• (Less importantly, one of my long term goals might change this logic again: https://github.com/fleaflet/flutter_map/projects/4#card-90964116)

[ignatz]

Just to note with the latest commit and culling tiles off screen (I thought we already did this!):

Maybe you did something else? I did check the filter I added (where(visibleTileRange.contains)) and it does filter out tiles. It's not a no-op.

Be careful not to break panBuffer and zoomBuffer (I haven't tested this yet)

It shouldn't. I'm not pruning any more aggressively.

[JaffaKetchup]

It appears this has broken the zooming pruning/replacement logic (this fork on the left, latest 'master' on the right):

2023-11-05.11-04-10.mp4

[ignatz]

Great catch with the zooming. It was the render-pruning (not storage pruning). I now took the cheap way out. I checked again, there's still render-pruning happening but there's plenty of optimization left on the table.

The problem was that when zooming in the center of the parent tile wasn't within visible bounds anymore. To be correct we'd have to check for overlapping bounding boxes instead.

[ignatz]

Just to roughly quanitfy the impact. I've seen roughly a 4x reduction in time spend per frame on "building" the tile-layer. For rendering only a map, overall I've seen a 30+% reduction on the ui thread.

[ignatz]

I clawed back some the render thread performance improvements left on the table, i.e. the render pruning is more aggressive again. It's not perfect, we could be more selective (like the storage pruning) when the tiles at the target zoom-level are not yet ready.

I'm also reaching a point where my UI thread has plenty of head-room and the vast majority of frame drops are constituted by the render thread. At this point any render savings, even at the expense of some more work on the UI thread, are worth their weight in gold.

[ignatz]

I couldn't resist. Now we're rendering the actual minimal number of tiles w/o visual regressions. No more render performance left on the table by the tile renderer.

[ignatz]

Doing a bit of qualitative testing, I must admit the savings to the render thread are not overly impressive. On the bright side, I'm happy that flutter is more efficient at drawing images than cavases :)

Panning before and after (where you'd expect render pruning to have a lesser effect):

Zooming where you'd expect a bigger effect (After and Before):

:/ Makes one wonder if the extra work on the UI thread is even worth it. The benefits the UI thread are certainly a lot more pronounced ~30%. Happy to discuss. Will likely also depend on the specific platform

[josxha]

hi @ignatz, thanks for the pull request. I haven't tested the changes yet but I like the optimisation tweaks. (:
Can you check my comments?

[mootw]

doing a bit of profiling on my end. with the PR and comparing it to master. doing a simple pinch and zoom in and out im getting an average raster time of 4.7ms, and UI time of 1.7 ms. on master im getting 4.0ms raster, and 2.3ms UI. This is with rendering 3 raster layers on an unfolded z fold 5. im not really sure where the increase in raster time is coming from. There is a reduction in UI time but im not sure if that makes sense to trade off with raster time (given that is going to be our limiting factor for any vector layers. I haven't taken a detailed look at the code yet though.

Outdated

[ignatz]

@mootw yeah, I also didn't see a significant change in raster times either on Android either. W/o any reliable statistics, I've seen it go either way. We're definitely rendering fewer tiles though. As for the explanation why this doesn't show clear savings, I can only offer hypotheses:

• Rendering images is super cheap
• The more frequent changes to the render tree entail other costs that cancel out.

FWIW, the canvaskit renderer I'm actually seeing a slight improvement 🤷

As for my argument weighing off ui-thread vs raster-thread time, you might have miss-understood my babbling. I can leave the rastering untouched and do even less on the UI thread. I deliberately, did more work on the ui threadto reduce the number of tiles being put on screen. However, as you noticed yourself, it doesn't seem to pay off on Android in a clear way. Might be interesting to look at iOS 🤷

EDIT: one more thing to keep in mind when playing around: if you're just pinching in and out across one zoom-boundary you may also trigger some caching effects, e.g. some raster cached images getting invalidated slightly sooner when taken off screen already before storage pruning.
To my previous point, having seen it swing either way. This is an example of a long multi-level zoom-out:

[josxha]

I had some issues with displaying the correct tiles. This only happens if the tiles are loading from the web and not in memory cached.

Aufzeichnung.2023-11-07.222307.mp4

Performed tests:

When zooming out:

1. Start demo app
2. Zoom out to the next tile zoom level (here to 4.34)
   Expected behaviour:

• The tiles for the new zoom level get displayed after they finished loading
  Actual behaviour:
• The tiles from the lower zoom level are still displayed (at 4s in the video)
• An additional input (here a drag gesture) is needed before the correct tiles show up (at 6s)

When zooming in:

1. Start demo app (at 11s)
2. Zoom in to the next tile zoom level (here to 5.56)
   Expected hehaviour:

• The tiles for the new zoom level get displayed after they finished loading
  Actual behaviour:
• Not all tiles get displayed on the screen (at 16s in the video or see the following screenshot)
• After an additional input gesture (here dragging) all the correct tiles show up (at 17s)

This behaviour can be even better reproduced by using the ZoomButtonsPlugins page of the example app.

1. Start demo app and navigate to ZoomButtonsPlugins page
2. Click the zoom-in or zoom-out button
   Expected behaviour and actual behaviour is the save as in the other performed tests.

[ignatz]

Great catch and hanks for the detailed explanation, that made it crystal clear. From the video is hard to judge when the follow-up input was given.

Should be fixed now.

For context, the z-index sorting is broken at head. Funny enough it has two bugs that almost cancel each other out:

1. First it sorts in the wrong order. Small zIndex should go to the back and not the front for the Stack to render them on top.
2. Fortunately, the zIndex computation is broken as well: maxZoom is double.infinity amking the entire sort a no-op 😆

As a result, we render them in insertion order, with later tiles rendered on top due to the default hash map implementation preserving insertion order. When I changed the map representation to a map that does not preserve insertion order, we started to stack them in random order and the broken sorting started to surface more prominently 🤣

[josxha]

Funny enough it has two bugs that almost cancel each other out.

Thats somehow kind of impressive.
I tested again and couldn't find any other issues.

You can decide how to resolve the last change request. After that it's ready to merge.

[ignatz]

I snuck in one more minor change. I merged TileImageManager.(createMissingTiles|createMissingTilesIn) to have fewer code paths doing the same thing but I'm also hoping the reduce the number of TileCoordinates created in a follow-up that we're not spamming the GC with thousands of coordinates per frame.

Thanks for the review. Great catch on the tile stacking order.

[josxha]

lgtm!

[JaffaKetchup]

Ok, I'll review this tomorrow as well.
Would also appreciate it if we could whittle down this PR/commit title, but not sure how we're going to do that (also, it looks like we've begun to follow conventional commits, so it must be fix or perf, not both, but this isn't too important).

[JaffaKetchup]

LGTM, thanks @ignatz!

[ignatz]

Thanks for the review.

also, it looks like we've begun to follow conventional commits, so it must be fix or perf, not both

🤣 great example of process getting in the way :hide: