Add zstd decoder by skyguard1 · Pull Request #13531 · netty/netty

skyguard1 · 2023-08-06T14:13:39Z

Motivation:

Zstandard(https://facebook.github.io/zstd/) is a high performance, high compression ratio compression algorithm,This pr is to add netty support for the zstandard algorithm,The implementation of zstandard algorithm relies on zstd-jni (https://github.com/luben/zstd-jni), which is an openSource third-party library,Apache Kafka is also using this library for message compression processing.
This is the copy of #10422

Modification:

Add ZstdDecoder and test case.

Result:

netty supports ZSTD with zstdDecoder

skyguard1 · 2023-08-06T14:27:09Z

Hi @normanmaurer @chrisvest
Sorry for the delay
There is something to be said about this change

Streaming decompression is currently the only method given by the author of zstd-jni to solve the problem of segmented transmission of compressed data blocks. The zstd native library does not currently support reading the length of a compressed data block, but the zstd native library plans to support reading IS_LAST_BLOCK in a future release.The author of zstd-jni believes that using ZstdInputStream will not bring more overhead than Zstd, although there will be more stream opening and closing in the segmented transmission scenario
See Support getting block info for decompression facebook/zstd#3709
Is it possible to provide a way to determine the size of the compressed data block when data is transmitted in segments luben/zstd-jni#165
Add Zstd.isLastBlock() for decompression luben/zstd-jni#271
Regarding IOUtils.copy(zstdIs, bos), it may be implemented in ZstdDecoder, considering that more dependencies will be introduced, but apache commons-compress is the commonly used tool, of course, there is no big problem in implementing this in ZstdDecoder

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

codec/pom.xml

normanmaurer · 2023-08-17T08:45:37Z

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

please implement this yourself so we dont need the extra dependency

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

codec/src/test/java/io/netty/handler/codec/compression/ZstdIntegrationTest.java

normanmaurer · 2023-08-24T12:13:33Z

@skyguard1 after checking the code in more detail I think there are multiple things that needs to be addressed. Give me a few days to dig into it.

normanmaurer · 2023-09-07T16:54:29Z

I didn't forget about this one... Its on my todo list for next week.

skyguard1 · 2023-09-18T12:19:52Z

@normanmaurer
ByteToMessageDecoder will not call decode() even if new data arrives, because ZstdDecoder reads in loop in decode(), so ByteToMessageDecoder's accumulator cannot be used to add bytes
But it can change while to if in decode()
I tested it. It works. Is there any problem with it?

abergmeier · 2023-10-30T00:05:20Z

Is there any news on this? We really could use it.

normanmaurer · 2023-10-30T07:50:08Z

@abergmeier unfortunately no... I am swamped atm with other stuff

KingMob · 2023-11-06T04:34:12Z

Looking forward to this landing!

maryantocinn · 2024-03-01T10:55:34Z

Any update on this?

normanmaurer · 2024-03-06T15:46:40Z

Unfortunately no and I don't know when I will have time

Signed-off-by: xingrufei <qhdxssm@qq.com>

This reverts commit 87531c7

Signed-off-by: xingrufei <qhdxssm@qq.com>

normanmaurer · 2024-03-14T11:30:15Z

@skyguard1 PTAL... sorry it took me so long

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

normanmaurer · 2024-03-15T16:52:44Z

@luben PTAL again

luben

LGTM. Thanks for making the suggested change.

normanmaurer · 2024-03-15T17:00:20Z

@luben one question... luben/zstd-jni#302 ... Is it somehow possible to limit the max block size so we are sure ZstdInputStream* will not buffer too much ?

normanmaurer · 2024-03-15T18:09:51Z

Ok this is complete now.. will merge once the build did pass once

normanmaurer · 2024-03-15T18:10:14Z

See luben/zstd-jni#302 for limit the block size

normanmaurer · 2024-03-15T18:37:53Z

@chrisvest can you have a look as well

chrisvest

Spotted a potential simplification, but otherwise looks good.

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

Signed-off-by: xingrufei <qhdxssm@qq.com>

codec/src/test/java/io/netty/handler/codec/compression/ZstdDecoderTest.java

Signed-off-by: xingrufei <qhdxssm@qq.com>

…oder.java Co-authored-by: Chris Vest <mr.chrisvest@gmail.com>

Signed-off-by: xingrufei <qhdxssm@qq.com>

yuzawa-san · 2024-03-16T12:37:54Z

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java

+    @Override
+    public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
+        super.handlerAdded(ctx);
+        zstdIs = new ZstdInputStreamNoFinalizer(inputStream);


the current usage will allocate internal byte[] for each instantiation which will contribute to memory pressure, but there is a option to use a BufferPool: https://github.com/luben/zstd-jni/blob/20014d19a91a1b04e4e198ac1cb85b5acb1c1142/src/main/java/com/github/luben/zstd/ZstdInputStreamNoFinalizer.java#L62

RecyclingBufferPool.INSTANCE is a stock implementation, but it may be possible to create an implementation which is backed by the netty's PooledAllocator if that is desired.

here is skeletal BufferPool implementation which is backed by Netty's allocator

public class ZstdBufferPool implements BufferPool { // the constructor seems to use only a single buffer private final List<ByteBuf> byteBufs = new ArrayList<>(1); @Override public ByteBuffer get(int capacity) { ByteBuf byteBuf = alloc.heapBuffer(capacity); byteBufs.add(byteBuf); return byteBuf.nioBuffer(); } @Override public void release(ByteBuffer buffer) { // pass, instead release these buffers after we close stream } // call this after the stream is closed public void releaseAll(){ for(ByteBuf byteBuf: byteBufs){ bufBufs.release(); } } }

actually nevermind, the implementation does not appear to support ByteBuffers which are slices of larger arrays, so I guess RecyclingBufferPool.INSTANCE is the only option.

https://github.com/luben/zstd-jni/blob/20014d19a91a1b04e4e198ac1cb85b5acb1c1142/src/main/java/com/github/luben/zstd/Zstd.java#L1538

https://github.com/luben/zstd-jni/blob/20014d19a91a1b04e4e198ac1cb85b5acb1c1142/src/main/java/com/github/luben/zstd/ZstdInputStreamNoFinalizer.java#L164

I think we should pull it in as it is ... we can do a follow for improvements later

normanmaurer · 2024-03-18T10:18:23Z

Thanks to all... This was merged :)

Motivation: Zstandard(https://facebook.github.io/zstd/) is a high performance, high compression ratio compression algorithm,This pr is to add netty support for the zstandard algorithm,The implementation of zstandard algorithm relies on zstd-jni (https://github.com/luben/zstd-jni), which is an openSource third-party library,Apache Kafka is also using this library for message compression processing. This is the copy of #10422 Modification: Add ZstdDecoder and test case. Result: netty supports ZSTD with zstdDecoder --------- Signed-off-by: xingrufei <qhdxssm@qq.com> Co-authored-by: xingrufei <xingrufei@yl.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com> Co-authored-by: Chris Vest <mr.chrisvest@gmail.com>

Motivation: Zstandard(https://facebook.github.io/zstd/) is a high performance, high compression ratio compression algorithm,This pr is to add netty support for the zstandard algorithm,The implementation of zstandard algorithm relies on zstd-jni (https://github.com/luben/zstd-jni), which is an openSource third-party library,Apache Kafka is also using this library for message compression processing. This is the copy of #10422 Modification: Add ZstdDecoder and test case. Result: netty supports ZSTD with zstdDecoder Signed-off-by: xingrufei <qhdxssm@qq.com> Co-authored-by: skyguard1 <qhdxssm@qq.com> Co-authored-by: xingrufei <xingrufei@yl.com> Co-authored-by: Chris Vest <mr.chrisvest@gmail.com>

luben reviewed Aug 16, 2023

View reviewed changes

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java Outdated Show resolved Hide resolved

skyguard1 force-pushed the add_zstd_decoder branch 2 times, most recently from 666e4c4 to 6822cd9 Compare August 16, 2023 23:27

normanmaurer reviewed Aug 17, 2023

View reviewed changes

codec/pom.xml Outdated Show resolved Hide resolved

normanmaurer requested changes Aug 17, 2023

View reviewed changes

skyguard1 force-pushed the add_zstd_decoder branch 2 times, most recently from bb488b0 to 16fd37a Compare August 17, 2023 11:28

normanmaurer requested changes Aug 22, 2023

View reviewed changes

skyguard1 force-pushed the add_zstd_decoder branch from 16fd37a to 123ed34 Compare August 22, 2023 08:56

skyguard1 mentioned this pull request Sep 7, 2023

ZstdDecoder missing #13587

Closed

skyguard1 requested a review from normanmaurer September 20, 2023 11:24

KingMob mentioned this pull request Nov 6, 2023

Add support for wrap-decompression middleware clj-commons/aleph#683

Open

mirromutth mentioned this pull request Jan 26, 2024

[feature] Compression protocol support asyncer-io/r2dbc-mysql#216

Closed

skyguard1 and others added 5 commits March 14, 2024 08:32

Update by review

2323b05

Signed-off-by: xingrufei <qhdxssm@qq.com>

format code

86cce35

Revert "format code"

0921103

This reverts commit 87531c7

Add decompressedSize to exception message

30d6c26

Signed-off-by: xingrufei <qhdxssm@qq.com>

Do not use buffers to reduce memory allocation

010bcb7

Signed-off-by: xingrufei <qhdxssm@qq.com>

normanmaurer force-pushed the add_zstd_decoder branch from 47f0d49 to 010bcb7 Compare March 14, 2024 07:33

refactor

349c652

luben reviewed Mar 15, 2024

View reviewed changes

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java Outdated Show resolved Hide resolved

normanmaurer added 3 commits March 15, 2024 17:22

No need for volatile

9b0c362

Null out ref

e630f71

Only alloc if needed

43c1756

luben approved these changes Mar 15, 2024

View reviewed changes

cleanup

d07ad4a

normanmaurer requested a review from chrisvest March 15, 2024 18:37

normanmaurer approved these changes Mar 15, 2024

View reviewed changes

chrisvest approved these changes Mar 16, 2024

View reviewed changes

codec/src/main/java/io/netty/handler/codec/compression/ZstdDecoder.java Outdated Show resolved Hide resolved

Use ZstdOutputStream test ZstdDecoder

e69772d

Signed-off-by: xingrufei <qhdxssm@qq.com>

chrisvest reviewed Mar 16, 2024

View reviewed changes

codec/src/test/java/io/netty/handler/codec/compression/ZstdDecoderTest.java Outdated Show resolved Hide resolved

codec/src/test/java/io/netty/handler/codec/compression/ZstdDecoderTest.java Outdated Show resolved Hide resolved

skyguard1 and others added 7 commits March 16, 2024 09:29

Use ZstdOutputStreamNoFinalizer to test ZstdDecoder

d19dde6

Signed-off-by: xingrufei <qhdxssm@qq.com>

Update codec/src/main/java/io/netty/handler/codec/compression/ZstdDec…

e44af43

…oder.java Co-authored-by: Chris Vest <mr.chrisvest@gmail.com>

simplify based on comment

6862d07

Update license and remove redundant initialize int

6f4f1d8

Signed-off-by: xingrufei <qhdxssm@qq.com>

Add null check for bytebuf

782ddc1

Signed-off-by: xingrufei <qhdxssm@qq.com>

Revert null check

5131b13

Signed-off-by: xingrufei <qhdxssm@qq.com>

Ensure Zstd availability

23fd430

Signed-off-by: xingrufei <qhdxssm@qq.com>

yuzawa-san reviewed Mar 16, 2024

View reviewed changes

Guard against cases when we can't load zstd libs

d239e18

normanmaurer merged commit 47e0f4f into netty:4.1 Mar 18, 2024

yuzawa-san mentioned this pull request Mar 18, 2024

Use buffer recycler for ZstdDecoder #13911

Closed

Uh oh!

Conversation

skyguard1 commented Aug 6, 2023

Uh oh!

skyguard1 commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

skyguard1 Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Aug 24, 2023

Uh oh!

normanmaurer commented Sep 7, 2023

Uh oh!

skyguard1 commented Sep 18, 2023

Uh oh!

abergmeier commented Oct 30, 2023

Uh oh!

normanmaurer commented Oct 30, 2023

Uh oh!

KingMob commented Nov 6, 2023

Uh oh!

maryantocinn commented Mar 1, 2024

Uh oh!

normanmaurer commented Mar 6, 2024

Uh oh!

normanmaurer commented Mar 14, 2024

Uh oh!

Uh oh!

normanmaurer commented Mar 15, 2024

Uh oh!

luben left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 15, 2024

Uh oh!

normanmaurer commented Mar 15, 2024

Uh oh!

normanmaurer commented Mar 15, 2024

Uh oh!

normanmaurer commented Mar 15, 2024

Uh oh!

chrisvest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuzawa-san Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

yuzawa-san Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

yuzawa-san Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

normanmaurer Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

skyguard1 commented Aug 6, 2023 •

edited

Loading