Skip to content

Expose Resource Timing API - #5991

Merged
anandthakker merged 19 commits into
mapbox:masterfrom
sbma44:expose-resource-timing-api
Jan 22, 2018
Merged

anandthakker merged 19 commits into
mapbox:masterfrom
sbma44:expose-resource-timing-api

Conversation

@sbma44

@sbma44 sbma44 commented Jan 12, 2018 •

Copy link
Copy Markdown
Member

Implements #5948. Much thanks to @anandthakker for his patient guidance!

Since the only source types that make network requests in workers are VT and GeoJSON, those are the only two source types with changes. Both now return resourceTiming properties on data events if the Map is instantiated with { collectResourceTiming: true }.

Because of differences between these sources, the location of the timing data differs between them. For VT workers, network requests are associated with individual tiles; for GeoJSON sources, they are associated with source addition and data update events. Consequently the location of the resourceTiming property varies depending on the source type -- it's on tiles for VT and is a top-level property for GeoJSON events.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs (JSDoc only right now)
  • manually test the debug page (done w/ custom local page with collectResourceTiming and a geoJSON source).
  • post benchmark scores

Benchmarks are running now but a looming ✈️ means I'll probably need to post the results next week. Because this feature is intended to be used primarily in diagnostic or development contexts and is disabled by default, I don't expect to see differences in the bench suite. Even when enabled, this code simply calls a browser API and stringifies a small amount of scalar data (and does so in a worker). If anyone feels that writing a bench test for this is appropriate, though, please let me know and I'll give it a shot.

@sbma44

sbma44 commented Jan 12, 2018

Copy link
Copy Markdown
Member Author

2018-01-12-15-33-localhost_9966

@anandthakker anandthakker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is mostly looking good. (I still wish there were a way for us to handle this in a more global/general manner, rather than threading this flag from Map down through individual Sources, but I don't see a good way to do that right now.)

Comment thread src/source/geojson_source.js Outdated
workerID: number;
_loaded: boolean;
_collectResourceTiming: boolean;
_resourceTimings: {[string]: Array<PerformanceResourceTiming>};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this.id is fixed, there's no need for a _resourceTimings map from id => result. Instead, we can change this field to _resourceTiming: Array<PerformanceResourceTiming> // resource timing data from the most recent request

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah ok. I had gotten the impression from the coalesce operation (and a superficial read of the #5988 issue title) that a single worker could wind up handling multiple sources. If that's not the case I agree that it should be simplified 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's true -- a single worker can handle multiple sources, but this here is the main-thread side (not the worker side) of the source

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, right you are. sorry to have missed that!

Comment thread src/source/geojson_source.js Outdated

const data = {};
data.dataType = 'source';
data.sourceDataType = 'metadata';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit:

const data = {
    dataType: 'source',
    sourceDataType: 'metadata'
}

(also below)

@sbma44 sbma44 Jan 17, 2018 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's actually what I wrote originally, but it didn't survive test-flow. I think this has to stay this way because of the subsequent potential assignment of data.resourceTiming. Flow complains about

const data = { x: 'y' };
data.i = 'j';

but not

const data = {};
data.x = 'y';
data.i = 'j';

The latter is a pattern I see in other parts of the codebase, so I emulated it. My knowledge of Flow is pretty rudimentary so if there's a better way to accommodate this case please lmk.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. Does the following avoid the flow error?

const data: MapDataType = {
    dataType: 'source',
    sourceDataType: 'metadata'
}

(you might have to import MapDataType from ui/events.js)

@sbma44

sbma44 commented Feb 14, 2018

Copy link
Copy Markdown
Member Author

Noting here: this behavior fails to return resource timing information in Firefox Quantum 58 due to https://bugzilla.mozilla.org/show_bug.cgi?id=1425458 . It looks like this will be resolved when Firefox Quantum 60 ships.

@sbma44 sbma44 mentioned this pull request Feb 14, 2018
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants