-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
WIP - share workers across Map instance with a global worker pool #2917
Conversation
Instead of having a mock Dispatcher for node, pull out the relevant code into a small wrapper, web_worker.js, which contains the node/browser code split to a smaller scope.
This prepares for workers being shared across map instances.
@@ -24,7 +24,7 @@ function WorkerTile(params) { | |||
} | |||
|
|||
WorkerTile.prototype.parse = function(data, layerFamilies, actor, rawTileData, callback) { | |||
|
|||
console.log('PARSING TILE', this.source, this.coord.z + '/' + this.coord.x + '/' + this.coord.y, this.uid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For demo purposes only ^
} else { | ||
tile.workerID = this.dispatcher.send('load tile', params, done.bind(this)); | ||
this.dispatcher.send('load tile', params, done.bind(this), tile.uid); |
There was a problem hiding this comment.
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.
|
||
if (this.layers[key]) return; | ||
|
||
var styleLayers = this.layers[key] = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this change, this.layers
and this.layerFamilies
gain a layer of indirection, becoming maps from a 'style key' to the style's set of layers and layer families. This allows a Worker to track multiple map instances' styles -- but by keying them off of a 'style key' rather than a map instance id, we allow for two map instances with the same style to share work.
@lucaswoj added some comments and notes in the diff -- I think this is ready for a first 👀 to validate / modify / reject the approach. |
wow! thank you @anandthakker! at first glance -- and those gifs!! -- this is really impressive. |
Thanks @mollymerp ! Let me know if y'all think this approach is viable (generally speaking), and if so, I'll take another pass to do some cleanup, benchmarking, and unit tests. |
// 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; |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
Before I delve into a line-by-line review here, may I ask a few questions about this PR?
|
parentListeners.splice(0, parentListeners.length); | ||
workerListeners.splice(0, workerListeners.length); | ||
}; | ||
this.id = util.uniqueId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug: this id needs to be unique across map instances, but util.uniqueId()
isn't.
@lucaswoj SHOOT, yes, I completely missed this, and I think it might be a dealbreaker for this approach. (Here, in trying to handle the race condition of one instance calling Maybe handling this will be best done by scrapping this approach and starting with something like this suggestion:
Closing this, but I'll dump a summary of the approach I started here, in case there are any parts of it that might be useful salvage or discussion: WorkerPoolEach Like before, dispatchers access workers by way of
(This is a logical diagram -- in reality, each Actor and Worker
Changes to
Smaller Changes
Other Questions
Right now, I think that's right: GeoJSONSource is still TBD.
This, I think, is handled by the "style key" system above. |
Aw shucks 😢. Thanks for your work on this nonetheless! |
Refs #899
Before:
After: