core(responsiveness): use new EventTiming trace event format #13979
Conversation
brendankenny
force-pushed
the
responsiveness-interaction
branch
from
296005e
to
da1b4c9
Compare
| trace, | ||
| settings: {throttlingMethod: 'provided'}, | ||
| }; | ||
| expect(Responsiveness.request(metricInputData, {computedCache: new Map()})) |
There was a problem hiding this comment.
| expect(Responsiveness.request(metricInputData, {computedCache: new Map()})) | |
| await expect(Responsiveness.request(metricInputData, {computedCache: new Map()})) |
- other calls like this in this file.
In 9.6.0 so far :) There are a couple more changes coming down the line. |
| * source. | ||
| * Note that (presumably due to rounding to ms), the interaction duration may not | ||
| * be the same value as `maxDuration`, just the closest value. Function will throw | ||
| * if the closest match is off by more than 4ms. |
There was a problem hiding this comment.
nit: instead of throw, maybe return null? would force caller to be sure to handle it
or is there a reason you'd prefer this computed artifact to error when no interaction is present?
There was a problem hiding this comment.
compute_ already returns a nullable value, so might as well make this function do that too?
There was a problem hiding this comment.
This shouldn't be a thing that happens, even the 4ms difference needs to be fixed upstream (something weird is happening with rounding before the trace events are emitted), so I'm ok calling it a fatal case.
FWIW I've never hit a case of 5ms, so I don't think this should happen in practice except in actually exceptional cases (like maybe an interaction right as the trace is starting or stopping and so the events are partially missing)
| }); | ||
|
|
||
| if (candidates.length) { | ||
| if (!candidates[0].args.data.frame) { |
| keyboard: KEYBOARD_EVENTS, | ||
| tapOrClick: CLICK_TAP_DRAG_EVENTS, | ||
| drag: CLICK_TAP_DRAG_EVENTS, | ||
| }; |
There was a problem hiding this comment.
Map?
It's probably better but in practice doesn't make any difference since it's not dynamic and the declaration is a lot uglier :)
| } | ||
| // TODO: seems to regularly happen up to 3ms and as high as 4. | ||
| if (minDurationDiff > 5) { | ||
| throw new Error(`no interaction event found within 5ms of responsiveness maxDuration (max: ${maxDuration}, closest ${bestMatchEvent.args.data.duration})`); // eslint-disable-line max-len |
There was a problem hiding this comment.
this is 5, but above in jsdoc "4ms" is mentioned as the threshold
There was a problem hiding this comment.
this is 5, but above in jsdoc "4ms" is mentioned as the threshold
yeah, it was to be really extra sure but I'll clarify, thx
|
On reflection, these event.dur changes are too invasive. I'll revert and put the TODO on the |
| "displayValue": "60 ms" | ||
| "score": null, | ||
| "scoreDisplayMode": "error", | ||
| "errorMessage": "This version of Chrome is too old to support 'detailed EventTiming trace events'. Use a newer version to see full results." |
There was a problem hiding this comment.
Could you rebaseline the flow artifacts? Should be able to use --rebaseline-artifacts Trace but there is no flag to modify a specific flow step yet.
There was a problem hiding this comment.
Could you rebaseline the flow artifacts? Should be able to use
--rebaseline-artifacts Tracebut there is no flag to modify a specific flow step yet.
TypeError: Cannot set property startTime of #<PerformanceEntry> which has only a getter at Object.normalizeTimingEntries (~/lighthouse/lighthouse-core/lib/asset-saver.js:324:22) at rebaselineArtifacts (~/lighthouse/lighthouse-core/scripts/update-flow-fixtures.js:114:16) at async ~/lighthouse/lighthouse-core/scripts/update-flow-fixtures.js:157:7
I'll add it to the TODO list in #13916 and we can fix/update in a follow up.
part of #13916
https://crrev.com/c/3632661 added a whole bunch of useful information to the
EventTimingtrace events, and this PR updates the responsiveness computed artifact to use them.The computed artifact now returns the interaction event because that's where all the truly useful timing is, and in follow up PRs it will allow the actionability audit and the
TraceElementsgatherer to both use that information without having to figure it out from theResponsivenessevent themselves.This also means the metric will use the timing that matches the interaction breakdown, which sometimes differs by up to 4ms from the
Responsiveness.Renderer.UserInteractionmaxDuration. Ideally we can fix this difference upstream so that the trace events always match in duration.Also updates
timespan-responsiveness-m103.trace.jsonto get the latest trace event format.