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

Cleanup & optimize performance of IO (switch to libspng) #57

Merged
merged 14 commits into from
Dec 16, 2021
Merged

Conversation

eWert-Online
Copy link
Collaborator

@eWert-Online eWert-Online commented Aug 15, 2021

This PR is an effort to switch from libpng to libspng.
This doubles the performance of tiff, bmp and jpg (probably webp and avif too).

The performance of png sadly drops by ~200ms (edit: ~100ms).
This is because the encoder of libspng runs much faster compared to libpng when given a single byte array.
Because png was the only format which didn't use the bigarray, but the row pointers, it now is a bit slower.

The encoder of libspng is not yet released officially, so maybe there are also some pending performance optimizations... 😄

Using libspng also reduced the binary size by ~100kb.
In the future, we could also think about using miniz instead of zlib. It is also lighter and in some cases faster than zlib.

The big advantage that we get with this rewrite, is that all io implementations now use a bigarray to store and read their data. (Also we only have 1 PNG implementation now instead of 2).
So implementing something like #53 will probably get a bit easier.
We could also think about giving ocaml / reasonml users (me 😄 ) the ability to get the raw bigarray data out of the io functors.

@dmtrKovalenko What do you think?

Todo

  • cross platform tests (windows and linux)

Performance Benchmarks

BMP:
benchmark-bmp

JPG / JPEG
benchmark-jpg

PNG 😞
benchmark-png

TIFF
benchmark-tiff

@eWert-Online eWert-Online marked this pull request as ready for review September 23, 2021 14:39
@eWert-Online
Copy link
Collaborator Author

eWert-Online commented Sep 23, 2021

@dmtrKovalenko Do you have any clue, why the windows pipeline gets an EACCES error?
Edit: It works on my local windows machine 😄
Edit 2: It could be related to this: esy/esy#1347
Edit 3: I guess it is more likely this issue: esy/esy#1083

@dmtrKovalenko
Copy link
Owner

I'll spend some time on weekend with this

@dmtrKovalenko
Copy link
Owner

dmtrKovalenko commented Sep 27, 2021

Thanks for this, but I want to understand how having 1 implementation will help to implement buffers? Buffer will always be the encoded array data that we need to decode using specific codec, right?

So implementing something like #53 will probably get a bit easier.

Actually this type of lib where we are fighting for performance having optimizations like using raw pointers data without any data copying makes sense as for me, because making us as fast as possible. WDYT?

@eWert-Online
Copy link
Collaborator Author

I just realized that I had an error in my thought process.
I thought we could just dump the data given from the buffer directly into the bigarray. Now I realize, that we still have to run it trough the decoder of the given type 🤦
So we could still use raw pointers in theory.

I still think libspng is an optimization over libpng in terms of speed though.
If the drop of ~100ms is your only concern with this PR, I think there are still some perfomance gains on the table I didn't yet explore, because I wasn't sure if you would be open to this change in general.

@dmtrKovalenko
Copy link
Owner

Yeah I think that we can merge it but left 2 different decoders only for benchmarks 🙈

@eWert-Online
Copy link
Collaborator Author

eWert-Online commented Sep 27, 2021

Yeah I think that we can merge it but left 2 different decoders only for benchmarks

I am not sure I understand this correctly. You want to have libpng and libspng at the same time?

@dmtrKovalenko
Copy link
Owner

Oh my gosh yeah that is not an option -__- I`ll get back to this code in a few hours with more fresh mind to figure everything out

@eWert-Online
Copy link
Collaborator Author

Well...let me first take a look, if I can optimize the performance of the PNG IO a bit further.
Maybe even making it faster than the one we had before 🙂
Currently decoding and encoding with libspng is faster than with libpng. The only thing slowing us down is that we are reading from ocamls bigarray with all its boxed data.

@ManasJayanth
Copy link
Contributor

re: Windows
Can someone try this nightly build and let me know if it still fails?
0.6.11-19279b

https://www.npmjs.com/package/@esy-nightly/esy/v/0.6.11-19279b

@dmtrKovalenko
Copy link
Owner

@ManasJayanth doesn't work https://dev.azure.com/dmtrkovalenko/odiff/_build/results?buildId=285&view=results basically get back to downloading esy@nightly here

@ManasJayanth
Copy link
Contributor

@dmtrKovalenko In the build logs you linked, I don't think it's using the nightly I suggested. Am I missing something?

@eWert-Online
Copy link
Collaborator Author

@dmtrKovalenko
I made soma adjustments. I mainly moved from a double nested for-loop for the x and y axis to a single loop.
We now have equal performance for png with the main branch:

diff
(main is the first one)

We are currently still incrementing x and y.
If I am not mistaken though, I think we can compute everything from the loop offset. Definetly something I want to investigate in the future.

@eWert-Online
Copy link
Collaborator Author

@dmtrKovalenko
Did you find time to look at this yet? 🙂
Is this approach something you could imagine moving forward with?

@dmtrKovalenko
Copy link
Owner

Sorry for so long delay, I had pretty busy period several months. I will take a look and merge this PR until the end of this week.

Honestly, I Was scared on the amount of code to read (the new deps) 😔

@dmtrKovalenko dmtrKovalenko self-requested a review December 16, 2021 10:23
Copy link
Owner

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Once again I am sorry for so long review

@dmtrKovalenko dmtrKovalenko merged commit 5bbbc13 into main Dec 16, 2021
@dmtrKovalenko dmtrKovalenko deleted the libspng branch December 16, 2021 10:27
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.

3 participants