feat(tti): increase robustness of perf metrics #1947
Conversation
- Adds two additional TTI candidates that don't rely on visual completeness. - First 500ms window after fMP where 90th percentile EIL is <50ms - First 5000ms window after fMP where 90th percentile EIL is <50ms (if at the end of trace, uses a minimum window of up to 1000ms) - Falls back to fMP when speedline fails to find screenshots in trace. - Adds timing data to trace-of-tab computed artifact.
| @@ -44,9 +44,9 @@ class EstimatedInputLatency extends Audit { | |||
| }; | |||
| } | |||
|
|
|||
| static calculate(speedline, model, trace) { | |||
| static calculate(tabTrace, model, trace) { | |||
| // Use speedline's first paint as start of range for input latency check. | |||
| estLatency: estLatency, | ||
| startTime: startTime.toFixed(1) | ||
| }); | ||
| const times = {fmpTiming, visuallyReadyTiming, endOfTraceTime}; |
There was a problem hiding this comment.
can you put a lastTraceEvtTs into trace-of-tab?
const traceEndTsInMS = trace.traceEvents.reduce(
(acc, traceEvent) => Math.max(acc, traceEvent.ts), 0) / 1000;it's from will's https://github.com/GoogleChrome/lighthouse/pull/1936/files#diff-fd99fc8068c6555d1fb3d1c70df7c29cR82
wec can then reuse here (and maybe drop the traceviewer dep) and also in will's #1936
There was a problem hiding this comment.
done, but naming traceEnd :) to match with event names :)
| @@ -37,8 +37,8 @@ describe('Performance: estimated-input-latency audit', () => { | |||
| const artifacts = generateArtifactsWithTrace({traceEvents: pwaTrace}); | |||
| return Audit.audit(artifacts).then(output => { | |||
| assert.equal(output.debugString, undefined); | |||
| assert.equal(output.rawValue, 17.4); | |||
| assert.equal(output.displayValue, '17.4ms'); | |||
| assert.equal(output.rawValue, 16.7); | |||
There was a problem hiding this comment.
to be clear, these changes are only due to the shift from speedline.first to FMP. yes?
| const timings = {}; | ||
|
|
||
| Object.keys(metrics).forEach(metric => { | ||
| timings[metric + 'Ts'] = metrics[metric] && metrics[metric].ts / 1000; |
There was a problem hiding this comment.
the structure of the extInfo objects in TTI/FMP includes both a timings object and a timestamps object. should we do the same here?
|
|
||
| Object.keys(metrics).forEach(metric => { | ||
| timings[metric + 'Ts'] = metrics[metric] && metrics[metric].ts / 1000; | ||
| timings[metric] = timings[metric + 'Ts'] - timings.navigationStartTs; |
There was a problem hiding this comment.
adding this guy in here means that if we pass along the data in extInfo then tools like pwmetrics could skip this work: https://github.com/paulirish/pwmetrics/blob/master/lib/metrics.ts#L54-L61
| */ | ||
| static findTTIAlpha(times, data) { | ||
| return TTIMetric._forwardWindowTTI( | ||
| Math.max(times.fmpTiming, times.visuallyReadyTiming), |
There was a problem hiding this comment.
can you add a comment here to clarify you are using fmp as fallback?
| @@ -63,9 +63,6 @@ class TTIMetric extends Audit { | |||
| return {currentLatency, foundLatencies}; | |||
| } | |||
|
|
|||
| // Clip to the trace end | |||
| endTime = Math.min(endTime, maxTime); | |||
There was a problem hiding this comment.
hmm! why'd you nuke this?
fwiw: https://docs.google.com/document/d/1b9slyaB9yho91YTOkAQfpCdULFkZM9LqsipcX3t7He8/edit#heading=h.ooalxriljq1i has details on the clipping approach for EIL
There was a problem hiding this comment.
oh I added this when I did the 5000ms - 1000ms if we reached the end of trace for WPT TTI, but after our convo on Friday about how fake that is I nuked the variable window and just kept the 5 seconds so it wasn't necessary anymore
done
|
First round of perf metric investigation, to be judged using #1936 :) Follow-up work includes adding in network reverse search methods for comparison.