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

Add budgets to Actor queue flushing #9022

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions debug/tile-cancellation.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<!DOCTYPE html>
<html>
<head>
<title>Mapbox GL JS debug page</title>
<meta charset='utf-8'>
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
<link rel='stylesheet' href='../dist/mapbox-gl.css' />
<style>
body { margin: 0; padding: 0; }
html, body, #map { height: 100%; }
</style>
</head>

<body>
<div id='map'></div>

<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>

var map = window.map = new mapboxgl.Map({
container: 'map',
zoom: 12.5,
center: [-77.01866, 38.888],
style: 'mapbox://styles/mapbox/streets-v10',
hash: true
});

map.once('idle', function() {
map.easeTo({center: [-77.01866, 38.888], zoom: 1, duration: 1000});

setTimeout(function() {
map.easeTo({center: [-77.01866, 38.888], zoom: 12.5, duration: 1000});
}, 1200);
});

</script>
</body>
</html>
54 changes: 38 additions & 16 deletions src/util/actor.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@

import {bindAll, isWorker, isSafari} from './util';
import window from './window';
import browser from './browser';
import {serialize, deserialize} from './web_worker_transfer';
import ThrottledInvoker from './throttled_invoker';

import type {Transferable} from '../types/transferable';
import type {Cancelable} from '../types/cancelable';

// Upper limit on time in ms, the actor allocates towards flushing the task queue per frame
const MAIN_THREAD_TIME_BUDGET = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We're measuring time in integer milliseconds, so a limit of 1ms very coarse.

// Upper limit of number of tasks executed in the a frame in the main thread
const MAIN_THREAD_TASK_BUDGET = 8;

/**
* An implementation of the [Actor design pattern](http://en.wikipedia.org/wiki/Actor_model)
* that maintains the relationship between asynchronous tasks and the objects
Expand All @@ -30,19 +36,21 @@ class Actor {
cancelCallbacks: { number: Cancelable };
invoker: ThrottledInvoker;
globalScope: any;
isWorker: boolean;

constructor(target: any, parent: any, mapId: ?number) {
this.target = target;
this.parent = parent;
this.mapId = mapId;
this.isWorker = isWorker();
this.callbacks = {};
this.tasks = {};
this.taskQueue = [];
this.cancelCallbacks = {};
bindAll(['receive', 'process'], this);
this.invoker = new ThrottledInvoker(this.process);
this.target.addEventListener('message', this.receive, false);
this.globalScope = isWorker() ? target : window;
this.globalScope = this.isWorker ? target : window;
}

/**
Expand Down Expand Up @@ -110,20 +118,15 @@ class Actor {
cancel();
}
} else {
// In workers, store the tasks that we need to process before actually processing them. This
// is necessary because we want to keep receiving messages, and in particular,
// <cancel> messages. Some tasks may take a while in the worker thread, so before
// executing the next task in our queue, postMessage preempts this and <cancel>
// messages can be processed. We're using a MessageChannel object to get throttle the
// process() flow to one at a time.
// On the main thread, store the tasks that we need to process before actually processing them. This
// is necessary because we want to keep receiving messages, and in particular, <cancel> messages.
// By batching and deferring their execution on the main thread, we can preempt them from being sent to the worker at all.
this.tasks[id] = data;
this.taskQueue.push(id);
if (isWorker()) {
this.invoker.trigger();
} else {
// In the main thread, process messages immediately so that other work does not slip in
// between getting partial data back from workers.
if (this.isWorker) {
this.process();
} else {
this.invoker.trigger();
}
}
}
Expand All @@ -132,20 +135,39 @@ class Actor {
if (!this.taskQueue.length) {
return;
}

//If this is a worker actor, then flush the entire task queue since we don't need to wait for
//cancel messages on the worker side, only on the main thread side, so we have Infinite budget for processing messages.
const timeBudget = this.isWorker ? Number.MAX_VALUE : MAIN_THREAD_TIME_BUDGET;
const taskBudget = this.isWorker ? Number.MAX_VALUE : MAIN_THREAD_TASK_BUDGET;

const start = browser.now();
let taskCtr = 0;
while (browser.now() - start < timeBudget && taskCtr < taskBudget && this.taskQueue.length > 0) {
this._processQueueTop();
taskCtr++;
}
// We've reached our budget for this frame, defer processingo the rest for the netx frame, this lets
Copy link
Contributor

Choose a reason for hiding this comment

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

some typos here

// the deferred tasks bet preempted on slower browsers.
if (this.taskQueue.length) {
this.invoker.trigger();
}
}

_processQueueTop() {
const id = this.taskQueue.shift();
const task = this.tasks[id];
delete this.tasks[id];
// Schedule another process call if we know there's more to process _before_ invoking the
// current task. This is necessary so that processing continues even if the current task
// doesn't execute successfully.
if (this.taskQueue.length) {
this.invoker.trigger();
}
// if (this.taskQueue.length) {
// this.invoker.trigger();
// }
if (!task) {
// If the task ID doesn't have associated task data anymore, it was canceled.
return;
}

if (task.type === '<response>') {
// The done() function in the counterpart has been called, and we are now
// firing the callback in the originating actor, if there is one.
Expand Down
26 changes: 9 additions & 17 deletions src/util/throttled_invoker.js
Original file line number Diff line number Diff line change
@@ -1,44 +1,36 @@
// @flow
import window from './window';

const raf = window.requestAnimationFrame ||
window.mozRequestAnimationFrame ||
window.webkitRequestAnimationFrame ||
window.msRequestAnimationFrame;
/**
* Invokes the wrapped function in a non-blocking way when trigger() is called. Invocation requests
* are ignored until the function was actually invoked.
*
* @private
*/
class ThrottledInvoker {
_channel: MessageChannel;
_triggered: boolean;
_callback: Function

constructor(callback: Function) {
this._callback = callback;
this._triggered = false;
if (typeof MessageChannel !== 'undefined') {
this._channel = new MessageChannel();
this._channel.port2.onmessage = () => {
this._triggered = false;
this._callback();
};
}
}

trigger() {
if (!this._triggered) {
raf(() => {
this._triggered = false;
this._callback();
});
this._triggered = true;
if (this._channel) {
this._channel.port1.postMessage(true);
} else {
setTimeout(() => {
this._triggered = false;
this._callback();
}, 0);
}
}
}

remove() {
delete this._channel;
this._callback = () => {};
}
}
Expand Down