From ca7ecde2112c2f12200568b035ca42612f113c98 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Sat, 6 Aug 2016 00:41:27 -0400 Subject: [PATCH 01/11] Enable Worker to do work for multiple map instances --- js/source/worker.js | 85 +++++++++++++++++++++++++------------------ js/style/style.js | 6 +-- js/util/actor.js | 14 ++++--- js/util/dispatcher.js | 3 +- 4 files changed, 64 insertions(+), 44 deletions(-) diff --git a/js/source/worker.js b/js/source/worker.js index b79a30747a9..09e0afbea32 100644 --- a/js/source/worker.js +++ b/js/source/worker.js @@ -15,29 +15,28 @@ function Worker(self) { this.self = self; this.actor = new Actor(self, this); - // simple accessor object for passing to WorkerSources - var styleLayers = { - getLayers: function () { return this.layers; }.bind(this), - getLayerFamilies: function () { return this.layerFamilies; }.bind(this) - }; + this.layers = {}; + this.layerFamilies = {}; - this.workerSources = { - vector: new VectorTileWorkerSource(this.actor, styleLayers), - geojson: new GeoJSONWorkerSource(this.actor, styleLayers) + this.workerSourceTypes = { + vector: VectorTileWorkerSource, + geojson: GeoJSONWorkerSource }; + // [mapId][sourceType] => worker source instance + this.workerSources = {}; + this.self.registerWorkerSource = function (name, WorkerSource) { - if (this.workerSources[name]) { + if (this.workerSourceTypes[name]) { throw new Error('Worker source with name "' + name + '" already registered.'); } - this.workerSources[name] = new WorkerSource(this.actor, styleLayers); + this.workerSourceTypes[name] = WorkerSource; }.bind(this); } util.extend(Worker.prototype, { - 'set layers': function(layers) { - this.layers = {}; - var that = this; + 'set layers': function(mapId, layers) { + var styleLayers = this.layers[mapId] = {}; // Filter layers and create an id -> layer map var childLayerIndicies = []; @@ -60,20 +59,21 @@ util.extend(Worker.prototype, { function setLayer(serializedLayer) { var styleLayer = StyleLayer.create( serializedLayer, - serializedLayer.ref && that.layers[serializedLayer.ref] + serializedLayer.ref && styleLayers[serializedLayer.ref] ); styleLayer.updatePaintTransitions({}, {transition: false}); - that.layers[styleLayer.id] = styleLayer; + styleLayers[styleLayer.id] = styleLayer; } - this.layerFamilies = createLayerFamilies(this.layers); + this.layerFamilies[mapId] = createLayerFamilies(this.layers[mapId]); }, - 'update layers': function(layers) { - var that = this; + 'update layers': function(mapId, layers) { var id; var layer; + var styleLayers = this.layers[mapId]; + // Update ref parents for (id in layers) { layer = layers[id]; @@ -87,41 +87,41 @@ util.extend(Worker.prototype, { } function updateLayer(layer) { - var refLayer = that.layers[layer.ref]; - if (that.layers[layer.id]) { - that.layers[layer.id].set(layer, refLayer); + var refLayer = styleLayers[layer.ref]; + if (styleLayers[layer.id]) { + styleLayers[layer.id].set(layer, refLayer); } else { - that.layers[layer.id] = StyleLayer.create(layer, refLayer); + styleLayers[layer.id] = StyleLayer.create(layer, refLayer); } - that.layers[layer.id].updatePaintTransitions({}, {transition: false}); + styleLayers[layer.id].updatePaintTransitions({}, {transition: false}); } - this.layerFamilies = createLayerFamilies(this.layers); + this.layerFamilies[mapId] = createLayerFamilies(this.layers[mapId]); }, - 'load tile': function(params, callback) { + 'load tile': function(mapId, params, callback) { var type = params.type || 'vector'; - this.workerSources[type].loadTile(params, callback); + this.getWorkerSource(mapId, type).loadTile(params, callback); }, - 'reload tile': function(params, callback) { + 'reload tile': function(mapId, params, callback) { var type = params.type || 'vector'; - this.workerSources[type].reloadTile(params, callback); + this.getWorkerSource(mapId, type).reloadTile(params, callback); }, - 'abort tile': function(params) { + 'abort tile': function(mapId, params) { var type = params.type || 'vector'; - this.workerSources[type].abortTile(params); + this.getWorkerSource(mapId, type).abortTile(params); }, - 'remove tile': function(params) { + 'remove tile': function(mapId, params) { var type = params.type || 'vector'; - this.workerSources[type].removeTile(params); + this.getWorkerSource(mapId, type).removeTile(params); }, - 'redo placement': function(params, callback) { + 'redo placement': function(mapId, params, callback) { var type = params.type || 'vector'; - this.workerSources[type].redoPlacement(params, callback); + this.getWorkerSource(mapId, type).redoPlacement(params, callback); }, /** @@ -130,13 +130,28 @@ util.extend(Worker.prototype, { * function taking `(name, workerSourceObject)`. * @private */ - 'load worker source': function(params, callback) { + 'load worker source': function(map, params, callback) { try { this.self.importScripts(params.url); callback(); } catch (e) { callback(e); } + }, + + getWorkerSource: function(mapId, type) { + if (!this.workerSources[mapId]) + this.workerSources[mapId] = {}; + if (!this.workerSources[mapId][type]) { + // simple accessor object for passing to WorkerSources + var styleLayers = { + getLayers: function () { return this.layers[mapId]; }.bind(this), + getLayerFamilies: function () { return this.layerFamilies[mapId]; }.bind(this) + }; + this.workerSources[mapId][type] = new this.workerSourceTypes[type](this.actor, styleLayers); + } + + return this.workerSources[mapId][type]; } }); diff --git a/js/style/style.js b/js/style/style.js index 83a1ee638f0..85497c89a83 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -731,7 +731,7 @@ Style.prototype = util.inherit(Evented, { // Callbacks from web workers - 'get sprite json': function(params, callback) { + 'get sprite json': function(_, params, callback) { var sprite = this.sprite; if (sprite.loaded()) { callback(null, { sprite: sprite.data, retina: sprite.retina }); @@ -742,7 +742,7 @@ Style.prototype = util.inherit(Evented, { } }, - 'get icons': function(params, callback) { + 'get icons': function(_, params, callback) { var sprite = this.sprite; var spriteAtlas = this.spriteAtlas; if (sprite.loaded()) { @@ -756,7 +756,7 @@ Style.prototype = util.inherit(Evented, { } }, - 'get glyphs': function(params, callback) { + 'get glyphs': function(_, params, callback) { var stacks = params.stacks, remaining = Object.keys(stacks).length, allGlyphs = {}; diff --git a/js/util/actor.js b/js/util/actor.js index dc09086cfeb..0c0fd9da40c 100644 --- a/js/util/actor.js +++ b/js/util/actor.js @@ -10,11 +10,13 @@ module.exports = Actor; * * @param {WebWorker} target * @param {WebWorker} parent + * @param {string|number} mapId A unique identifier for the Map instance using this Actor. * @private */ -function Actor(target, parent) { +function Actor(target, parent, mapId) { this.target = target; this.parent = parent; + this.mapId = mapId; this.callbacks = {}; this.callbackID = 0; this.receive = this.receive.bind(this); @@ -32,17 +34,19 @@ Actor.prototype.receive = function(message) { if (callback) callback(data.error || null, data.data); } else if (typeof data.id !== 'undefined' && this.parent[data.type]) { // data.type == 'load tile', 'remove tile', etc. - this.parent[data.type](data.data, done.bind(this)); - } else if (typeof data.id !== 'undefined' && this.parent.workerSources) { + this.parent[data.type](data.mapId, data.data, done.bind(this)); + } else if (typeof data.id !== 'undefined' && this.parent.getWorkerSource) { // data.type == sourcetype.method var keys = data.type.split('.'); - this.parent.workerSources[keys[0]][keys[1]](data.data, done.bind(this)); + var workerSource = this.parent.getWorkerSource(data.mapId, keys[0]); + workerSource[keys[1]](data.data, done.bind(this)); } else { this.parent[data.type](data.data); } function done(err, data, buffers) { this.postMessage({ + mapId: this.mapId, type: '', id: String(id), error: err ? String(err) : null, @@ -54,7 +58,7 @@ Actor.prototype.receive = function(message) { Actor.prototype.send = function(type, data, callback, buffers) { var id = null; if (callback) this.callbacks[id = this.callbackID++] = callback; - this.postMessage({ type: type, id: String(id), data: data }, buffers); + this.postMessage({ mapId: this.mapId, type: type, id: String(id), data: data }, buffers); }; /** diff --git a/js/util/dispatcher.js b/js/util/dispatcher.js index 7ca9bd7c367..cc7a9206e9b 100644 --- a/js/util/dispatcher.js +++ b/js/util/dispatcher.js @@ -16,9 +16,10 @@ module.exports = Dispatcher; function Dispatcher(length, parent) { this.actors = []; this.currentActor = 0; + this.id = util.uniqueId(); for (var i = 0; i < length; i++) { var worker = new WebWorker(); - var actor = new Actor(worker, parent); + var actor = new Actor(worker, parent, this.id); actor.name = "Worker " + i; this.actors.push(actor); } From 927d29310b5230b49ed6a9c37ed0a94752911608 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Sat, 6 Aug 2016 01:20:08 -0400 Subject: [PATCH 02/11] Add minimal worker pool --- debug/shared_workers.html | 122 ++++++++++++++++++++++++++++++++++ js/util/dispatcher.js | 11 +-- js/util/worker_pool.js | 35 ++++++++++ test/js/source/worker.test.js | 75 ++++++++++++++++----- 4 files changed, 220 insertions(+), 23 deletions(-) create mode 100644 debug/shared_workers.html create mode 100644 js/util/worker_pool.js diff --git a/debug/shared_workers.html b/debug/shared_workers.html new file mode 100644 index 00000000000..e36330e7fa7 --- /dev/null +++ b/debug/shared_workers.html @@ -0,0 +1,122 @@ + + + + Mapbox GL JS debug page + + + + + + + + +
+
+
+
+ +
+
+
+
+ +
+ + + + + + + + + + diff --git a/js/util/dispatcher.js b/js/util/dispatcher.js index cc7a9206e9b..51e9fa3b41f 100644 --- a/js/util/dispatcher.js +++ b/js/util/dispatcher.js @@ -1,10 +1,11 @@ 'use strict'; +var WorkerPool = require('./worker_pool'); var util = require('./util'); var Actor = require('./actor'); -var WebWorker = require('./web_worker'); module.exports = Dispatcher; +Dispatcher.workerPool = new WorkerPool(); /** * Responsible for sending messages from a {@link Source} to an associated @@ -17,8 +18,9 @@ function Dispatcher(length, parent) { this.actors = []; this.currentActor = 0; this.id = util.uniqueId(); + var workers = Dispatcher.workerPool.acquire(this.id, length); for (var i = 0; i < length; i++) { - var worker = new WebWorker(); + var worker = workers[i]; var actor = new Actor(worker, parent, this.id); actor.name = "Worker " + i; this.actors.push(actor); @@ -66,9 +68,8 @@ Dispatcher.prototype = { }, remove: function() { - for (var i = 0; i < this.actors.length; i++) { - this.actors[i].target.terminate(); - } + Dispatcher.workerPool.release(this.id); this.actors = []; } }; + diff --git a/js/util/worker_pool.js b/js/util/worker_pool.js new file mode 100644 index 00000000000..246cf80b1b7 --- /dev/null +++ b/js/util/worker_pool.js @@ -0,0 +1,35 @@ +'use strict'; + +var assert = require('assert'); +var WebWorker = require('./web_worker'); + +module.exports = WorkerPool; + +function WorkerPool() { + this.workers = []; + this.active = {}; +} + +WorkerPool.prototype = { + acquire: function (mapId, workerCount) { + this._resize(workerCount); + this.active[mapId] = workerCount; + return this.workers.slice(0, workerCount); + }, + + release: function (mapId) { + delete this.active[mapId]; + if (Object.keys(this.active).length === 0) { + this.workers.forEach(function (w) { w.terminate(); }); + this.workers = []; + } + }, + + _resize: function (len) { + assert(typeof len === 'number'); + while (this.workers.length < len) { + this.workers.push(new WebWorker()); + } + } +}; + diff --git a/test/js/source/worker.test.js b/test/js/source/worker.test.js index 66d04dcc708..25846da8d25 100644 --- a/test/js/source/worker.test.js +++ b/test/js/source/worker.test.js @@ -26,7 +26,7 @@ test('before', function(t) { test('load tile', function(t) { t.test('calls callback on error', function(t) { var worker = new Worker(_self); - worker['load tile']({ + worker['load tile'](0, { source: 'source', uid: 0, url: 'http://localhost:2900/error' @@ -42,25 +42,25 @@ test('load tile', function(t) { test('set layers', function(t) { var worker = new Worker(_self); - worker['set layers']([ + worker['set layers'](0, [ { id: 'one', type: 'circle', paint: { 'circle-color': 'red' } }, { id: 'two', type: 'circle', paint: { 'circle-color': 'green' } }, { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'blue' } } ]); - t.equal(worker.layers.one.id, 'one'); - t.equal(worker.layers.two.id, 'two'); - t.equal(worker.layers.three.id, 'three'); + t.equal(worker.layers[0].one.id, 'one'); + t.equal(worker.layers[0].two.id, 'two'); + t.equal(worker.layers[0].three.id, 'three'); - t.equal(worker.layers.one.getPaintProperty('circle-color'), 'red'); - t.equal(worker.layers.two.getPaintProperty('circle-color'), 'green'); - t.equal(worker.layers.three.getPaintProperty('circle-color'), 'blue'); + t.equal(worker.layers[0].one.getPaintProperty('circle-color'), 'red'); + t.equal(worker.layers[0].two.getPaintProperty('circle-color'), 'green'); + t.equal(worker.layers[0].three.getPaintProperty('circle-color'), 'blue'); - t.equal(worker.layerFamilies.one.length, 1); - t.equal(worker.layerFamilies.one[0].id, 'one'); - t.equal(worker.layerFamilies.two.length, 2); - t.equal(worker.layerFamilies.two[0].id, 'two'); - t.equal(worker.layerFamilies.two[1].id, 'three'); + t.equal(worker.layerFamilies[0].one.length, 1); + t.equal(worker.layerFamilies[0].one[0].id, 'one'); + t.equal(worker.layerFamilies[0].two.length, 2); + t.equal(worker.layerFamilies[0].two[0].id, 'two'); + t.equal(worker.layerFamilies[0].two[1].id, 'three'); t.end(); }); @@ -68,21 +68,21 @@ test('set layers', function(t) { test('update layers', function(t) { var worker = new Worker(_self); - worker['set layers']([ + worker['set layers'](0, [ { id: 'one', type: 'circle', paint: { 'circle-color': 'red' } }, { id: 'two', type: 'circle', paint: { 'circle-color': 'green' } }, { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'blue' } } ]); - worker['update layers']({ + worker['update layers'](0, { one: { id: 'one', type: 'circle', paint: { 'circle-color': 'cyan' } }, two: { id: 'two', type: 'circle', paint: { 'circle-color': 'magenta' } }, three: { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'yellow' } } }); - t.equal(worker.layers.one.getPaintProperty('circle-color'), 'cyan'); - t.equal(worker.layers.two.getPaintProperty('circle-color'), 'magenta'); - t.equal(worker.layers.three.getPaintProperty('circle-color'), 'yellow'); + t.equal(worker.layers[0].one.getPaintProperty('circle-color'), 'cyan'); + t.equal(worker.layers[0].two.getPaintProperty('circle-color'), 'magenta'); + t.equal(worker.layers[0].three.getPaintProperty('circle-color'), 'yellow'); t.end(); }); @@ -99,6 +99,45 @@ test('redo placement', function(t) { worker['redo placement']({type: 'test', mapbox: true}); }); +test('update layers isolates different instances\' data', function(t) { + var worker = new Worker(_self); + + worker['set layers'](0, [ + { id: 'one', type: 'circle', paint: { 'circle-color': 'red' } }, + { id: 'two', type: 'circle', paint: { 'circle-color': 'green' } }, + { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'blue' } } + ]); + + worker['set layers'](1, [ + { id: 'one', type: 'circle', paint: { 'circle-color': 'red' } }, + { id: 'two', type: 'circle', paint: { 'circle-color': 'green' } }, + { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'blue' } } + ]); + + worker['update layers'](1, { + one: { id: 'one', type: 'circle', paint: { 'circle-color': 'cyan' } }, + two: { id: 'two', type: 'circle', paint: { 'circle-color': 'magenta' } }, + three: { id: 'three', ref: 'two', type: 'circle', paint: { 'circle-color': 'yellow' } } + }); + + t.equal(worker.layers[0].one.id, 'one'); + t.equal(worker.layers[0].two.id, 'two'); + t.equal(worker.layers[0].three.id, 'three'); + + t.equal(worker.layers[0].one.getPaintProperty('circle-color'), 'red'); + t.equal(worker.layers[0].two.getPaintProperty('circle-color'), 'green'); + t.equal(worker.layers[0].three.getPaintProperty('circle-color'), 'blue'); + + t.equal(worker.layerFamilies[0].one.length, 1); + t.equal(worker.layerFamilies[0].one[0].id, 'one'); + t.equal(worker.layerFamilies[0].two.length, 2); + t.equal(worker.layerFamilies[0].two[0].id, 'two'); + t.equal(worker.layerFamilies[0].two[1].id, 'three'); + + + t.end(); +}); + test('after', function(t) { server.close(t.end); }); From 14845459ca26a08a15d4756a5306998fbe2e29f9 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 13:01:56 -0400 Subject: [PATCH 03/11] Include mapId in Actors' callback tracking --- js/util/actor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/js/util/actor.js b/js/util/actor.js index 0c0fd9da40c..9aefb0298de 100644 --- a/js/util/actor.js +++ b/js/util/actor.js @@ -56,8 +56,8 @@ Actor.prototype.receive = function(message) { }; Actor.prototype.send = function(type, data, callback, buffers) { - var id = null; - if (callback) this.callbacks[id = this.callbackID++] = callback; + var id = callback ? this.mapId + ':' + this.callbackID++ : null; + if (callback) this.callbacks[id] = callback; this.postMessage({ mapId: this.mapId, type: type, id: String(id), data: data }, buffers); }; From 6b171e692b5d3383be829d1dcda53a241854b167 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 19:41:10 -0400 Subject: [PATCH 04/11] Unit test Dispatcher's use of WorkerPool --- js/util/dispatcher.js | 2 +- test/js/util/dispatcher.test.js | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 test/js/util/dispatcher.test.js diff --git a/js/util/dispatcher.js b/js/util/dispatcher.js index 51e9fa3b41f..1515734f26b 100644 --- a/js/util/dispatcher.js +++ b/js/util/dispatcher.js @@ -19,7 +19,7 @@ function Dispatcher(length, parent) { this.currentActor = 0; this.id = util.uniqueId(); var workers = Dispatcher.workerPool.acquire(this.id, length); - for (var i = 0; i < length; i++) { + for (var i = 0; i < workers.length; i++) { var worker = workers[i]; var actor = new Actor(worker, parent, this.id); actor.name = "Worker " + i; diff --git a/test/js/util/dispatcher.test.js b/test/js/util/dispatcher.test.js new file mode 100644 index 00000000000..5911061b1b9 --- /dev/null +++ b/test/js/util/dispatcher.test.js @@ -0,0 +1,39 @@ +'use strict'; + +var test = require('tap').test; +var Dispatcher = require('../../../js/util/dispatcher'); +var WebWorker = require('../../../js/util/web_worker'); +var originalWorkerPool = Dispatcher.workerPool; + +test('Dispatcher', function (t) { + t.test('requests and releases workers from pool', function (t) { + var dispatcher; + var workers = [new WebWorker(), new WebWorker()]; + + var releaseCalled = []; + Dispatcher.workerPool = { + acquire: function (id, count) { + t.equal(count, 2); + return workers; + }, + release: function (id) { + releaseCalled.push(id); + } + }; + + dispatcher = new Dispatcher(2, {}); + t.same(dispatcher.actors.map(function (actor) { return actor.target; }), workers); + dispatcher.remove(); + t.equal(dispatcher.actors.length, 0, 'actors discarded'); + t.same(releaseCalled, [dispatcher.id]); + t.end(); + }); + + t.end(); +}); + +test('after', function (t) { + Dispatcher.workerPool = originalWorkerPool; + t.end(); +}); + From 3cc30bc62182419be7155b65b9b8e9035b5ef4ba Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 19:53:58 -0400 Subject: [PATCH 05/11] Fix benchmark --- bench/benchmarks/buffer.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bench/benchmarks/buffer.js b/bench/benchmarks/buffer.js index 61d0b13ce9c..0b8f1a8fd61 100644 --- a/bench/benchmarks/buffer.js +++ b/bench/benchmarks/buffer.js @@ -91,14 +91,14 @@ function preloadAssets(stylesheet, callback) { style.on('load', function() { function getGlyphs(params, callback) { - style['get glyphs'](params, function(err, glyphs) { + style['get glyphs'](0, params, function(err, glyphs) { assets.glyphs[JSON.stringify(params)] = glyphs; callback(err, glyphs); }); } function getIcons(params, callback) { - style['get icons'](params, function(err, icons) { + style['get icons'](0, params, function(err, icons) { assets.icons[JSON.stringify(params)] = icons; callback(err, icons); }); @@ -185,10 +185,10 @@ var createLayerFamiliesCacheValue; function createLayerFamilies(layers) { if (layers !== createLayerFamiliesCacheKey) { var worker = new Worker({addEventListener: function() {} }); - worker['set layers'](layers); + worker['set layers'](0, layers); createLayerFamiliesCacheKey = layers; - createLayerFamiliesCacheValue = worker.layerFamilies; + createLayerFamiliesCacheValue = worker.layerFamilies[0]; } return createLayerFamiliesCacheValue; } From 55d2d855d49aaa80fed19d2ae4a513a94de9133c Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 23:14:22 -0400 Subject: [PATCH 06/11] Add WorkerPool unit tests --- test/js/util/worker_pool.test.js | 46 ++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 test/js/util/worker_pool.test.js diff --git a/test/js/util/worker_pool.test.js b/test/js/util/worker_pool.test.js new file mode 100644 index 00000000000..f1f085d677c --- /dev/null +++ b/test/js/util/worker_pool.test.js @@ -0,0 +1,46 @@ +'use strict'; + +var test = require('tap').test; +var WorkerPool = require('../../../js/util/worker_pool'); + +test('WorkerPool', function (t) { + t.test('#acquire', function (t) { + var pool = new WorkerPool(); + + t.equal(pool.workers.length, 0); + var workers1 = pool.acquire('map-1', 4); + t.equal(workers1.length, 4); + + var workers2 = pool.acquire('map-2', 8); + t.equal(workers2.length, 8); + t.equal(pool.workers.length, 8); + + // check that the two different dispatchers' workers arrays correspond + workers1.forEach(function (w, i) { t.equal(w, workers2[i]); }); + t.end(); + }); + + t.test('#release', function (t) { + var pool = new WorkerPool(); + pool.acquire('map-1', 1); + var workers = pool.acquire('map-2', 4); + var terminated = 0; + workers.forEach(function (w) { + w.terminate = function () { terminated += 1; }; + }); + + pool.release('map-2'); + t.comment('keeps workers if a dispatcher is still active'); + t.equal(terminated, 0); + t.equal(pool.workers.length, 4); + + t.comment('terminates workers if no dispatchers are active'); + pool.release('map-1'); + t.equal(terminated, 4); + t.equal(pool.workers.length, 0); + + t.end(); + }); + + t.end(); +}); From 93107411ebba881ed4c1a607cacc6477b35f3c0a Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 23:53:15 -0400 Subject: [PATCH 07/11] Add a unit test for 2 Actors using the same Worker --- test/js/util/actor.test.js | 36 +++++++++++++++++++++++++++++++++ test/js/util/dispatcher.test.js | 21 ++++++++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 test/js/util/actor.test.js diff --git a/test/js/util/actor.test.js b/test/js/util/actor.test.js new file mode 100644 index 00000000000..df02df69c24 --- /dev/null +++ b/test/js/util/actor.test.js @@ -0,0 +1,36 @@ +'use strict'; + +var test = require('tap').test; +var proxyquire = require('proxyquire'); +var Actor = require('../../../js/util/actor'); + +test('Actor', function (t) { + t.test('forwards resopnses to correct callback', function (t) { + var WebWorker = proxyquire('../../../js/util/web_worker', { + '../source/worker': function Worker(self) { + this.self = self; + this.actor = new Actor(self, this); + this.test = function (mapId, params, callback) { + setTimeout(callback, 0, null, params); + }; + } + }); + + var worker = new WebWorker(); + + var m1 = new Actor(worker, {}, 'map-1'); + var m2 = new Actor(worker, {}, 'map-2'); + + t.plan(4); + m1.send('test', { value: 1729 }, function (err, response) { + t.error(err); + t.same(response, { value: 1729 }); + }); + m2.send('test', { value: 4104 }, function (err, response) { + t.error(err); + t.same(response, { value: 4104 }); + }); + }); + + t.end(); +}); diff --git a/test/js/util/dispatcher.test.js b/test/js/util/dispatcher.test.js index 5911061b1b9..49b603ec759 100644 --- a/test/js/util/dispatcher.test.js +++ b/test/js/util/dispatcher.test.js @@ -1,6 +1,7 @@ 'use strict'; var test = require('tap').test; +var proxyquire = require('proxyquire'); var Dispatcher = require('../../../js/util/dispatcher'); var WebWorker = require('../../../js/util/web_worker'); var originalWorkerPool = Dispatcher.workerPool; @@ -26,6 +27,20 @@ test('Dispatcher', function (t) { dispatcher.remove(); t.equal(dispatcher.actors.length, 0, 'actors discarded'); t.same(releaseCalled, [dispatcher.id]); + + restoreWorkerPool(); + t.end(); + }); + + test('creates Actors with unique map id', function (t) { + var Dispatcher = proxyquire('../../../js/util/dispatcher', { './actor': Actor }); + + var ids = []; + function Actor (target, parent, mapId) { ids.push(mapId); } + + var dispatchers = [new Dispatcher(1, {}), new Dispatcher(1, {})]; + t.same(ids, dispatchers.map(function (d) { return d.id; })); + t.end(); }); @@ -33,7 +48,11 @@ test('Dispatcher', function (t) { }); test('after', function (t) { - Dispatcher.workerPool = originalWorkerPool; + restoreWorkerPool(); t.end(); }); +function restoreWorkerPool () { + Dispatcher.workerPool = originalWorkerPool; +} + From 5db49e553d5eff660c5db53e51c141c5daf3b174 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 11 Aug 2016 23:58:47 -0400 Subject: [PATCH 08/11] Fix parameter in Worker 'redo placement' test --- test/js/source/worker.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/source/worker.test.js b/test/js/source/worker.test.js index 25846da8d25..45c6c3946c9 100644 --- a/test/js/source/worker.test.js +++ b/test/js/source/worker.test.js @@ -96,7 +96,7 @@ test('redo placement', function(t) { }; }); - worker['redo placement']({type: 'test', mapbox: true}); + worker['redo placement'](0, {type: 'test', mapbox: true}); }); test('update layers isolates different instances\' data', function(t) { From 28977776e1e36f29fe1cf61e75f7e911c82a33e8 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Fri, 12 Aug 2016 00:34:40 -0400 Subject: [PATCH 09/11] Add map load benchmark --- bench/benchmarks/map_load.js | 40 ++++++++++++++++++++++++++++++++++++ bench/index.js | 1 + 2 files changed, 41 insertions(+) create mode 100644 bench/benchmarks/map_load.js diff --git a/bench/benchmarks/map_load.js b/bench/benchmarks/map_load.js new file mode 100644 index 00000000000..01de2faefe4 --- /dev/null +++ b/bench/benchmarks/map_load.js @@ -0,0 +1,40 @@ +'use strict'; + +var Evented = require('../../js/util/evented'); +var util = require('../../js/util/util'); +var formatNumber = require('../lib/format_number'); + +module.exports = function(options) { + var evented = util.extend({}, Evented); + + var mapsOnPage = 6; + + evented.fire('log', { message: 'Creating ' + mapsOnPage + ' maps' }); + + var loaded = 0; + var start = Date.now(); + for (var i = 0; i < mapsOnPage; i++) { + var map = options.createMap({}); + map.on('load', onload.bind(null, map)); + map.on('error', function (err) { + evented.fire('error', err); + }); + } + + function onload () { + if (++loaded >= mapsOnPage) { + var duration = Date.now() - start; + evented.fire('end', { + message: formatNumber(duration) + ' ms, loaded ' + mapsOnPage + ' maps.', + score: duration + }); + done(); + } + } + + function done () { + } + + return evented; +}; + diff --git a/bench/index.js b/bench/index.js index 85381648229..b1f34add9be 100644 --- a/bench/index.js +++ b/bench/index.js @@ -195,6 +195,7 @@ var BenchmarksView = React.createClass({ }); var benchmarks = { + 'load-multiple-maps': require('./benchmarks/map_load'), buffer: require('./benchmarks/buffer'), fps: require('./benchmarks/fps'), 'frame-duration': require('./benchmarks/frame_duration'), From bb84534924d962d4f3844ac81463c94cf292fda5 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Wed, 17 Aug 2016 18:08:57 -0400 Subject: [PATCH 10/11] Move the static worker pool instance to style.js This is still not great, but it's better than having it in Dispatcher. Ultimately the right place to put it is at the root level, i.e. mapbox-gl.js, and then pass it through, but that will be a noisy change due the number of 'new Map()' and 'new Style()' in the unit tests --- js/style/style.js | 5 ++++- js/util/dispatcher.js | 9 ++++----- test/js/util/dispatcher.test.js | 19 +++++-------------- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/js/style/style.js b/js/style/style.js index 85497c89a83..dc9f98f1adc 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -18,12 +18,15 @@ var QueryFeatures = require('../source/query_features'); var SourceCache = require('../source/source_cache'); var styleSpec = require('./style_spec'); var StyleFunction = require('./style_function'); +var WorkerPool = require('../util/worker_pool'); + +var workerPool = new WorkerPool(); module.exports = Style; function Style(stylesheet, animationLoop, workerCount) { this.animationLoop = animationLoop || new AnimationLoop(); - this.dispatcher = new Dispatcher(workerCount || 1, this); + this.dispatcher = new Dispatcher(workerPool, workerCount || 1, this); this.spriteAtlas = new SpriteAtlas(1024, 1024); this.lineAtlas = new LineAtlas(256, 512); diff --git a/js/util/dispatcher.js b/js/util/dispatcher.js index 1515734f26b..c12a11724ba 100644 --- a/js/util/dispatcher.js +++ b/js/util/dispatcher.js @@ -1,11 +1,9 @@ 'use strict'; -var WorkerPool = require('./worker_pool'); var util = require('./util'); var Actor = require('./actor'); module.exports = Dispatcher; -Dispatcher.workerPool = new WorkerPool(); /** * Responsible for sending messages from a {@link Source} to an associated @@ -14,11 +12,12 @@ Dispatcher.workerPool = new WorkerPool(); * @interface Dispatcher * @private */ -function Dispatcher(length, parent) { +function Dispatcher(workerPool, length, parent) { + this.workerPool = workerPool; this.actors = []; this.currentActor = 0; this.id = util.uniqueId(); - var workers = Dispatcher.workerPool.acquire(this.id, length); + var workers = this.workerPool.acquire(this.id, length); for (var i = 0; i < workers.length; i++) { var worker = workers[i]; var actor = new Actor(worker, parent, this.id); @@ -68,7 +67,7 @@ Dispatcher.prototype = { }, remove: function() { - Dispatcher.workerPool.release(this.id); + this.workerPool.release(this.id); this.actors = []; } }; diff --git a/test/js/util/dispatcher.test.js b/test/js/util/dispatcher.test.js index 49b603ec759..bb71257c661 100644 --- a/test/js/util/dispatcher.test.js +++ b/test/js/util/dispatcher.test.js @@ -4,7 +4,7 @@ var test = require('tap').test; var proxyquire = require('proxyquire'); var Dispatcher = require('../../../js/util/dispatcher'); var WebWorker = require('../../../js/util/web_worker'); -var originalWorkerPool = Dispatcher.workerPool; +var WorkerPool = require('../../../js/util/worker_pool'); test('Dispatcher', function (t) { t.test('requests and releases workers from pool', function (t) { @@ -12,7 +12,7 @@ test('Dispatcher', function (t) { var workers = [new WebWorker(), new WebWorker()]; var releaseCalled = []; - Dispatcher.workerPool = { + var workerPool = { acquire: function (id, count) { t.equal(count, 2); return workers; @@ -22,13 +22,12 @@ test('Dispatcher', function (t) { } }; - dispatcher = new Dispatcher(2, {}); + dispatcher = new Dispatcher(workerPool, 2, {}); t.same(dispatcher.actors.map(function (actor) { return actor.target; }), workers); dispatcher.remove(); t.equal(dispatcher.actors.length, 0, 'actors discarded'); t.same(releaseCalled, [dispatcher.id]); - restoreWorkerPool(); t.end(); }); @@ -38,7 +37,8 @@ test('Dispatcher', function (t) { var ids = []; function Actor (target, parent, mapId) { ids.push(mapId); } - var dispatchers = [new Dispatcher(1, {}), new Dispatcher(1, {})]; + var workerPool = new WorkerPool(); + var dispatchers = [new Dispatcher(workerPool, 1, {}), new Dispatcher(workerPool, 1, {})]; t.same(ids, dispatchers.map(function (d) { return d.id; })); t.end(); @@ -47,12 +47,3 @@ test('Dispatcher', function (t) { t.end(); }); -test('after', function (t) { - restoreWorkerPool(); - t.end(); -}); - -function restoreWorkerPool () { - Dispatcher.workerPool = originalWorkerPool; -} - From f21dcedf0f2b35e9b05d5e48880e0408c1697a63 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Fri, 19 Aug 2016 00:05:52 -0400 Subject: [PATCH 11/11] Use a fixed worker pool size --- js/global_worker_pool.js | 16 ++++++++++++++ js/mapbox-gl.js | 2 ++ js/style/style.js | 8 +++---- js/ui/map.js | 11 ++-------- js/util/dispatcher.js | 4 ++-- js/util/worker_pool.js | 29 +++++++++++++------------ test/js/style/style.test.js | 7 ------- test/js/ui/map.test.js | 12 ----------- test/js/util/dispatcher.test.js | 11 ++++++---- test/js/util/worker_pool.test.js | 36 +++++++++++++++++++++----------- 10 files changed, 72 insertions(+), 64 deletions(-) create mode 100644 js/global_worker_pool.js diff --git a/js/global_worker_pool.js b/js/global_worker_pool.js new file mode 100644 index 00000000000..3391b3e74ce --- /dev/null +++ b/js/global_worker_pool.js @@ -0,0 +1,16 @@ +'use strict'; +var WorkerPool = require('./util/worker_pool'); + +var globalWorkerPool; + +/** + * Creates (if necessary) and returns the single, global WorkerPool instance + * to be shared across each Map + * @private + */ +module.exports = function getGlobalWorkerPool () { + if (!globalWorkerPool) { + globalWorkerPool = new WorkerPool(); + } + return globalWorkerPool; +}; diff --git a/js/mapbox-gl.js b/js/mapbox-gl.js index d1533d84343..7618b57e3bd 100644 --- a/js/mapbox-gl.js +++ b/js/mapbox-gl.js @@ -13,6 +13,8 @@ mapboxgl.Attribution = require('./ui/control/attribution'); mapboxgl.Popup = require('./ui/popup'); mapboxgl.Marker = require('./ui/marker'); +mapboxgl.WorkerPool = require('./util/worker_pool'); + mapboxgl.Style = require('./style/style'); mapboxgl.LngLat = require('./geo/lng_lat'); diff --git a/js/style/style.js b/js/style/style.js index dc9f98f1adc..d641b21c5c7 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -18,15 +18,13 @@ var QueryFeatures = require('../source/query_features'); var SourceCache = require('../source/source_cache'); var styleSpec = require('./style_spec'); var StyleFunction = require('./style_function'); -var WorkerPool = require('../util/worker_pool'); - -var workerPool = new WorkerPool(); +var getWorkerPool = require('../global_worker_pool'); module.exports = Style; -function Style(stylesheet, animationLoop, workerCount) { +function Style(stylesheet, animationLoop) { this.animationLoop = animationLoop || new AnimationLoop(); - this.dispatcher = new Dispatcher(workerPool, workerCount || 1, this); + this.dispatcher = new Dispatcher(getWorkerPool(), this); this.spriteAtlas = new SpriteAtlas(1024, 1024); this.lineAtlas = new LineAtlas(256, 512); diff --git a/js/ui/map.js b/js/ui/map.js index 38802322917..1a52ae29e23 100755 --- a/js/ui/map.js +++ b/js/ui/map.js @@ -52,8 +52,7 @@ var defaultOptions = { failIfMajorPerformanceCaveat: false, preserveDrawingBuffer: false, - trackResize: true, - workerCount: Math.max(browser.hardwareConcurrency - 1, 1) + trackResize: true }; /** @@ -114,7 +113,6 @@ var defaultOptions = { * @param {number} [options.zoom=0] The initial zoom level of the map. If `zoom` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`. * @param {number} [options.bearing=0] The initial bearing (rotation) of the map, measured in degrees counter-clockwise from north. If `bearing` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`. * @param {number} [options.pitch=0] The initial pitch (tilt) of the map, measured in degrees away from the plane of the screen (0-60). If `pitch` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`. - * @param {number} [options.workerCount=navigator.hardwareConcurrency - 1] The number of WebWorkers that Mapbox GL JS should use to process vector tile data. * @example * var map = new mapboxgl.Map({ * container: 'map', @@ -128,15 +126,10 @@ var Map = module.exports = function(options) { options = util.extend({}, defaultOptions, options); - if (options.workerCount < 1) { - throw new Error('workerCount must an integer greater than or equal to 1.'); - } - this._interactive = options.interactive; this._failIfMajorPerformanceCaveat = options.failIfMajorPerformanceCaveat; this._preserveDrawingBuffer = options.preserveDrawingBuffer; this._trackResize = options.trackResize; - this._workerCount = options.workerCount; this._bearingSnap = options.bearingSnap; if (typeof options.container === 'string') { @@ -641,7 +634,7 @@ util.extend(Map.prototype, /** @lends Map.prototype */{ } else if (style instanceof Style) { this.style = style; } else { - this.style = new Style(style, this.animationLoop, this._workerCount); + this.style = new Style(style, this.animationLoop); } this.style diff --git a/js/util/dispatcher.js b/js/util/dispatcher.js index c12a11724ba..d45a562acc8 100644 --- a/js/util/dispatcher.js +++ b/js/util/dispatcher.js @@ -12,12 +12,12 @@ module.exports = Dispatcher; * @interface Dispatcher * @private */ -function Dispatcher(workerPool, length, parent) { +function Dispatcher(workerPool, parent) { this.workerPool = workerPool; this.actors = []; this.currentActor = 0; this.id = util.uniqueId(); - var workers = this.workerPool.acquire(this.id, length); + var workers = this.workerPool.acquire(this.id); for (var i = 0; i < workers.length; i++) { var worker = workers[i]; var actor = new Actor(worker, parent, this.id); diff --git a/js/util/worker_pool.js b/js/util/worker_pool.js index 246cf80b1b7..3ac9e1b0e0f 100644 --- a/js/util/worker_pool.js +++ b/js/util/worker_pool.js @@ -2,33 +2,36 @@ var assert = require('assert'); var WebWorker = require('./web_worker'); +var browser = require('./browser'); module.exports = WorkerPool; +WorkerPool.WORKER_COUNT = Math.max(browser.hardwareConcurrency - 1, 1); + function WorkerPool() { - this.workers = []; + this.workerCount = WorkerPool.WORKER_COUNT; this.active = {}; } WorkerPool.prototype = { - acquire: function (mapId, workerCount) { - this._resize(workerCount); - this.active[mapId] = workerCount; - return this.workers.slice(0, workerCount); + acquire: function (mapId) { + if (!this.workers) { + assert(typeof this.workerCount === 'number'); + this.workers = []; + while (this.workers.length < this.workerCount) { + this.workers.push(new WebWorker()); + } + } + + this.active[mapId] = true; + return this.workers.slice(); }, release: function (mapId) { delete this.active[mapId]; if (Object.keys(this.active).length === 0) { this.workers.forEach(function (w) { w.terminate(); }); - this.workers = []; - } - }, - - _resize: function (len) { - assert(typeof len === 'number'); - while (this.workers.length < len) { - this.workers.push(new WebWorker()); + this.workers = null; } } }; diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js index d36dfc47e20..cb44c3b1cb2 100644 --- a/test/js/style/style.test.js +++ b/test/js/style/style.test.js @@ -1232,10 +1232,3 @@ test('Style#addSourceType', function (t) { t.end(); }); - -test('Style creates correct number of workers', function(t) { - var style = new Style(createStyleJSON(), null, 3); - t.equal(style.dispatcher.actors.length, 3); - t.ok(style); - t.end(); -}); diff --git a/test/js/ui/map.test.js b/test/js/ui/map.test.js index abc2d49ee2c..407688f2621 100755 --- a/test/js/ui/map.test.js +++ b/test/js/ui/map.test.js @@ -6,7 +6,6 @@ var window = require('../../../js/util/browser').window; var Map = require('../../../js/ui/map'); var Style = require('../../../js/style/style'); var LngLat = require('../../../js/geo/lng_lat'); -var browser = require('../../../js/util/browser'); var sinon = require('sinon'); var fixed = require('../../testutil/fixed'); @@ -993,17 +992,6 @@ test('Map', function(t) { t.end(); }); - t.test('workerCount option', function(t) { - var map = createMap({ style: createStyle() }); - t.equal(map.style.dispatcher.actors.length, browser.hardwareConcurrency - 1, 'workerCount defaults to hardwareConcurrency - 1'); - map = createMap({ style: createStyle(), workerCount: 3 }); - t.equal(map.style.dispatcher.actors.length, 3, 'workerCount option is used'); - t.throws(function () { - createMap({ workerCount: 0 }); - }); - t.end(); - }); - test('render stabilizes', function (t) { var style = createStyle(); style.sources.mapbox = { diff --git a/test/js/util/dispatcher.test.js b/test/js/util/dispatcher.test.js index bb71257c661..328ce02a01a 100644 --- a/test/js/util/dispatcher.test.js +++ b/test/js/util/dispatcher.test.js @@ -13,8 +13,7 @@ test('Dispatcher', function (t) { var releaseCalled = []; var workerPool = { - acquire: function (id, count) { - t.equal(count, 2); + acquire: function () { return workers; }, release: function (id) { @@ -22,7 +21,7 @@ test('Dispatcher', function (t) { } }; - dispatcher = new Dispatcher(workerPool, 2, {}); + dispatcher = new Dispatcher(workerPool, {}); t.same(dispatcher.actors.map(function (actor) { return actor.target; }), workers); dispatcher.remove(); t.equal(dispatcher.actors.length, 0, 'actors discarded'); @@ -37,10 +36,14 @@ test('Dispatcher', function (t) { var ids = []; function Actor (target, parent, mapId) { ids.push(mapId); } + var previousWorkerCount = WorkerPool.WORKER_COUNT; + WorkerPool.WORKER_COUNT = 1; + var workerPool = new WorkerPool(); - var dispatchers = [new Dispatcher(workerPool, 1, {}), new Dispatcher(workerPool, 1, {})]; + var dispatchers = [new Dispatcher(workerPool, {}), new Dispatcher(workerPool, {})]; t.same(ids, dispatchers.map(function (d) { return d.id; })); + WorkerPool.WORKER_COUNT = previousWorkerCount; t.end(); }); diff --git a/test/js/util/worker_pool.test.js b/test/js/util/worker_pool.test.js index f1f085d677c..a1a0a9bb8ee 100644 --- a/test/js/util/worker_pool.test.js +++ b/test/js/util/worker_pool.test.js @@ -1,19 +1,31 @@ 'use strict'; var test = require('tap').test; +var proxyquire = require('proxyquire'); var WorkerPool = require('../../../js/util/worker_pool'); test('WorkerPool', function (t) { + t.test('.WORKER_COUNT', function (t) { + var WorkerPool = proxyquire('../../../js/util/worker_pool', { + './browser': { hardwareConcurrency: 15 } + }); + t.equal(WorkerPool.WORKER_COUNT, 14); + + WorkerPool.WORKER_COUNT = 4; + t.end(); + }); + t.test('#acquire', function (t) { - var pool = new WorkerPool(); + // make sure we're actually creating some workers + t.ok(WorkerPool.WORKER_COUNT > 0); - t.equal(pool.workers.length, 0); - var workers1 = pool.acquire('map-1', 4); - t.equal(workers1.length, 4); + var pool = new WorkerPool(); - var workers2 = pool.acquire('map-2', 8); - t.equal(workers2.length, 8); - t.equal(pool.workers.length, 8); + t.notOk(pool.workers); + var workers1 = pool.acquire('map-1'); + var workers2 = pool.acquire('map-2'); + t.equal(workers1.length, WorkerPool.WORKER_COUNT); + t.equal(workers2.length, WorkerPool.WORKER_COUNT); // check that the two different dispatchers' workers arrays correspond workers1.forEach(function (w, i) { t.equal(w, workers2[i]); }); @@ -22,8 +34,8 @@ test('WorkerPool', function (t) { t.test('#release', function (t) { var pool = new WorkerPool(); - pool.acquire('map-1', 1); - var workers = pool.acquire('map-2', 4); + pool.acquire('map-1'); + var workers = pool.acquire('map-2'); var terminated = 0; workers.forEach(function (w) { w.terminate = function () { terminated += 1; }; @@ -32,12 +44,12 @@ test('WorkerPool', function (t) { pool.release('map-2'); t.comment('keeps workers if a dispatcher is still active'); t.equal(terminated, 0); - t.equal(pool.workers.length, 4); + t.ok(pool.workers.length > 0); t.comment('terminates workers if no dispatchers are active'); pool.release('map-1'); - t.equal(terminated, 4); - t.equal(pool.workers.length, 0); + t.equal(terminated, WorkerPool.WORKER_COUNT); + t.notOk(pool.workers); t.end(); });