Merged
Conversation
As an improvement, the new implementation does not rely on reflection to adjust for a starting checksum. Instead, it uses adler32_combine, as that computation is reasonably cheap anyhow. Compared to Adler32Ext, the adjustment is also only performed when a starting checksum other than the default is provided.
Compared to the inlined version of Adler32, this one unfortunately still relies on reflection for adjusting the starting checksum. This is due to the fact that crc32_combine is much more expensive, and completely dominates the runtime up to quite long byte lists. However, in the event that the reflection trick stops working, we now fall back to the slower approach, ensuring compatibility with an unlikely future Java release where the internal CRC32 field name is changed. Compared to CRC32Ext, however, neither the reflection nor the slow path is used when the starting checksum matches the default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The main change here is that CRC32Ext and Adler32Ext were replaced/inlined into RubyZlib. The abstractions are not used elsewhere in the code, and considering the obvious copy-paste errors in CRC32Ext, I'm guessing they haven't been used outside RubyZlib.
For Adler32, the reflection trick previously used was replaced with a call to adler32_combine, which has reasonable performance (and does not rely on accessing a private field).
For CRC32, we still (conditionally) use the reflection trick for optimum performance as using crc32_combine completely dominates the runtime for anything but really long byte lists. Compared to the previous version, we at least fall back on a slower solution in the unlikely even that the private variable name would change in a future release.
For both cases, this change optimises the normal case (with default starting checksums) to not perform any additional computation. Thus, the normal case neither requires reflection, nor slow calls to combine.
While changing the RubyZlib class, the TODO regarding using the CRC-32 table from JZlib was resolved as well.