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

paulirish · 2021-02-08T21:57:46Z

as explored in #11289, we can spend a lot of time in image-elements gathering on some pages. https://www.kohls.com is one such repro.

this PR adds a time budget the expensive parts of gathering that data.

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

fix #11289
also related: #12054

… 50 images

lighthouse-core/gather/gatherers/image-elements.js

connorjclark · 2021-02-09T16:46:48Z

lighthouse-core/gather/gatherers/image-elements.js

@@ -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.


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.

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...

lighthouse-core/gather/gatherers/image-elements.js

brendankenny · 2021-02-09T17:38:27Z

lighthouse-core/gather/gatherers/image-elements.js

    for (let element of elements) {
      // Pull some of our information directly off the network record.
      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) {


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?

(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)

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}?

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

Co-authored-by: Connor Clark <[email protected]>

lighthouse-core/gather/gatherers/image-elements.js

connorjclark

comments etc.

…ndefined. but anyway now its explicitly null. yay

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

brendankenny · 2021-02-10T23:47:41Z

I don't love what feels like rushing in to a new cssSizing property in a public artifact that was just refactored, but I do love the concept of these now optional properties being contained in an optional nested object to make consumption easier. WDYT about some sort of psuedoprivate __experimentalFetchedCssRules that gives us more time to sit with it and discuss #12077 ? I'm fine with it as-is if the rest of the team wants to move forward with cssSizing though

+1. Seems like a good eng sync topic given that we all seem to be on ever so slightly different pages :)

Not smoke testing this doesn't feel good, but I have no suggestions for a good way to test it :/

Co-authored-by: Connor Clark <[email protected]>

types/artifacts.d.ts

paulirish · 2021-02-10T23:59:12Z

I don't love what feels like rushing in to a new cssSizing property in a public artifact that was just refactored, but I do love the concept of these now optional properties being contained in an optional nested object to make consumption easier. WDYT about some sort of psuedoprivate __experimentalFetchedCssRules that gives us more time to sit with it and discuss #12077 ? I'm fine with it as-is if the rest of the team wants to move forward with cssSizing though

+1. Seems like a good eng sync topic given that we all seem to be on ever so slightly different pages :)

works for me. renamed to _privateCssSizing for now.

brendankenny

LGTM

brendankenny · 2021-02-11T00:18:17Z

types/artifacts.d.ts

-        /** The CSS width property of the image element. */
+        /**
+         * The CSS width property of the image element.
+         * @deprecated Use `element?._privateCssSizing.width` instead.


should these deprecations be TODO since we don't actually want anyone using _privateCssSizing yet?

yes definitely. done.

brendankenny · 2021-02-11T00:20:45Z

types/artifacts.d.ts

        cssHeight?: string;
+        /**
+         * The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected.
+         * @private


seems not worth it to ts-expect-error all uses of this to keep it @private? We could actually mark this @deprecated which is kind of backwards but would discourage use and strike it out in vscode but still leave it visible for type checking. Maybe there are other options?

i dig it. done. and added some careful wording around it.

lighthouse-core/gather/gatherers/image-elements.js

Co-authored-by: Brendan Kenny <[email protected]>

paulirish added 2 commits February 4, 2021 20:16

core(image-elements): speed up by only fetching CSS rules for largest…

f6f8069

… 50 images

create a time budget

7c6f6ea

paulirish requested a review from a team as a code owner February 8, 2021 21:57

paulirish requested review from connorjclark and removed request for a team February 8, 2021 21:57

google-cla bot added the cla: yes label Feb 8, 2021

devtools-bot assigned connorjclark Feb 8, 2021

devtools-bot added the waiting4reviewer label Feb 8, 2021

connorjclark reviewed Feb 9, 2021

View reviewed changes

lighthouse-core/gather/gatherers/image-elements.js Outdated Show resolved Hide resolved

connorjclark reviewed Feb 9, 2021

View reviewed changes

lighthouse-core/gather/gatherers/image-elements.js Outdated Show resolved Hide resolved

connorjclark approved these changes Feb 9, 2021

View reviewed changes

brendankenny reviewed Feb 9, 2021

View reviewed changes

Update lighthouse-core/gather/gatherers/image-elements.js

f94f851

Co-authored-by: Connor Clark <[email protected]>

vercel bot deployed to Preview February 10, 2021 00:48 View deployment

connorjclark reviewed Feb 10, 2021

View reviewed changes

lighthouse-core/gather/gatherers/image-elements.js Show resolved Hide resolved

connorjclark reviewed Feb 10, 2021

View reviewed changes

lighthouse-core/gather/gatherers/image-elements.js Outdated Show resolved Hide resolved

connorjclark requested changes Feb 10, 2021

View reviewed changes

comment and rename from feedback

a6b5ec2

connorjclark changed the title ~~core(image-elements): set a time budget so we don't spent >3s gathering~~ core(image-elements): set 3s time budget Feb 10, 2021

paulirish added 7 commits February 10, 2021 11:41

invert name and check once

b07eb63

add log message on reachedGatheringBudget

6e00733

return null

6908ee2

Merge remote-tracking branch 'origin/master' into fasterimageelments

449f39b

Merge remote-tracking branch 'origin/master' into fasterimageelments

689c1c1

rename these audit methods. its not about VALIDITY but SIZEDEDNESS

8e411dd

handle the data-not-gathered case

0fbc7d4

paulirish mentioned this pull request Feb 10, 2021

ImageElement gathererer fixups #12077

Closed

cssWidth/height as empty string never made sense. previously it was u…

6bfb5ea

…ndefined. but anyway now its explicitly null. yay

vercel bot deployed to Preview February 10, 2021 21:27 View deployment

connorjclark reviewed Feb 10, 2021

View reviewed changes

lighthouse-core/test/audits/unsized-images-test.js Show resolved Hide resolved

Apply suggestions from code review

b80096c

Co-authored-by: Connor Clark <[email protected]>

vercel bot deployed to Preview February 10, 2021 23:48 View deployment

lint

7d80b31

paulirish force-pushed the fasterimageelments branch from 2ec71e0 to 7d80b31 Compare February 10, 2021 23:49

vercel bot deployed to Preview February 10, 2021 23:49 View deployment

brendankenny reviewed Feb 10, 2021

View reviewed changes

types/artifacts.d.ts Outdated Show resolved Hide resolved

paulirish added 2 commits February 10, 2021 15:57

rename to _privateCssSizing

99b3d4a

set as optional. thx brendan

8503001

vercel bot deployed to Preview February 10, 2021 23:58 View deployment

brendankenny reviewed Feb 11, 2021

View reviewed changes

Update lighthouse-core/gather/gatherers/image-elements.js

7e26ae2

Co-authored-by: Brendan Kenny <[email protected]>

vercel bot deployed to Preview February 11, 2021 18:50 View deployment

paulirish added 3 commits February 11, 2021 10:52

remove unncessary return value from fetchElementWithSizeInformation()

b19a8d3

adjust type definitions, based on brendan's feedback

3a5eea3

Merge remote-tracking branch 'origin/master' into fasterimageelments

df4eb47

vercel bot deployed to Preview February 11, 2021 19:02 View deployment

drop @deprecated per connor

36b31f3

vercel bot deployed to Preview February 11, 2021 19:05 View deployment

connorjclark approved these changes Feb 11, 2021

View reviewed changes

paulirish added the land-when-ci-is-green label Feb 11, 2021

paulirish changed the title ~~core(image-elements): set 5s time budget~~ core(image-elements): set 5s time budget, add _privateCssSizing Feb 11, 2021

devtools-bot merged commit 4202f19 into master Feb 11, 2021

devtools-bot deleted the fasterimageelments branch February 11, 2021 19:36

devtools-bot removed the land-when-ci-is-green label Feb 11, 2021

paulirish added a commit that referenced this pull request Mar 23, 2021

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

7d32380

paulirish mentioned this pull request Apr 29, 2021

Font size gatherer is slow #12425

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

paulirish commented Feb 8, 2021

connorjclark Feb 9, 2021

paulirish Feb 10, 2021

brendankenny Feb 9, 2021

brendankenny Feb 9, 2021

connorjclark Feb 10, 2021

paulirish Feb 10, 2021 •

edited

Loading

connorjclark left a comment

brendankenny commented Feb 10, 2021

paulirish commented Feb 10, 2021

brendankenny left a comment

brendankenny Feb 11, 2021

paulirish Feb 11, 2021

brendankenny Feb 11, 2021

paulirish Feb 11, 2021

	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;
	}

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

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

Conversation

paulirish commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

brendankenny commented Feb 10, 2021

paulirish commented Feb 10, 2021

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Feb 10, 2021 •

edited

Loading