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

LoAF Summary attribution information #574

Open
wants to merge 19 commits into
base: v5
Choose a base branch
from
Open

LoAF Summary attribution information #574

wants to merge 19 commits into from

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented Dec 6, 2024

Fixes #559

Includes object like this:

"longAnimationFrameSummary": {
    "numLongAnimationFrames": 3,
    "numIntersectingScripts": 9,
    "slowestScript": {
      "entry": // Not Shown,
      "subpart": "processingDuration",
      "intersectingDuration": 395,
      "totalDuration": 395,
      "compileDuration": 0,
      "executionDuration": 395,
      "forcedStyleAndLayoutDuration": 144,
      "pauseDuration": 0,
      "invokerType": "event-listener",
      "invoker": "BUTTON.onclick",
      "sourceURL": "",
      "sourceFunctionName": "",
      "sourceCharPosition": -1,
    },
    "totalDurationsPerSubpart": {
        "inputDelay": {
            "user-callback": 28,
            "event-listener": 185
        },
        "processingDuration": {
            "event-listener": 434
        }
    },
    "totalForcedStyleAndLayoutDuration": 144,
    "totalNonForcedStyleAndLayoutDuration": 3144099.700000003,
    "totalScriptDuration": 647
}

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

Thanks Barry! Left a few comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
}
totalScriptTime += script.duration;
numScripts++;
totalForcedStyleAndLayout += script.forcedStyleAndLayoutDuration;
Copy link
Member

Choose a reason for hiding this comment

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

I think there's an open question whether we should only be taking the blocking/intersecting portion of all duration metrics. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately we can't for forced style and layout since we don't have the timings LoAF. Maybe that's a good reason to keep this simple and just take it for all intersecting scripts (which is what it does currently)?

src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

incidentally, it seems like we're leaning subparts, not phases for LCP, so we might want to do the same thing here (or object quickly on the LCP side of things :)

src/types/inp.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
src/attribution/onINP.ts Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

incidentally, it seems like we're leaning subparts, not phases for LCP, so we might want to do the same thing here (or object quickly on the LCP side of things :)

I'm cool with subparts so switched to that throughout.

@tunetheweb
Copy link
Member Author

@rviscomi I've flattened the structure, and updated the comment in #574 (comment) to show that.

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

LGTM! I'll try it out in the next RC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants