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

Use OpenJDK CRC32C while available #13834

Open
franz1981 opened this issue Feb 9, 2024 · 4 comments
Open

Use OpenJDK CRC32C while available #13834

franz1981 opened this issue Feb 9, 2024 · 4 comments

Comments

@franz1981
Copy link
Contributor

franz1981 commented Feb 9, 2024

Snappy protocol seems to leverage to our Netty CRC32C using a big lookup table and no CPU-specific intrinsics to perform xoring etc etc; OpenJDK seems to have what we need, see https://github.com/openjdk/jdk/blob/52d497619e58a5677bc4a015b1bd87f600f23837/src/java.base/share/classes/java/util/zip/CRC32C.java#L218

It would be nice to:

  1. Use https://openjdk.org/jeps/416 in order to use static final reflection to call into C2C32C byte[] batch intrinsified method at https://github.com/openjdk/jdk/blob/52d497619e58a5677bc4a015b1bd87f600f23837/src/java.base/share/classes/java/util/zip/CRC32C.java#L218
  2. If not possible, verify if the table generation approach at https://github.com/openjdk/jdk/blob/52d497619e58a5677bc4a015b1bd87f600f23837/src/java.base/share/classes/java/util/zip/CRC32C.java#L74 is it worthy
  3. for both: run/build a crc32c & snappy-specific JMH benchmark to verify if is it worthy, perf-wise

NOTE:
although modfying

abstract class ByteBufChecksum implements Checksum {
by adding a new optimized reflective type seems the right thing, it can hide some subtle potential regressions and behaviours which should be considered, ie

  1. new CRC32C is available only after Java >= 9, differently from CRC32 which existing from long time (java 1.1)
  2. if we perform Chechsum wrapping via
    method.invoke(checksum, CompressionUtil.safeNioBuffer(b, off, len));
    , the Method::invoke which piggyback to an invokeinterface (see https://docs.oracle.com/javase/8/docs/api/java/util/zip/Checksum.html) making it no longer bi-morphic because we will have a third JDK Checksum receiver which call the update method, potentially slowing down everyone (I have a solution for that, actually, but is not nide :P ) (- the same problem happen at )
@adwsingh
Copy link
Contributor

adwsingh commented Mar 5, 2024

@franz1981 are you working on this? If not I would be happy to pick up.

@franz1981
Copy link
Contributor Author

thanks @adwsingh yep, please go ahead, I'm not working on it ATM

@franz1981
Copy link
Contributor Author

Ping @adwsingh any cycles to work on this?
If not I can give it to another folk which was interested on it

@adwsingh
Copy link
Contributor

adwsingh commented Apr 3, 2024

Yeah I spent some time last week understanding the issue and making some code changes to test. I can probably pull of a PR by this weekend,

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

No branches or pull requests

2 participants