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

core(image-elements): cap natural size fetch time #7274

Merged
merged 4 commits into from
Feb 26, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ class Driver {
* @return {Promise<*>}
*/
async _evaluateInContext(expression, contextId) {
// Use a higher than default timeout if the user hasn't specified a specific timeout.
// Otherwise, use whatever was requested.
const timeout = this._nextProtocolTimeout === DEFAULT_PROTOCOL_TIMEOUT ?
60000 : this._nextProtocolTimeout;
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
const evaluationParams = {
// We need to explicitly wrap the raw expression for several purposes:
// 1. Ensure that the expression will be a native Promise and not a polyfill/non-Promise.
Expand All @@ -372,11 +376,11 @@ class Driver {
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true,
timeout: 60000,
timeout,
contextId,
};

this.setNextProtocolTimeout(60000);
this.setNextProtocolTimeout(timeout);
const response = await this.sendCommand('Runtime.evaluate', evaluationParams);
if (response.exceptionDetails) {
// An error occurred before we could even create a Promise, should be *very* rare
Expand Down
24 changes: 18 additions & 6 deletions lighthouse-core/gather/gatherers/image-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ function collectImageElementInfo() {
displayedHeight: element.clientHeight,
clientRect: getClientRect(element),
// CSS Images do not expose natural size, we'll determine the size later
naturalWidth: Number.MAX_VALUE,
naturalHeight: Number.MAX_VALUE,
naturalWidth: 0,
naturalHeight: 0,
isCss: true,
isPicture: false,
usesObjectFit: false,
Expand Down Expand Up @@ -130,12 +130,14 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct loading the image to get the size info was taking a long time because it was reissuing the request.

/** @type {{naturalWidth: number, naturalHeight: number}} */
const size = await driver.evaluateAsync(`(${determineNaturalSize.toString()})(${url})`);
return Object.assign(element, size);
} catch (_) {
// determineNaturalSize fails on invalid images, which we treat as non-visible
return Object.assign(element, {naturalWidth: 0, naturalHeight: 0});
return element;
}
}

Expand All @@ -147,7 +149,9 @@ class ImageElements extends Gatherer {
async afterPass(passContext, loadData) {
const driver = passContext.driver;
const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => {
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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is technically the only fix explicitly required for #7273, but seemed like a good idea to address other potential situations too

map[record.url] = record;
}

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

should this be using the transferSize fallback like below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

.slice(0, 50);
for (let element of elements) {
// Pull some of our information directly off the network record.
const networkRecord = indexedNetworkRecords[element.src] || {};
Expand All @@ -177,8 +184,13 @@ class ImageElements extends Gatherer {

// Images within `picture` behave strangely and natural size information isn't accurate,
// CSS images have no natural size information at all. Try to get the actual size if we can.
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

if (
(element.isPicture || element.isCss) &&
networkRecord &&
top50Images.includes(networkRecord)
) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll just convert the default to be these since the goal is to set every image to 0 or the real value anyhow.

element = await this.fetchElementWithSizeInformation(driver, element);
}

Expand Down
2 changes: 1 addition & 1 deletion types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ declare global {
HTMLWithoutJavaScript: {bodyText: string, hasNoScript: boolean};
/** Whether the page ended up on an HTTPS page after attempting to load the HTTP version. */
HTTPRedirect: {value: boolean};
/** Information on size and loading for all the images in the page. */
/** Information on size and loading for all the images in the page. Natural size information for `picture` and CSS images is only available if the image was one of the largest 50 images. */
ImageElements: Artifacts.ImageElement[];
/** Information on JS libraries and versions used by the page. */
JSLibraries: {name: string, version: string, npmPkgName: string}[];
Expand Down