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

perf(gatherers): skip optimization of cross origin images #2154

Merged
merged 2 commits into from
May 9, 2017

Conversation

patrickhulce
Copy link
Collaborator

in the name of Operation Yaquina Bay #2146, fixes #1853

shaves about ~25 seconds off heavy sites like cnn.com

@patrickhulce patrickhulce changed the title perf(gatherers): skip optimization for cross origin images perf(gatherers): skip optimization of cross origin images May 4, 2017
@ebidel
Copy link
Contributor

ebidel commented May 4, 2017

It's too bad we're thinking about disabling this one. Images are the biggest source of bloat on the web and are often x-domain b/c of CDNs.

@patrickhulce
Copy link
Collaborator Author

@ebidel I definitely want to bring it back when we have a protocol method for doing the conversion instead of our current back and forth massive JSON serialization
LH -> getRequestBody -> DT
DT -> 📷 -> LH
LH -> evaluateScript(data: base64,📷 ) -> DT
DT -> bytes saved -> LH

@patrickhulce
Copy link
Collaborator Author

My gut also says that if you're doing the right thing and using a CDN, it's relatively unlikely you're sending a full bitmap down the wire, and we'll still be able to identify non-responsive cross-origin images, unused cross-origin images, etc.

My idea for a follow-up replacement would be to flag very large images beyond some KB / pixel threshold and only process those

@ebidel
Copy link
Contributor

ebidel commented May 4, 2017

Ok. I think @brendankenny also mentioned it., but can you get a meta issue going that tracks all of these removals/disables so we can re-address after IO?

@brendankenny
Copy link
Member

should this also have something in the description or in the results about unchecked images to make it clear this isn't the whole picture images-wise? Like a non-fatal version of how notifications/geolocation warn that they weren't able to check because a site is on http. It would be easy to conclude your images are all great because they weren't listed.

@patrickhulce
Copy link
Collaborator Author

should this also have something in the description or in the results about unchecked images to make it clear this isn't the whole picture images-wise? Like a non-fatal version of how notifications/geolocation warn that they weren't able to check because a site is on http. It would be easy to conclude your images are all great because they weren't listed.

That's a great point. Unfortunately, given that this about is about to be only displayed as the sparkline, it would be strange to show it empty just for the debug string, yet hiding it in the passed section feels like it'd never be seen either. Go with debug string and hope people expand that passed section?

@paulirish paulirish mentioned this pull request May 5, 2017
40 tasks
@paulirish paulirish added the OYB label May 5, 2017
@brendankenny
Copy link
Member

Go with debug string and hope people expand that passed section?

will debugString be visible for sparklines by default? Having it that way would certainly be a good motivator to get the protocol support landed :)

@patrickhulce
Copy link
Collaborator Author

will debugString be visible for sparklines by default?

yes, but if the score is 100 it'll be shown in passed audits (not as a sparkline with the other byte efficiency audits)

@paulirish paulirish merged commit d0cb646 into master May 9, 2017
@paulirish paulirish deleted the fewer_optimized_images branch May 9, 2017 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimized Images reports too much JPEG savings
4 participants