search

ruby / zlib

Ruby interface for the zlib compression/decompression library
Other
49 stars 35 forks source link

CRC32C Checksum support #83

Open mullermp opened 3 months ago

mullermp commented 3 months ago

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 commented 3 months ago

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

sorah commented 3 months ago

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.

mullermp commented 3 months ago

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 commented 3 months ago

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.

jeremyevans commented 3 months ago

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.

mullermp commented 3 months ago

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