Skip to content

Tile combination by zoom level#3704

Closed
jesusbotella wants to merge 6 commits into
visgl:masterfrom
CartoDB:aggregate-tiles-by-zoom
Closed

Tile combination by zoom level#3704
jesusbotella wants to merge 6 commits into
visgl:masterfrom
CartoDB:aggregate-tiles-by-zoom

Conversation

@jesusbotella
Copy link
Copy Markdown
Contributor

For #3695

Background

While using Deck.gl's TileLayer implementation to render MVT tiles retrieved from our backend, the main issue that we found is that the tile rendering process takes more time than it should to avoid lag and provide a nice user experience while panning and zooming the map.

Understanding Deck.gl's TileLayer behaviour, we found out that the TileLayer is a CompositeLayer that renders N sublayers (GeoJSON layer by default), two for each of the tiles within the viewport at least.

Viewport in a 1080p display at fullscreen contains 24 tiles, excluding the main TileLayer and derived layers from points, lines and polygons, which could make a total of 49 layers (or even almost 2x with polygons and fill and stroke layers) if every tile contains 1 geometry at least.

Performing analysis using Chrome DevTools' performance tool, the rendering process for a dataset with 16k lines takes an avg. of 1062ms taking a sample of 10 executions, which is way more to what it is recommended to avoid lag or hangs when interacting with the content.

This is how the flame chart looks like:
Screenshot 2019-09-26 at 12 59 12

Proposal

We thought of combining all tiles within a single zoom level into a single tile to ease the rendering process and make it faster. This is what changes are about in this PR.

We introduced a new concept called CompositeTile, which is just an abstraction for holding a set of tiles and taking care of all data management so that we only create a single layer in renderLayers method.

That way we would only render 3 or 4 layers at most in each renderLayers cycle, lowering the time that each requestAnimationHandler cycle takes, and improving the performance.

This is how the flame chart looks like:
Screenshot 2019-09-26 at 13 04 56

We created a set of different examples with different geometries to assess our performance improvements. Those examples are here: https://deck-comparisons.glitch.me. You will find there the examples with the dataset specifications.

Performance Improvements

We performed a set of performance comparisons between the production version of Deck.gl and our custom Deck.gl version with those changes applied.

Tests were executed with the same configuration as in the screenshot above (local assets, advanced paint instrumentation enabled, and no CPU throttling). Measures are taken from the latest animation frame where all layers are rendered to avoid flakiness or tests variation due to network fluctuations.

These are the stats that we got:

Points' example - 500k points

Avg. Median Max. Min. P80
Original 127.42ms 124.89ms 155.85ms 107.26ms 139.57ms
Improved 78.5ms 69.31ms 143.69ms 51.56ms 97.82ms
Percentages of Improvement 38.4% 44.5% 7.81% 51.93% 29.92%

Lines' example - 16k lines

Avg. Median Max. Min. P80
Original 1062.09ms 993.58ms 1398ms 788.35ms 1319ms
Improved 343.05ms 336.245ms 457.56ms 289.32ms 368.26ms
Percentages of Improvement 67.71% 66.16% 67.28% 63.31% 72.09%

Percentages of improvement seem good in both cases and we would like to know what you think about this approach of combining tiles, and if it is good to go for this approach.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+2.7%) to 82.673% when pulling 4d56a71 on CartoDB:aggregate-tiles-by-zoom into e6834f6 on uber:master.

@Pessimistress
Copy link
Copy Markdown
Collaborator

Pessimistress commented Sep 26, 2019

@jesusbotella Thank you for the detailed probe into this issue.

If I understand it correctly, your proposal is to merge all visible tiles into one dataset (layer) per zoom level.

I have some concerns about introducing the new "combined tile" concept:

  • Depending on the size of data, it may actually cost more to create new attributes every time data is updated.
  • Not all tile data comes in array format.
  • Not all data types can be combined then rendered with a single layer, for example this.

I think the root issue in the original approach was that we call renderSubLayer even when nothing has changed for a particular tile. The render and subsequent layer diffing both have perf costs. Instead, we could cache and reuse the rendered layer with the tile.

Edit: I put together a proof of concept: https://github.com/uber/deck.gl/compare/x/tile-layer-cache?expand=1 Could you give it a test?

@jesusbotella
Copy link
Copy Markdown
Contributor Author

Thank you for the fast response @Pessimistress.

Yours are fair concerns that conflict with this approach of combining all tiles' data into a single tile. Could you please elaborate more on the first issue? I would like to fully understand what it is about.

And thank you very much for putting together that proof of concept, I tested it thoroughly and it dramatically improves the animation frame handler execution times. Would you have plans of developing/merging that improvement to the master branch in the near feature or do you want us to take a further look into it? We're really interested in that improvement.

Our particular interest in combining tiles comes from the use case of rendering MVT tiles as you had in your TileLayer example, and especially when it comes to rendering polygons split into different tiles. Seeing that this approach is not feasible for a general TileLayer, do you think it would make sense to create a more specific MVTTileLayer for that?

The issue with the polygons, as you may imagine, is that as polygons are cut between tiles, the representation is not ideal because of the tile border axis. We came up with a mechanism to merge all the parts into one polygon and then represent it within the "global" layer. But if we don't have a single tile, we would need to link the polygon to a specific tile and then omit it in other tiles.

@Pessimistress
Copy link
Copy Markdown
Collaborator

Pessimistress commented Sep 30, 2019

Could you please elaborate more on the first issue?

If we combine all visible tiles into one layer, then as you pan around and tiles load, that layer needs to reprocess all geometries that are visible, instead of those of the newly loaded tile. The cost of this depends on how much data is visible in one viewport.

Would you have plans of developing/merging that improvement to the master branch in the near feature?

I'm glad that it works well! I will open a PR. Timing is a little awkward given that we are publishing 7.3.0 today. We can discuss whether this change can be justified as a patch.

rendering polygons split into different tiles

That is a fair point that this layer does not properly address right now. I think a dedicated MVTTileLayer might be the best solution. What are you using for decoding?

@jesusbotella
Copy link
Copy Markdown
Contributor Author

jesusbotella commented Oct 1, 2019

If we combine all visible tiles into one layer, then as you pan around and tiles load, that layer needs to reprocess all geometries that are visible, instead of those of the newly loaded tile. The cost of this depends on how much data is visible in one viewport.

Yes, sure, it makes sense. I understand your point now.

I'm glad that it works well! I will open a PR. Timing is a little awkward given that we are publishing 7.3.0 today. We can discuss whether this change can be justified as a patch.

Ok, I'll follow that PR to keep in touch with this!

That is a fair point that this layer does not properly address right now. I think a dedicated MVTTileLayer might be the best solution. What are you using for decoding?

I'm using @mapbox/vector-tile to read the protobuf and get a GeoJSON for every feature within the layer. Still executing in main thread, but soon to be in a WebWorker.

For the merging approach, I had a features' store and then I was merging polygon vertices grouped by a unique property (defined by the user/developer) shared among features. That was the first approach but we are doing performance reviews and we might change it for a better approach.

@jesusbotella
Copy link
Copy Markdown
Contributor Author

Going to close this PR because perfomance improvements have been merged into master. We'll open up a new PR once we have our geometry merge changes ready to be shown and discussed.

Thank you very much!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants