Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dedupe calls to performance.measure #10321

Merged
merged 6 commits into from
Feb 11, 2021
Merged

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Jan 22, 2021

Launch Checklist

  • briefly describe the changes in this PR
    • This fixes Setting map option collectResourceTiming to true results in console errors #10303
    • I believe the error was always possible to hit but used to be very rare. With the introduction of two-phased tile loading in v2.0.0, the problem became much more prominent. Two-phased tile loading leads to two calls to loadTile and ultimately to perf.finish(). On the first call to perf.finish(), the entries for this._marks.start are cleared from the performance entry buffer. On the second call to perf.finish(), performance.measure() tries to use this._marks.start as the start mark but can't find entries in the performance buffer and throws an error.
    • This PR removes the fallback performance measurements because the Performance API is now supported in all GL JS supported browsers. It also moves the performance measurement for vector tiles into the DedupedRequest class so that the measurement isn't dependent on this.scheduler for timing.
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Prevent collectResourceTiming from causing errors when loading vector tiles</changelog>

@ryanhamley ryanhamley self-assigned this Jan 22, 2021
@ansis
Copy link
Contributor

ansis commented Jan 22, 2021

I'm seeing a different problem that was introduced in v2.0.0.

perf.finish() only gets called in the callback that only gets called when the scheduler decides to call it:

const resourceTimingData = perf.finish();

this.scheduler.add(() => {
callback(err, result);
}, metadata);

This means it might be delayed significantly for some request types.

Fixing this might involve moving the perf = new RequestPerformance(...) and perf.finish() into DedupedRequest. I think this might also avoid the underlying issue in this pr.

But I'm also not fully sure how this specific metric is used so we should look at that more carefully at some point.

@ryanhamley ryanhamley force-pushed the dedupe-performance-measurements branch from 84b47b9 to 047e383 Compare January 26, 2021 20:44
@ryanhamley ryanhamley force-pushed the dedupe-performance-measurements branch from 047e383 to 9ac1e59 Compare January 29, 2021 00:17
@ryanhamley ryanhamley requested a review from ansis January 29, 2021 00:21
@@ -214,7 +223,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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get rid of the RequestPerformance class and replace it with a method that does what getMeasurement() does?

Both here and in geojson_worker_source.js we could do something like this:

_resourceTiming: getResourceTiming(params.request)

and remove the:

  • new RequestPerformance()
  • the stuff in DedupedRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep 👍 much simpler

@ryanhamley ryanhamley merged commit 7ff2838 into main Feb 11, 2021
@ryanhamley ryanhamley deleted the dedupe-performance-measurements branch February 11, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting map option collectResourceTiming to true results in console errors
2 participants