JdkZlibEncoder can use pooled heap buffers for deflater input by dferstay · Pull Request #11891 · netty/netty

dferstay · 2021-12-04T20:02:07Z

Motivation:

Previously, when the input ByteBuf was not heap allocated we copied the
input into a new byte array allocated on the heap for every encode() call.
This puts high-throughput clients that use the JdkZlibEncoder under memory
pressure.

Modifications:

Now, when the input ByteBuf is not heap allocated we copy the input into a
heap ByteBuf allocated from the configured ByteBufAllocator (which might be pooled), releasing it after
the encode() has completed.

Result:

The result is less heap allocation and GC activity when the PooledByteBufAllocator is used (which is the default)

Fixes #11890

Signed-off-by: Daniel Ferstay [email protected]

Motivation: Previously, when the input ByteBuf was not heap allocated we copied the input into a new byte array allocated on the heap for every encode() call. This puts high-throughput clients that use the JdkZlibEncoder under memory pressure. Modifications: Now, when the input ByteBuf is not heap allocated we copy the input into a heap ByteBuf allocated from the PooledByteBufAllocator, releasing it after the encode() has completed. Result: The result is less heap allocation and GC activity. Fixes netty#11890 Signed-off-by: Daniel Ferstay <[email protected]>

dferstay · 2021-12-04T20:14:40Z

Below is a memory profile before the change.

dferstay · 2021-12-04T20:16:15Z

After the change JdkZlibEncoder.encode is not burning so bright.

normanmaurer · 2021-12-05T07:48:23Z

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

+                // skip all bytes as we will consume all of them
+                uncompressed.skipBytes(len);
+            } else {
+                heapBuf = PooledByteBufAllocator.DEFAULT.heapBuffer(len, len);


Suggested change

heapBuf = PooledByteBufAllocator.DEFAULT.heapBuffer(len, len);

heapBuf = ctx.alloc().heapBuffer(len, len);

Thank you; done.

normanmaurer · 2021-12-05T07:49:03Z

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

+                        // We did not consume everything but the buffer is not writable anymore. Increase the capacity to
+                        // make more room.
+                        out.ensureWritable(out.writerIndex());
+                    }


Please revert the formatting changes.

@normanmaurer ,
Are you referring to the additional indentation? If so, it is due to the addition of the try / finally block to ensure that the allocated heapBuf is released on method return. Do do you want me to remove the try / finally?

Check the checkstyle errors

Got it; the checkstyle errors should now be addressed.

normanmaurer · 2021-12-05T07:49:18Z

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

+                writeHeader = false;
+                if (wrapper == ZlibWrapper.GZIP) {
+                    out.writeBytes(gzipHeader);
+                }


please revert formatting changes

normanmaurer · 2021-12-05T07:49:31Z

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

-        }
+            if (wrapper == ZlibWrapper.GZIP) {
+                crc.update(inAry, offset, len);
+            }


please revert formatting changes

normanmaurer · 2021-12-05T07:50:04Z

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

-            uncompressed.readBytes(inAry);
-            offset = 0;
-        }
+        ByteBuf heapBuf = null;


make this final and assign in the if / else blocks

@normanmaurer ,

make this final and assign in the if / else blocks

I introduced a try / finally so that we would not leak the reference to the allocated heap buffer; this accounts for the formatting changes above and it also confuses the Java compiler when making heapBuf final; see attached screenshot.

normanmaurer · 2021-12-05T07:50:24Z

Also please sign or ICLA: https://netty.io/s/icla

dferstay · 2021-12-06T18:01:56Z

Also please sign or ICLA: https://netty.io/s/icla

Signed :)

normanmaurer · 2021-12-07T11:04:16Z

@dferstay can you please revert the formatting changes ?

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

…Encoder.java

normanmaurer · 2021-12-08T10:43:06Z

@dferstay thanks a lot ! Would be cool if you can add details to the adopters page: https://netty.io/wiki/adopters.html

dferstay · 2021-12-08T16:45:55Z

@dferstay thanks a lot ! Would be cool if you can add details to the adopters page: https://netty.io/wiki/adopters.html

@normanmaurer,
Cheers; will do! 👍

…11891) Motivation: Previously, when the input ByteBuf was not heap allocated we copied the input into a new byte array allocated on the heap for every encode() call. This puts high-throughput clients that use the JdkZlibEncoder under memory pressure. Modifications: Now, when the input ByteBuf is not heap allocated we copy the input into a heap ByteBuf allocated from the configured ByteBufAllocator (which might be pooled), releasing it after the encode() has completed. Result: The result is less heap allocation and GC activity when the PooledByteBufAllocator is used (which is the default) Fixes netty#11890 Signed-off-by: Daniel Ferstay <[email protected]> Co-authored-by: Daniel Ferstay <[email protected]> Co-authored-by: Norman Maurer <[email protected]>

normanmaurer requested changes Dec 5, 2021

View reviewed changes

use ByteBufAllocator associated with the channel handler context

a16d32c

fix checkstyle errors

ffafae8

normanmaurer reviewed Dec 8, 2021

View reviewed changes

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

normanmaurer reviewed Dec 8, 2021

View reviewed changes

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

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

4a08b61

…Encoder.java

normanmaurer changed the title ~~JdkZlibEncoder uses pooled heap buffers for deflater input~~ JdkZlibEncoder can use pooled heap buffers for deflater input Dec 8, 2021

normanmaurer merged commit d80a34e into netty:4.1 Dec 8, 2021

normanmaurer added this to the 4.1.71.Final milestone Dec 8, 2021

	heapBuf = PooledByteBufAllocator.DEFAULT.heapBuffer(len, len);
	heapBuf = ctx.alloc().heapBuffer(len, len);

Uh oh!

Conversation

dferstay commented Dec 4, 2021 • edited by normanmaurer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dferstay commented Dec 4, 2021

Uh oh!

dferstay commented Dec 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dferstay Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Dec 5, 2021

Uh oh!

dferstay commented Dec 6, 2021

Uh oh!

normanmaurer commented Dec 7, 2021

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Dec 8, 2021

Uh oh!

dferstay commented Dec 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dferstay commented Dec 4, 2021 •

edited by normanmaurer

Loading

dferstay Dec 6, 2021 •

edited

Loading