core(image-elements): cap natural size fetch time #7274
Conversation
| if (/^image/.test(record.mimeType) && record.finished) { | ||
| // The network record is only valid for size information if it finished with a successful status | ||
| // code that indicates a complete resource response. | ||
| if (/^image/.test(record.mimeType) && record.finished && record.statusCode === 200) { |
There was a problem hiding this comment.
this is technically the only fix explicitly required for #7273, but seemed like a good idea to address other potential situations too
| @@ -130,6 +130,8 @@ class ImageElements extends Gatherer { | |||
| async fetchElementWithSizeInformation(driver, element) { | |||
| const url = JSON.stringify(element.src); | |||
| try { | |||
| // We don't want this to take forever, 250ms should be enough for images that are cached | |||
| driver.setNextProtocolTimeout(250); | |||
There was a problem hiding this comment.
one interesting aspect of our protocol timeouts is the effect of a bunch of orphaned protocol commands still running in the page. In this case there's also the Runtime.evaluate timeout, so they're at least somewhat cut off from further action, but what was being slow here, re-fetching images to get size info?
There was a problem hiding this comment.
Correct loading the image to get the size info was taking a long time because it was reissuing the request.
| // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image. | ||
| if ((element.isPicture || element.isCss) && networkRecord) { | ||
| // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image, | ||
| // or it's not in the top 50 largest images. |
There was a problem hiding this comment.
maybe we should include in the docs for the artifact that picture or css images don't have size info if they aren't in the top 50 largest images?
| (element.isPicture || element.isCss) && | ||
| networkRecord && | ||
| top50Images.includes(networkRecord) | ||
| ) { |
There was a problem hiding this comment.
for the else case of this conditional, it seems like we need to zero out naturalWidth/naturalHeight for element.isPicture || element.isCss like fetchElementWithSizeInformation does since they won't be good values to trust otherwise?
There was a problem hiding this comment.
I'll just convert the default to be these since the goal is to set every image to 0 or the real value anyhow.
| @@ -163,6 +167,9 @@ class ImageElements extends Gatherer { | |||
| const elements = await driver.evaluateAsync(expression); | |||
|
|
|||
| const imageUsage = []; | |||
| const top50Images = Object.values(indexedNetworkRecords) | |||
| .sort((a, b) => b.resourceSize - a.resourceSize) | |||
There was a problem hiding this comment.
should this be using the transferSize fallback like below?
There was a problem hiding this comment.
IMO, no. That fallback is for making sure we don't miss the super rare case of an image being GZIPped in our final savings estimate but has negligible effect on whether it counts in the top 50.
|
Instead of doing We could also then set an overall timeout for the entire |
How so? Or is this assuming the worst case imagineable where all 50 images need fetching and the gatherer takes 12.5 seconds.
We certainly could, just somewhat complicates things a bit and the idea of trying to load 50 images all at once if things go poorly means scares me a bit. I'd vote to avoid doing it until we have a compelling example case for it. |
Summary
Caps our time spent fetching natural size information in
ImageElementsgatherer.Aside, this got me thinking that we really need artifact smoketests. The current logic is covered by existing smoketests, but it'd be a lot more comfortable to make changes to gatherers with lots of
evaluateAsyncif we could easily add gatherer smoketests.Related Issues/PRs
closes #7273