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

Fix Issue 22998 - Update to zlib 1.2.12 #8428

Merged
merged 1 commit into from
Apr 10, 2022
Merged

Conversation

ibara
Copy link
Contributor

@ibara ibara commented Apr 9, 2022

Hello --

I've updated the copy of zlib in Phobos.

Here's a reprinting of the big-ticket items from the zlib.net homepage:
Version 1.2.12 has these key improvements over 1.2.11:

  • Fix a deflate bug when using the Z_FIXED strategy that can result in out-of-bound accesses.
  • Fix a deflate bug when the window is full in deflate_stored().
  • Speed up CRC-32 computations by a factor of 1.5 to 3.
  • Use the hardware CRC-32 instruction on ARMv8 processors.
  • Speed up crc32_combine() with powers of x tables.
  • Add crc32_combine_gen() and crc32_combine_op() for fast combines.
    Due to the bug fixes, any installations of 1.2.11 should be replaced with 1.2.12.

Also maintains the fix from here:
ibara@1b6cf80

Everything appears OK on my OpenBSD/amd64 machine. But perhaps more testing from others is in order.

@ibara
Copy link
Contributor Author

ibara commented Apr 9, 2022

(Force-pushed to quell a buildkite/phobos error complaining about the style of a comment...)
And also to maybe to get the bot to recognize this PR.

Give the same result as crc32_combine(), using op in place of len2. op is
is generated from len2 by crc32_combine_gen(). This will be faster than
crc32_combine() if the generated op is used more than once.
*/

/* various hacks, don't look :) */
Copy link
Member

Choose a reason for hiding this comment

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

🙈

etc/c/zlib.d Outdated
(the size of the history buffer). It should be in the range 8 .. 15 for this
(the size of the history buffer). It should be in the range 8..15 for this
Copy link
Member

Choose a reason for hiding this comment

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

The style checker will always fire for this, so better revert to the previous range style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 9, 2022

Thanks for your pull request and interest in making D better, @ibara! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22998 normal Update to zlib 1.2.12

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8428"

@dkorpel
Copy link
Contributor

dkorpel commented Apr 9, 2022

Please add "Fix Issue 22998" to the commit title so the bot links it to the bugzilla issue.

Update zlib to the 1.2.12 release
Version 1.2.12 has these key improvements over 1.2.11:
* Fix a deflate bug when using the Z_FIXED strategy that can result in out-of-bound accesses.
* Fix a deflate bug when the window is full in deflate_stored().
* Speed up CRC-32 computations by a factor of 1.5 to 3.
* Use the hardware CRC-32 instruction on ARMv8 processors.
* Speed up crc32_combine() with powers of x tables.
* Add crc32_combine_gen() and crc32_combine_op() for fast combines.
Due to the bug fixes, any installations of 1.2.11 should be replaced with 1.2.12.
@RazvanN7 RazvanN7 merged commit 5bd99f7 into dlang:master Apr 10, 2022
@ibara ibara deleted the zlib-1.2.12 branch April 10, 2022 15:46
@lulcat
Copy link

lulcat commented Apr 12, 2022

Hi. I was just looking at the zlib 1.2.11 to 1.2.12 this very moment and saw your post from these days. Great! This was due to wanting to update zstd to current. I believe that finally there has come a natural replacement of the venerable zlib/[gzip]. Anyway, thanks for this, and wondering @ibara , if this is something you might join in or have plans about etc? I will test the 1.2.12, I suppose I need to grab the git version?
On a side note, why is the sources of zlib-1.2.11 in etc/c/zlib? I noticed it was mainly the !_DMD macro thing which was the only difference. Does dmd compile with those sources implicitly or something? (I ask because I am uncertain if I locally should drop zstd lib sources potentially in there or so).

@ibara
Copy link
Contributor Author

ibara commented Apr 12, 2022

Since you pinged me, when you say updating zstd, do you mean the dub package here: https://code.dlang.org/packages/zstd ? If so, it's not my package so I can't tell you when it will be updated.

If you're talking about replacing zlib with zstd in Phobos, then I imagine that would be a bigger change in need of serious discussion (and numbers) to demonstrate that it is a net gain. I am not part of the DLF, just a user and I guess de facto OpenBSD OS maintainer for D, so I doubt a proposal from me would carry much weight without lots of hard numbers. I don't have the time to do that right now.

@ibuclaw
Copy link
Member

ibuclaw commented Apr 13, 2022

I'm hearing whispers that 1.2.13 is already upcoming due to a CRC regression that's been fixed in the devel branch. If it isn't happening before the next release, perhaps we should cherry-pick?

@ibuclaw
Copy link
Member

ibuclaw commented Apr 13, 2022

@ibara I've just remembered that a changelog entry would be ideal so it gets picked up in the notes for the next release.

@ibara
Copy link
Contributor Author

ibara commented Apr 13, 2022

I thought the bugzilla tracking and closing would do that for us. But can add a changelog entry as well.

@ibara
Copy link
Contributor Author

ibara commented Apr 13, 2022

I'm hearing whispers that 1.2.13 is already upcoming due to a CRC regression that's been fixed in the devel branch. If it isn't happening before the next release, perhaps we should cherry-pick?

Good idea.

@lulcat
Copy link

lulcat commented Apr 14, 2022

Looking at how that bug is worded, cherrypicking might not be uber important (I don't dare say), but had it been literally erronous crc's in current, then yes. Regarding zstd @ibara , yes I mean tthe dub package. I have updated the interface file (using dstep and cleaning up a few bits and bobs there), but I haven't touched the d files the original author had layered on top. It works as before it seems. With that said, in the examples, I get a true vs. false result in 'streaming.d' when doing binary files vs text files. So I assume it's got to do with some string encoding or something although I am not quite getting it why it happens. I'm not tremendous at all with D so. With that said, should someone wish to update it, I can link the dstep file, although it did a fairly good job. Failed to convert two 0ULL to ulong and fixing the version string pretty much as before/zlib.d does. MIght be a digression from this but I include a link anyway. Can remove if preferred to not derail. :)

https://gist.github.com/lulcat/3221f8d30fc8a8aa12faa4024c56edf0

P.S. Forgot to mention, great stuff updating the zlib package, important to have these things up to date.

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

Successfully merging this pull request may close these issues.

6 participants