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

Decrease optipng level #2143

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
7 participants
@vandie
Copy link
Contributor

vandie commented Jan 7, 2019

After seeing the following graph here we decided to test optimsation level 2 for OptiPNG instead of 7. This is because, acording to the graph we would notice no real difference in avarage file size but would notice a speed increase with our builds.

image

After testing , we noticed our theme build go from a build time of over 10 mins (just on the 94% asset optimisation step) to an avarage of 150.75s. All the while, we only gained 10-15kb per png file which we believe more than reasonable for a build speed increase of this magnatude.

@mmirus

This comment has been minimized.

Copy link
Member

mmirus commented Jan 7, 2019

From my (very limited) testing this seems like a safe and significant build performance win. Wouldn't generally encourage folks to have large PNGs bundled with themes, but I'm sure there are valid use cases for it--might as well make it the best experience possible for those times where it makes sense.

Thanks for the PR!

@mmirus

mmirus approved these changes Jan 7, 2019

@swalkinshaw

This comment has been minimized.

Copy link
Member

swalkinshaw commented Jan 7, 2019

10mins -> 150s is incredible. However, I do think 10-15kb per png is significant for production builds.

What about level 6?

@vandie

This comment has been minimized.

Copy link
Contributor Author

vandie commented Jan 8, 2019

10mins -> 150s is incredible. However, I do think 10-15kb per png is significant for production builds.

What about level 6?

I think it depends how PNG heavy a site is and how large the PNGs are before building. If you have 100s of PNGs then yeah, 15kb per PNG is a lot but let's remember that this is only including PNGs from the theme itself, not those uploaded to Wordpress. If you have 100s of PNGs in just the theme, I'd think that somthing was up.

Just my opinion, feel free to disagree.

@mmirus

This comment has been minimized.

Copy link
Member

mmirus commented Jan 8, 2019

That's a fair point, @vandie. Would you mind testing it at 6 (or I would suggest 5, actually), though, and letting us know what balance that strikes between build speed and file size? I rigged up some tests on my end but I think it would be more meaningful to hear how it compares for your use case.

@vandie

This comment has been minimized.

Copy link
Contributor Author

vandie commented Jan 9, 2019

@mmirus Putting it on 5 upped the time to 319s and saved an extra 6kb per file. If that's more along what you'd want, I'll push up a commit to this PR.

@mmirus

This comment has been minimized.

Copy link
Member

mmirus commented Jan 9, 2019

I'm personally okay with level 2, but defer to @swalkinshaw or @retlehs. My take is that for the number of PNGs typically bundled with a theme, 10-15kb isn't significant. If it becomes an area where more careful optimization is needed for a specific project, the images could easily be hand compressed before being dropped into the assets folder (I do that with any image I use anyways) or the level could be tweaked for the needs of that site.

I'm happy with either outcome since this is a big build time improvement in both cases.

PS - thank you for running the additional test!

@JulienMelissas

This comment has been minimized.

Copy link
Member

JulienMelissas commented Jan 9, 2019

Cool results & discussion here. I'm going to play devil's advocate and say... my build processes all run in a CI/CD workflow. An extra few seconds (even a minute) on a production build is usually not important compared to the fact that I could be saving a few kb per image.

I'd be fine with going with level 2, but maybe adding something in the docs linking back to this discussion and letting people know that it can be modified if your site has a lot of assets and you'd rather wait.

@vandie

This comment has been minimized.

Copy link
Contributor Author

vandie commented Jan 9, 2019

@JulienMelissas, No worries about devil's advocate, it's good to hear other opinions. We also use CI but we actually make multiple builds on mulitiple projects a day for our staging & testing servers so the time quickly adds up. Also going from setting 7 to setting 2 saves (as I said in the first post) about 8 minutes. Which is significantly more than the single minute extra you said you were ok with. I would however agree with linking back to this discussion as it may help others in future.

@QWp6t

This comment has been minimized.

Copy link
Member

QWp6t commented Jan 10, 2019

I'm more with @JulienMelissas on this one.

In Sage 10, we have config.enabled.imagemin, which would allow a developer to disable imagemin in local development (via config-local.json) while still having your CI run it.

It would be trivial to have Sage 9 work the same way. Just edit L23 (in webpack.config.optimize.js) to read config.enabled.imagemin and then add imagemin: argv.watch to config.js.

You could even add support for another parameter if your particular use case warrants it, so maybe your CI passes --imagemin to yarn build:production (in which case you'd have imagemin: argv.imagemin in config.js) when merging into master or when tagging a new release, but I wouldn't want to complicate Sage 9 (or 10) in this way. It's merely an example for how adding such a property to config.enabled could be further utilized.

All this aside, we should at least be caching the results of imagemin. That way maybe an initial local build takes 15 minutes in some scenarios, but subsequent builds should be much faster.

tl;dr- I'd suggest caching compressed images and adding a config.enabled.imagemin option to config.js to provide greater flexibility for disabling imagemin.

@vandie

This comment has been minimized.

Copy link
Contributor Author

vandie commented Jan 14, 2019

Ok, so were do we stand here?

@QWp6t QWp6t requested review from austinpray , knowler , retlehs and swalkinshaw Jan 14, 2019

@knowler
Copy link
Member

knowler left a comment

I almost never have PNGs in themes — I pull in client-uploaded images or am using JPGs/SVGs — but for the sake of testing I tried it with just one photo and I definitely noticed a huge improvement in speed and not a lot of difference in result with the decreased setting to 2. pngquant seems to be what is doing all the heavylifting PNG-wise anyway (disabling optipng all together didn’t make a significant difference).

I also agree with adding the note to the docs and a potential setting for enabling or disabling Imagemin in Sage’s asset config (or maybe difference image optimization profiles? i.e. 0 = disabled, 1 = some optimization, 2 = average optimization, 3 = more optimization).

@QWp6t QWp6t merged commit 899ceac into roots:master Jan 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@QWp6t

This comment has been minimized.

Copy link
Member

QWp6t commented Jan 15, 2019

Thanks for the PR!

@vandie

This comment has been minimized.

Copy link
Contributor Author

vandie commented Jan 15, 2019

No worries

ptrckvzn added a commit to ptrckvzn/sage that referenced this pull request Jan 30, 2019

Merge remote-tracking branch 'upstream/master' into coriander
* upstream/master:
  Filter template hierarchy for embed templates (roots#2145)
  Decrease optipng level (roots#2143)
  Update sponsors [ci skip]
  Unescape post titles
  9.0.7
  When you're sloppy and forget things 😇
  Require Node 8+, also test PHP 7.2 and 7.3
  Shoutout to npm: https://npm.community/t/npm-install-downgrading-resolved-packages-from-https-to-http-registry-in-package-lock-json/1818
  Update to Bootstrap 4.2.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.