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

Jump to bottom

WIP - share workers across Map instance with a global worker pool #2917

Closed
wants to merge 9 commits into from
Closed

WIP - share workers across Map instance with a global worker pool #2917

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2cc58b4
Unify node/browser dispatcher code
Jul 27, 2016
02614e7
Add a mapInstanceId param to inter-thread methods
Jul 27, 2016
50b4c08
WIP - share workers across Map instances
Jul 28, 2016
fb0b68d
Add comment re: geojson worker key
Jul 28, 2016
48a173f
s/map/mapId/
Jul 28, 2016
3ce8ceb
Add comment re: tile uid change
Jul 28, 2016
01ca9f7
Add some documentation to worker pool
Jul 28, 2016
069871b
Add comments about worker "styles" interface
Jul 28, 2016
e07e77f
Fix old workerID reference
Aug 3, 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
58 changes: 33 additions & 25 deletions debug/multiple.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@
<link rel='stylesheet' href='../dist/mapbox-gl.css' />
<style>
body { margin: 0; padding: 0; }
.map { width: 300px; height: 300px; display: block; float: left; margin: 5px; }
.map { width: 150px; height: 150px; display: block; float: left; margin: 5px; }
</style>
</head>

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

<script src='mapbox-gl.js'></script>
<script src='access-token.js'></script>

Expand All @@ -27,6 +24,10 @@
"url": "mapbox://mapbox.satellite",
"tileSize": 256
},
"street": {
"type": "vector",
"url": "mapbox://mapbox.mapbox-streets-v7",
}
},
"layers": [{
"id": "background",
Expand All @@ -38,28 +39,38 @@
"id": "satellite",
"type": "raster",
"source": "satellite"
}, {
"id": "streets",
"type": "line",
"source": "street",
"source-layer": "road",
"paint": {
"line-color": "#aaff00",
"line-width": 5
}
}]
};

var map1 = new mapboxgl.Map({
container: 'map1',
minZoom: 14,
zoom: 17,
center: [-122.514426, 37.562984],
bearing: -96,
style: style,
hash: false
});
document.addEventListener('DOMContentLoaded', function () {
var count = window.location.hash.substring(1)
for (var i = 0; i < (isNaN(count) ? 2 : count); i++) {
var div = document.createElement('div')
var id = div.id = 'map' + i
div.className = 'map'
document.body.appendChild(div)
var map = new mapboxgl.Map({
container: id,
minZoom: 14,
zoom: 14,
center: [-122.514426, 37.562984],
bearing: -96,
style: style,
hash: false
})

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

function popup(e) {
if (e.originalEvent.shiftKey) return;
Expand All @@ -69,9 +80,6 @@
.addTo(this);
}

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

</script>

</body>
Expand Down
17 changes: 11 additions & 6 deletions js/source/geojson_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,19 @@ GeoJSONSource.prototype = util.inherit(Evented, /** @lends GeoJSONSource.prototy
options.data = JSON.stringify(data);
}

// We use a 'worker key' that's tied to the current geojson data for
// this source, so that when we ask the worker for tiles, the request
// goes to the same worker that parsed/prepared the geojson.
var workerKey = options.url || options.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we reference this workerKey against a map. Will this work of options.data is an object?

var object = {};

object[{foo: true}] = 'foo';
object[{bar: true}] = 'bar';

object[{foo: true}] // => 'bar';
object['[Object object]'] // => 'bar'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the line 138 above ensures that this won't happen -- but now that I'm looking at this again, I do think it's not especially clear; I'll make it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I forgot that we stringified it. LGTM 👍


// target {this.type}.loadData rather than literally geojson.loadData,
// so that other geojson-like source types can easily reuse this
// implementation
this.workerID = this.dispatcher.send(this.type + '.loadData', options, function(err) {
this.dispatcher.send(this.type + '.loadData', options, function(err) {
this._loaded = true;
this.workerKey = workerKey;
callback(err);

}.bind(this));
}.bind(this), workerKey);
},

loadTile: function (tile, callback) {
Expand All @@ -164,7 +169,7 @@ GeoJSONSource.prototype = util.inherit(Evented, /** @lends GeoJSONSource.prototy
showCollisionBoxes: this.map.showCollisionBoxes
};

tile.workerID = this.dispatcher.send('load tile', params, function(err, data) {
this.dispatcher.send('load tile', params, function(err, data) {

tile.unloadVectorData(this.map.painter);

Expand All @@ -184,7 +189,7 @@ GeoJSONSource.prototype = util.inherit(Evented, /** @lends GeoJSONSource.prototy

return callback(null);

}.bind(this), this.workerID);
}.bind(this), this.workerKey);
},

abortTile: function(tile) {
Expand All @@ -193,7 +198,7 @@ GeoJSONSource.prototype = util.inherit(Evented, /** @lends GeoJSONSource.prototy

unloadTile: function(tile) {
tile.unloadVectorData(this.map.painter);
this.dispatcher.send('remove tile', { uid: tile.uid, source: this.id }, function() {}, tile.workerID);
this.dispatcher.send('remove tile', { uid: tile.uid, source: this.id }, function() {}, this.workerKey);
},

serialize: function() {
Expand Down
3 changes: 2 additions & 1 deletion js/source/geojson_worker_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,12 @@ GeoJSONWorkerSource.prototype = util.inherit(VectorTileWorkerSource, /** @lends
* Defers to {@link GeoJSONWorkerSource#loadGeoJSON} for the fetching/parsing,
* expecting `callback(error, data)` to be called with either an error or a
* parsed GeoJSON object.
* @param {string} mapId A unique identifier for the map instance making this request
* @param {object} params
* @param {string} params.source The id of the source.
* @param {Function} callback
*/
loadData: function (params, callback) {
loadData: function (mapId, params, callback) {
var handleData = function(err, data) {
if (err) return callback(err);
if (typeof data != 'object') {
Expand Down
6 changes: 5 additions & 1 deletion js/source/source_cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,11 @@ SourceCache.prototype = util.inherit(Evented, {
if (!tile) {
var zoom = coord.z;
var overscaling = zoom > this.maxzoom ? Math.pow(2, zoom - this.maxzoom) : 1;
tile = new Tile(wrapped, this.tileSize * overscaling, this.maxzoom);
// Using [source id][coord id] for the tile's unique id allows it to
// be unique within this map instance while avoiding duplicated
// worker-side parsing across different map instances
var tileUID = this.id + coord.id;
tile = new Tile(tileUID, wrapped, this.tileSize * overscaling, this.maxzoom);
this.loadTile(tile, this._tileLoaded.bind(this, tile));
}

Expand Down
4 changes: 2 additions & 2 deletions js/source/tile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ module.exports = Tile;
* @param {number} size
* @private
*/
function Tile(coord, size, sourceMaxZoom) {
function Tile(id, coord, size, sourceMaxZoom) {
this.coord = coord;
this.uid = util.uniqueId();
this.uid = id;
this.uses = 0;
this.tileSize = size;
this.sourceMaxZoom = sourceMaxZoom;
Expand Down
10 changes: 5 additions & 5 deletions js/source/vector_tile_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,11 @@ VectorTileSource.prototype = util.inherit(Evented, {
showCollisionBoxes: this.map.showCollisionBoxes
};

if (tile.workerID) {
if (tile.state === 'loaded') {
params.rawTileData = tile.rawTileData;
this.dispatcher.send('reload tile', params, done.bind(this), tile.workerID);
this.dispatcher.send('reload tile', params, done.bind(this), tile.uid);
} else {
tile.workerID = this.dispatcher.send('load tile', params, done.bind(this));
this.dispatcher.send('load tile', params, done.bind(this), tile.uid);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the change to Dispatcher (using the new WorkerPool), wherein to send multiple requests to the same worker, dispatcher.send() accepts a 'key' (tile.uid here) and guarantees that two requests sent with the same key go to the same worker.

}

function done(err, data) {
Expand All @@ -85,11 +85,11 @@ VectorTileSource.prototype = util.inherit(Evented, {
},

abortTile: function(tile) {
this.dispatcher.send('abort tile', { uid: tile.uid, source: this.id }, null, tile.workerID);
this.dispatcher.send('abort tile', { uid: tile.uid, source: this.id }, null, tile.uid);
},

unloadTile: function(tile) {
tile.unloadVectorData(this.map.painter);
this.dispatcher.send('remove tile', { uid: tile.uid, source: this.id }, null, tile.workerID);
this.dispatcher.send('remove tile', { uid: tile.uid, source: this.id }, null, tile.uid);
}
});
55 changes: 36 additions & 19 deletions js/source/vector_tile_worker_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ module.exports = VectorTileWorkerSource;
* This class is designed to be easily reused to support custom source types
* for data formats that can be parsed/converted into an in-memory VectorTile
* representation. To do so, create it with
* `new VectorTileWorkerSource(actor, styleLayers, customLoadVectorDataFunction)`.
* `new VectorTileWorkerSource(actor, styles, customLoadVectorDataFunction)`.
*
* @class VectorTileWorkerSource
* @private
* @param {Function} [loadVectorData] Optional method for custom loading of a VectorTile object based on parameters passed from the main-thread Source. See {@link VectorTileWorkerSource#loadTile}. The default implementation simply loads the pbf at `params.url`.
*/
function VectorTileWorkerSource (actor, styleLayers, loadVectorData) {
function VectorTileWorkerSource (actor, styles, loadVectorData) {
this.actor = actor;
this.styleLayers = styleLayers;
this.styles = styles;

if (loadVectorData) { this.loadVectorData = loadVectorData; }

Expand All @@ -31,6 +31,7 @@ VectorTileWorkerSource.prototype = {
/**
* Implements {@link WorkerSource#loadTile}. Delegates to {@link VectorTileWorkerSource#loadVectorData} (which by default expects a `params.url` property) for fetching and producing a VectorTile object.
*
* @param {string} mapId A unique identifier for the map instance making this request
* @param {object} params
* @param {string} params.source The id of the source for which we're loading this tile.
* @param {string} params.uid The UID for this tile.
Expand All @@ -41,55 +42,70 @@ VectorTileWorkerSource.prototype = {
* @param {number} params.pitch
* @param {boolean} params.showCollisionBoxes
*/
loadTile: function(params, callback) {
var source = params.source,
loadTile: function(mapId, params, callback) {
var style = this.styles.getKey(mapId),
source = params.source,
uid = params.uid;

if (!this.loading[source])
this.loading[source] = {};
var loading = this.loading[style + source];

if (!loading)
loading = this.loading[style + source] = {};

if (loading[uid]) {
loading[uid].callbacks.push(callback);
return;
}

var tile = loading[uid] = new WorkerTile(params);
tile.callbacks = [callback];
callback = function (err, data) {
tile.callbacks.forEach(function(cb) { cb(err, data); });
};

var tile = this.loading[source][uid] = new WorkerTile(params);
tile.abort = this.loadVectorData(params, done.bind(this));

function done(err, data) {
delete this.loading[source][uid];
delete loading[uid];

if (err) return callback(err);
if (!data) return callback(null, null);

tile.data = data.tile;
tile.parse(tile.data, this.styleLayers.getLayerFamilies(), this.actor, data.rawTileData, callback);
tile.parse(tile.data, this.styles.getLayerFamilies(mapId), this.actor, data.rawTileData, callback);

this.loaded[source] = this.loaded[source] || {};
this.loaded[source][uid] = tile;
this.loaded[style + source] = this.loaded[style + source] || {};
this.loaded[style + source][uid] = tile;
}
},

/**
* Implements {@link WorkerSource#reloadTile}.
*
* @param {string} mapId A unique identifier for the map instance making this request
* @param {object} params
* @param {string} params.source The id of the source for which we're loading this tile.
* @param {string} params.uid The UID for this tile.
*/
reloadTile: function(params, callback) {
var loaded = this.loaded[params.source],
reloadTile: function(mapId, params, callback) {
var loaded = this.loaded[this.styles.getKey(mapId) + params.source],
uid = params.uid;
if (loaded && loaded[uid]) {
var tile = loaded[uid];
tile.parse(tile.data, this.styleLayers.getLayerFamilies(), this.actor, params.rawTileData, callback);
tile.parse(tile.data, this.styles.getLayerFamilies(mapId), this.actor, params.rawTileData, callback);
}
},

/**
* Implements {@link WorkerSource#abortTile}.
*
* @param {string} mapId A unique identifier for the map instance making this request
* @param {object} params
* @param {string} params.source The id of the source for which we're loading this tile.
* @param {string} params.uid The UID for this tile.
*/
abortTile: function(params) {
var loading = this.loading[params.source],
abortTile: function(mapId, params) {
var loading = this.loading[this.styles.getKey(mapId) + params.source],
uid = params.uid;
if (loading && loading[uid] && loading[uid].abort) {
loading[uid].abort();
Expand All @@ -100,12 +116,13 @@ VectorTileWorkerSource.prototype = {
/**
* Implements {@link WorkerSource#removeTile}.
*
* @param {string} mapId A unique identifier for the map instance making this request
* @param {object} params
* @param {string} params.source The id of the source for which we're loading this tile.
* @param {string} params.uid The UID for this tile.
*/
removeTile: function(params) {
var loaded = this.loaded[params.source],
removeTile: function(mapId, params) {
var loaded = this.loaded[this.styles.getKey(mapId) + params.source],
uid = params.uid;
if (loaded && loaded[uid]) {
delete loaded[uid];
Expand Down
Loading