-
Notifications
You must be signed in to change notification settings - Fork 125
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
Process files in parallel #531
Conversation
Pure curiosity, is there some kind of thread pool sharing between files? To avoid just reimplementing GNU parallel, as I commented previously. |
@q3cpma Yes, oxipng uses Rayon with a single thread pool which is used for all parallel tasks, including reduction evaluations and compression trials. Since this was already a part of oxipng, the change required here was essentially just replacing the files iterator with a parallel iterator. Rayon handles the workload automatically - see here for more info: https://github.com/rayon-rs/rayon/blob/master/FAQ.md |
On Sun Jul 9, 2023 at 10:13 PM CEST, andrews05 wrote:
@q3cpma Yes, oxipng uses Rayon with a single thread pool which is used for all parallel tasks, including reduction evaluations and compression trials. Since this was already a part of oxipng, the change required here was essentially just replacing the files iterator with a parallel iterator. Rayon handles the workload automatically - see here for more info: https://github.com/rayon-rs/rayon/blob/master/FAQ.md
Cool! I kinda expected this, knowing the Rust ecosystem, but still worth asking.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
b804acb
to
5b38f61
Compare
I'm sorry, just saw this & read the There it mentions having to do special adaptations for |
I'm not sure entirely about wasm-bindgen-rayon, the last of my knowledge is that wasm does not have proper threading support and so wasm users will need to disable the "parallel" feature in oxipng. It's possible that may have changed and there's a way to support threading on wasm now, someone more specialized in wasm may know. |
This PR makes the oxipng binary process multiple files in parallel, finally fulfilling shssoichiro#275. There seemed to be some debate about whether oxipng _should_ do this or not but there's a couple of reasons I think it makes sense: 1. The concern seemed mostly around the complexity of such a feature. Not to worry, it was trivial* 🙂 2. Since then, oxipng has dropped from a max of something like 180 simultaneous compression trials down to 10, which is very much a good thing but it does mean it's not utilising any more cores than that. Some benchmarks on around 100 files on a machine with 8 cores: Level | Master time | PR time -|-|- 2 | 28.303 | 19.005 3 | 36.507 | 23.089 5 | 1:10.86 | 1:16.01 *Some additional changes were required in order to make sure sensible output is printed to the terminal, since things won't be in order anymore. Here's some example output from before: ``` Processing: tests/files/fully_optimized.png file size = 67 bytes (0 bytes = 0.00% decrease) File already optimized Processing: tests/files/corrupted_header.png Invalid PNG header detected Processing: tests/files/verbose_mode.png file size = 102480 bytes (12228 bytes = 10.66% decrease) Output: tests/files/verbose_mode.png ``` And after: ``` Processing: tests/files/verbose_mode.png Processing: tests/files/fully_optimized.png Processing: tests/files/corrupted_header.png tests/files/corrupted_header.png: Invalid PNG header detected tests/files/fully_optimized.png: Could not optimize further, no change written 102480 bytes (10.66% smaller): tests/files/verbose_mode.png ``` Closes shssoichiro#275, shssoichiro#84, shssoichiro#169, shssoichiro#196 and shssoichiro#419. [edit] This is the last thing I wanted to land before the next release 🥳
This PR makes the oxipng binary process multiple files in parallel, finally fulfilling #275. There seemed to be some debate about whether oxipng should do this or not but there's a couple of reasons I think it makes sense:
Some benchmarks on around 100 files on a machine with 8 cores:
*Some additional changes were required in order to make sure sensible output is printed to the terminal, since things won't be in order anymore. Here's some example output from before:
And after:
Closes #275, #84, #169, #196 and #419.
[edit] This is the last thing I wanted to land before the next release 🥳