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

Upload one raster tile texture per render frame #7592

Closed
wants to merge 7 commits into from

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Nov 14, 2018

Launch Checklist

This closes #7451

It also completes some of the work in #7405

  • briefly describe the changes in this PR
    • introduces a queue to limit raster tile texture upload to the GPU to one texture per render frame
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page

screen shot 2018-11-14 at 3 42 03 pm

Benchmarks don't show any major performance improvement or degradation

screen shot 2018-11-20 at 4 31 39 pm

screen shot 2018-11-20 at 4 31 49 pm

screen shot 2018-11-20 at 4 32 19 pm

screen shot 2018-11-20 at 4 32 27 pm


if (context.extTextureFilterAnisotropic) {
gl.texParameterf(gl.TEXTURE_2D, context.extTextureFilterAnisotropic.TEXTURE_MAX_ANISOTROPY_EXT, context.extTextureFilterAnisotropicMax);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're now deferring the action of uploading the tile, abortTile no longer cancels everything: There could be a situation where the request responds with a tile, we store in the queue, then the tile gets aborted/deleted. We now still hold on to the tile and upload it to the GPU, but it'll never be used and gets GCed once the callback/tile object is removed from the textureQueue. We should add a way to either remove requests from the textureQueue that reference this tile, or use another mechanism to prevent tiles from remaining alive longer than they need to.

Copy link
Contributor Author

@ryanhamley ryanhamley Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to check that the tile pulled from textureQueue doesn't have the aborted property before uploading to the GPU? Or would that be unreliable and not cover all potential cases? It seems to work but I'm not 100% sure how to ensure that the aborted tiles aren't being uploaded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works; another option would be to remove the object from the queue when the tile is aborted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/source/source.js Outdated Show resolved Hide resolved
@ryanhamley ryanhamley changed the title [WIP] Upload one raster tile texture per render frame Upload one raster tile texture per render frame Nov 21, 2018
@ryanhamley
Copy link
Contributor Author

ryanhamley commented Nov 28, 2018

My manual testing seems to indicate that 2 is an optimum number of textures to upload at once. 1 and 2 textures both show a roughly 12% dropped frame rate while at 4 textures per frame, the dropped frame rate increases to roughly 20%. 1 texture per frame is too slow, especially on pitched maps while 2 textures per frame looks much snappier and uses half as many frames.

I've also implemented the queue on a per-source basis rather than per-style because having a single queue per style leads to the possibility of tiles for multiple raster(-dem) layers being jumbled together in the queue which can result in not being able to render all necessary tiles. It leads to some awkward type checking to make Flow happy but it's the simplest implementation I could think of.

@andycalder
Copy link
Contributor

andycalder commented Feb 1, 2019

This looks like a great performance improvement to me! I tested this on a satellite map with 60 degree pitch and the frame rate difference is very noticeable on initial map load when the tiles have been cached by the browser. I'd love to see this PR move forward. Is it a work in progress?

Some things I noticed:

  • This breaks the tile fade-in transition because the _tileLoaded() callback sets tile.timeAdded immediately, and this starts the opacity interpolation before the tile is even rendered. I quick-fixed this by moving the callback to textureCallback(). Visual comparison with master is more favourable when the fade works.
  • For the raster-dem case, it looks like you're queueing the DEM decode operation, which happens in a web worker, instead of the actual texture upload. Texture upload happens in draw_hillshade.js.

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Feb 1, 2019

Thanks for the feedback @andycalder! It's a work in progress, yes. We've been investigating a few different ways of making performance better with raster image loading. #7405 is the master ticket for this work. A related improvement I've been working on is using the Image Bitmap API in browsers that support it which you can see in https://github.com/mapbox/mapbox-gl-js/tree/image-bitmap-prototype (very rough prototype).

I actually hadn't seen a huge improvement with this PR and in fact, when combined with #7497 the performance seemed worse since raster requests were essentially being rate limited twice. It definitely requires more investigation. Are you able to share a minimal example in which you saw a performance improvement?

@ryanhamley
Copy link
Contributor Author

Also, I don't think the version of this branch on Github had been updated with #7497 previously so I'd recommend trying it now that I've rebased against master @andycalder

@andycalder
Copy link
Contributor

@ryanhamley #7497 is trying to solve the same problem, right? So implementing this PR would mean reverting #7497 and you wouldn't be throttling rasters twice.

If I understand correctly, the underlying problem is texture uploads to the GPU (gl.texImage2D()) taking too long. I don't think #7497 solves this problem because it limits parallel image requests to 16 but the bottleneck for texture upload is far less than 16 per frame. The master ticket #7405 notes that a single texture upload takes ~7.5ms and you noted above that 4 textures per frame degrades the frame rate. Wouldn't it be better to leave the requests alone and limit the offending texture upload.

On my machine, when I refresh https://jsbin.com/xubonu (v0.52.0) the frame rate is terrible. Using my stagger-texture-uploads build, the frame rate is much better (I'm not sure how to host a repro of this). The tradeoff is that it takes slightly longer to populate the map with tiles.

The Image Bitmap API is interesting and it looks like it's probably the ideal solution but browser support is really unfortunate.

@vakila
Copy link

vakila commented Nov 5, 2019

Closing this as stale/no longer WIP per chat with @ryanhamley

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.

Stagger texture uploads across multiple consecutive frames
5 participants