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

Pre-filter sources for queryRenderFeatures #2877

Merged
merged 4 commits into from
Jul 18, 2016
Merged

Conversation

anandthakker
Copy link
Contributor

@anandthakker anandthakker commented Jul 15, 2016

This change aims to improve queryRenderedFeatures performance in cases where a layers parameter is present by only querying the sources referenced by the requested layers.

This can lead to significant savings in some cases because (a) the first time a newly-loaded source tile is queried, it must parse the pbf data into a VectorTile, and (b) tiles for some sources may be reloaded quite often if, for instance, setFilter or setData are being used for "hover"-like interactivity. (See also #2874)

@mourner
Copy link
Member

mourner commented Jul 15, 2016

Nice! Let's fix the build.

@anandthakker
Copy link
Contributor Author

^ last commit should fix the build and adds a test of the new behavior

@anandthakker
Copy link
Contributor Author

@mourner @lucaswoj this is ready for 👀 now.

(Also, I apologize for creating extra noise over in the feature-id-shader ticket)

} else {
sourcesToQuery = this.sources;
}
for (var id in sourcesToQuery) {
Copy link
Contributor

@lucaswoj lucaswoj Jul 18, 2016

Choose a reason for hiding this comment

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

Do you think we could get by with a simpler implementation like this 👇 ?

for (var id in this.sources) {
    if (!params.layers) continue;
    if (params.layers.some(function(layerId) { return this._layers[layerId].source === id; }) continue;
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, 👍 for filtering the sources within the loop like you propose.

I'm a little bit wary of using array.some() here rather than a plain for loop over params.layers, only because this method could get called with a pretty high frequency, e.g., in a mousemove. handler, where this might make a difference. (But I admit this is based on intuition, not quantitative evidence)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the impact of Array.some or equivalent logic will ever be a bottleneck next to Source#queryRenderedFeatures. If it becomes one, we will identify it in the profiler and look for optimizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, fair enough 😄

@anandthakker
Copy link
Contributor Author

@lucaswoj ^ simplified the logic -- I did it this way because the "no-loop-func" linter rule prohibited the particular form you suggested

@mourner
Copy link
Member

mourner commented Jul 18, 2016

Given that we're dealing with sources, the difference between discussed forms will be negligible because there are typically only a very few sources on a particular map. However, in other situations like this where there are more objects to deal with (like layers), the original form with a simple for loop and a hash is highly preferable because it's O(n) rather than O(n^2).

@lucaswoj
Copy link
Contributor

If we can't do the O(n^2) logic as a one-liner, we should revert to the O(n) logic. I pushed a commit that implements this. We can merge on 🍏

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