-
Notifications
You must be signed in to change notification settings - Fork 169
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
[PR] Add support for native memory compression and decompression #311
base: master
Are you sure you want to change the base?
Conversation
Added support to compress and decompress native memory buffers
This feature is essential for any application which works with off heap memory directly. |
Will add unit tests. |
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. Left some nits on the code. Can you add some tests
char *dst_buff = (char *) dst; | ||
char *src_buff = (char *) src; |
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.
this should be void *
if (NULL == (void *) dst) return -ZSTD_error_memory_allocation; | ||
if (NULL == (void *) src) return -ZSTD_error_memory_allocation; |
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.
may be move these below, after you cast dst
to dst_buff
char *dst_buff = (char *) dst; | ||
char *src_buff = (char *) src; |
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.
void *
if (NULL == (void *) dst) return -ZSTD_error_memory_allocation; | ||
if (NULL == (void *) src) return -ZSTD_error_memory_allocation; |
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.
move below
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #311 +/- ##
============================================
- Coverage 60.01% 57.88% -2.13%
- Complexity 308 312 +4
============================================
Files 26 26
Lines 1473 1541 +68
Branches 170 186 +16
============================================
+ Hits 884 892 +8
- Misses 434 494 +60
Partials 155 155 |
Sure, will add test this weekend. Thank you for the review @luben |
@@ -0,0 +1,27 @@ | |||
#!/bin/bash | |||
|
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.
Can you make this filename more descriptive and put a comment on top? Or just don't include it in the commit?
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 will remove this file from the commit. I had tried to add tests to this feature, but it has turned out that it's not that straightforward task. I had to enable sun.misc.Unsafe
access to be able to allocate native memory and Scala (sbt) just makes this impossible (at least for me). I have zero experience in Scala and its tooling. In Java 9+ the access to this class is prohibited by default, so you have to specify additional command line args:
java --add-opens jdk.unsupported/sun.misc=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED
For some reason this does not work with sbt
. Probably I did something wrong.
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.
Can you try to add these options to https://github.com/luben/zstd-jni/blob/master/build.sbt#L39 . I just pushed a change to run the tests in forked JVM so these options will apply.
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 recently moved repo fork to a new owner (organization) - https://github.com/carrotdata/zstd-jni/tree/master. The current PR is in some kind of a zombie state after that. I am going to close this PR and open new one, @luben . What do you think?
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.
install-jar.sh
has been removed.
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 recently moved repo fork to a new owner (organization) - https://github.com/carrotdata/zstd-jni/tree/master. The current PR is in some kind of a zombie state after that. I am going to close this PR and open new one, @luben . What do you think?
Sure, we can continue on a new PR
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.
It looks like we can keep the old one. I removed 'install-jar.sh
and rebased it to the luben:master
.
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 just pushed a change to run the tests in forked JVM so these options will apply.
Hmm, it was not so easy - forking broke builds on Windows and Mac OS, not sure why. So I reverted that
These binary files should not be checked in git - I re-build them on each supported platform for each release. |
Files have been removed. |
Basing anything off
|
It s long way to go until all Java code with direct |
Performance wise, Unsafe no longer wins. |
Jetty? Can it handle 500K+ RPS out of the box? Really doubt :). JFF is finally on par with JNI or slightly better, but for direct memory access and manipulations of bits and bytes outside of Java heap, Unsafe is the champ. And you missed my reqs - JDK 11+ support (actually Java 8+). |
500K+ requests per second is not hard to do. This has been done on an official release of Eclipse Jetty 10, and Jetty 11, and Jetty 12 servers (all of which do not have Unsafe operations anymore). The setup is as follows ...
This setup results in sub 50 byte requests, and sub 200 byte responses (or about 120 bytes for request on network, and 280 bytes for response on network), which is only really useful for load testing the server for requests per second and latency metrics. When I monitor (with something like wireshark) with 1 client to confirm its setup, I'm looking at the total bytes on the network and wanting something sub 400 bytes per request/response exchange and no FIN (we should be using persistent connections). Hitting 510k requests per second is very attainable on a 10GbE network against a Jetty Server with a decent networking interface (some crappy 10GbE interfaces cannot get close to even 20% saturation). Java 8 went EOSL in many contexts already. (eg: google cloud dropped it Jan 2024) |
https://medium.com/deno-the-complete-reference/netty-vs-jetty-hello-world-performance-e9ce990a9294 far from 510K RPS. May be its attainable, may be its - not. Not, I presume. Any Java network server which utilizes any type of thread pool executors will be handicapped due to significant thread context switch overhead. You are free to share links, which confirm, that 510K RPS is attainable for Jetty. I have not managed to find any proof of that statement, quite contrary, I found many benchmarks with a very abysmal performance and latency numbers. I am the developer of the
This is 535K RPS with p99.9 latency less than 1ms. These numbers are within 5% of native Other benchmark results (memory consumption, surprise, surprise) are here:
|
An unconfigured Jetty and testing on the same machine, that person just tested the performance of their localhost network stack, nothing else. That is a horrible set of tests and doesn't test performance of Jetty.
Jetty doesn't use native JVM thread pool executors, it's got it's own and a EatWhatYouKill model that minimizes thread context switching, we even see improvements on CPU caching with this model. When we participated in the TechEmpower benchmarks years ago (back in Jetty 10.0.0 days) we were consistently in the top 5%, and when we learned the tricks of the those above us we could easily get into the top 3%, but those tricks were not representing real world scenarios. |
Congrats, that's a really fantastic outcome. Anyway, this is devolved into a totally different set of arguments. Eclipse Jetty just has to monitor how the new JVMs react to our usage of the current state of zstd-jni. (so far it looks like we have to, at a minimum, document the demands that zstd-jni put to ByteBufferPool implementation, and the JVM command line switches necessary to allow zstd-jni to function.) |
Update README to include custom build steps
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.
please, don't put binaries in the source code.
This PR introduces support for handling native memory buffers that are allocated using the
sun.misc.Unsafe.allocateMemory
API. With this update, it is now possible to compress and decompress data between two native memory buffers, as well as transfer data from a byte array to native memory and vice versa.