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

Update zlib-ng to 2.1.3 #133

Merged
merged 4 commits into from
Jul 23, 2023
Merged

Update zlib-ng to 2.1.3 #133

merged 4 commits into from
Jul 23, 2023

Conversation

JakubOnderka
Copy link
Contributor

@JakubOnderka JakubOnderka commented Jun 10, 2023

zlib-ng 2.1.2 is the first stable release of the 2.1.x branch.

Most important optimizations and Enhancements:

  • Decompression is a lot faster (56% faster measured on AVX2-capable x86-64)
  • Compresson is improved for Level 9, at the cost of a little performance.
  • Compression is improved for Level 3, by switching from deflate_fast to deflate_medium.

Full changelog

@JohnTitor JohnTitor closed this Jun 10, 2023
@JohnTitor JohnTitor reopened this Jun 10, 2023
@JohnTitor
Copy link
Member

Hm, I'm not sure if it's the fault of ctest but CI fails:

  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1295): warning C4047: 'return': 'int (__cdecl *)(zng_streamp,int,const char *,int)' differs in levels of indirection from 'int'
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1300): error C2065: 'zng_deflateInit2_': undeclared identifier
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1300): warning C4047: 'return': 'int (__cdecl *)(zng_streamp,int,int,int,int,int,const char *,int)' differs in levels of indirection from 'int'
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1350): error C2065: 'zng_inflateBackInit_': undeclared identifier
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1350): warning C4047: 'return': 'int (__cdecl *)(zng_streamp,int,unsigned char *,const char *,int)' differs in levels of indirection from 'int'
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1370): error C2065: 'zng_inflateInit_': undeclared identifier
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1370): warning C4047: 'return': 'int (__cdecl *)(zng_streamp,const char *,int)' differs in levels of indirection from 'int'
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1375): error C2065: 'zng_inflateInit2_': undeclared identifier
  D:\a\libz-sys\libz-sys\target\x86_64-pc-windows-msvc\debug\build\systest-zng-5622f69de174c92e\out\all.c(1375): warning C4047: 'return': 'int (__cdecl *)(zng_streamp,int,const char *,int)' differs in levels of indirection from 'int'

@Byron
Copy link
Member

Byron commented Jun 11, 2023

Thanks for the update! I am quite new here and am as puzzled as you why these symbols would go missing all of the sudden. Is this something that reproduces locally?

@JakubOnderka JakubOnderka force-pushed the update-zlib-ng branch 4 times, most recently from fd34141 to dde2cdb Compare June 15, 2023 14:49
@JakubOnderka
Copy link
Contributor Author

JakubOnderka commented Jun 15, 2023

Build tests fixed, but I am not 100 % sure if everything is correct. Can you please check it before merging?

@Byron
Copy link
Member

Byron commented Jun 16, 2023

Thanks so much!

CI is happy now and I wonder if that's all we need. Maybe there is other (avoidable) compatibility problems that arise when this crate is used by flate2 or other major dependents? It seems only manual testing can show what would happen, and I'd be satisfied knowing that flate2 still works with this version of the crate, or that we are not missing anything here to maintain compatibility.

@JakubOnderka JakubOnderka force-pushed the update-zlib-ng branch 4 times, most recently from 1688401 to 76c6f33 Compare June 18, 2023 08:18
@thaliaarchi
Copy link

FYI, zlib-ng 2.1.3, which has now been released, is also marked as stable.

@Byron
Copy link
Member

Byron commented Jul 23, 2023

I'd be the first to want to merge this, and it all looks good to me, but that doesn't mean much.
Could I have another set of eyes?

@Byron Byron changed the title Update zlib-ng to 2.1.2 Update zlib-ng to 2.1.3 Jul 23, 2023
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

I decided to fix my one issue, which was running tests through flate2. It turned out that what's on main doesn't actually compile for me, at least not on MacOS, whereas this branch works just fine. All flate2 tests pass, and I wonder if I should adjust the CI to test libz-sys and libz-ng-sys with the latest version of flate2 as well.

That way we could be more confident that nothing is broken, at least according to one of the main consumers of this crate.

In any case, I'd be willing to merge now.

@Byron Byron mentioned this pull request Jul 23, 2023
@Byron
Copy link
Member

Byron commented Jul 23, 2023

With the additional testing in 5f2cf8d it turns out that what's in this PR is actually working with flate2, whereas what's in main causes failures with zlib-ng and zlib-ng-compat due to memory corruption.

Thus I will merge this PR now and see that I create a new point release.

@Byron Byron merged commit 0b1d46e into rust-lang:main Jul 23, 2023
@Byron Byron removed the request for review from joshtriplett July 23, 2023 15:21
@Byron
Copy link
Member

Byron commented Jul 23, 2023

A new release was published.

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