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

Optimize visible coordinates retrieval #6998

Merged
merged 2 commits into from Jul 23, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jul 20, 2018

Simplifies and significantly optimizes visible coordinates retrieval. A part of my performance push for fixing #6643. Contains two independent changes (please rebase instead of squashing):

  • The first commit simplifies and optimizes visible coordinates retrieval during the three painter rendering passes. Instead of getting the same coords multiple times, we retrieve them once for each source at the top and then use the cached objects.
  • The second commit fixes a performance regression introduced by sort renderable tiles by y and x to match -native #5601, which made the sorting logic for renderable ids more expensive. The commit makes it only use fancy sorting for symbol layers, and simple zoom sorting for everything else.

Combined, on a test case from #6643, the PR makes retrieving coordinates 3 times faster — on a single fly animation profile, down from 700ms (21.8% of _render) to 230ms (7.8%).

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality already covered
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

if (sourceCache) {
draw.debug(this, sourceCache, sourceCache.getVisibleCoordinates());
for (const id in sourceCaches) {
draw.debug(this, sourceCaches[id], coordsAscending[id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like any behavior changes here, but it got me thinking -- would it be better/more consistent if we drew debug tile boundaries as the union of tiles in all source caches?

Copy link
Member Author

Choose a reason for hiding this comment

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

We used to draw debug boundaries for all source caches, but I felt it looked very confusing with multiple sources. You can try it but I think it's more useful to have clear, legible boundaries for at least one source.

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

👍 These look like very sensible changes to me. I tried playing around with the code locally to look for situations where non-symbol layers might have become dependent on the symbol sorting but couldn't think of anything.

@mourner
Copy link
Member Author

mourner commented Jul 23, 2018

Pushed a fix that makes the tests pass without reverting them. Benchmarks look unaffected, because they don't exercise heavy zooming or small tiles.

image

@mourner mourner merged commit e9e08e2 into master Jul 23, 2018
@mourner mourner deleted the optimize-painter-coords branch July 23, 2018 07:43
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.

None yet

2 participants