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

Improve performance by not reparsing line and fill buckets when overzooming #2999

Closed
mb12 opened this issue Aug 12, 2016 · 9 comments
Closed
Labels
needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@mb12
Copy link

mb12 commented Aug 12, 2016

mapbox-gl-js version:master

Steps to Trigger Behavior

  1. For a source with maxzoom=15, zoom to level 16, 17 and so on.
  2. Instead of re-using the bucket that was created at zoom 15, line and fill buckets are re-created. This is redundant computation (tesselating lines and fills) and can be avoided by scaling u_matrix appropriately.
    For symbol buckets, it is needed for correctness since symbols have to be re-placed. However this computation can be avoid for line and fill buckets.

Expected Behavior

Actual Behavior

@lucaswoj
Copy link
Contributor

Thanks @mb12! This is a good idea.

We currently make decisions about reparsing tiles based upon source type (see Source#reparseOverscaled). Because Tiles are the smallest unit of reparseable data and there is one tile per source, moving from a per-source to a per-layer decision would probably require refactoring per #2432.

My only question about this approach is on geometry precision. Do we encode higher precision geometries in reparsed overzoomed tiles? Does that higher precision matter for rendering?

@lucaswoj lucaswoj changed the title Line and Fill Buckets are being re-created when overzooming (overscaling > 1). Refactor to avoid reparsing line and fill buckets when overzooming Aug 15, 2016
@lucaswoj lucaswoj added refactoring 🏗️ performance ⚡ Speed, stability, CPU usage, memory usage, or power usage needs discussion 💬 and removed refactoring 🏗️ labels Aug 15, 2016
@lucaswoj lucaswoj changed the title Refactor to avoid reparsing line and fill buckets when overzooming Improve performance by not reparsing line and fill buckets when overzooming Aug 15, 2016
@mb12
Copy link
Author

mb12 commented Aug 17, 2016

@lucaswoj All geometries are normalized to 13 bits irrespective of the precision in the pbf file. This is because all vertices are shorts in vertex shader. Of the 16 bits, 13 bits are used to encode vertex coordinate. The remaining 3 bits are used for other purposes(normal etc).

@mourner
Copy link
Member

mourner commented Aug 17, 2016

Worth noting that it probably won't have a significant impact on performance because maxzoom tiles are typically very small compared to lower zoom tiles, and coupled with less loaded tiles when overscaling, processing them doesn't take a lot of time.

@lucaswoj
Copy link
Contributor

@mourner If you're reasonably confident this isn't worth pursuing from a perf standpoint, feel free to close this Issue.

@mb12
Copy link
Author

mb12 commented Aug 17, 2016

@lucaswoj and @mourner This would eliminate 2/3rds of the CPU time being used to re-create line and fill buckets. This will have a noticeable improvement in performance for tiles with tons of dense contours / finer hill shading.

@jfirebaugh
Copy link
Contributor

However this computation can be avoid for line and fill buckets.

This isn't strictly true. Line and fill properties can be zoom functions, which must be recalculated at z16, z17, etc., even if the source maximum zoom is z15.

@mb12
Copy link
Author

mb12 commented Aug 21, 2016

@jfirebaugh Changing paint property of a layer should not require re-creating the buckets again since they map to a uniform in the vertex/fragment shaders. The most common use of zoom functions is to change the extrude width for lines, opacity for fills, radius for circles etc (which are all paint properties).

From a cursory inspection of bright-v9.json, I do not see a single layout property that is a zoom function inf bright-v9.json. The common use case is just changing the line width, which is a paint property, that should not require re-computing buckets.

@mourner
Copy link
Member

mourner commented Aug 22, 2016

This would eliminate 2/3rds of the CPU time being used to re-create line and fill buckets. This will have a noticeable improvement in performance for tiles with tons of dense contours / finer hill shading.

I was referring to the fact that data density will typically peak around z11-12 and then decrease sharply on higher zooms, to the point where on z16+ the amount of data will be very low compared to lower zooms in most maps, unless you're overscaling from a relatively low zoom (like z14, rather than z16 Mapbox Streets uses). Additionally, less tiles will be loaded on higher zooms when overscaling tiles.

@jfirebaugh
Copy link
Contributor

Closing; the core team believes that this would not be a meaningful optimization. @mb12 if you disagree, feel free to submit a PR that implements this optimization and provides a benchmark for comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

4 participants