-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc: timings script #9723
misc: timings script #9723
Conversation
lighthouse-core/scripts/timings.js
Outdated
.array('urls') | ||
.string('lh-flags') | ||
.default('lh-flags', '') | ||
// Why is the printing for examples so awful? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still on ancient yargs so I'm sure it's been fixed in a version released in the past 5 years :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome script :)
lighthouse-core/scripts/timings.js
Outdated
.array('urls') | ||
.string('lh-flags') | ||
.default('lh-flags', '') | ||
// Why is the printing for examples so awful? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still on ancient yargs so I'm sure it's been fixed in a version released in the past 5 years :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really handy. Do we want a more descriptive name for the script? I don't have a good suggestion, though
lighthouse-core/scripts/timings.js
Outdated
/** @type {LH.Result} */ | ||
const lhr = JSON.parse(lhrJson); | ||
|
||
for (const measureName of lhr.timing.entries.map(entry => entry.name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it might make sense to s/measure/timing for a lot of these names, to avoid splitting the nomenclature from timing
/Execution timings
/timingEntries
we use for the values in all of the core files. Not a huge deal, but it does make it harder to remember in this file that there are accumulations of lhr.timing
entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gave it a shot
|
SGTM |
function summarize() { | ||
// `${url}@@@${entry.name}` -> duration | ||
/** @type {Map<string, number[]>} */ | ||
const durationsMap = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is better. Now we have timing
, measure
, and duration
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ⏱📊
For example usages, see #9712 and #9713