-
Notifications
You must be signed in to change notification settings - Fork 445
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
Extend INP attribution with extra LoAF information: longest script and buckets #592
base: v5
Are you sure you want to change the base?
Conversation
Before review, some input on naming (since you asked!) "thrashing" is only thrashing if there is actually duplication of effort. Forced style and layout can happen during script for several reasons and often it just eagerly does what would otherwise happened lazily, not actually more work. So its hard to know if its thrashing without looking. (Also WDYT:
Or, prepare for a sort of "sub-parts":
RE: RE: I'm sort of worried that you could have multiple long scripts (such as a long event and input delay). Given that we have actual INP sub-parts, and access to the loaf entry with the full list of scripts, I'm just not sure about the value of highlighting just a single longest script-- but no specific concerns either. |
Oh I think I see now the value of Maybe instead of longestScript with an overlappingDuration we have a specific value for this:
...which could be null or not and would have extra metadata like "overlappingDuration" or "blockingDurationAdded" or something. All the other scripts in the LoAF entries after this one would affect the main processing time of the frame? |
Dammit! I thought I'd found a workaround to avoid confusing with the "total style and layout" not including forced 😔
I'm not loving the inconsistency away from the LoAF I'm still not 100% convinced knowing forcedStyleAndLayout is that useful (and therefore is worth the confusion with end of frame style and layout). Once you know the script is long running, it should be easy to repeat that in devtools and then see it forced style and layout (whether due to thrashing, or just an early style and layout calc). So I think labelling this as just "script" time is maybe OK. So maybe this should just be dropped to avoid that confusion. As to the by subpart breakdown, I actually experimented with fully subparts (and have this on another branch): ![]() In the end I decided that was very verbose. People will really care about all the scripts (in which case look at the LoAF entries directly), or the longest script (in which case you need to consider intersecting durations only so library should help make that easier). Having script details for each subpart seems a bit verbose for the library to expose. So I took an opinionated choice of only showing the longest one, but reporting extra details (like which subpart it was in and the intersecting duration). Part of my issue with #574 is I found the
Yes that's correct. It's to explain when a script duration exceeds the total INP. Or when script is just a small part of INP. Maybe this is niche enough that we can just accept it will be over the duration sometimes and skip it?
Well the way I see it, this new data in this PR allow you to see if your problem is:
So the longest script is still valuable even if it's not in input delay. Granted you don't need |
Updated |
One minor concern I have with adding new "duration" properties to the top-level attribution object is that then it's much harder to know which properties are part of the official INP subparts and which properties are just other duration values not related to the official subparts. I'm not sure if there's much we can do about that, and anything we do would likely be inconsistent with the subparts in LCP attribution, but I wanted to make the comment anyway and have the discussion. |
It's a fair concern. I can think of two options to solve this:
They could also be totals for when INP spans more that one frame (so I think there are positives and negatives to both options even with that that:
At the moment I'm thinking 2) makes more sense. The third option is to just leave it as is. WDYT? |
Yeah, option (2) seems reasonable to me. My first reaction was that any style/layout/presentation work not in the last frame should be part of "input delay", but the more I thought about it the more I think there is value in an "alternate" breakdown, e.g. phases vs. buckets—where the buckets are not necessarily sequential and help you determine whether poor INP is primarily caused by script, style/layout, or off-main-thread work. That said,
I'm not super bothered by this—however, I do think it's a bit weird that "presentation delay" in the subparts is not the same thing as "presentation delay" in What if we didn't limit this to just presentation and included all off-main-thread work (including off-main thread input delay)? Then we could call it something like Also @mmocny since he may have opinions here. |
But how can we measure off main thread work? The only reason we have the framePresentationDelay is because it’s the time between LoAF end and INP end. Which btw means we also don’t have this for prior frames. But then we also don’t need it for them either when looking at buckets, as we’d double count as that would overlap the next frame starting. So framePresentationDelay should stay as it is, without the total I think. Which also means it’s back to being a subpart of presentationDelay so maybe that’s OK? Alternatively we just exclude framePresentationDelay and leave it as unattributed time—and maybe we should add totalUnattributedTime for that matter? |
I think you can use LoAF to determine what was happening when the interaction took place, and if it's not happening during script execution or style/layout, then you can assume it's off-main delay. I'm not sure if that's always 100% true, but with some quick testing it seems to be.
For bucketing, I don't think you would double count it—at least not how I was imagining things working. E.g. with bucketing you wouldn't have input delay, so any style/layout or presentation time in the previous frame would contribute to those respective buckets.
I think this would be a reasonable option as well. And we could document the various things that could end up being "unattributed", e.g. off-main presentation, input delay with no LoAF script attribution, <5ms, etc. |
Here's an example from just yesterday showing there is on-main-thread unattributed time: https://webperformance.slack.com/archives/C04BK7K1X/p1741616198612019
I meant if we could measure off-main thread frame presentation delay for the first LoAF, it would overlap the on-main thread second LoAF. But we can't do measure that anyway so doesn't really matter. So agree it couldn't overlap.
The other option is to bucket end of frame styleAndLayout and frame presentation delay as totalRenderTime so you'd have:
|
Interesting. I'm having trouble reproducing a LoAF for that frame. Are you able to repro it? Is the style block that shows up in the trace just a gap in the LoAF?
What if instead of trying to solve the presentation delay naming issue, we just didn't have a property for that at all. So we only have the following (all of which include intersecting durations from all reported LoAFs):
TBH, if we did want to have another bucket within the "presentation" subpart, I'd rather have something like |
I like where this conversation ended up, and I think the final diagram Barry put together is starting to look really good. (I haven't started to review the patch for details, but will do so after a few questions).
|
To answer your questions:
To me the subparts are our primary recommendation (is it your interaction that's the problem, or general business?), and the buckets give an alternative view as to what's the overall problem (is it primarily scripting, s&l, paint, something we don't know, or a mixture of all three?). It's not quite the full invoker/invoker-type combo as was proposed in #574 but I found that overly complex to grok so hopefully this is, as you say, "just right". So with this we now kind of have the view you see in DevTools - the subparts is like the Interactions lane with the 3 phases, and the buckets are like the Summary breakdown if you highlight that section of the flame chart. We of course don't have the full flame chart details, but the Longest Script at least gives the entry point of the worst part of that (which is probably where you should concentrate if that's large). And those wanting a fuller picture, to be even closer to the flame chart can use the LoAF entries to get all the scripts if they so desire, but that's overkill for many IMHO. With subparts, buckets, and longest script we think we're striking a balance between enough detail, but not too much complexity. |
Ah, thanks. In that case it ~mostly what I wanted: off-main-presentation-delay. I think that given the history, probably "presentation delay" is not a good name to use, so:
I think that the four timestamps:
Are all useful because it lets you "draw" the interaction. All the 3 phases can easily be computed from these values, but maybe only the "input delay" should be done by default, as the other buckets just do a better job? Not sure!
+1. |
Alternatively, having the 3 subpart durations avoids breaking backwards compatibility and allows those timings to be easily computed from them anyway! So not seeing the need to change this. Also not understanding what you mean by "but maybe only the "input delay" should be done by default, as the other buckets just do a better job?"? |
I meant, maybe exposed as a unique value by default-- but I missed that we already expose these values so not worth removing. |
src/attribution/onINP.ts
Outdated
@@ -260,6 +261,91 @@ export const onINP = ( | |||
return intersectingLoAFs; | |||
}; | |||
|
|||
const attributeLoAFDetails = (attribution: INPAttribution, value: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sending first pass comments before reviewing this fyi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth adding another test that creates a situation where there are two LoAFs, just to make sure that case is handled. (I'm happy to offer some ideas for how to do that if it's unclear.)
Co-authored-by: Philip Walton <[email protected]>
Given the flakiness of LoAFs in CI already at the moment (it's one of the tests that fails most often), and your recent discovery that they aren't always emitted in time without experimental web platform features enabled, I'd prefer to wait until one or both of those are fixes before expanding on this in the test suite WDYT @philipwalton ? |
Sure, happy to address as a separate PR, but we should manually verify that it works in these cases and then come up with a plan to ensure it's properly tested in the future. |
Closes #559
This is an alternative to #574 which provides some extra attribution information:
totalScriptDuration
- the total duration of all intersecting scriptstotalStyleAndLayoutDuration
- The total style and layout duration including any end-of-frame style and layout duration plus any forced style and layout duration.totalPaintDuration
- the off-main-thread presentation delays (end of LoAF -> end of INP).totalUnattributedDuration
- The total unattributed time not included in any of the previous totals including scripts < 5 milliseconds and other timings not attributed by LoAF (including when a frame is < 50ms and so has no LoAF).longestScript
- the longest intersection scriptentry
- the entrysubpart
- the subpart the script ocured inintersectingDuration
- the intersecting duration (as it may be not all of the script intersected the INP time).The first four give more information on whether this is a script, or style and layout, or frame presentation delay.
The last item pulls out the important script (by intersecting duration, which is not easily obtainable form the current raw LoAF entry).
Example output: