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 ByteBufChecksum optimisation for CRC32 and Adler32 #9242

Merged

Conversation

Projects
None yet
4 participants
@iamaleksey
Copy link
Contributor

commented Jun 14, 2019

Motivation:

Because of a simple bug in ByteBufChecksum#updateByteBuffer(Checksum),
ReflectiveByteBufChecksum is never used for CRC32 and Adler32, resulting
in direct ByteBuffers being checksummed byte by byte, which is
undesriable.

Modification:

Fix ByteBufChecksum#updateByteBuffer(Checksum) method to pass the
correct argument to Method#invoke(Checksum, ByteBuffer).

Result:

ReflectiveByteBufChecksum will now be used for Adler32 and CRC32 on
Java8+ and direct ByteBuffers will no longer be checksummed on slow
byte-by-byte basis.

@netty-bot

This comment has been minimized.

Copy link

commented Jun 14, 2019

Can one of the admins verify this patch?

Fix ByteBufChecksum optimisation for CRC32 and Adler32
Motivation:

Because of a simple bug in ByteBufChecksum#updateByteBuffer(Checksum),
ReflectiveByteBufChecksum is never used for CRC32 and Adler32, resulting
in direct ByteBuffers being checksummed byte by byte, which is
undesriable.

Modification:

Fix ByteBufChecksum#updateByteBuffer(Checksum) method to pass the
correct argument to Method#invoke(Checksum, ByteBuffer).

Result:

ReflectiveByteBufChecksum will now be used for Adler32 and CRC32 on
Java8+ and direct ByteBuffers will no longer be checksummed on slow
byte-by-byte basis.

@iamaleksey iamaleksey force-pushed the iamaleksey:fix-bytebuf-checksum-optimisation branch from 747fe0c to fa9beac Jun 14, 2019

@njhill

njhill approved these changes Jun 14, 2019

Copy link
Member

left a comment

Nice one!

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit a29532d into netty:4.1 Jun 16, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 16, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@iamaleksey thanks a lot!

@normanmaurer normanmaurer self-assigned this Jun 16, 2019

@normanmaurer normanmaurer added the defect label Jun 16, 2019

normanmaurer added a commit that referenced this pull request Jun 16, 2019

Fix ByteBufChecksum optimisation for CRC32 and Adler32 (#9242)
Motivation:

Because of a simple bug in ByteBufChecksum#updateByteBuffer(Checksum),
ReflectiveByteBufChecksum is never used for CRC32 and Adler32, resulting
in direct ByteBuffers being checksummed byte by byte, which is
undesriable.

Modification:

Fix ByteBufChecksum#updateByteBuffer(Checksum) method to pass the
correct argument to Method#invoke(Checksum, ByteBuffer).

Result:

ReflectiveByteBufChecksum will now be used for Adler32 and CRC32 on
Java8+ and direct ByteBuffers will no longer be checksummed on slow
byte-by-byte basis.
@iamaleksey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@normanmaurer You are welcome (:

There are a few more issues in this area - some of them major. I'll try to file PRs for all of them next week.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.