-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Coalesce high-frequency 'setData' calls #5902
Conversation
@ChrisLoer This is great! Although I don't notice much improvement in terms of the draggable symbol being 100% in sync/as fast as the cursor, I do notice it's no longer playing catchup with the cursor and instead closer to where it should be. |
Yup. I didn't know about #3692, but it sounds like I've implemented something similar to #3692 (comment). Maybe this is enough to solve all of that problem? |
Yeah, fundamentally using GeoJSON for this kind of animation can be problematic (re-uploading/re-parsing the entire GeoJSON data set for every motion). Longer term we're interested in adding some sort of interface that would be more efficient for this type of animation. |
Nice @ChrisLoer. Going to test this branch for common animation use cases cc @cmtoomey |
I added some comments and shortened the "coalesce" callback slightly by sending the "coalesce" message directly from the Playing around with different sizes of data set gave me an appreciation for why we might want to build in (separate?) time-based throttling. Although the current changes prevent the worker from building up an arbitrarily long queue, it's still easy to peg the map with enough requests to make everything feel pretty sluggish. It's easy to extend the coalescing behavior to include a maximum frequency (say, once every 100ms) that prevents pegging to 100% CPU usage for what should be relatively cheap. But it's a trade-off: some animations will look smoother running as fast as they can... |
This prevents high-frequency calls to setData from building up a backlog.
10c545c
to
e9e0f20
Compare
👏 |
@ChrisLoer and @anandthakker Would throttling of setdata calls be added to native as well? |
@mb12 I haven't looked at it closely, but I think the architecture on native is fairly different (I think the equivalent |
mapbox-gl-js handles it itself since mapbox/mapbox-gl-js#5902
mapbox-gl-js handles it itself since mapbox/mapbox-gl-js#5902
mapbox-gl-js handles it itself since mapbox/mapbox-gl-js#5902
mapbox-gl-js handles it itself since mapbox/mapbox-gl-js#5902
Re-implement PR mapbox#5902 including fix for issue mapbox#5970.
Re-implement PR mapbox#5902 including fix for issue mapbox#5970.
This is an exploratory PR motivated by various example animations provided in #5716. The common theme was using high-frequency
setData
calls to animate items in a GeoJSON source.When you send
setData
calls to the source faster than they can be processed, you end up with an unbounded backlog of requests on the GeoJSON worker, and every request gets processed in order (also severalreloadTile
requests get added to the backlog for everysetData
) no matter how far behind you are. Non-GeoJSON requests to the same worker may also starve.This PR introduces a coalescing mechanism on
setData
so that the worker will always finish anyloadData
call it starts, and it will always process the lastloadData
it receives, but it is allowed to discard calls that arrive while it is already in the middle of processing an earlier request.I couldn't figure out a good way to self-send a "coalesced" message so I ended up round-tripping a "coalesce" message back to the parent, which adds a little latency but I think should still be correct.
The
set_data.html
debug page contains a relatively slowsetData
animation that exercises this functionality, and there are currently log statements that record how many calls are getting coalesced.On my machine, this demo page works ok without the coalescing, and coalescing only seems to skip roughly half of the
setData
calls, so maybe this isn't that important an optimization. But just upping the size of the GeoJSON feature set is enough to make the animation start developing a large backlog if it doesn't have coalescing enabled./cc @ansis @jfirebaugh @andrewharvey