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

[] add a cache to style-layer-index so we don't recalculate keys #8122

Merged
merged 3 commits into from
Jun 4, 2019

Conversation

vallendm
Copy link
Contributor

@vallendm vallendm commented Apr 4, 2019

Launch Checklist

Test case: 33000 post codes are displayed on a map in a single fill layer. The tileset that its based upon contains postcodes for other countries so to reduce memory usage - a filter is applied on the layer so that only the 33000 postcodes that we're interested in are displayed.
Next we add a second layer for region outlines. When we mouseover a region - we set this "highlight" layer to show the single item.
Issue is that the speed of updating the "highlight" layer is slow. Profiling has shown this is due to the filter applied on the original region layer. When the "highlight" layer is changed - a broadcast event is sent to each tileworker - with the updated style. The tileworker then updates its styles based on the broadcast - and calculates a key for each layer. The key includes the filter - so to make the key it requires iterating over the 33000 items.

However this layer isn't one which is changing. In this case it seem a waste to have to regenerate the key.So add a key cache to the style_layer_index. When the key is calculated - save it in the cache. For updated layers - remove the key from the cache so we get a new key generated. For layers which stay the same - just use the existing key from the cache.

With this change - the testcase average time to display a hightlight was 381.03ms - whereas without the change the average highlight time is 830.58

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Hey, this looks great — thanks for taking on this! Can we update the groupByLayout function to make cacheKeys optional (so that keys aren't cached if you don't provide it)? We expose it in a public API of the style-spec, so it would be nice to avoid a breaking change.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Would you mind fixing the lint errors now?

@vallendm
Copy link
Contributor Author

Sorry about that - I'm used to a different bracket/space pattern. Fixed now.

@mourner mourner merged commit 6f1525e into mapbox:master Jun 4, 2019
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.

3 participants