-
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
core(estimated-input-latency): use a 5s rolling window #4989
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,8 @@ const Util = require('../report/v2/renderer/util'); | |
const TracingProcessor = require('../lib/traces/tracing-processor'); | ||
const LHError = require('../lib/errors'); | ||
|
||
const ROLLING_WINDOW_SIZE = 5000; | ||
|
||
class EstimatedInputLatency extends Audit { | ||
/** | ||
* @return {!AuditMeta} | ||
|
@@ -19,8 +21,7 @@ class EstimatedInputLatency extends Audit { | |
name: 'estimated-input-latency', | ||
description: 'Estimated Input Latency', | ||
helpText: 'The score above is an estimate of how long your app takes to respond to user ' + | ||
'input, in milliseconds. There is a 90% probability that a user encounters this amount ' + | ||
'of latency, or less. 10% of the time a user can expect additional latency. If your ' + | ||
'input, in milliseconds, during the busiest 5s window of page load. If your ' + | ||
'latency is higher than 50 ms, users may perceive your app as laggy. ' + | ||
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/estimated-input-latency).', | ||
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC, | ||
|
@@ -45,23 +46,36 @@ class EstimatedInputLatency extends Audit { | |
throw new LHError(LHError.errors.NO_FMP); | ||
} | ||
|
||
const latencyPercentiles = TracingProcessor.getRiskToResponsiveness(tabTrace, startTime); | ||
const ninetieth = latencyPercentiles.find(result => result.percentile === 0.9); | ||
const rawValue = parseFloat(ninetieth.time.toFixed(1)); | ||
const events = TracingProcessor.getMainThreadTopLevelEvents(tabTrace, startTime) | ||
.filter(evt => evt.duration >= 1); | ||
|
||
const candidateStartEvts = events.filter(evt => evt.duration >= 10); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly, can we raise 10 to something like 50? Seems like we'd lose precision in the "you're good" area of scores, but that doesn't seem too bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah sgtm 👍 much faster too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't that make the max score the PODR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, only filtering the start event times |
||
|
||
let worst90thPercentileLatency = 16; | ||
for (const startEvt of candidateStartEvts) { | ||
const latencyPercentiles = TracingProcessor.getRiskToResponsiveness( | ||
events, | ||
startEvt.start, | ||
startEvt.start + ROLLING_WINDOW_SIZE, | ||
[0.9] | ||
); | ||
|
||
worst90thPercentileLatency = Math.max( | ||
latencyPercentiles[0].time, | ||
worst90thPercentileLatency | ||
); | ||
} | ||
|
||
const score = Audit.computeLogNormalScore( | ||
ninetieth.time, | ||
worst90thPercentileLatency, | ||
context.options.scorePODR, | ||
context.options.scoreMedian | ||
); | ||
|
||
return { | ||
score, | ||
rawValue, | ||
displayValue: Util.formatMilliseconds(rawValue, 1), | ||
extendedInfo: { | ||
value: latencyPercentiles, | ||
}, | ||
rawValue: worst90thPercentileLatency, | ||
displayValue: Util.formatMilliseconds(worst90thPercentileLatency, 1), | ||
}; | ||
} | ||
|
||
|
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.
since a blank trace gets a value of 16, can't we safely remove any events with a duration < 16?
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.
well a trace with constant 16ms events would technically get a EQT of 24ms since we arbitrarily add 16ms to whatever value we compute, I'm all for doing more filtering here though, who cares about your small events