Simplify RequestPerformance class and dedupe vector tile measurements

mapbox · Jan 29, 2021 · 9ac1e59 · 9ac1e59
1 parent 075d29a
commit 9ac1e59
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 134 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -237,75 +237,21 @@ 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',
         uid: 0,
         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();
-    });
-});