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

CRC32C Checksum support #83

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

CRC32C Checksum support #83

wants to merge 4 commits into from

Conversation

mullermp
Copy link

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.

Copy link
Member

@sorah sorah left a comment

Choose a reason for hiding this comment

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

zlib gem is a frontend for zlib native library, so I'm wondering whether it is appropriate to introduce adopted code (even with a tiny modification) here. Maybe creating a separate native extension gem (by bringing this adopted and modified code) satisfies the need?

ext/zlib/zlib.c Show resolved Hide resolved
@mullermp
Copy link
Author

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

@sorah
Copy link
Member

sorah commented Jun 11, 2024

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
Copy link
Author

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
Copy link
Author

Seems that Zlib's author does not want CRC32C upstream - madler/zlib#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
Copy link
Contributor

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
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

3 participants