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

Don't require image bin deps till necessary #125

Merged
merged 1 commit into from
Jan 8, 2014

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Dec 4, 2013

Related to gh-123

@sindresorhus
Copy link
Member

What's the problem? And you need to show some numbers.

@nschonni
Copy link
Contributor Author

nschonni commented Dec 7, 2013

The problem/solution is from @cowboy #123 (comment)
Sorry for the lack of stats, but I got bitten with that issue with time-grunt no longer functioning with the latest Grunt release.

The test target in this repo will not show the benefit For those repos only using one or two of the optimizers, they will not get the penalty of requiring the bin just to resolve a path they won't use.

@shama
Copy link
Member

shama commented Dec 8, 2013

Just did a quick benchmark with this. Seems a bit faster:

shama in ~/Documents/www/grunt-contrib/imagemin on master*
$ time grunt imagemin
Running "imagemin:dist" (imagemin) task
✔ test/fixtures/test.png (saved 10.14 kB)
✔ test/fixtures/test.jpg (saved 5.86 kB)
✔ test/fixtures/test.gif (saved 7 B)
✔ test/fixtures/test-uppercase.PNG (saved 10.14 kB)
✔ test/fixtures/test-uppercase.JPG (saved 5.86 kB)
✔ test/fixtures/test-uppercase.GIF (saved 7 B)
Minified 6 images (saved 32.01 kB)

Done, without errors.

real    0m1.623s
user    0m1.383s
sys 0m0.174s

shama in ~/Documents/www/grunt-contrib/imagemin on nschonni/lazy-require-deps*
$ time grunt imagemin
Running "imagemin:dist" (imagemin) task
✔ test/fixtures/test.png (saved 10.14 kB)
✔ test/fixtures/test.jpg (saved 5.86 kB)
✔ test/fixtures/test.gif (saved 7 B)
✔ test/fixtures/test-uppercase.PNG (saved 10.14 kB)
✔ test/fixtures/test-uppercase.JPG (saved 5.86 kB)
✔ test/fixtures/test-uppercase.GIF (saved 7 B)
Minified 6 images (saved 32.01 kB)

Done, without errors.

real    0m0.463s
user    0m0.412s
sys 0m0.054s

@nschonni
Copy link
Contributor Author

nschonni commented Dec 9, 2013

Thanks @shama !

@nschonni
Copy link
Contributor Author

@sindresorhus anything else to change for this?

@kevva
Copy link
Member

kevva commented Dec 20, 2013

@nschonni, this module is being rewritten (#97). The core has moved to https://github.com/kevva/image-min. Maybe we can change it there.

@nschonni
Copy link
Contributor Author

@kevva I think the rewrite is a good idea, and this can definitely be upstreamed. I'd just like to see this land before that so people can get the benefit now.

@nschonni
Copy link
Contributor Author

nschonni commented Jan 6, 2014

@sindresorhus should I close this

sindresorhus added a commit that referenced this pull request Jan 8, 2014
Don't require image bin deps till necessary
@sindresorhus sindresorhus merged commit c99ac8e into gruntjs:master Jan 8, 2014
@sindresorhus
Copy link
Member

@nschonni can you open a ticket on https://github.com/kevva/image-min/issues about fixing the actual loading speed problem?

@nschonni nschonni deleted the lazy-require-deps branch January 8, 2014 19:27
@nschonni
Copy link
Contributor Author

nschonni commented Jan 8, 2014

@kevva already did the same thing a few weeks ago https://github.com/kevva/image-min/blob/master/imagemin.js#L93 or where you thinking about a general performance issue?

@sindresorhus
Copy link
Member

@nschonni i'd rather figure out why it's so slow to load.

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

Successfully merging this pull request may close these issues.

4 participants