From 6560f929efc53884d77c9d24d394cfca9220d16b Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Thu, 25 Feb 2021 23:49:37 +0200 Subject: [PATCH 1/9] simplify geojson source by coalescing on the main thread --- src/source/geojson_source.js | 93 +++++++------------ src/source/geojson_worker_source.js | 81 +--------------- test/unit/source/geojson_source.test.js | 44 +++------ .../unit/source/geojson_worker_source.test.js | 88 ------------------ 4 files changed, 48 insertions(+), 258 deletions(-) diff --git a/src/source/geojson_source.js b/src/source/geojson_source.js index c8397ab334f..5b66a991f5f 100644 --- a/src/source/geojson_source.js +++ b/src/source/geojson_source.js @@ -15,6 +15,7 @@ import type Actor from '../util/actor.js'; import type {Callback} from '../types/callback.js'; import type {GeoJSON, GeoJSONFeature} from '@mapbox/geojson-types'; import type {GeoJSONSourceSpecification, PromoteIdSpecification} from '../style-spec/types.js'; +import type {Cancelable} from '../types/cancelable.js'; /** * A source containing GeoJSON. @@ -79,9 +80,10 @@ class GeoJSONSource extends Evented implements Source { map: Map; actor: Actor; _loaded: boolean; + _coalesce: ?boolean; + _metadataFired: ?boolean; _collectResourceTiming: boolean; - _resourceTiming: Array; - _removed: boolean; + _pendingLoad: ?Cancelable; /** * @private @@ -100,7 +102,6 @@ class GeoJSONSource extends Evented implements Source { this.tileSize = 512; this.isTileClipped = true; this.reparseOverscaled = true; - this._removed = false; this._loaded = false; this.actor = dispatcher.getActor(); @@ -110,7 +111,6 @@ class GeoJSONSource extends Evented implements Source { this._options = extend({}, options); this._collectResourceTiming = options.collectResourceTiming; - this._resourceTiming = []; if (options.maxzoom !== undefined) this.maxzoom = options.maxzoom; if (options.type) this.type = options.type; @@ -147,29 +147,9 @@ class GeoJSONSource extends Evented implements Source { }, options.workerOptions); } - load() { - this.fire(new Event('dataloading', {dataType: 'source'})); - this._updateWorkerData((err) => { - if (err) { - this.fire(new ErrorEvent(err)); - return; - } - - const data: Object = {dataType: 'source', sourceDataType: 'metadata'}; - if (this._collectResourceTiming && this._resourceTiming && (this._resourceTiming.length > 0)) { - data.resourceTiming = this._resourceTiming; - this._resourceTiming = []; - } - - // although GeoJSON sources contain no metadata, we fire this event to let the SourceCache - // know its ok to start requesting tiles. - this.fire(new Event('data', data)); - }); - } - onAdd(map: Map) { this.map = map; - this.load(); + this.setData(this._data); } /** @@ -180,21 +160,7 @@ class GeoJSONSource extends Evented implements Source { */ setData(data: GeoJSON | string) { this._data = data; - this.fire(new Event('dataloading', {dataType: 'source'})); - this._updateWorkerData((err) => { - if (err) { - this.fire(new ErrorEvent(err)); - return; - } - - const data: Object = {dataType: 'source', sourceDataType: 'content'}; - if (this._collectResourceTiming && this._resourceTiming && (this._resourceTiming.length > 0)) { - data.resourceTiming = this._resourceTiming; - this._resourceTiming = []; - } - this.fire(new Event('data', data)); - }); - + this._updateWorkerData(); return this; } @@ -262,7 +228,15 @@ class GeoJSONSource extends Evented implements Source { * handles loading the geojson data and preparing to serve it up as tiles, * using geojson-vt or supercluster as appropriate. */ - _updateWorkerData(callback: Callback) { + _updateWorkerData() { + // if there's an earlier loadData to finish, wait until it finishes and then do another update + if (this._pendingLoad) { + this._coalesce = true; + return; + } + + this.fire(new Event('dataloading', {dataType: 'source'})); + this._loaded = false; const options = extend({}, this.workerOptions); const data = this._data; @@ -276,24 +250,28 @@ class GeoJSONSource extends Evented implements Source { // target {this.type}.loadData rather than literally geojson.loadData, // so that other geojson-like source types can easily reuse this // implementation - this.actor.send(`${this.type}.loadData`, options, (err, result) => { - if (this._removed || (result && result.abandoned)) { + this._pendingLoad = this.actor.send(`${this.type}.loadData`, options, (err, result) => { + this._loaded = true; + this._pendingLoad = null; + + if (err) { + this.fire(new ErrorEvent(err)); return; } - this._loaded = true; + // although GeoJSON sources contain no metadata, we fire this event at first + // to let the SourceCache know its ok to start requesting tiles. + const data: Object = {dataType: 'source', sourceDataType: this._metadataFired ? 'content' : 'metadata'}; + if (this._collectResourceTiming && result && result.resourceTiming && result.resourceTiming[this.id]) { + data.resourceTiming = result.resourceTiming[this.id]; + } + this.fire(new Event('data', data)); + this._metadataFired = true; - if (result && result.resourceTiming && result.resourceTiming[this.id]) - this._resourceTiming = result.resourceTiming[this.id].slice(0); - // Any `loadData` calls that piled up while we were processing - // this one will get coalesced into a single call when this - // 'coalesce' message is processed. - // We would self-send from the worker if we had access to its - // message queue. Waiting instead for the 'coalesce' to round-trip - // through the foreground just means we're throttling the worker - // to run at a little less than full-throttle. - this.actor.send(`${this.type}.coalesce`, {source: options.source}, null); - callback(err); + if (this._coalesce) { + this._updateWorkerData(); + this._coalesce = false; + } }); } @@ -350,8 +328,9 @@ class GeoJSONSource extends Evented implements Source { } onRemove() { - this._removed = true; - this.actor.send('removeSource', {type: this.type, source: this.id}); + if (this._pendingLoad) { + this._pendingLoad.cancel(); + } } serialize() { diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js index 420802a6ac5..b6295e8354a 100644 --- a/src/source/geojson_worker_source.js +++ b/src/source/geojson_worker_source.js @@ -77,11 +77,6 @@ function loadGeoJSONTile(params: RequestedTileParameters, callback: LoadVectorDa }); } -export type SourceState = - | 'Idle' // Source empty or data loaded - | 'Coalescing' // Data finished loading, but discard 'loadData' messages until receiving 'coalesced' - | 'NeedsLoadData'; // 'loadData' received while coalescing, trigger one more 'loadData' on receiving 'coalesced' - /** * The {@link WorkerSource} implementation that supports {@link GeoJSONSource}. * This class is designed to be easily reused to support custom source types @@ -94,11 +89,6 @@ export type SourceState = */ class GeoJSONWorkerSource extends VectorTileWorkerSource { loadGeoJSON: LoadGeoJSON; - _state: SourceState; - _pendingCallback: Callback<{ - resourceTiming?: {[_: string]: Array}, - abandoned?: boolean }>; - _pendingLoadDataParams: LoadGeoJSONParameters; _geoJSONIndex: GeoJSONIndex /** @@ -131,39 +121,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { * @param callback * @private */ - loadData(params: LoadGeoJSONParameters, callback: Callback<{ - resourceTiming?: {[_: string]: Array}, - abandoned?: boolean }>) { - if (this._pendingCallback) { - // Tell the foreground the previous call has been abandoned - this._pendingCallback(null, {abandoned: true}); - } - this._pendingCallback = callback; - this._pendingLoadDataParams = params; - - if (this._state && - this._state !== 'Idle') { - this._state = 'NeedsLoadData'; - } else { - this._state = 'Coalescing'; - this._loadData(); - } - } - - /** - * Internal implementation: called directly by `loadData` - * or by `coalesce` using stored parameters. - */ - _loadData() { - if (!this._pendingCallback || !this._pendingLoadDataParams) { - assert(false); - return; - } - const callback = this._pendingCallback; - const params = this._pendingLoadDataParams; - delete this._pendingCallback; - delete this._pendingLoadDataParams; - + loadData(params: LoadGeoJSONParameters, callback: Callback<{resourceTiming?: {[_: string]: Array}}>) { const requestParam = params && params.request; const perf = requestParam && requestParam.collectResourceTiming; @@ -209,35 +167,6 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { }); } - /** - * While processing `loadData`, we coalesce all further - * `loadData` messages into a single call to _loadData - * that will happen once we've finished processing the - * first message. {@link GeoJSONSource#_updateWorkerData} - * is responsible for sending us the `coalesce` message - * at the time it receives a response from `loadData` - * - * State: Idle - * ↑ | - * 'coalesce' 'loadData' - * | (triggers load) - * | ↓ - * State: Coalescing - * ↑ | - * (triggers load) | - * 'coalesce' 'loadData' - * | ↓ - * State: NeedsLoadData - */ - coalesce() { - if (this._state === 'Coalescing') { - this._state = 'Idle'; - } else if (this._state === 'NeedsLoadData') { - this._state = 'Coalescing'; - this._loadData(); - } - } - /** * Implements {@link WorkerSource#reloadTile}. * @@ -289,14 +218,6 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { } } - removeSource(params: {source: string}, callback: Callback) { - if (this._pendingCallback) { - // Don't leak callbacks - this._pendingCallback(null, {abandoned: true}); - } - callback(); - } - getClusterExpansionZoom(params: {clusterId: number}, callback: Callback) { try { callback(null, this._geoJSONIndex.getClusterExpansionZoom(params.clusterId)); diff --git a/test/unit/source/geojson_source.test.js b/test/unit/source/geojson_source.test.js index 8a61fd1192f..c9e56c2604e 100644 --- a/test/unit/source/geojson_source.test.js +++ b/test/unit/source/geojson_source.test.js @@ -77,13 +77,13 @@ test('GeoJSONSource#setData', (t) => { source.once('data', t.end); source.setData({}); }); - source.load(); + source.setData({}); }); t.test('fires "dataloading" event', (t) => { const source = createSource(); source.on('dataloading', t.end); - source.load(); + source.setData({}); }); t.test('respects collectResourceTiming parameter on source', (t) => { @@ -106,25 +106,6 @@ test('GeoJSONSource#setData', (t) => { t.end(); }); -test('GeoJSONSource#onRemove', (t) => { - t.test('broadcasts "removeSource" event', (t) => { - const source = new GeoJSONSource('id', {data: {}}, wrapDispatcher({ - send(type, data, callback) { - t.false(callback); - t.equal(type, 'removeSource'); - t.deepEqual(data, {type: 'geojson', source: 'id'}); - t.end(); - }, - broadcast() { - // Ignore - } - })); - source.onRemove(); - }); - - t.end(); -}); - test('GeoJSONSource#update', (t) => { const transform = new Transform(); transform.resize(200, 200); @@ -142,7 +123,7 @@ test('GeoJSONSource#update', (t) => { }); /* eslint-disable no-new */ - new GeoJSONSource('id', {data: {}}, mockDispatcher).load(); + new GeoJSONSource('id', {data: {}}, mockDispatcher).setData({}); }); t.test('forwards geojson-vt options with worker request', (t) => { @@ -167,7 +148,7 @@ test('GeoJSONSource#update', (t) => { tolerance: 0.25, buffer: 16, generateId: true - }, mockDispatcher).load(); + }, mockDispatcher).setData({}); }); t.test('forwards Supercluster options with worker request', (t) => { @@ -193,7 +174,7 @@ test('GeoJSONSource#update', (t) => { clusterRadius: 100, clusterMinPoints: 3, generateId: true - }, mockDispatcher).load(); + }, mockDispatcher).setData({}); }); t.test('transforms url before making request', (t) => { @@ -224,7 +205,7 @@ test('GeoJSONSource#update', (t) => { if (e.sourceDataType === 'metadata') t.end(); }); - source.load(); + source.setData({}); }); t.test('fires "error"', (t) => { @@ -243,7 +224,7 @@ test('GeoJSONSource#update', (t) => { t.end(); }); - source.load(); + source.setData({}); }); t.test('sends loadData request to dispatcher after data update', (t) => { @@ -271,7 +252,7 @@ test('GeoJSONSource#update', (t) => { } }); - source.load(); + source.setData({}); }); t.end(); @@ -285,8 +266,7 @@ test('GeoJSONSource#serialize', (t) => { }; t.test('serialize source with inline data', (t) => { const source = new GeoJSONSource('id', {data: hawkHill}, mockDispatcher); - source.map = mapStub; - source.load(); + source.onAdd(mapStub); t.deepEqual(source.serialize(), { type: 'geojson', data: hawkHill @@ -296,8 +276,7 @@ test('GeoJSONSource#serialize', (t) => { t.test('serialize source with url', (t) => { const source = new GeoJSONSource('id', {data: 'local://data.json'}, mockDispatcher); - source.map = mapStub; - source.load(); + source.onAdd(mapStub); t.deepEqual(source.serialize(), { type: 'geojson', data: 'local://data.json' @@ -307,8 +286,7 @@ test('GeoJSONSource#serialize', (t) => { t.test('serialize source with updated data', (t) => { const source = new GeoJSONSource('id', {data: {}}, mockDispatcher); - source.map = mapStub; - source.load(); + source.onAdd(mapStub); source.setData(hawkHill); t.deepEqual(source.serialize(), { type: 'geojson', diff --git a/test/unit/source/geojson_worker_source.test.js b/test/unit/source/geojson_worker_source.test.js index 75a180d15c2..e7b4a466a90 100644 --- a/test/unit/source/geojson_worker_source.test.js +++ b/test/unit/source/geojson_worker_source.test.js @@ -39,7 +39,6 @@ test('reloadTile', (t) => { function addData(callback) { source.loadData({source: 'sourceId', data: JSON.stringify(geoJson)}, (err) => { - source.coalesce({source: 'sourceId'}); t.equal(err, null); callback(); }); @@ -150,90 +149,3 @@ test('resourceTiming', (t) => { t.end(); }); - -test('loadData', (t) => { - const layers = [ - { - id: 'layer1', - source: 'source1', - type: 'symbol', - }, - { - id: 'layer2', - source: 'source2', - type: 'symbol', - } - ]; - - const geoJson = { - "type": "Feature", - "geometry": { - "type": "Point", - "coordinates": [0, 0] - } - }; - - const layerIndex = new StyleLayerIndex(layers); - function createWorker() { - const worker = new GeoJSONWorkerSource(actor, layerIndex, [], true); - - // Making the call to loadGeoJSON asynchronous - // allows these tests to mimic a message queue building up - // (regardless of timing) - const originalLoadGeoJSON = worker.loadGeoJSON; - worker.loadGeoJSON = function(params, callback) { - setTimeout(() => { - originalLoadGeoJSON(params, callback); - }, 0); - }; - return worker; - } - - t.test('abandons coalesced callbacks', (t) => { - // Expect first call to run, second to be abandoned, - // and third to run in response to coalesce - const worker = createWorker(); - worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => { - t.equal(err, null); - t.notOk(result && result.abandoned); - worker.coalesce({source: 'source1'}); - }); - - worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => { - t.equal(err, null); - t.ok(result && result.abandoned); - }); - - worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => { - t.equal(err, null); - t.notOk(result && result.abandoned); - t.end(); - }); - }); - - t.test('removeSource aborts callbacks', (t) => { - // Expect: - // First loadData starts running before removeSource arrives - // Second loadData is pending when removeSource arrives, gets cancelled - // removeSource is executed immediately - // First loadData finishes running, sends results back to foreground - const worker = createWorker(); - worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => { - t.equal(err, null); - t.notOk(result && result.abandoned); - t.end(); - }); - - worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => { - t.equal(err, null); - t.ok(result && result.abandoned); - }); - - worker.removeSource({source: 'source1'}, (err) => { - t.notOk(err); - }); - - }); - - t.end(); -}); From 457f808ad2ecdb90b2c2f5511734f1a2f0790740 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Fri, 26 Feb 2021 11:59:39 +0200 Subject: [PATCH 2/9] only queue worker tasks explicitly for loadTile/getResource --- src/source/geojson_source.js | 2 +- src/source/raster_dem_tile_source.js | 2 +- src/source/vector_tile_source.js | 8 ++++---- src/util/actor.js | 13 ++++++------- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/source/geojson_source.js b/src/source/geojson_source.js index 5b66a991f5f..9ddd6996383 100644 --- a/src/source/geojson_source.js +++ b/src/source/geojson_source.js @@ -311,7 +311,7 @@ class GeoJSONSource extends Evented implements Source { tile.loadVectorData(data, this.map.painter, message === 'reloadTile'); return callback(null); - }); + }, undefined, message === 'loadTile'); } abortTile(tile: Tile) { diff --git a/src/source/raster_dem_tile_source.js b/src/source/raster_dem_tile_source.js index 8a9d8075628..c09dd21e602 100644 --- a/src/source/raster_dem_tile_source.js +++ b/src/source/raster_dem_tile_source.js @@ -77,7 +77,7 @@ class RasterDEMTileSource extends RasterTileSource implements Source { if (!tile.actor || tile.state === 'expired') { tile.actor = this.dispatcher.getActor(); - tile.actor.send('loadDEMTile', params, done.bind(this)); + tile.actor.send('loadDEMTile', params, done.bind(this), undefined, true); } } } diff --git a/src/source/vector_tile_source.js b/src/source/vector_tile_source.js index d80a3a3b2bc..4f8ae6b742a 100644 --- a/src/source/vector_tile_source.js +++ b/src/source/vector_tile_source.js @@ -227,13 +227,13 @@ class VectorTileSource extends Evented implements Source { expires: data.expires, rawData: data.rawData.slice(0) }; - if (tile.actor) tile.actor.send('loadTile', params, done.bind(this)); + if (tile.actor) tile.actor.send('loadTile', params, done.bind(this), undefined, true); } }, true); tile.request = {cancel}; } else { - tile.request = tile.actor.send('loadTile', params, done.bind(this)); + tile.request = tile.actor.send('loadTile', params, done.bind(this), undefined, true); } } else if (tile.state === 'loading') { @@ -277,14 +277,14 @@ class VectorTileSource extends Evented implements Source { delete tile.request; } if (tile.actor) { - tile.actor.send('abortTile', {uid: tile.uid, type: this.type, source: this.id}, undefined); + tile.actor.send('abortTile', {uid: tile.uid, type: this.type, source: this.id}); } } unloadTile(tile: Tile) { tile.unloadVectorData(); if (tile.actor) { - tile.actor.send('removeTile', {uid: tile.uid, type: this.type, source: this.id}, undefined); + tile.actor.send('removeTile', {uid: tile.uid, type: this.type, source: this.id}); } } diff --git a/src/util/actor.js b/src/util/actor.js index 5cb2f5834c9..1a6f1da1189 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -107,13 +107,12 @@ class Actor { cancel.cancel(); } } else { - if (isWorker() || data.mustQueue) { - // In workers, store the tasks that we need to process before actually processing them. This - // is necessary because we want to keep receiving messages, and in particular, - // messages. Some tasks may take a while in the worker thread, so before - // executing the next task in our queue, postMessage preempts this and - // messages can be processed. We're using a MessageChannel object to get throttle the - // process() flow to one at a time. + if (data.mustQueue) { + // for tasks that are often cancelled, such as loadTile, store them before actually + // processing them. This is necessary because we want to keep receiving messages. + // Some tasks may take a while in the worker thread, so before executing the next task + // in our queue, postMessage preempts this and messages can be processed. + // We're using a MessageChannel object to get throttle the process() flow to one at a time. const callback = this.callbacks[id]; const metadata = (callback && callback.metadata) || {type: "message"}; this.cancelCallbacks[id] = this.scheduler.add(() => this.processTask(id, data), metadata); From 92c4ca9218b96cf2f65e575881a6274c84426e2e Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Mon, 8 Mar 2021 17:17:26 +0200 Subject: [PATCH 3/9] limit scheduler to the worker, queue getGlyphs/Images callbacks --- src/source/worker_tile.js | 6 +++--- src/util/actor.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/source/worker_tile.js b/src/source/worker_tile.js index 92c369b0877..a35979de578 100644 --- a/src/source/worker_tile.js +++ b/src/source/worker_tile.js @@ -169,7 +169,7 @@ class WorkerTile { glyphMap = result; maybePrepare.call(this); } - }, undefined, undefined, taskMetadata); + }, undefined, true, taskMetadata); } else { glyphMap = {}; } @@ -182,7 +182,7 @@ class WorkerTile { iconMap = result; maybePrepare.call(this); } - }, undefined, undefined, taskMetadata); + }, undefined, true, taskMetadata); } else { iconMap = {}; } @@ -195,7 +195,7 @@ class WorkerTile { patternMap = result; maybePrepare.call(this); } - }, undefined, undefined, taskMetadata); + }, undefined, true, taskMetadata); } else { patternMap = {}; } diff --git a/src/util/actor.js b/src/util/actor.js index 1a6f1da1189..ce1fee6bae6 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -107,8 +107,8 @@ class Actor { cancel.cancel(); } } else { - if (data.mustQueue) { - // for tasks that are often cancelled, such as loadTile, store them before actually + if (isWorker() && data.mustQueue) { + // for worker tasks that are often cancelled, such as loadTile, store them before actually // processing them. This is necessary because we want to keep receiving messages. // Some tasks may take a while in the worker thread, so before executing the next task // in our queue, postMessage preempts this and messages can be processed. From 334c6568ecdd037d0ecec19e82c82064d2399fb7 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Mon, 8 Mar 2021 17:25:05 +0200 Subject: [PATCH 4/9] make sure all worker tasks are measured in metrics --- src/util/actor.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util/actor.js b/src/util/actor.js index ce1fee6bae6..15d54200d94 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -4,6 +4,7 @@ import {bindAll, isWorker, isSafari} from './util.js'; import window from './window.js'; import {serialize, deserialize} from './web_worker_transfer.js'; import Scheduler from './scheduler.js'; +import {PerformanceUtils} from './performance.js'; import type {Transferable} from '../types/transferable.js'; import type {Cancelable} from '../types/cancelable.js'; @@ -119,12 +120,14 @@ class Actor { } else { // In the main thread, process messages immediately so that other work does not slip in // between getting partial data back from workers. + // Do the same for worker tasks that need processing as soon as possible. this.processTask(id, data); } } } processTask(id: number, task: any) { + const m = isWorker() ? PerformanceUtils.beginMeasure('workerTask') : undefined; if (task.type === '') { // The done() function in the counterpart has been called, and we are now // firing the callback in the originating actor, if there is one. @@ -166,6 +169,7 @@ class Actor { done(new Error(`Could not find function ${task.type}`)); } } + if (m) PerformanceUtils.endMeasure(m); } remove() { From 0651ec6389efae70d0028b9ebb25b80e4487beb4 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Mon, 8 Mar 2021 17:39:31 +0200 Subject: [PATCH 5/9] make sure we coalesce GeoJSON updates even after error --- src/source/geojson_source.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/source/geojson_source.js b/src/source/geojson_source.js index 9ddd6996383..18dc17ef851 100644 --- a/src/source/geojson_source.js +++ b/src/source/geojson_source.js @@ -256,17 +256,17 @@ class GeoJSONSource extends Evented implements Source { if (err) { this.fire(new ErrorEvent(err)); - return; - } - // although GeoJSON sources contain no metadata, we fire this event at first - // to let the SourceCache know its ok to start requesting tiles. - const data: Object = {dataType: 'source', sourceDataType: this._metadataFired ? 'content' : 'metadata'}; - if (this._collectResourceTiming && result && result.resourceTiming && result.resourceTiming[this.id]) { - data.resourceTiming = result.resourceTiming[this.id]; + } else { + // although GeoJSON sources contain no metadata, we fire this event at first + // to let the SourceCache know its ok to start requesting tiles. + const data: Object = {dataType: 'source', sourceDataType: this._metadataFired ? 'content' : 'metadata'}; + if (this._collectResourceTiming && result && result.resourceTiming && result.resourceTiming[this.id]) { + data.resourceTiming = result.resourceTiming[this.id]; + } + this.fire(new Event('data', data)); + this._metadataFired = true; } - this.fire(new Event('data', data)); - this._metadataFired = true; if (this._coalesce) { this._updateWorkerData(); From 83d06d870b36ac31dc6939abebe29b743de43e1a Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Mon, 8 Mar 2021 18:25:06 +0200 Subject: [PATCH 6/9] don't measure non-scheduled tasks twice --- src/util/actor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/actor.js b/src/util/actor.js index 15d54200d94..be9200a054a 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -121,13 +121,14 @@ class Actor { // In the main thread, process messages immediately so that other work does not slip in // between getting partial data back from workers. // Do the same for worker tasks that need processing as soon as possible. + const m = isWorker() ? PerformanceUtils.beginMeasure('workerTask') : undefined; this.processTask(id, data); + if (m) PerformanceUtils.endMeasure(m); } } } processTask(id: number, task: any) { - const m = isWorker() ? PerformanceUtils.beginMeasure('workerTask') : undefined; if (task.type === '') { // The done() function in the counterpart has been called, and we are now // firing the callback in the originating actor, if there is one. @@ -169,7 +170,6 @@ class Actor { done(new Error(`Could not find function ${task.type}`)); } } - if (m) PerformanceUtils.endMeasure(m); } remove() { From 0cce742f852c629e86ee1a14eab051d9002582cb Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Tue, 9 Mar 2021 08:58:39 +0200 Subject: [PATCH 7/9] one more queueing fix --- src/source/worker.js | 2 +- src/util/actor.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/source/worker.js b/src/source/worker.js index d2466a697d5..b1336bec6bd 100644 --- a/src/source/worker.js +++ b/src/source/worker.js @@ -235,7 +235,7 @@ export default class Worker { // use a wrapped actor so that we can attach a target mapId param // to any messages invoked by the WorkerSource const actor = { - send: (type, data, callback, mustQueue, _, metadata) => { + send: (type, data, callback, _, mustQueue, metadata) => { this.actor.send(type, data, callback, mapId, mustQueue, metadata); }, scheduler: this.actor.scheduler diff --git a/src/util/actor.js b/src/util/actor.js index be9200a054a..920b6b5d552 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -151,6 +151,7 @@ class Actor { type: '', sourceMapId: this.mapId, error: err ? serialize(err) : null, + mustQueue: task.mustQueue, data: serialize(data, buffers) }, buffers); } : (_) => { From e7aa5dc67eb79b61bb4e8179a4a67eb97e0efdef Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Tue, 9 Mar 2021 09:05:34 +0200 Subject: [PATCH 8/9] revert only queueing on the worker --- src/source/worker_tile.js | 6 +++--- src/util/actor.js | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/source/worker_tile.js b/src/source/worker_tile.js index a35979de578..9356cf11076 100644 --- a/src/source/worker_tile.js +++ b/src/source/worker_tile.js @@ -169,7 +169,7 @@ class WorkerTile { glyphMap = result; maybePrepare.call(this); } - }, undefined, true, taskMetadata); + }, undefined, false, taskMetadata); } else { glyphMap = {}; } @@ -182,7 +182,7 @@ class WorkerTile { iconMap = result; maybePrepare.call(this); } - }, undefined, true, taskMetadata); + }, undefined, false, taskMetadata); } else { iconMap = {}; } @@ -195,7 +195,7 @@ class WorkerTile { patternMap = result; maybePrepare.call(this); } - }, undefined, true, taskMetadata); + }, undefined, false, taskMetadata); } else { patternMap = {}; } diff --git a/src/util/actor.js b/src/util/actor.js index 920b6b5d552..cdf9f6b7ed9 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -108,7 +108,7 @@ class Actor { cancel.cancel(); } } else { - if (isWorker() && data.mustQueue) { + if (data.mustQueue) { // for worker tasks that are often cancelled, such as loadTile, store them before actually // processing them. This is necessary because we want to keep receiving messages. // Some tasks may take a while in the worker thread, so before executing the next task @@ -151,7 +151,6 @@ class Actor { type: '', sourceMapId: this.mapId, error: err ? serialize(err) : null, - mustQueue: task.mustQueue, data: serialize(data, buffers) }, buffers); } : (_) => { From 32c29c5ad370ccd003b45c2145af1e50a0864e40 Mon Sep 17 00:00:00 2001 From: Ansis Brammanis Date: Thu, 11 Mar 2021 16:25:00 -0500 Subject: [PATCH 9/9] let Scheduler decide which tasks should be immediate --- src/util/actor.js | 6 +----- src/util/scheduler.js | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/util/actor.js b/src/util/actor.js index cdf9f6b7ed9..dce203ab9cd 100644 --- a/src/util/actor.js +++ b/src/util/actor.js @@ -4,7 +4,6 @@ import {bindAll, isWorker, isSafari} from './util.js'; import window from './window.js'; import {serialize, deserialize} from './web_worker_transfer.js'; import Scheduler from './scheduler.js'; -import {PerformanceUtils} from './performance.js'; import type {Transferable} from '../types/transferable.js'; import type {Cancelable} from '../types/cancelable.js'; @@ -108,7 +107,7 @@ class Actor { cancel.cancel(); } } else { - if (data.mustQueue) { + if (data.mustQueue || isWorker()) { // for worker tasks that are often cancelled, such as loadTile, store them before actually // processing them. This is necessary because we want to keep receiving messages. // Some tasks may take a while in the worker thread, so before executing the next task @@ -120,10 +119,7 @@ class Actor { } else { // In the main thread, process messages immediately so that other work does not slip in // between getting partial data back from workers. - // Do the same for worker tasks that need processing as soon as possible. - const m = isWorker() ? PerformanceUtils.beginMeasure('workerTask') : undefined; this.processTask(id, data); - if (m) PerformanceUtils.endMeasure(m); } } } diff --git a/src/util/scheduler.js b/src/util/scheduler.js index 58e200a8443..343eb705e5a 100644 --- a/src/util/scheduler.js +++ b/src/util/scheduler.js @@ -22,7 +22,22 @@ class Scheduler { add(fn: () => void, metadata: Object) { const id = this.nextId++; - this.tasks[id] = {fn, metadata, priority: getPriority(metadata), id}; + const priority = getPriority(metadata); + + if (priority === 0) { + // Process tasks with priority 0 immediately. Do not yield to the event loop. + const m = isWorker() ? PerformanceUtils.beginMeasure('workerTask') : undefined; + try { + fn(); + } finally { + if (m) PerformanceUtils.endMeasure(m); + } + return { + cancel: () => {} + }; + } + + this.tasks[id] = {fn, metadata, priority, id}; this.taskQueue.push(id); this.invoker.trigger(); return {