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.

Sign up for GitHub

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 a minimal global worker pool #2952

Merged
merged 18 commits into from
Aug 23, 2016
Merged

Add a minimal global worker pool #2952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ffaf8e8
Enable Worker to do work for multiple map instances
Aug 6, 2016
9b887bf
Add minimal worker pool
Aug 6, 2016
a7d50e8
Include mapId in Actors' callback tracking
Aug 11, 2016
71c9f6d
Unit test Dispatcher's use of WorkerPool
Aug 11, 2016
63089fe
Fix benchmark
Aug 11, 2016
20bf840
Add WorkerPool unit tests
Aug 12, 2016
93ba7fd
Add a unit test for 2 Actors using the same Worker
Aug 12, 2016
1747dc6
Fix parameter in Worker 'redo placement' test
Aug 12, 2016
78540cf
Add map load benchmark
Aug 12, 2016
2e65562
Move the static worker pool instance to style.js
Aug 17, 2016
ba02e2a
Use a fixed worker pool size
Aug 19, 2016
7210aaf
Use empty style in load-multiple-maps benchmark
Aug 21, 2016
6ac077d
Drop shared_workers debug in favor of multiple.html
Aug 21, 2016
0c687bb
WorkerPool.WORKER_COUNT => mapboxgl.workerCount
Aug 21, 2016
ff7c9e2
Fix webpack build test
Aug 21, 2016
13b53ed
Use more descriptive parameter name, mapId
Aug 21, 2016
1efb12f
Rename layers => layerDefinitions, styleLayers => layers
Aug 21, 2016
ea12906
Remove mapboxgl.WorkerPool
Aug 21, 2016
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
8 changes: 4 additions & 4 deletions bench/benchmarks/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down Expand Up @@ -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;
}
46 changes: 46 additions & 0 deletions bench/benchmarks/map_load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'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({
style: {
version: 8,
sources: {},
layers: []
}
});
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;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

As written, this benchmark is highly dependent on the tester's network speed. Can you think of a way to remove that dependency? What are we testing in this benchmark that isn't captured by the buffer benchmark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. My intent in this benchmark was to measure the impact of sending all the worker code to each of the worker threads on the map load time. We can eliminate any outside network use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the benchmark to use an empty style, so it should now not depend at all on network speed

1 change: 1 addition & 0 deletions bench/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
54 changes: 30 additions & 24 deletions debug/multiple.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
</style>
</head>

<div class='map' id='map1'></div>
<div class='map' id='map2'></div>
<div class='map'></div>
<div class='map'></div>
<div class='map'></div>
<div class='map'></div>

<script src='/dist/mapbox-gl-dev.js'></script>
<script src='/debug/access-token-generated.js'></script>
Expand All @@ -27,6 +29,10 @@
"url": "mapbox://mapbox.satellite",
"tileSize": 256
},
"streets": {
"type": "vector",
"url": "mapbox://mapbox.mapbox-streets-v7"
}
},
"layers": [{
"id": "background",
Expand All @@ -38,28 +44,32 @@
"id": "satellite",
"type": "raster",
"source": "satellite"
}, {
"id": "road",
"type": "line",
"source": "streets",
"source-layer": "road",
"paint": { "line-color": "rgb(55,89,144)", "line-width": 4 }
}]
};

var map1 = new mapboxgl.Map({
container: 'map1',
minZoom: 14,
zoom: 17,
center: [-122.514426, 37.562984],
bearing: -96,
style: style,
hash: false
});
function addMap (container) {
var map = new mapboxgl.Map({
container: container,
minZoom: 14,
zoom: 15.5,
center: [-122.514426, 37.562984],
bearing: -96,
style: style,
hash: false
});
map.on('click', popup.bind(map))
}

var map2 = new mapboxgl.Map({
container: 'map2',
minZoom: 14,
zoom: 17,
center: [-122.514426, 37.562984],
bearing: -96,
style: style,
hash: false
});
var containers = document.querySelectorAll('.map')
for (var i = 0; i < containers.length; i++) {
addMap(containers[i]);
}

function popup(e) {
if (e.originalEvent.shiftKey) return;
Expand All @@ -68,10 +78,6 @@
.setHTML('<h3>' + e.point.x + ', ' + e.point.y + '</h3>')
.addTo(this);
}

map1.on('click', popup.bind(map1));
map2.on('click', popup.bind(map2));

</script>

</body>
Expand Down
16 changes: 16 additions & 0 deletions js/global_worker_pool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
};
4 changes: 4 additions & 0 deletions js/mapbox-gl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
'use strict';

var browser = require('./util/browser');

// jshint -W079
var mapboxgl = module.exports = {};

mapboxgl.version = require('../package.json').version;
mapboxgl.workerCount = Math.max(browser.hardwareConcurrency - 1, 1);


mapboxgl.Map = require('./ui/map');
mapboxgl.Control = require('./ui/control/control');
Expand Down
99 changes: 57 additions & 42 deletions js/source/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,33 @@ 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, layerDefinitions) {
var layers = this.layers[mapId] = {};

// Filter layers and create an id -> layer map
var childLayerIndicies = [];
for (var i = 0; i < layers.length; i++) {
var layer = layers[i];
for (var i = 0; i < layerDefinitions.length; i++) {
var layer = layerDefinitions[i];
if (layer.type === 'fill' || layer.type === 'line' || layer.type === 'circle' || layer.type === 'symbol') {
if (layer.ref) {
childLayerIndicies.push(i);
Expand All @@ -54,74 +53,75 @@ util.extend(Worker.prototype, {

// Create an instance of StyleLayer per layer
for (var j = 0; j < childLayerIndicies.length; j++) {
setLayer(layers[childLayerIndicies[j]]);
setLayer(layerDefinitions[childLayerIndicies[j]]);
}

function setLayer(serializedLayer) {
var styleLayer = StyleLayer.create(
serializedLayer,
serializedLayer.ref && that.layers[serializedLayer.ref]
serializedLayer.ref && layers[serializedLayer.ref]
);
styleLayer.updatePaintTransitions({}, {transition: false});
that.layers[styleLayer.id] = styleLayer;
layers[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, layerDefinitions) {
var id;
var layer;

var layers = this.layers[mapId];

// Update ref parents
for (id in layers) {
layer = layers[id];
for (id in layerDefinitions) {
layer = layerDefinitions[id];
if (layer.ref) updateLayer(layer);
}

// Update ref children
for (id in layers) {
layer = layers[id];
for (id in layerDefinitions) {
layer = layerDefinitions[id];
if (!layer.ref) updateLayer(layer);
}

function updateLayer(layer) {
var refLayer = that.layers[layer.ref];
if (that.layers[layer.id]) {
that.layers[layer.id].set(layer, refLayer);
var refLayer = layers[layer.ref];
if (layers[layer.id]) {
layers[layer.id].set(layer, refLayer);
} else {
that.layers[layer.id] = StyleLayer.create(layer, refLayer);
layers[layer.id] = StyleLayer.create(layer, refLayer);
}
that.layers[layer.id].updatePaintTransitions({}, {transition: false});
layers[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);
},

/**
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are source keyed by type? What happens if there are two vector sources on the same map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are keyed by (mapId, type) pairs. They're keyed by mapId so that WorkerSource code wouldn't need to change (i.e., each WorkerSource instance is still tied to a specific map -- see bullet 3 in "Specific Notes"). They're keyed by type because each WorkerSource handles requests from multiple individual sources (note that this has been true since before the custom source refactor)

Copy link
Contributor

@lucaswoj lucaswoj Aug 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

They're keyed by type because each WorkerSource handles requests from multiple individual sources (note that this has been true since before the custom source refactor)

Dang. I didn't know that. This code is 👍 as written.

if (!this.workerSources[mapId])
this.workerSources[mapId] = {};
if (!this.workerSources[mapId][type]) {
// simple accessor object for passing to WorkerSources
var layers = {
getLayers: function () { return this.layers[mapId]; }.bind(this),
getLayerFamilies: function () { return this.layerFamilies[mapId]; }.bind(this)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use an accessor object instead of a simpler {layers: [], layerFamilies: []} object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - I think this is a holdover: before this change, this.layers and this.layerFamilies were not container objects but rather direct references to the layers and layerFamiliers, respectively, and so the accessor functions were necessary because the reference this.layers would change when 'update layers' was called. You're right: that's no longer needed.

this.workerSources[mapId][type] = new this.workerSourceTypes[type](this.actor, layers);
}

return this.workerSources[mapId][type];
}
});

Expand Down
Loading