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

Evaluate query results #9282

Closed
wants to merge 1 commit into from
Closed

Evaluate query results #9282

wants to merge 1 commit into from

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Feb 10, 2020

Launch Checklist

This is a potential fix for #8373 It's an alternate approach to #9198 but its performance seems to be about the same

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • 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/map-design-team if this PR includes style spec or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

@ryanhamley
Copy link
Contributor Author

This shows a sample flame chart using the query_features.html debug page without a geometry passed to queryRenderedFeatures (e.g. queryRenderedFeatures(null, options))
Screen Shot 2020-02-10 at 1 36 54 PM

This is for a random query with a geometry (queryRenderedFeatures(box, options)). In isolation, evaluateQueryResults seems to almost always be fast. This is confusing me since the QueryPoint benchmark shows a major performance regression with both approaches I've tried.
Screen Shot 2020-02-10 at 3 30 27 PM

The benchmark results seem broadly similar to #9198
Screen Shot 2020-02-10 at 3 03 10 PM

Screen Shot 2020-02-10 at 3 15 23 PM

cc @mourner

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.

This seems to be heading in the right direction — we see a major improvement in the QueryBox bench (the many-features use case which the original issue was about), although this stems from the serializedLayers caching which we could cut out and merge in a separate PR.

For the queryPoint bench, you could run the Chrome Profiler while the benchmark is running to see what's the major offender. My guess is listImages which could be a major hit considering previous issues like #9261.

@ryanhamley
Copy link
Contributor Author

closing this in favor of #9198

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.

2 participants