core(prioritize-lcp-image): better identify lcp request #14804
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
| // If there are still multiple candidates, at this point it appears the page | ||
| // simply made multiple requests for the image. The first loaded is the best | ||
| // guess of the request that made the image available for use. |
There was a problem hiding this comment.
this happens more often than you'd think it would, so there's not much that can be done besides pick one. Maybe we'll find more criteria in the future that will further reduce real duplicate candidate cases
| // If there are still multiple candidates, at this point it appears the page | ||
| // simply made multiple requests for the image. The first loaded is the best | ||
| // guess of the request that made the image available for use. | ||
| return candidates.sort((a, b) => a.networkEndTime - b.networkEndTime)[0]; |
There was a problem hiding this comment.
Out of order network requests are not something I've actually observed, but NetworkRecords are ordered by the order requestWillBeSent messages come in, so possibly, in theory, they could be out of order, and it probably makes more sense to look at the first finished request rather than the first one to start.
It doesn't feel like the renderer and network stack would let that occur in any way that really matters for image requests in particular, but better to be sure.
| @@ -52,7 +59,9 @@ describe('Performance: prioritize-lcp-image audit', () => { | |||
| networkRequestTime: 0, | |||
| networkEndTime: 500, | |||
| timing: {receiveHeadersEnd: 500}, | |||
| transferSize: 400, | |||
There was a problem hiding this comment.
these all really needed transfer sizes because otherwise they all completed in a single round trip making any dependency changes difficult to differentiate
| }, | ||
| { | ||
| requestId: '3', | ||
| resourceType: 'Script', | ||
| priority: 'High', | ||
| isLinkPreload: false, | ||
| networkRequestTime: 1000, | ||
| networkEndTime: 5000, |
There was a problem hiding this comment.
and it didn't make sense for the LCP image initiator to finish after the LCP image request (some script preload scanner from the future? :) but more importantly finish after the 4500ms LCP. The simulator did its best, but again made changing any tests and getting a different result difficult.
that's the intention of the smoke test change |
| score: 1, | ||
| numericValue: 0, | ||
| // In CI, there can sometimes be slight savings. | ||
| numericValue: '0±50', |
brendankenny
force-pushed
the
lcp-dedupe
branch
from
5bbcc2d to
bfa6053
Compare
brendankenny
force-pushed
the
lcp-dedupe
branch
from
bfa6053 to
0790a22
Compare
Addresses the third and fourth bullet points of #14300 (though image redirects still need work, see below).
Currently the LCP image network request is the first request found with a matching URL. This PR adds a more in-depth search for the request, also matching frame, resource type, if the timing is possible, etc. The in-page URL and paint trace event also give the initial URL for the image, but this change now follows any redirects to find the actual request that returned the LCP image.
The initiator path is still often incorrect (e.g. we're missing the intermediate redirect requests in two tests below) because we're using
traversewhich gives the shortest path through the dependency graph. Because we're pretty aggressive in adding dependencies, this is often not the initiator path we want to be looking at (e.g. both initiators and redirects are added as direct dependencies, so the shortest path will always skip redirects, causing the aforementioned missing intermediate redirect requests). I have an immediate followup PR for this, just need to finish tests.