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

Process files in parallel #531

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

andrews05
Copy link
Collaborator

@andrews05 andrews05 commented Jul 9, 2023

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:

  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 #275, #84, #169, #196 and #419.

[edit] This is the last thing I wanted to land before the next release 🥳

@q3cpma
Copy link

q3cpma commented Jul 9, 2023

Pure curiosity, is there some kind of thread pool sharing between files? To avoid just reimplementing GNU parallel, as I commented previously.

@andrews05
Copy link
Collaborator Author

@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

@q3cpma
Copy link

q3cpma commented Jul 9, 2023 via email

Copy link
Owner

@shssoichiro shssoichiro left a 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

@shssoichiro shssoichiro merged commit 4ae64c5 into shssoichiro:master Jul 11, 2023
11 checks passed
@TPS
Copy link

TPS commented Jul 11, 2023

I'm sorry, just saw this & read the rayon ReadMe, which jogged a memory:

There it mentions having to do special adaptations for wasm targets via wasm-bindgen-rayon. I know a while ago some special work was done in oxipng to get that working. Does it need to be adjusted in light of this PR & related?

@shssoichiro
Copy link
Owner

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.

@andrews05 andrews05 deleted the parallel-files branch July 11, 2023 20:03
Pr0methean pushed a commit to Pr0methean/oxipng that referenced this pull request Dec 1, 2023
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 🥳
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.

Please support running in parallel across multiple files
4 participants