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

Map stutters when using feature state data-join technique #7133

Closed
mourner opened this issue Aug 15, 2018 · 4 comments
Closed

Map stutters when using feature state data-join technique #7133

mourner opened this issue Aug 15, 2018 · 4 comments
Assignees
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@mourner
Copy link
Member

mourner commented Aug 15, 2018

Since the old data-join technique is currently in a heavy performance regression (#6367), we now recommend a new technique based on setFeatureState. However, it's not an ideal solution too because it stutters pretty badly on tile loads.

Demo (courtesy of @ryanbaumann):
https://bl.ocks.org/ryanbaumann/733ba99c5ca1d9d15259081b395e4b00/414e974f854c20c263edddc6c9cc934d0956a9ac

Steps to trigger: Zoom back and forth.
Expected Behavior: Map zooms smoothly as if you never set any feature state.
Actual Behavior: Map stutters heavily.

Even if you remove setFeatureState on hover, the stutters are not better.

@asheemmamoowala we should investigate what exactly makes it stutter so badly and try to get it to a non-feature-state level of performance. Preliminarily I see that the stutters are spent on two things (both happen on tile.setFeatureState when a tile is added, cached or non cached):

  1. Reevaluating expressions for all features.
  2. Reuploading the paint buffers.
@mourner mourner added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Aug 15, 2018
@asheemmamoowala
Copy link
Contributor

One of the issues was rebuilding the expression because they are not fully deserialized: #6255. I've left a note inline as a reminder:

if ((binder: any).expression.isStateDependent === true) {
//AHM: Remove after https://github.com/mapbox/mapbox-gl-js/issues/6255
const value = layer.paint.get(property);

Reuploading the paint buffers.

I thought @ChrisLoer addressed this in v0.47 with #6853

@mourner
Copy link
Member Author

mourner commented Aug 29, 2018

@asheemmamoowala digging deeper into this today, I think the current implementation simply doesn't fit the use case of "set mostly static feature state for a hundred thousand features". To solve the stutter, the only approach I can think of is to broadcast all feature state changes to the workers, and then populate paint buffers of newly loaded tiles on the worker side using this feature state (instead of updating paint buffers for all features on every tile load). Does this sound feasible?

@mourner mourner self-assigned this Aug 29, 2018
@asheemmamoowala
Copy link
Contributor

That's what I was thinking of in opening #7122.
My thoughts so far on this are that it would be too expensive to pass the entire state to each worker for every tile? Especially because state may change between the first batch insert, and when the tile is actually being loaded.

One way around this would be to use a pull method for feature state, where SourceFeatureState can receive messages from a worker, and respond with previously loaded state for only the required feature ids. Similar to have image and glyph requests are made.

@mourner
Copy link
Member Author

mourner commented Aug 29, 2018

Oh, I didn't initially understand what #7122 was about — thanks for clearing this up! I think we can close this one as duplicate and continue discussion there.

@mourner mourner closed this as completed Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

2 participants