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.

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(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

merged 2 commits into from
Dec 11, 2023

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Dec 11, 2023

As a first step to evaluating where Long Animation Frames (LoAF) will be a better measure of main-thread blocking than long tasks, this PR adds the longest LoAFs (after FCP) by duration and blockingDuration to the debug data of the max-potential-fid hidden audit.

No visible changes. The primary purpose is to have the data in HTTP Archive for querying and comparing to other metrics like MPFID and TBT.

Tests are minimal because we have fairly minimal tests for MPFID in the first place (e.g. no smoke tests). Happy to add more if it seems necessary.

@brendankenny brendankenny requested a review from a team as a code owner December 11, 2023 21:19
@brendankenny brendankenny requested review from adamraine and removed request for a team December 11, 2023 21:19
Comment on lines 12870 to 12871
{"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.

@@ -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.

@brendankenny brendankenny merged commit bb84b18 into main Dec 11, 2023
29 checks passed
@brendankenny brendankenny deleted the mp-loaf branch December 11, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants