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.

Sign up for GitHub

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

Jump to bottom

core(mpfid): add max LoAFs to debugdata #15684

Merged
merged 2 commits into from
Dec 11, 2023
Merged

core(mpfid): add max LoAFs to debugdata #15684

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c86098
core(mpfid): add max LoAFs to debugdata
brendankenny Dec 11, 2023
1a7a86e
fix sample trace indent
brendankenny Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions core/audits/metrics/max-potential-fid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import {Audit} from '../audit.js';
import {MaxPotentialFID as ComputedFid} from '../../computed/metrics/max-potential-fid.js';
import {ProcessedTrace} from '../../computed/processed-trace.js';
import {ProcessedNavigation} from '../../computed/processed-navigation.js';
import * as i18n from '../../lib/i18n/i18n.js';

const UIStrings = {
Expand Down Expand Up @@ -48,6 +50,51 @@ class MaxPotentialFID extends Audit {
};
}

/**
* Extract potential LoAF replacements for MPFID from the trace to log in
* debugdata details.
* @param {LH.Artifacts.ProcessedTrace} processedTrace
* @param {LH.Artifacts.ProcessedNavigation} processedNavigation
* @return {{type: 'debugdata', observedMaxDurationLoaf: LH.TraceEvent, observedMaxBlockingLoaf: LH.TraceEvent}|undefined}
*/
static getLongAnimationFrameDetails(processedTrace, processedNavigation) {
const {firstContentfulPaint} = processedNavigation.timestamps;
const loafs = processedTrace.mainThreadEvents.filter(evt => {
return evt.name === 'LongAnimationFrame' &&
evt.ph === 'b' &&
evt.ts >= firstContentfulPaint;
});

let currentMaxDuration = -Infinity;
let currentMaxDurationLoaf;
let currentMaxBlocking = -Infinity;
let currentMaxBlockingLoaf;

for (const loaf of loafs) {
const loafDuration = loaf.args?.data?.duration;
const loafBlocking = loaf.args?.data?.blockingDuration;
// Should never happen, so mostly keeping the type checker happy.
if (loafDuration === undefined || loafBlocking === undefined) continue;

if (loafDuration > currentMaxDuration) {
currentMaxDuration = loafDuration;
currentMaxDurationLoaf = loaf;
}
if (loafBlocking > currentMaxBlocking) {
currentMaxBlocking = loafBlocking;
currentMaxBlockingLoaf = loaf;
}
}

if (!currentMaxDurationLoaf || !currentMaxBlockingLoaf) return;

return {
type: 'debugdata',
observedMaxDurationLoaf: currentMaxDurationLoaf,
observedMaxBlockingLoaf: currentMaxBlockingLoaf,
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
Expand All @@ -61,6 +108,11 @@ class MaxPotentialFID extends Audit {
settings: context.settings, URL: artifacts.URL};
const metricResult = await ComputedFid.request(metricComputationData, context);

const processedTrace = await ProcessedTrace.request(trace, context);
const processedNavigation = await ProcessedNavigation.request(trace, context);
const details = MaxPotentialFID.getLongAnimationFrameDetails(processedTrace,
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with sticking this in debug data. What's the reasoning for putting it in MPFID as opposed to TBT though?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the reasoning for putting it in MPFID as opposed to TBT though?

It made sense without thinking about it too much, but I don't think I can make a solid case for it. Happy to move it if it seems more logical there.

Copy link
Member

Choose a reason for hiding this comment

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

Meh I think it's fine. TBT is just where my brain went since that's an official metric.

processedNavigation);

return {
score: Audit.computeLogNormalScore(
{p10: context.options.p10, median: context.options.median},
Expand All @@ -69,6 +121,7 @@ class MaxPotentialFID extends Audit {
numericValue: metricResult.timing,
numericUnit: 'millisecond',
displayValue: str_(i18n.UIStrings.ms, {timeInMs: metricResult.timing}),
details,
};
}
}
Expand Down
2 changes: 2 additions & 0 deletions core/test/results/artifacts/defaultPass.trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12867,6 +12867,8 @@
{"args":{},"cat":"disabled-by-default-devtools.timeline","dur":6,"name":"RunTask","ph":"X","pid":31161,"tdur":6,"tid":259,"ts":8703546608,"tts":6493080},
{"args":{},"cat":"loading","dur":1,"name":"BackgroundHTMLParser::PumpTokenizer","ph":"X","pid":31161,"tdur":1,"tid":259,"ts":8703546610,"tts":6493082},
{"args":{},"cat":"disabled-by-default-devtools.timeline","dur":1174877,"name":"RunTask","ph":"X","pid":31161,"tdur":904908,"tid":259,"ts":8703546617,"tts":6493089},
{"args":{"data":{"blockingDuration":1139.261,"duration":1199.518,"numScripts":1,"renderDuration":14.777,"styleAndLayoutDuration":13.034}},"cat":"devtools.timeline","id2":{"local":"0x13b00174d98"},"name":"LongAnimationFrame","ph":"b","pid":31161,"scope":"devtools.timeline","tid":259,"ts":8703546175},
{"args":{},"cat":"devtools.timeline","id2":{"local":"0x13b00174d98"},"name":"LongAnimationFrame","ph":"e","pid":31161,"scope":"devtools.timeline","tid":259,"ts":8704745693},
Copy link
Member Author

Choose a reason for hiding this comment

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

These events are fake to avoid the big disruption of updating the sample trace, but the times are based on the actual MPFID long task, so the LoAF being added to the trace is plausible.

{"args":{"beginData":{"frame":"537BC44B4EE044D67F9FFE7A76173AB1","startLine":242,"url":"http://localhost:10200/dobetterweb/dbw_tester.html"},"endData":{"endLine":453}},"cat":"devtools.timeline","dur":1174038,"name":"ParseHTML","ph":"X","pid":31161,"tdur":904068,"tid":259,"ts":8703546619,"tts":6493094},
{"args":{},"cat":"blink,loading","dur":762,"name":"HTMLDocumentParser::processTokenizedChunkFromBackgroundParser","ph":"X","pid":31161,"tdur":762,"tid":259,"ts":8703546626,"tts":6493098},
{"args":{"data":{"frame":"537BC44B4EE044D67F9FFE7A76173AB1"}},"cat":"disabled-by-default-devtools.timeline","name":"ScheduleStyleRecalculation","ph":"I","pid":31161,"s":"t","tid":259,"ts":8703546637,"tts":6493111},
Expand Down
47 changes: 46 additions & 1 deletion core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,52 @@
"scoreDisplayMode": "numeric",
"numericValue": 1174.877,
"numericUnit": "millisecond",
"displayValue": "1,170 ms"
"displayValue": "1,170 ms",
"details": {
"type": "debugdata",
"observedMaxDurationLoaf": {
"args": {
"data": {
"blockingDuration": 1139.261,
"duration": 1199.518,
"numScripts": 1,
"renderDuration": 14.777,
"styleAndLayoutDuration": 13.034
}
},
"cat": "devtools.timeline",
"id2": {
"local": "0x13b00174d98"
},
"name": "LongAnimationFrame",
"ph": "b",
"pid": 31161,
"scope": "devtools.timeline",
"tid": 259,
"ts": 8703546175
},
"observedMaxBlockingLoaf": {
"args": {
"data": {
"blockingDuration": 1139.261,
"duration": 1199.518,
"numScripts": 1,
"renderDuration": 14.777,
"styleAndLayoutDuration": 13.034
}
},
"cat": "devtools.timeline",
"id2": {
"local": "0x13b00174d98"
},
"name": "LongAnimationFrame",
"ph": "b",
"pid": 31161,
"scope": "devtools.timeline",
"tid": 259,
"ts": 8703546175
}
}
},
"cumulative-layout-shift": {
"id": "cumulative-layout-shift",
Expand Down
2 changes: 2 additions & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,8 @@ export interface TraceEvent {
type?: string;
functionName?: string;
name?: string;
duration?: number;
blockingDuration?: number;
};
frame?: string;
name?: string;
Expand Down
Loading