per-SourceType WorkerSource -> per-source-instance WorkerSource by ChrisLoer · Pull Request #5988 · mapbox/mapbox-gl-js

ChrisLoer · 2018-01-11T23:16:33Z

Based on @jfirebaugh 's feedback on #5983, I started exploring instantiating WorkerSources on a per-source basis instead of a per-source-type basis.

The initial set of changes were not too bad, and it allows for a nice simplification of geojson_worker_source that still supports the setData coalescing behavior.

My biggest concern is how this will affect custom sources -- requiring a source name to be included (either in parameters or in the "data.type" argument) is probably breaking the implied API?

removeSource also needs some re-thinking to handle messages that arrive after a source has been removed (or, moving even more into edge-case territory, to handle messages that were sent to a previous instantiation of a source, which has been removed and then re-added).

The unit tests that handle multiple sources are currently broken, but render/query tests pass.

/cc @jfirebaugh @anandthakker

anandthakker

👍 making each worker source simpler is great, and I'm not really worried about consequences for custom sources

anandthakker · 2018-01-12T20:41:33Z


    onRemove() {
+        // FIX: Why do we broadcast instead of sending to this.workerID?
        this.dispatcher.broadcast('removeSource', { type: this.type, source: this.id });


Good point -- a VectorTileSource has multiple corresponding workers, but a GeoJSONSource doesn't.

ChrisLoer · 2018-01-18T20:43:27Z

Since we reverted the GeoJSON coalescing changes, I cherry picked them into this branch and re-based it on top of master. After talking with @anandthakker, I went for the narrowest fix I could figure out for the potential removeSource race condition -- I just make GeoJSONSource track if it's been removed, and if so it will skip sending further messages in response to a loadData callback.

GeoJSONSource appears to be the only source implementation that actually sends removeSource, which helps keep the scope of this change small. It does however imply that we're leaking WorkerSource objects on the background whenever non-GeoJSON sources are removed. This is probably (?) not too big a deal -- previously, we were leaking entries in the shared loaded/loading maps, whereas now we're leaking an object which is basically just those entries along with some references to shared state (actor, styleLayerIndex, etc.).

ChrisLoer · 2018-01-18T22:13:24Z

@mollymerp, I made analogous changes to raster_dem_tile_worker_source, maybe you could take a glance at them? It seems pretty simple, but I'm almost totally unfamiliar with that source (TIL: DEM = Digital Elevation Model).

anandthakker

@ChrisLoer I think this looks great, and 👍 on the narrow fix for the removeSource issue on GeoJSONSource.

anandthakker · 2018-01-19T03:34:36Z

+    _state: SourceState;
+    _pendingCallback: Callback<boolean>;
+    _pendingLoadDataParams: LoadGeoJSONParameters;
+    _geoJSONIndex: GeoJSONIndex // object mapping source ids to geojson-vt-like tile indexes


nit: the line comment is now obsolete

anandthakker · 2018-01-19T03:55:39Z

            this.parent[data.type](data.sourceMapId, deserialize(data.data), done);
        } else if (typeof data.id !== 'undefined' && this.parent.getWorkerSource) {
-            // data.type == sourcetype.method
+            // data.type == sourcetype.sourcename.method


Nit: also update the doc comment above send() to reflect this change

mollymerp

Yep, changes on RasterDEMWorkerSource look good to me!

ChrisLoer · 2018-01-19T17:05:01Z

OK, updated comments and squashed to two commits: (1) re-introduces GeoJSON loadData coalescing, and (2) does the WorkerSource re-factoring.

My plan is still to hold off merging this until after we've released .44, just in case...

ChrisLoer · 2018-01-25T21:04:40Z

I just rebased on top of master, which turned out to be a bit messy because these changes collided with the new resource timing collection from PR #5991.

@sbma44 can you point me to a good way to sanity-check that this PR won't break yours?

sbma44 · 2018-01-25T21:22:35Z

@ChrisLoer sure thing--hopefully the unit tests capture a bunch of this, but for integration testing/sanity checking here's what I used:

'use strict';

const TOKEN = 'pk.xyz123'

let gd = {
  type: "Feature",
  properties: {},
  geometry: {
    type: "Point",
    coordinates: [
      -88.6,
      36.6
    ]
  }
};

mapboxgl.accessToken = TOKEN;
const map = new mapboxgl.Map({
    container: 'map',
    style: 'mapbox://styles/mapbox/streets-v9',
    collectResourceTiming: true
});

map.on('load', () => {
	map.addSource('blah', {
	    type: 'geojson',
	    data: 'http://localhost:5000/test.geojson'
	});

	window.setTimeout(() => {
		map.getSource('blah').setData('http://localhost:5000/test2.geojson');

		window.setTimeout(() => {
			map.getSource('blah').setData(gd);
		}, 2000);
	}, 1000);
});

map.on('data', (mde) => {
	console.log(mde);
});

to adapt it you will need to:

populate the access token
make a local geojson available at the two embedded URLs (doesn't matter what)
load the page and check console output for the presence of resourceTiming on:
- the tile property of the source data event associated with vector tile loads
- the event itself of geojson source metadata events
- the event itself of geojson source content events (for updates)

ChrisLoer · 2018-01-25T21:48:27Z

Thanks @sbma44! Looks like it's coming through fine. 👍

mollymerp

this looks good to me, but it might be worth checking the benchmarks?

mollymerp · 2018-01-25T23:31:11Z

+    function createWorker() {
+        const worker = new GeoJSONWorkerSource(null, layerIndex);
+
+        // Making the call to loadGeoJSON to asynchronous


nit - tiny typo

Making the call to loadGeoJSON to asynchronous

Re-implement PR #5902 including fix for issue #5970.

- Instead of making WorkerSource implementations responsible for tracking which source's messages they're processing, instantiate a different WorkerSource for each source, and make Worker responsible for forwarding messages to the correct instance. - Simplifies GeoJSONWorkerSource's coalescing logic. - Introduces a somewhat subtle requirement for Source implementers: any Source that sends a 'removeSource' message is responsible for sending no further messages. To re-add a Source with the same ID, create a new Source object and start sending messages. A new WorkerSource will be instantiated with matching state.

ChrisLoer · 2018-01-25T23:59:49Z

Benchmark results don't seem to show any big changes:

https://bl.ocks.org/anonymous/raw/9187a9a85d29a307fe03fe1f86f3d61c/

ChrisLoer added the under development label Jan 11, 2018

ChrisLoer requested review from anandthakker and jfirebaugh January 11, 2018 23:16

anandthakker reviewed Jan 12, 2018

View reviewed changes

sbma44 mentioned this pull request Jan 17, 2018

Expose Resource Timing API #5991

Merged

5 tasks

ChrisLoer force-pushed the split-worker-source branch from 58f3642 to 6109cd2 Compare January 18, 2018 20:34

ChrisLoer changed the base branch from cloer-5970 to master January 18, 2018 20:35

anandthakker reviewed Jan 19, 2018

View reviewed changes

mollymerp reviewed Jan 19, 2018

View reviewed changes

ChrisLoer force-pushed the split-worker-source branch from f97c6fa to 4ea6197 Compare January 19, 2018 17:01

ChrisLoer changed the title ~~WIP: per-SourceType WorkerSource -> per-source-instance WorkerSource~~ per-SourceType WorkerSource -> per-source-instance WorkerSource Jan 19, 2018

ChrisLoer added refactoring 🏗️ and removed under development labels Jan 19, 2018

anandthakker mentioned this pull request Jan 22, 2018

Don't mix coalesce calls from multiple GeoJSON sources (issue #5970) #5983

Closed

ChrisLoer mentioned this pull request Jan 25, 2018

v0.44.0 #6057

Merged

ChrisLoer force-pushed the split-worker-source branch from 4ea6197 to ae67df9 Compare January 25, 2018 21:02

mollymerp approved these changes Jan 25, 2018

View reviewed changes

ChrisLoer added 3 commits January 25, 2018 15:42

Coalesce GeoJSON "loadData" requests.

e89aa36

Re-implement PR #5902 including fix for issue #5970.

Merge fix ups

ada68c9

ChrisLoer force-pushed the split-worker-source branch from ae67df9 to ada68c9 Compare January 25, 2018 23:43

ChrisLoer merged commit d81d4fb into master Jan 29, 2018

ChrisLoer deleted the split-worker-source branch January 29, 2018 15:58

ChrisLoer mentioned this pull request Jan 29, 2018

Automatically batch the work done by GeoJSON#setData() #3692

Closed

ChrisLoer mentioned this pull request Mar 7, 2018

geoJSON source setData should cancel outstanding setData #6256

Closed

Conversation

ChrisLoer commented Jan 11, 2018

Uh oh!

anandthakker left a comment

Choose a reason for hiding this comment

Uh oh!

anandthakker Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jan 18, 2018

Uh oh!

ChrisLoer commented Jan 18, 2018

Uh oh!

anandthakker left a comment

Choose a reason for hiding this comment

Uh oh!

anandthakker Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

anandthakker Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

mollymerp left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jan 19, 2018

Uh oh!

ChrisLoer commented Jan 25, 2018

Uh oh!

sbma44 commented Jan 25, 2018

Uh oh!

ChrisLoer commented Jan 25, 2018

Uh oh!

mollymerp left a comment

Choose a reason for hiding this comment

Uh oh!

mollymerp Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jan 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants