Add LZ4 compressor with Java9 perf improvements #77153
Conversation
Tim-Brooks
added
>non-issue
:Distributed/Network
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Have you been able to get a sense of how much faster compression/decompression get with this change? |
|
@jpountz - I added the tests from the lz4-java library. It does require committing binary files. I hope that is fine. I benchmarked this using JMH on m5d.4xlarge. The compress benchmark is compressing ~1MB of highly compressible observability data 64KB at a time. And the uncompress benchmark uncompresses those 64KB blocks. I think the forked version is faster at decompressing the data than the unsafe version because there is a place in the Unsafe version where data is being copied twice (unnecessarily). LZ4UnsafeUtils.java static void safeIncrementalCopy(byte[] dest, int matchOff, int dOff, int matchLen) {
for(int i = 0; i < matchLen; ++i) {
dest[dOff + i] = dest[matchOff + i];
UnsafeUtils.writeByte(dest, dOff + i, UnsafeUtils.readByte(dest, matchOff + i));
}
} |
|
Also ARM (m6gd.4xlarge): |
There was a problem hiding this comment.
Thanks for running these benchmarks @tbrooks8, the results are super impressive. Let's share this on the lz4-java repository?
It does require committing binary files. I hope that is fine.
Hmm good question. On the one hand I don't like checking in files that don't compress well, but on the other hand I would like to make sure we have solid testing, as any bug here could have terrible consequences like silent data corruption. I wonder what other options we have, e.g. could the build download them?
server/src/test/java/org/elasticsearch/common/compress/lz4/ESLZ4Tests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/common/compress/lz4/ESLZ4CompressorTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/compress/lz4/ESLZ4Compressor.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/compress/lz4/ESLZ4Decompressor.java
Outdated
Show resolved
Hide resolved
| matchOff += 4; | ||
| int dec = 0; | ||
|
|
||
| assert dOff >= matchOff && dOff - matchOff < 8; |
There was a problem hiding this comment.
I do notice that this is a copy from original, but I wonder how dOff - matchOff can ever be >= 4 since we handle that case above (and then add 4 to both here)? I feel like I am missing something obvious, help me 🙂
There was a problem hiding this comment.
No idea. I accidentally had copied the class file opposed to the original source. I modified this PR to copy the original source which logically makes more sense. If it is reordered in the class file, well 🤷♂️.
server/src/test/java/org/elasticsearch/common/compress/lz4/ESLZ4CompressorTests.java
Outdated
Show resolved
Hide resolved
libs/lz4/src/test/java/org/elasticsearch/lz4/ESLZ4CompressorTests.java
Outdated
Show resolved
Hide resolved
libs/lz4/src/test/java/org/elasticsearch/lz4/ESLZ4DecompressorTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I want to be very careful with these changes given our history with LZO, where a very subtle bug caused index files to be silently corrupted upon recovery. I like the suggestion that @henningandersen made to compare that your fork produces the same bytes but we don't seem to be testing this on the test that runs on real-world data (or did I miss it?). This small addition would help me feel more confident that the changes are correct since real-world data tends to have patterns that are hard to reproduce in synthetic data.
Other than that the change looks good to me. I hope we'll be able to merge these changes upstream in the near future to be able to move back to something that is more widely deployed.
libs/lz4/src/test/java/org/elasticsearch/lz4/ESLZ4CompressorTests.java
Outdated
Show resolved
Hide resolved
I added this assertion to compare against the lz4-java safe instance. |
| tester.copyOf(data), off, len, | ||
| compressed, 0, maxCompressedLength); | ||
|
|
||
| // Modified to compress using an unforked lz4-java compressor and verify that the results are same. |
|
|
||
| public static final LZ4Compressor INSTANCE = new ESLZ4Compressor(); | ||
|
|
||
| ESLZ4Compressor() { |
There was a problem hiding this comment.
I think this is redundant since the default constructor will be created by javac
| return dOff - destOff; | ||
| } | ||
|
|
||
| public int compress(byte[] src, int srcOff, int srcLen, byte[] dest, int destOff, int maxDestLen) { |
There was a problem hiding this comment.
I believe @Override is missed here since we override the compress method from LZ4Compressor
| public class ESLZ4Decompressor extends LZ4FastDecompressor { | ||
| public static final LZ4FastDecompressor INSTANCE = new ESLZ4Decompressor(); | ||
|
|
||
| ESLZ4Decompressor() { |
There was a problem hiding this comment.
Ditto here: I believe the constructor is redundant.
| ESLZ4Decompressor() { | ||
| } | ||
|
|
||
| public int decompress(byte[] src, int srcOff, byte[] dest, int destOff, int destLen) { |
There was a problem hiding this comment.
I think @Override is missed here, decompress is overridden from LZ4FastDecompressor
| int literalLen = token >>> 4; | ||
| if (literalLen == 15) { | ||
| byte len; | ||
| for(boolean var11 = true; (len = SafeUtils.readByte(src, sOff++)) == -1; literalLen += 255) { |
There was a problem hiding this comment.
var11 seems to be some decompiling artifact? It doesn't seem to be used anywhere.
| int matchLen = token & 15; | ||
| if (matchLen == 15) { | ||
| byte len; | ||
| for(boolean var15 = true; (len = SafeUtils.readByte(src, sOff++)) == -1; matchLen += 255) { |
There was a problem hiding this comment.
Ditto for var15, seems to be a redundant variable
| } | ||
| } | ||
|
|
||
| public int decompress(ByteBuffer src, int srcOff, ByteBuffer dest, int destOff, int destLen) { |
There was a problem hiding this comment.
I think @Override is missed here
| } | ||
|
|
||
| static void safeIncrementalCopy(byte[] dest, int matchOff, int dOff, int matchLen) { | ||
| for (int i = 0; i < matchLen; ++i) { |
There was a problem hiding this comment.
Can this loop be replaced with if (matchLen >= 0) System.arraycopy(dest, matchOff, dest, dOff, matchLen)?
| // Modified wildIncrementalCopy to mirror version in LZ4UnsafeUtils | ||
| static void wildIncrementalCopy(byte[] dest, int matchOff, int dOff, int matchCopyEnd) { | ||
| if (dOff - matchOff < 4) { | ||
| for (int i = 0; i < 4; ++i) { |
There was a problem hiding this comment.
I think we can do System.arraycopy(dest, matchOff, dest, dOff, 4) here
| } | ||
|
|
||
| static int encodeSequence(byte[] src, int anchor, int matchOff, int matchRef, int matchLen, byte[] dest, int dOff, int destEnd) { | ||
| final int runLen = matchOff - anchor; |
There was a problem hiding this comment.
runLen seems to be redundant
|
Have we considered simply doing a proper fork of https://github.com/lz4/lz4-java and referencing that rather than vendoring the code into the Elasticsearch codebase? Since the intention is for this to be temporary we could just fork, publish the JAR as a GitHub package and pull it in as a normal dependency. We'd also have a better audit of how it differs from upstream and could easily verify behavior but just running the existing test suites. Since https://github.com/tbrooks8/lz4-java/tree/do_not_copy_twice already exists we can just use that. |
|
Recognizing that I know nothing about this.
I assume this is easy? Like I would not need to do the whole publish to Maven thing for my fork? Do we feel comfortable using my account's fork, or should we just be forking it into an You did not address this: we would need CI on the forked version. Is that something we can setup? |
These are all valid suggestions for cleaning up the code. However, we are attempting to only make behavioral changes that are necessary for our performance goals. We would like to otherwise keep it identical with the original code. |
Yes, should be easy. I can help facilitate the publication bit. Using an
We don't need CI if we don't intend on doing active development on this fork. If we're just implementing a patch we can manually test and build the artifacts for publication. |
|
I would prefer to keep this as is, perhaps zip'ing the test resources if that helps a little (I think it is not strictly necessary). We are not copying the entire lz4 code, only the relevant pieces that we need to subclass and overwrite. We want to be able to move forward with this for possible inclusion into 7.x. I think having a separate fork will make it difficult to support, since the fix and release process will be known only to a small group. It will be sort of temporary, but we do not know nor can we control the timeline for getting this included into Even if we were to fork, we should keep the changes in new classes like Tim did in this PR to ensure that if someone ends up using lz4-java in our code base, they get the original version not the modified version (which may not fit every purpose, for instance it currently assumes only few threads use the compressor). So it is less of a fork with modification and much more new classes with improved behavior for our usage pattern. First step as we see it is to get this into our main branch to get feedback on the full suite of benchmarks. |
| @@ -0,0 +1,256 @@ | |||
| /* | |||
There was a problem hiding this comment.
Yeah, we need to fix the license header here. We should remove the Elastic header and instead add @notice to the first line of the Apache 2 header to indicate this is vendored code. Just do a search on that string to see other examples. Same applies to all files in this project.
That makes sense. If the changes aren't a general solution it's probably going to take more work to get things merged upstream. That said, we should still aim to do this vs vendoring code into our codebase. |
Java9 added a number of features that are useful to improve compression and decompression. These include the Arrays#mismatch method and VarHandles. This commit adds compression tools forked from the java-lz4 library which include these improvements. We hope to contribute these changes back to the original project, however the project currently supports Java7 so this is not possible at the moment.
|
Do we have an open issue to add this new lib jar to release manager? As it is we've broken testing for external plugin authors. cc @rjernst |
For posterity I've added this new artifact to release manager. |
Java9 added a number of features that are useful to improve compression and decompression. These include the Arrays#mismatch method and VarHandles. This commit adds compression tools forked from the java-lz4 library which include these improvements. We hope to contribute these changes back to the original project, however the project currently supports Java7 so this is not possible at the moment.
Java9 added a number of features that are useful to improve compression
and decompression. These include the Arrays#mismatch method and
VarHandles. This commit adds compression tools forked from the java-lz4
library which include these improvements. We hope to contribute these
changes back to the original project, however the project currently
supports Java7 so this is not possible at the moment.