jrudolph commented on PR #2409: URL: https://github.com/apache/pekko/pull/2409#issuecomment-3468248622
> `ZstdDirectBufferCompressingStreamNoFinalizer` and related classes were designed to work with sources that provide direct `ByteBuffer`'s i.e. Netty/Java NIO however in our case we are dealing with `ByteString` where we don't have direct `ByteBuffer`'s (`ByteString.asByteBuffer` is not a direct byte buffer) so we always need up having to copy into an array that then gets pushed into a direct `ByteBuffer` before going into the compress function. Using `ZstdCompressCtx` we can just directly feed/get the byte arrays. You can ignore the copy cost, compression is not cheap anyway. Also, the copying is likely happening anyways as you otherwise have to globally lock the heap array and interfere with GC... > Now lets get onto buffering It's a complex topic how and when a (streaming) compressor should outputs compressed data. Roughly speaking, for best compression, you leave the decision completely to the compressor. For more interactive usage you want to send any data as early as possible (kind of a nagle situation but for compression). The compressor cannot know what you want and we cannot know what the user wants. Also note, that streaming does not equal interactive usage, you might want to stream data to run with less memory, so for the (native) compressor API you don't want to make the size of the chunks part of the decision of when to flush. For our streaming API, the problem is that we cannot easily provide the signal when to flush using a raw ByteString based stream. For that reason, in the deflate compressor we allow to set a flush mode and flush after each ByteString. That way the user still has full control over when the flushing does happen (by buffering data in the stream if required). We should not add any inner logic on top of that. Let's not overthink the interface between our compression component and the native component. In general, if we can provide all of incoming data in one go to the compressor that's ok because it's already on the heap anyway and owned by us. But we should never hold anything back. > When it comes to decompression, the logic can reman as is. We don't need to use a `DirectByteBufferPool` because we only create 2 buffers per stream instantiation (**not** per element of stream) and those `ByteBuffer`'s are reused for each incoming element, albeit it should be updated so that those `ByteBuffer`'s get cleaned up with `if (ByteBufferCleaner.isSupported) ByteBufferCleaner.clean(byteBuffer)`. Using `ZstdDecompressCtx` isn't going to gain us much net benefit as we need to use the native streaming capabilities of zstd-jni to accept incoming data until we can get to a point where we can decompress. In any case, we should avoid cycling through direct buffers. E.g. using one for the lifetime of the whole stream could be acceptable. Btw. zstd is somewhat different from deflate in that regard, you can only ever decompress full blocks while deflate can at least in theory start streaming while still receiving a block (see https://github.com/facebook/zstd/issues/1359 and https://fastcompression.blogspot.com/2016/04/working-with-streaming.html, the tldr; is that a compressed block is max 128kb anyway and that usually dwarfs any LZ77 context buffers you might have to keep around anyways). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
