-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add native Lz4, Snappy, and Zstd #201
Conversation
That's awesome! |
I think that you could add |
@dain we should apply for ARM runners so we can add a coverage here as well |
Right now some of the tests are not passing on ARM (due to the lack of the libgplcompression for aarch64) |
Some benchmarks: |
Ya, I think we should make this a proper module, not that most people use module capable systems.... follow up work
Looks like the beta is open now https://github.blog/2024-06-03-arm64-on-github-actions-powering-faster-more-efficient-build-systems/
For benchmarks, you'll want to look at these algorithms:
Then in the DataSet you'll want to narrow down to one of the collections, or it will take forever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
src/main/java/io/airlift/compress/deflate/DeflateCompressor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/airlift/compress/deflate/DeflateCompressor.java
Outdated
Show resolved
Hide resolved
private static final MethodHandle IS_ERROR_METHOD; | ||
private static final MethodHandle GET_ERROR_NAME_METHOD; | ||
|
||
// TODO should we just hardcode this to 3? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we? or should this be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we include our own libraries, loading this value from the library seems redundant.
The underlying format for zstd in Hadoop is the standard framed format. The Hadoop JNI code for Zstd corrupts the process symbol table on Linux by loading the Zstd library into the global process symbol table.
|
||
# Requirements | ||
|
||
This library requires a Java 1.8+ virtual machine containing the `sun.misc.Unsafe` interface running on a little endian platform. | ||
This library requires a Java 22+ virtual machine containing the `sun.misc.Unsafe` interface running on a little endian platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider building a multi-release JAR, so us miserable folks stuck on Java 1.8 can still use the pure Java bits of this library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2.x branch (master) uses the new java.lang.foreign
APIs that are only present in Java 22+. The release-0.x
branch contains the code that works with Java 1.8.
d5637e8
to
8e35673
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
a51471a
to
62ef599
Compare
Can you move the code to |
I think the key wording here is "an executable JAR". So this only works for the application's JAR being run with |
Typo in commit message |
|
||
private static String getPlatform() | ||
{ | ||
String name = System.getProperty("os.name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like this idea. We could convert the OS names to macos
and linux
, which would make the directories cleaner:
String name = System.getProperty("os.name");
name = switch (name) {
case "Linux" -> "linux";
case "Mac OS X" -> "macos";
default -> throw new LinkageError("Unsupported OS platform: " + name);
}
This will require changing the previous commit that downloads the native libraries.
Add support for native Lz4, Snappy, and Zstd using the
java.lang.foreign
APIs