-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(lcp-lazy-loaded): add LCP savings estimate #15064
Conversation
core/audits/lcp-lazy-loaded.js
Outdated
// We don't know when the LCP resource was first "seen" before being lazy loaded. | ||
// Our best estimate for LCP savings is the entire LCP load delay. | ||
lcpSavings = lcpBreakdown.loadStart - lcpBreakdown.ttfb; |
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 believe lcpImageRequest.networkRequestTime - lcpImageRequest.rendererStartTime
is this time (though still not entirely fair to blame all of it on the lazy loading)? Unless there's some optimizations like e.g. the preload scanner skipping loading="lazy"
resources and letting the full parser handle it
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 believe lcpImageRequest.networkRequestTime - lcpImageRequest.rendererStartTime is this time
From my testing this isn't the case (although would be nice if it was), rendererStartTime
is not when the request was "discovered" but when the renderer decided to start the lazy request.
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.
And turns out
Unless there's some optimizations like e.g. the preload scanner skipping loading="lazy" resources and letting the full parser handle it
is indeed a thing:
if (type == ResourceType::kImage && is_img &&
IsLazyLoadImageDeferable(document_parameters)) {
return nullptr;
}
:)
It actually makes sense that the renderer would be the owner of this because it has to know about layout, current viewport etc, but I didn't think about that.
I guess we know it's bounded by at least ttfb and rendererStartTime
, then? Still not great though. My main issue with this wide estimate is that it makes it as big an opportunity as using fetchpriority=high
, when it almost never will be.
Including loading=lazy
is just a little bit worse than having no fetchpriority/loading attributes on the image at all, because the browser does eventually figure out what's going on once layout occurs. See the testing in https://web.dev/lcp-lazy-loading/#testing-a-fix, for instance
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.
Longe term, if we fold this advice into prioritize-lcp-image
this savings estimate will make more sense.
One thing we could do is take a statistical guess at the savings. Apply a 13%/15% (https://web.dev/lcp-lazy-loading/#causal-performance) savings to LCP and bound by ttfb and rendererStartTime
.
This PR uses the LCP load delay as the estimated LCP savings for the
lcp-lazy-loaded
audit. It might be possible to get a better estimate by simulating the page graph with modified fetch priority.That being said, this audit might be going away as part of #13738. I wanted to reduce the titanic deck chair shuffling in this PR by using a very simple estimate.