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

Few fixes and additional API's for performance metrics instrumentation #9179

Closed
wants to merge 14 commits into from

Conversation

arindam1993
Copy link
Contributor

This is a follow up of #9035, with a few fixes and some additional API methods that help improve the reliability of collected metrics.
Primarily this adds 3 new methods:
map.style.disableFetchCancellation()/map.style.enableFetchCancellation(): This lets the test-runner disable request cancellation for a cache-prewarm run, which is run prior to the actual measurement runs. This ensures that all possible tiles that are required for the run are precached during the actual measurement run.

map.style.getNumInflightRequests(): This is used by the test-runner to wait until all in-flight requests are finished for a cache-prewarm run, again ensuring maximum cache-warmth. 🔥

Launch Checklist

  • briefly describe the changes in this PR

window.fetch(request).then(response => {
_inflightRequestCount = Math.max(_inflightRequestCount - 1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the max there to handle double-decrements in case both an error handler and .cancel() are called? Would doing something id-based be more reliable?

const reqId = nextId++;
activeRequests[reqId] = true;
delete activeRequests[reqId];
infLightRequests = Object.keys(activeRequests).length;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is correct! I thought about that too, but I was trying to keep the instrumentation as low runtime overhead as possible

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

Is this waiting on anything besides the render test?

Comment on lines 367 to 370
* Apply queued style updates in a batch and recalculate zoom-dependent paint properties.
*
* @param parameters
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an automatic linting change but the comment is misaligned now.

@ryanhamley ryanhamley force-pushed the fix-intrumentation branch from 23a5ab4 to fdb251f Compare March 7, 2020 00:41
@arindam1993
Copy link
Contributor Author

Is this waiting on anything besides the render test?
Nope nothing else and I have still no idea why adding an extra function is breaking esm :(

@asheemmamoowala asheemmamoowala marked this pull request as draft June 17, 2020 18:00
@arindam1993 arindam1993 changed the base branch from master to main June 18, 2020 18:13
@ryanhamley
Copy link
Contributor

Do we still need this @arindam1993 ?

@mourner
Copy link
Member

mourner commented Dec 10, 2020

Looks like this is stale now — let's revisit later if it's still relevant.

@mourner mourner closed this Dec 10, 2020
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.

4 participants