zlib icon indicating copy to clipboard operation
zlib copied to clipboard

CRC32C Checksum support

Open mullermp opened this issue 1 year ago • 6 comments

Adds CRC32C checksum support into Zlib, which includes methods: crc32c_table, crc32c, and crc32c_combine. Whenever CRC32 is supported, these methods should also exist.

Zlib itself does NOT have CRC32C support. CRC32C is a similar calculation to CRC32C except using a different polynomial. In this approach, I am defining a crc32c method that follows the interface of do_checksum and the function it calls in Zlib. For crc32c_combine, I had to replicate the existing crc32_combine code but with the other polynomial.

I've also refactored crc_table to be called crc32_table but otherwise preserved backwards compatibility by having both methods.

mullermp avatar Jun 11 '24 17:06 mullermp

I had chatted with @hsbt and he seemed ok with the idea.

mullermp avatar Jun 11 '24 18:06 mullermp

Umm I'd like to confirm your motivation or background to introduce an adopted code in the frontend gem. I'm not strongly opposite, but it sounds bringing some complexity to the simple frontend code.

sorah avatar Jun 11 '24 18:06 sorah

The motivation was for CRC32C to be in a bundled gem such as zlib instead of having our customers install other gems to use it. S3 provides checksum support using CRC32C. In general, we prefer not to take dependencies where possible, unless it's on stdlib. We currently have a C implementation in the aws-crt gem which uses FFI. I do agree it's weird to have this because zlib itself does not have CRC32C. I've opened a feature request in the zlib repo for that but it's unlikely if it will ever be serviced, hence this is the best option I saw. I think long term, if CRC32C is introduced into C Zlib, this code can be refactored away. My understanding is that it's functioning correct as is (I've compared it to other implementations). I'm sure there are better, faster, implementations of it.

mullermp avatar Jun 11 '24 18:06 mullermp

Seems that Zlib's author does not want CRC32C upstream - https://github.com/madler/zlib/issues/981. It would still be nice to be part of this gem but I'm open to other ideas. I think a bundled gem of some sort is preferred in either case.

mullermp avatar Jun 11 '24 18:06 mullermp

I am against including this in zlib. It would be best to maintain this as a separate gem. There would have to be a strong case made for bundling such a gem with Ruby, considering the direction of moving things out of stdlib to bundled gems, and unbundling gems previously bundled.

jeremyevans avatar Jun 11 '24 19:06 jeremyevans

I understand your position. I'm happy to drop this if the ruby core team wants.

mullermp avatar Jun 11 '24 19:06 mullermp