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.

Sign up for GitHub

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

Jump to bottom

core(image-elements): set 5s time budget, add _privateCssSizing #12065

Merged
merged 25 commits into from
Feb 11, 2021
Merged

core(image-elements): set 5s time budget, add _privateCssSizing #12065

Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f6f8069
core(image-elements): speed up by only fetching CSS rules for largest…
paulirish Feb 5, 2021
7c6f6ea
create a time budget
paulirish Feb 8, 2021
f94f851
Update lighthouse-core/gather/gatherers/image-elements.js
paulirish Feb 10, 2021
a6b5ec2
comment and rename from feedback
paulirish Feb 10, 2021
b07eb63
invert name and check once
paulirish Feb 10, 2021
6e00733
add log message on reachedGatheringBudget
paulirish Feb 10, 2021
6908ee2
return null
paulirish Feb 10, 2021
449f39b
Merge remote-tracking branch 'origin/master' into fasterimageelments
paulirish Feb 10, 2021
689c1c1
Merge remote-tracking branch 'origin/master' into fasterimageelments
paulirish Feb 10, 2021
8e411dd
rename these audit methods. its not about VALIDITY but SIZEDEDNESS
paulirish Feb 10, 2021
0fbc7d4
handle the data-not-gathered case
paulirish Feb 10, 2021
6bfb5ea
cssWidth/height as empty string never made sense. previously it was u…
paulirish Feb 10, 2021
37663b4
3s => 5s. lint.
paulirish Feb 10, 2021
9bc2c89
a cssSizing object. keep cssWidth/height the same for now
paulirish Feb 10, 2021
fea8885
5s
paulirish Feb 10, 2021
dc7f4a0
yarn update:sample-artifacts ImageElements plus v minor handtweaks
paulirish Feb 10, 2021
b80096c
Apply suggestions from code review
paulirish Feb 10, 2021
7d80b31
lint
paulirish Feb 10, 2021
99b3d4a
rename to _privateCssSizing
paulirish Feb 10, 2021
8503001
set as optional. thx brendan
paulirish Feb 10, 2021
7e26ae2
Update lighthouse-core/gather/gatherers/image-elements.js
paulirish Feb 11, 2021
b19a8d3
remove unncessary return value from fetchElementWithSizeInformation()
paulirish Feb 11, 2021
3a5eea3
adjust type definitions, based on brendan's feedback
paulirish Feb 11, 2021
df4eb47
Merge remote-tracking branch 'origin/master' into fasterimageelments
paulirish Feb 11, 2021
36b31f3
drop @deprecated per connor
paulirish Feb 11, 2021
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
27 changes: 20 additions & 7 deletions lighthouse-core/gather/gatherers/image-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,10 @@ class ImageElements extends Gatherer {
}

/**
* Images might be sized via CSS. In order to compute unsized-images failures, we need to collect
* matched CSS rules to see if this is the case.
* Perf warning: Running this for 700 elements takes 1s to 5s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checking: did you mean 50 elements here (the previous limit)?

3s is currently the ~85th percentile on LR staging.

I presume that is 3s for the top 50, so it is surprising that 700 would only take 1-5s.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah thx for bringing attention to these numbers.

Just checking: did you mean 50 elements here (the previous limit)?

nah i was just trying to provide some general context. though TBH the count of total imageElements doesn't correlate too well. kohls has 700 and takes 1s on my machine and 5s on LR... however like 500 of them are data uris and 495 of those are reusing network resources.
but the gatherer cost correlates with # of network-resource-associated non-css non-shadowdom images that we do these protocol calls for. (something like ~160 for kohls i think? eh.)

I'll remove this particular sentence and replace it with something more useful.

3s is currently the ~85th percentile on LR staging.

I presume that is 3s for the top 50, so it is surprising that 700 would only take 1-5s.

this 3s 85th percentile figure is accurate for staging LR, which has the top50 filter for fetchElementWithSizeInformation but not for fetchSourceRules.

700 does sound like a lot, but language selectors w/ flag sprites and other lazyload image stuff, the number can get high pretty quickly. shrug. i couldn't tell you what the distribution of ImageElements.length is for LR, but this investigation does make me curious...

* @url http://go/dwoqq (googlers only)
* @param {Driver} driver
* @param {string} devtoolsNodePath
* @param {LH.Artifacts.ImageElement} element
Expand Down Expand Up @@ -308,30 +312,39 @@ class ImageElements extends Gatherer {

/** @type {Array<LH.Artifacts.ImageElement>} */
const imageUsage = [];
const top50Images = Object.values(indexedNetworkRecords)
.sort((a, b) => b.resourceSize - a.resourceSize)
.slice(0, 50);

await Promise.all([
driver.sendCommand('DOM.enable'),
driver.sendCommand('CSS.enable'),
driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}),
]);

// Sort (in-place) as largest images descending
elements.sort((a, b) => {
const aRecord = indexedNetworkRecords[a.src] || {};
const bRecord = indexedNetworkRecords[b.src] || {};
return bRecord.resourceSize - aRecord.resourceSize;
});

// Don't allow more than 3s of this expensive protocol work
paulirish marked this conversation as resolved.
Show resolved Hide resolved
let hasBudget = true;
paulirish marked this conversation as resolved.
Show resolved Hide resolved
setTimeout(_ => (hasBudget = false), 3000);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

for (let element of elements) {
// Pull some of our information directly off the network record.
paulirish marked this conversation as resolved.
Show resolved Hide resolved
const networkRecord = indexedNetworkRecords[element.src] || {};
element.mimeType = networkRecord.mimeType;

if (!element.isInShadowDOM && !element.isCss) {
// Need source rules to determine if sized via CSS (for unsized-images).
if (!element.isInShadowDOM && !element.isCss && hasBudget) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding this bailout is the big change. Won't this lead to false failures? My read of

lighthouse/lighthouse-core/audits/unsized-images.js

Lines 70 to 82 in aaf1365

static isSizedImage(image) {
const attrWidth = image.attributeWidth;
const attrHeight = image.attributeHeight;
const cssWidth = image.cssWidth;
const cssHeight = image.cssHeight;
const widthIsValidAttribute = UnsizedImages.isValidAttr(attrWidth);
const widthIsValidCss = UnsizedImages.isValidCss(cssWidth);
const heightIsValidAttribute = UnsizedImages.isValidAttr(attrHeight);
const heightIsValidCss = UnsizedImages.isValidCss(cssHeight);
const validWidth = widthIsValidAttribute || widthIsValidCss;
const validHeight = heightIsValidAttribute || heightIsValidCss;
return validWidth && validHeight;
}

is that if fetchSourceRules isn't run, then cssWidth and cssHeight will be undefined, and so images could be falsely marked as unsized. We should probably fail the other way, not making a claim if we don't have the info?

Copy link
Member

Choose a reason for hiding this comment

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

(also it looks like we don't have any unsized-image tests with cssWidth: undefined, cssHeight: undefined. Not a huge deal because we rely on falsy behavior so the tests using the empty string are equivalent, but we should probably add a few at some point since that's an allowed state)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I did miss this change. The previous PR was just focused on saving time for the second request for the source rules, and that's all I was looking at.

So the long tail is not that the 50 largest elements take a long time in fetchElementWithSizeInformation , but that all the elements take a long time to go thru fetchSourceRules

We should probably fail the other way, not making a claim if we don't have the info?

Should we mark these fields null to signify we don't know, and undefined if the element size is not specified (aka when getEffectiveSizingRule returns undefined)? Or put cssWidth/cssHeight into an optional property cssSizing?: {width?: string, height?: string}?

Copy link
Member Author

@paulirish paulirish Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we discussed in chat.. highlights from that:

I tend to think of undefined as equivalent to "not set" and null as equivalent to "set and has no value".

👍 👍 👍 👍 👍 👍

idea for imageElement.cssSizing obj width width/height props.. obj is appropriately null/undefined if skipped etc.
obj could include the naturalWidth bits as well and be fetchedDimensions or we keep 'em separate.

it would be nice on the next major to do a minor rethink of ImageElements to make it easier on audits using them, since naturalWidth/naturalHeight is in a similar but not quite the same situation as cssWidth/cssHeight after this change
(to add to the null/undefined confusion, don't forget the return value of el.getAttribute('notASetAttribute') :)

edit: filed in #12077

await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element);
}
// 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,
// or it's not in the top 50 largest images.
// Additional fetch is expensive; don't bother if we don't have a networkRecord for the image.
if (
(element.isPicture || element.isCss || element.srcset) &&
top50Images.includes(networkRecord)
networkRecord && hasBudget
) {
element = await this.fetchElementWithSizeInformation(driver, element);
}
Expand Down