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): cache naturalSize results #9818

Merged
merged 3 commits into from
Oct 11, 2019
Merged

Conversation

paulirish
Copy link
Member

This url is quite slow to run: https://www.selfridges.com/US/en/cat/womens/newin/

On the CLI, the ImageElements gatherer can take 25-30 seconds to run.

Turns out that gatherer finds ~2000 images, however 99% of them are country flag DOM elements with a css background using the exact same sprite image.


While we have added that smart only-do-the-50-most-important-network-images check... we don't handle the case of multiple background images pointing to the same asset that's within the top 50.

But now we do. :)

@connorjclark
Copy link
Collaborator

how much faster tho

@paulirish
Copy link
Member Author

how much faster tho

25s to 1s on selfridges

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

great find! LGTM

@brendankenny
Copy link
Member

25s to 1s on selfridges

that doesn't seem possible, fwiw, or the protocol timeout is broken :)

// We don't want this to take forever, 250ms should be enough for images that are cached
driver.setNextProtocolTimeout(250);

I get like 8s -> 250ms, which is still 👍

@brendankenny
Copy link
Member

25s to 1s on selfridges

that doesn't seem possible, fwiw, or the protocol timeout is broken :)

oh, I take it back. It's because they all share a network record so we just repeatedly fetch the same images 2000 times? Dear lord, yeah, good find.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants