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

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

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

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

yes definitely. done.

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.