Add a minimal global worker pool

[anandthakker]

Closes #899

@lucaswoj this is a much more focused / less ambitious change than #2917. It adds a global worker pool, making only the minimal changes necessary to allow a Worker to serve multiple map instances correctly.

It does not attempt to deduplicate the work that is done for different map instances. Nonetheless, there are benefits to sharing workers. Immediate benefit: the overhead to create a worker is significant, because it essentially involves copying all of the worker code over from the main thread. Future benefit: using shared workers paves the way for more ambitious performance inter-map-instance improvements like #2951

Specific Notes

• The majority of the +505 lines come from new tests, benchmark, and debug page.
• Essentially, the simple strategy here is to change the worker's state-keeping objects Worker#layers, and Worker#layerFamilies into objects keyed off of mapId.
• The WorkerSource abstraction helps here, because it allowed for implementing per-map state management for things like geojson-vt indexes and cached WorkerTiles, but with pretty minimal code changes. Before this, we instantiated a single WorkerSource instance for each source type. Now, when a WorkerSource is registered, we just hold on to the class, and instantiate a separate one for each map (or rather, for each mapId). Doing it this way means that none of the code in GeoJSONWorkerSource or VectorTileWorkerSource had to be touched.
• The workerCount Map option is removed in favor of mapboxgl.WorkerPool.WORKER_COUNT

Checklist

• briefly describe the changes in this PR
• Add unit test to assert that Dispatcher correctly consumes WorkerPool
• Add unit tests for WorkerPool
• Add unit test for two Actors using the same underlying Worker.
• (n/a) document any changes or additions to public APIs
• post scores for all benchmarks on your branch and master
• do a quick sanity check on the debug page
• get a PR review 👍 / 👎

[anandthakker]

Rebasing, previous at d3d0787

[anandthakker]

FWIW: I'm using this branch in a project where we have multiple maps on a page, and even just having a shared pool--without actually deduping any of the work--significantly reduces the initial load time, because initializing each Worker is a pretty significant overhead.

[anandthakker]

Updated top of ticket with rationale and the checklist from the new PR template

[anandthakker]

Rebased, with previous head = 94dfd96

[anandthakker]

Wouldn't expect these changes to affect the benchmarks much, but that's partly the point of them, right?

This branch

buffer: 886 ms
fps: 60 fps
frame-duration: 6.9 ms, 0% > 16ms
query-point: 0.64 ms
query-box: 47.80 ms
geojson-setdata-small: 13 ms
geojson-setdata-large: 115 ms

Master

Broke w/ console errors at the end of query-point, but here are the first
three:

buffer: 868 ms
fps: 58 fps
frame-duration: 6.8 ms, 0% > 16ms

[anandthakker]

Added a benchmark to measure the time from new Map to the load event, with multiple maps on a page.

master

load-multiple-maps: 7,024 ms, loaded 6 maps.

This branch

load-multiple-maps: 4,778 ms, loaded 6 maps.

[anandthakker]

In light of a global worker pool, I don't know that either of the following seem necessary/useful: (a) having different map instances use a different number of workers, or (b) being able to resize the worker pool.

Does it make sense to remove the workerCount option from Map (and Style and Dispatcher)? Instead there could be an "advanced" API like mapboxgl.workerPool.setWorkerCount(), which would need to be called before any map instances were constructed.

[mourner]

Makes sense. I would go for something like mapboxgl.WORKER_COUNT — it would only be changed for development/debugging purposes anyway.

[anandthakker]

I would go for something like mapboxgl.WORKER_COUNT — it would only be changed for development/debugging purposes anyway.

I think it could be set by a user who was tying to balance a GL JS map versus other cpu-intensive stuff in a web app. (For instance: in a project we're working on, I was thinking of intentionally limiting the GL JS worker count to 3/4 of the available workers, so that 1/4 could be dedicated to keeping some other expensive data-munging off the main thread to keep the UI snappier)

[anandthakker]

^ but, with that said, I don't actually have any objections to making it a constant like you proposed, @mourner

[anandthakker]

Actually, just a note that making this a constant on the mapboxgl object causes some weirdness, because worker_pool then has to depend on the parent mapboxgl module. Means having to be careful about a dependency cycle, and I think might also make unit tests a bit more awkward. How about mapboxgl.workerPool.WORKER_COUNT? @mourner

[anandthakker]

Went with mapboxgl.WorkerPool.WORKER_COUNT - @mourner let me know what you think.

(note: also rebased to master)

[anandthakker]

Updated the top of the ticket. @lucaswoj while I'm very much in favor of #3034, I would propose that the changes in this PR provide some immediate benefits and incur little to no cost. What do you think?

[lucaswoj]

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?

[anandthakker]

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

[anandthakker]

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

[lucaswoj]

This is ready to merge on 🍏 . Just rebased to include #3043.

[anandthakker]

Whoops, will also need #3052 to get a :green: here. Once that's merged I'll rebase again and then (hopefully) merge

[anandthakker]

Rebasing -- previous at 84ce97b

[anandthakker]

Success! Merging.

[anandthakker]

Ack - just realized that I dropped the ball on this comment -- the diff changed while I was addressing something else, and so I forgot about it.

@lucaswoj if I don't get a chance to open a PR patching that today, I'll file an issue so it doesn't get lost

[lucaswoj]

@anandthakker It's a minor change, don't stress over it.