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

Improve cancellation of tile loads #8633

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Improve cancellation of tile loads #8633

merged 3 commits into from
Aug 21, 2019

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Aug 14, 2019

Our Actor object is responsible for communicating with other threads, either by sending messages from the main thread to web workers, or vice versa. When calling Actor#send(), calls that provided a callback returned a Cancelable object that supported calling .cancel() for aborting the request. This only worked in a very small number of instances: When the action that was triggered in the corresponding thread was asynchronous, and supported cancellation itself.

This PR changes this to allow cancellation of a much wider array of requests. During high pressure workload phases, e.g. while zooming rapidly, tasks could pile up in the worker: in particular loadTile messages, that were followed by abortTile messages for the same tile uid. This PR now allows canceling messages while the wait in the worker's queue to be processed. This keeps the task from ever starting in the worker thread. When zooming quickly while loading tiles, this means that we now issue fewer HTTP requests that get cancelled immediately. Similarly, canceling HTTP requests on the worker were delayed by computing tasks on the worker.

Before this PR, the Actor processed messages sent from the counterpart via postMessage synchronously. This means that starting a task, followed immediately by a cancellation never actually canceled the task, because the worker first processed the task, then the cancelation. In this PR, I decoupled the message event from processing by putting tasks in a queue, then triggering processing with setTimeout(..., 0). This means that at most 1 task is processed per event loop iteration. This allows message processing run way more frequently, and consequently allows us to process cancel messages more quickly.

I opted for this model instead of a more generic priority queue because we don't have any use cases for a priority queue other than canceling tasks.

This PR also contains the addition of a loaded method to Source objects. This is necessary to correctly classify the loaded state of a source because we can now no longer rely on all tasks sent to a worker being processed

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.

Read through the changes and didn't find any obvious things to fix, but there's a ton of logic so very easy to miss something. But hopefully we'd catch any serious issues during manual and automated testing, so in addition to my trust in @kkaefer's judgement, I'm 👍 on merging this in time for this week's beta.

@ansis
Copy link
Contributor

ansis commented Aug 20, 2019

This is really great!

Read through the changes and didn't find any obvious things to fix, but there's a ton of logic so very easy to miss something. But hopefully we'd catch any serious issues during manual and automated testing

Yep, I agree. I did some manual testing on a bunch of the debug pages and didn't notice anything.

@mannnick24
Copy link
Contributor

mannnick24 commented Aug 28, 2019

@mourner and @kkaefer I believe there is now a race condition related to triggerRepaint in the map.render

in map.js is this
if (this._sourcesDirty || this._repaint || this._styleDirty || this._placementDirty) {
this.triggerRepaint();
} else if (!this.isMoving() && this.loaded()) {
this.fire(new Event('idle'));
}
and I think the tiles may not have loaded, but sourcesDirty could be false,

anyhoo, on IE we may or may not get a render after the map reports loaded() our product always waits for a a render with the map loaded (if dataloading) so our viz hangs in IE

Here is a fiddle, that may or may not trigger the fitbounds in IE - chrome seems to always pass
loadedrender.zip

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.

6 participants