From f3db97bea80bd05f1943634a4b46890a9ae07b7a Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Fri, 22 Jan 2021 14:41:24 -0800 Subject: [PATCH 1/6] Dedupe calls to performance.measure --- src/source/vector_tile_worker_source.js | 3 ++- src/util/performance.js | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/source/vector_tile_worker_source.js b/src/source/vector_tile_worker_source.js index fef1781e27d..42002928b87 100644 --- a/src/source/vector_tile_worker_source.js +++ b/src/source/vector_tile_worker_source.js @@ -198,7 +198,8 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { if (response.cacheControl) cacheControl.cacheControl = response.cacheControl; const resourceTiming = {}; - if (perf) { + // because of two-phase tile loading, it's necessary to dedupe perf marking to avoid errors + if (perf && !perf._marksWereCleared()) { const resourceTimingData = perf.finish(); // it's necessary to eval the result of getEntriesByName() here via parse/stringify // late evaluation in the main thread causes TypeError: illegal invocation diff --git a/src/util/performance.js b/src/util/performance.js index d12ed992ccd..8b4f5de00b6 100644 --- a/src/util/performance.js +++ b/src/util/performance.js @@ -178,6 +178,10 @@ export class RequestPerformance { performance.mark(this._marks.start); } + + _marksWereCleared() { + return performance.getEntriesByName(this._marks.start).length === 0; + } finish() { performance.mark(this._marks.end); From 075d29adfa3fad8effebd65a503cdd0cc40826cb Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Tue, 26 Jan 2021 12:44:00 -0800 Subject: [PATCH 2/6] Fix lint --- src/util/performance.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/performance.js b/src/util/performance.js index 8b4f5de00b6..6c33be77897 100644 --- a/src/util/performance.js +++ b/src/util/performance.js @@ -178,7 +178,7 @@ export class RequestPerformance { performance.mark(this._marks.start); } - + _marksWereCleared() { return performance.getEntriesByName(this._marks.start).length === 0; } From 9ac1e59d569f8770634aa85f469a2d4360c428e9 Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Thu, 28 Jan 2021 16:16:24 -0800 Subject: [PATCH 3/6] Simplify RequestPerformance class and dedupe vector tile measurements --- src/source/geojson_worker_source.js | 2 +- src/source/vector_tile_worker_source.js | 35 ++++++---- src/util/performance.js | 32 ++------- .../unit/source/geojson_worker_source.test.js | 32 --------- .../source/vector_tile_worker_source.test.js | 66 ++----------------- 5 files changed, 33 insertions(+), 134 deletions(-) diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js index 9b1407a691e..f5f73a7d417 100644 --- a/src/source/geojson_worker_source.js +++ b/src/source/geojson_worker_source.js @@ -196,7 +196,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { const result = {}; if (perf) { - const resourceTimingData = perf.finish(); + const resourceTimingData = perf.getMeasurement(); // it's necessary to eval the result of getEntriesByName() here via parse/stringify // late evaluation in the main thread causes TypeError: illegal invocation if (resourceTimingData) { diff --git a/src/source/vector_tile_worker_source.js b/src/source/vector_tile_worker_source.js index 42002928b87..1f5c5d6de16 100644 --- a/src/source/vector_tile_worker_source.js +++ b/src/source/vector_tile_worker_source.js @@ -44,9 +44,25 @@ export type LoadVectorData = (params: RequestedTileParameters, callback: LoadVec export class DedupedRequest { entries: { [string]: Object }; scheduler: ?Scheduler; + _perf: RequestPerformance | boolean; + _resourceTiming: { [string]: Object }; + constructor(scheduler?: Scheduler) { this.entries = {}; this.scheduler = scheduler; + this._resourceTiming = {}; + } + + _measurePerformance() { + if (this._perf && this._perf instanceof RequestPerformance) { + const resourceTimingData = this._perf.getMeasurement(); + // console.log('resourceTimingData: ', resourceTimingData); + // it's necessary to eval the result of getEntriesByName() here via parse/stringify + // late evaluation in the main thread causes TypeError: illegal invocation + if (resourceTimingData.length > 0) { + this._resourceTiming.resourceTiming = JSON.parse(JSON.stringify(resourceTimingData)); + } + } } request(key: string, metadata: Object, request: any, callback: LoadVectorDataCallback) { @@ -61,6 +77,7 @@ export class DedupedRequest { } else { callback(err, result); } + this._measurePerformance(); return () => {}; } @@ -77,6 +94,7 @@ export class DedupedRequest { } else { cb(err, result); } + this._measurePerformance(); } setTimeout(() => delete this.entries[key], 1000 * 3); }); @@ -176,8 +194,9 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { loadTile(params: WorkerTileParameters, callback: WorkerTileCallback) { const uid = params.uid; - const perf = (params && params.request && params.request.collectResourceTiming) ? - new RequestPerformance(params.request) : false; + const requestParam = params && params.request; + this.deduped._perf = (requestParam && requestParam.collectResourceTiming) ? + new RequestPerformance(requestParam) : false; const workerTile = this.loading[uid] = new WorkerTile(params); workerTile.abort = this.loadVectorData(params, (err, response) => { @@ -197,16 +216,6 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { if (response.expires) cacheControl.expires = response.expires; if (response.cacheControl) cacheControl.cacheControl = response.cacheControl; - const resourceTiming = {}; - // because of two-phase tile loading, it's necessary to dedupe perf marking to avoid errors - if (perf && !perf._marksWereCleared()) { - const resourceTimingData = perf.finish(); - // it's necessary to eval the result of getEntriesByName() here via parse/stringify - // late evaluation in the main thread causes TypeError: illegal invocation - if (resourceTimingData) - resourceTiming.resourceTiming = JSON.parse(JSON.stringify(resourceTimingData)); - } - // response.vectorTile will be present in the GeoJSON worker case (which inherits from this class) // because we stub the vector tile interface around JSON data instead of parsing it directly workerTile.vectorTile = response.vectorTile || new vt.VectorTile(new Protobuf(rawTileData)); @@ -215,7 +224,7 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { if (err || !result) return callback(err); // Transferring a copy of rawTileData because the worker needs to retain its copy. - callback(null, extend({rawTileData: rawTileData.slice(0)}, result, cacheControl, resourceTiming)); + callback(null, extend({rawTileData: rawTileData.slice(0)}, result, cacheControl, this.deduped._resourceTiming)); }); }; diff --git a/src/util/performance.js b/src/util/performance.js index 6c33be77897..a99e8fab7e9 100644 --- a/src/util/performance.js +++ b/src/util/performance.js @@ -167,38 +167,14 @@ export const PerformanceUtils = { * @private */ export class RequestPerformance { - _marks: {start: string, end: string, measure: string}; + _url: string; constructor (request: RequestParameters) { - this._marks = { - start: [request.url, 'start'].join('#'), - end: [request.url, 'end'].join('#'), - measure: request.url.toString() - }; - - performance.mark(this._marks.start); - } - - _marksWereCleared() { - return performance.getEntriesByName(this._marks.start).length === 0; + this._url = request.url.toString(); } - finish() { - performance.mark(this._marks.end); - let resourceTimingData = performance.getEntriesByName(this._marks.measure); - - // fallback if web worker implementation of perf.getEntriesByName returns empty - if (resourceTimingData.length === 0) { - performance.measure(this._marks.measure, this._marks.start, this._marks.end); - resourceTimingData = performance.getEntriesByName(this._marks.measure); - - // cleanup - performance.clearMarks(this._marks.start); - performance.clearMarks(this._marks.end); - performance.clearMeasures(this._marks.measure); - } - - return resourceTimingData; + getMeasurement() { + return performance.getEntriesByName(this._url); } } diff --git a/test/unit/source/geojson_worker_source.test.js b/test/unit/source/geojson_worker_source.test.js index a4a93841f52..75a180d15c2 100644 --- a/test/unit/source/geojson_worker_source.test.js +++ b/test/unit/source/geojson_worker_source.test.js @@ -137,38 +137,6 @@ test('resourceTiming', (t) => { }); }); - t.test('loadData - url (resourceTiming fallback method)', (t) => { - const sampleMarks = [100, 350]; - const marks = {}; - const measures = {}; - t.stub(perf, 'getEntriesByName').callsFake((name) => { return measures[name] || []; }); - t.stub(perf, 'mark').callsFake((name) => { - marks[name] = sampleMarks.shift(); - return null; - }); - t.stub(perf, 'measure').callsFake((name, start, end) => { - measures[name] = measures[name] || []; - measures[name].push({ - duration: marks[end] - marks[start], - entryType: 'measure', - name, - startTime: marks[start] - }); - return null; - }); - t.stub(perf, 'clearMarks').callsFake(() => { return null; }); - t.stub(perf, 'clearMeasures').callsFake(() => { return null; }); - - const layerIndex = new StyleLayerIndex(layers); - const source = new GeoJSONWorkerSource(actor, layerIndex, [], true, (params, callback) => { return callback(null, geoJson); }); - - source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}}, (err, result) => { - t.equal(err, null); - t.deepEquals(result.resourceTiming.testSource, [{"duration": 250, "entryType": "measure", "name": "http://localhost/nonexistent", "startTime": 100}], 'got expected resource timing'); - t.end(); - }); - }); - t.test('loadData - data', (t) => { const layerIndex = new StyleLayerIndex(layers); const source = new GeoJSONWorkerSource(actor, layerIndex, [], true); diff --git a/test/unit/source/vector_tile_worker_source.test.js b/test/unit/source/vector_tile_worker_source.test.js index 20a0224bf92..0f6ee2584f5 100644 --- a/test/unit/source/vector_tile_worker_source.test.js +++ b/test/unit/source/vector_tile_worker_source.test.js @@ -237,7 +237,9 @@ test('VectorTileWorkerSource provides resource timing information', (t) => { const source = new VectorTileWorkerSource(actor, layerIndex, [], true, loadVectorData); - t.stub(perf, 'getEntriesByName').callsFake(() => { return [ exampleResourceTiming ]; }); + t.stub(perf, 'getEntriesByName').callsFake(() => { + return [ exampleResourceTiming ]; + }); source.loadTile({ source: 'source', @@ -245,67 +247,11 @@ test('VectorTileWorkerSource provides resource timing information', (t) => { tileID: {overscaledZ: 0, wrap: 0, canonical: {x: 0, y: 0, z: 0, w: 0}}, request: {url: 'http://localhost:2900/faketile.pbf', collectResourceTiming: true} }, (err, res) => { + // simulate a deduped request calling the perf measurement + source.deduped._measurePerformance(); + res.resourceTiming = source.deduped._resourceTiming.resourceTiming; t.false(err); t.deepEquals(res.resourceTiming[0], exampleResourceTiming, 'resourceTiming resp is expected'); t.end(); }); }); - -test('VectorTileWorkerSource provides resource timing information (fallback method)', (t) => { - const rawTileData = fs.readFileSync(path.join(__dirname, '/../../fixtures/mbsv5-6-18-23.vector.pbf')); - - function loadVectorData(params, callback) { - return callback(null, { - vectorTile: new vt.VectorTile(new Protobuf(rawTileData)), - rawData: rawTileData, - cacheControl: null, - expires: null - }); - } - - const layerIndex = new StyleLayerIndex([{ - id: 'test', - source: 'source', - 'source-layer': 'test', - type: 'fill' - }]); - - const source = new VectorTileWorkerSource(actor, layerIndex, [], true, loadVectorData); - const url = 'http://localhost:2900/faketile.pbf'; - - const sampleMarks = {}; - sampleMarks[`${url}#start`] = 100; - sampleMarks[`${url}#end`] = 350; - const marks = {}; - const measures = {}; - t.stub(perf, 'getEntriesByName').callsFake((name) => { return measures[name] || []; }); - t.stub(perf, 'mark').callsFake((name) => { - if (sampleMarks[name]) { - marks[name] = sampleMarks[name]; - } - return null; - }); - t.stub(perf, 'measure').callsFake((name, start, end) => { - measures[name] = measures[name] || []; - measures[name].push({ - duration: marks[end] - marks[start], - entryType: 'measure', - name, - startTime: marks[start] - }); - return null; - }); - t.stub(perf, 'clearMarks').callsFake(() => { return null; }); - t.stub(perf, 'clearMeasures').callsFake(() => { return null; }); - - source.loadTile({ - source: 'source', - uid: 0, - tileID: {overscaledZ: 0, wrap: 0, canonical: {x: 0, y: 0, z: 0, w: 0}}, - request: {url, collectResourceTiming: true} - }, (err, res) => { - t.false(err); - t.deepEquals(res.resourceTiming[0], {"duration": 250, "entryType": "measure", "name": "http://localhost:2900/faketile.pbf", "startTime": 100}, 'resourceTiming resp is expected'); - t.end(); - }); -}); From 6fd5923bc42770b134c87c58cc160dd01090137b Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Thu, 28 Jan 2021 16:28:44 -0800 Subject: [PATCH 4/6] Revert whitespace change --- test/unit/source/vector_tile_worker_source.test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/unit/source/vector_tile_worker_source.test.js b/test/unit/source/vector_tile_worker_source.test.js index 0f6ee2584f5..ce58df4c3ed 100644 --- a/test/unit/source/vector_tile_worker_source.test.js +++ b/test/unit/source/vector_tile_worker_source.test.js @@ -237,9 +237,7 @@ test('VectorTileWorkerSource provides resource timing information', (t) => { const source = new VectorTileWorkerSource(actor, layerIndex, [], true, loadVectorData); - t.stub(perf, 'getEntriesByName').callsFake(() => { - return [ exampleResourceTiming ]; - }); + t.stub(perf, 'getEntriesByName').callsFake(() => { return [ exampleResourceTiming ]; }); source.loadTile({ source: 'source', From 2539c3d7776a5049bef4a874b1cf8c083aa27a7d Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Fri, 29 Jan 2021 10:30:26 -0800 Subject: [PATCH 5/6] Remove log statement --- src/source/vector_tile_worker_source.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/source/vector_tile_worker_source.js b/src/source/vector_tile_worker_source.js index 1f5c5d6de16..c194a2cbdac 100644 --- a/src/source/vector_tile_worker_source.js +++ b/src/source/vector_tile_worker_source.js @@ -56,7 +56,6 @@ export class DedupedRequest { _measurePerformance() { if (this._perf && this._perf instanceof RequestPerformance) { const resourceTimingData = this._perf.getMeasurement(); - // console.log('resourceTimingData: ', resourceTimingData); // it's necessary to eval the result of getEntriesByName() here via parse/stringify // late evaluation in the main thread causes TypeError: illegal invocation if (resourceTimingData.length > 0) { From 31387ed9718d8d072636b946dce2a27e0ad06a76 Mon Sep 17 00:00:00 2001 From: Ryan Hamley Date: Wed, 10 Feb 2021 16:42:51 -0800 Subject: [PATCH 6/6] Simplify code --- src/source/geojson_worker_source.js | 8 ++--- src/source/vector_tile_worker_source.js | 35 +++++++------------ src/util/performance.js | 19 ++-------- .../source/vector_tile_worker_source.test.js | 3 -- 4 files changed, 20 insertions(+), 45 deletions(-) diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js index f5f73a7d417..420802a6ac5 100644 --- a/src/source/geojson_worker_source.js +++ b/src/source/geojson_worker_source.js @@ -2,7 +2,7 @@ import {getJSON} from '../util/ajax.js'; -import {RequestPerformance} from '../util/performance.js'; +import {getPerformanceMeasurement} from '../util/performance.js'; import rewind from '@mapbox/geojson-rewind'; import GeoJSONWrapper from './geojson_wrapper.js'; import vtpbf from 'vt-pbf'; @@ -164,8 +164,8 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { delete this._pendingCallback; delete this._pendingLoadDataParams; - const perf = (params && params.request && params.request.collectResourceTiming) ? - new RequestPerformance(params.request) : false; + const requestParam = params && params.request; + const perf = requestParam && requestParam.collectResourceTiming; this.loadGeoJSON(params, (err: ?Error, data: ?Object) => { if (err || !data) { @@ -196,7 +196,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { const result = {}; if (perf) { - const resourceTimingData = perf.getMeasurement(); + const resourceTimingData = getPerformanceMeasurement(requestParam); // it's necessary to eval the result of getEntriesByName() here via parse/stringify // late evaluation in the main thread causes TypeError: illegal invocation if (resourceTimingData) { diff --git a/src/source/vector_tile_worker_source.js b/src/source/vector_tile_worker_source.js index c194a2cbdac..a7caffac3b7 100644 --- a/src/source/vector_tile_worker_source.js +++ b/src/source/vector_tile_worker_source.js @@ -6,7 +6,7 @@ import vt from '@mapbox/vector-tile'; import Protobuf from 'pbf'; import WorkerTile from './worker_tile.js'; import {extend} from '../util/util.js'; -import {RequestPerformance} from '../util/performance.js'; +import {getPerformanceMeasurement} from '../util/performance.js'; import {Evented} from '../util/evented.js'; import type { @@ -40,28 +40,13 @@ export type LoadVectorDataCallback = Callback; export type AbortVectorData = () => void; export type LoadVectorData = (params: RequestedTileParameters, callback: LoadVectorDataCallback) => ?AbortVectorData; - export class DedupedRequest { entries: { [string]: Object }; scheduler: ?Scheduler; - _perf: RequestPerformance | boolean; - _resourceTiming: { [string]: Object }; constructor(scheduler?: Scheduler) { this.entries = {}; this.scheduler = scheduler; - this._resourceTiming = {}; - } - - _measurePerformance() { - if (this._perf && this._perf instanceof RequestPerformance) { - const resourceTimingData = this._perf.getMeasurement(); - // it's necessary to eval the result of getEntriesByName() here via parse/stringify - // late evaluation in the main thread causes TypeError: illegal invocation - if (resourceTimingData.length > 0) { - this._resourceTiming.resourceTiming = JSON.parse(JSON.stringify(resourceTimingData)); - } - } } request(key: string, metadata: Object, request: any, callback: LoadVectorDataCallback) { @@ -76,7 +61,6 @@ export class DedupedRequest { } else { callback(err, result); } - this._measurePerformance(); return () => {}; } @@ -93,7 +77,6 @@ export class DedupedRequest { } else { cb(err, result); } - this._measurePerformance(); } setTimeout(() => delete this.entries[key], 1000 * 3); }); @@ -194,8 +177,7 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { const uid = params.uid; const requestParam = params && params.request; - this.deduped._perf = (requestParam && requestParam.collectResourceTiming) ? - new RequestPerformance(requestParam) : false; + const perf = requestParam && requestParam.collectResourceTiming; const workerTile = this.loading[uid] = new WorkerTile(params); workerTile.abort = this.loadVectorData(params, (err, response) => { @@ -222,8 +204,17 @@ class VectorTileWorkerSource extends Evented implements WorkerSource { workerTile.parse(workerTile.vectorTile, this.layerIndex, this.availableImages, this.actor, (err, result) => { if (err || !result) return callback(err); - // Transferring a copy of rawTileData because the worker needs to retain its copy. - callback(null, extend({rawTileData: rawTileData.slice(0)}, result, cacheControl, this.deduped._resourceTiming)); + const resourceTiming = {}; + if (perf) { + // Transferring a copy of rawTileData because the worker needs to retain its copy. + const resourceTimingData = getPerformanceMeasurement(requestParam); + // it's necessary to eval the result of getEntriesByName() here via parse/stringify + // late evaluation in the main thread causes TypeError: illegal invocation + if (resourceTimingData.length > 0) { + resourceTiming.resourceTiming = JSON.parse(JSON.stringify(resourceTimingData)); + } + } + callback(null, extend({rawTileData: rawTileData.slice(0)}, result, cacheControl, resourceTiming)); }); }; diff --git a/src/util/performance.js b/src/util/performance.js index a99e8fab7e9..6e43a69f3e7 100644 --- a/src/util/performance.js +++ b/src/util/performance.js @@ -160,22 +160,9 @@ export const PerformanceUtils = { } }; -/** - * Safe wrapper for the performance resource timing API in web workers with graceful degradation - * - * @param {RequestParameters} request - * @private - */ -export class RequestPerformance { - _url: string; - - constructor (request: RequestParameters) { - this._url = request.url.toString(); - } - - getMeasurement() { - return performance.getEntriesByName(this._url); - } +export function getPerformanceMeasurement(request: ?RequestParameters) { + const url = request ? request.url.toString() : undefined; + return performance.getEntriesByName(url); } export default performance; diff --git a/test/unit/source/vector_tile_worker_source.test.js b/test/unit/source/vector_tile_worker_source.test.js index ce58df4c3ed..e10230a2b82 100644 --- a/test/unit/source/vector_tile_worker_source.test.js +++ b/test/unit/source/vector_tile_worker_source.test.js @@ -245,9 +245,6 @@ test('VectorTileWorkerSource provides resource timing information', (t) => { tileID: {overscaledZ: 0, wrap: 0, canonical: {x: 0, y: 0, z: 0, w: 0}}, request: {url: 'http://localhost:2900/faketile.pbf', collectResourceTiming: true} }, (err, res) => { - // simulate a deduped request calling the perf measurement - source.deduped._measurePerformance(); - res.resourceTiming = source.deduped._resourceTiming.resourceTiming; t.false(err); t.deepEquals(res.resourceTiming[0], exampleResourceTiming, 'resourceTiming resp is expected'); t.end();