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

Create a global worker pool #899

Closed
jfirebaugh opened this issue Dec 19, 2014 · 12 comments
Closed

Create a global worker pool #899

jfirebaugh opened this issue Dec 19, 2014 · 12 comments
Assignees

Comments

@jfirebaugh
Copy link
Contributor

mapbox-gl-js should use navigator.hardwareConcurrency - 1 workers no matter how many maps are on a page or how many times you call map.setStyle().

@jfirebaugh
Copy link
Contributor Author

Not sure how much this matters. If web workers are implemented as green threads then it may not.

@anandthakker
Copy link
Contributor

@jfirebaugh I'd like to be able to control the number of workers that mapbox-gl uses -- I'm envisioning using it in an app that also has some other heavy lifting to do, so that I might want to use a couple workers of my own.

As you point out, this is less of an issue if browsers implement web workers are green threads, but it could still be useful to be able to tune it.

@anandthakker
Copy link
Contributor

anandthakker commented Jun 1, 2016

Another, perhaps much more important, reason that this could matter: if there are multiple maps on the same page that use the same sources, there could be a lot of redundant parsing/processing of the same vector tile data. A worker pool shared among map instances could really help to reduce that, but it would require some pretty unwieldy logic for both distinguishing and correlating source ids from different instances.

@anandthakker
Copy link
Contributor

@jfirebaugh @lucaswoj would you be open to a PR to address this?

@mourner
Copy link
Member

mourner commented Jun 2, 2016

@anandthakker we're always open to PRs :)

@anandthakker
Copy link
Contributor

we're always open to PRs :)

@mourner Hah! I guess what I really mean to ask is: this would definitely add a bit of complexity, and perhaps it has other drawbacks that I haven't thought of. Given that, do you all feel that the benefits for this change would be worth the added complexity?

@mourner
Copy link
Member

mourner commented Jun 2, 2016

@anandthakker depends on the amount of added complexity, which is hard to judge before I see a PR. :) But you could outline the approach you'd like to take first before writing any code.

@anandthakker
Copy link
Contributor

Here is what I am thinking:

  • In Dispatcher, replace this.actors and this.currentActor with a module-scoped ActorPool instance.
  • ActorPool looks like:
{
   forEach: (fn: (Actor) => void) => void,
   getActor: (id) => Actor // with 'id' now a property of Actor instances, managed by the pool
   getCurrentActor: () => Actor
}
  • In the worker, use params.url instead of params.source as the key for this.loading, this.loaded, so that if two maps use the same source with different source id's, there's still a chance for parsed/cached tiles to be reused
  • Similar approach for geoJSONIndexes, but only for ones where the data is passed as a url (as opposed to geojson literal)

@anandthakker
Copy link
Contributor

Questions:

  • Dispatchers need a way to claim/release the actor pool resource, so that workers can be terminated when they're not needed anymore. What's the best way to handle this?
  • Options interface could be a little screwy: presumably, the pool will initialize itself based on the number-of-worker options given to map -- what if different map instances have different options?

I guess a way to do this is to have ActorPool as follows:

{
   requestActors: (numActors: number) => token,
   releaseActors: (token) => void,
   forEach: (fn: (Actor) => void) => void,
   getActor: (id) => Actor // with 'id' now a property of Actor instances, managed by the pool
   getCurrentActor: () => Actor
}

@anandthakker
Copy link
Contributor

anandthakker commented Jun 2, 2016

Another issue: workers also have a this.layers and this.layerFamilies, used somehow in tile parsing. In a global pool, a worker would need to hold on to more than one of these.

This would require a way to identify each style instance that's in use; e.g.: this.layers becomes a map from some kind of style ID to the layers/layerFamilies data for that style, and then the params to load tile include the style ID so that the correct layer data can be used for parsing the tile.

The actual mapbox style ID probably doesn't work, since the style can be given as JSON; maybe a hash of the serialized style? Alternatively, and simpler, the token that each dispatcher instance gets from requestActors could always be included in messages to the worker. The slight downside is that layers and layerFamilies would include duped data for multiple maps using the same style.

@anandthakker
Copy link
Contributor

@lucaswoj @mourner What do you think of the approach outlined above ^? Does it sound worth pursuing?

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 6, 2016

I support any interface that makes the workers stateless (or, at least, appear to be stateless to their callers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants