Implementation of ResponseWriter using pooled Netty buffers #2805

spericas · 2021-02-25T19:43:03Z

Changes in this PR:

Some new methods in DataChunk
New DataChunk implementation based on ByteBuf (Netty)
Changes in ResponseWriter to use the new DataChunk implementation (ByteBufDataChunk)
Changes in BareResponseImpl to consume ByteBufDataChunk's
Allocation of underlying ByteBuf's in ByteBufDataChunk using Netty's pool

With all these changes, the DataChunk's passed from Jersey/Helidon's layer are now backed by pooled Netty's ByteBufs. This should reduced the amount of time spent running GC's in MP applications.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

…it tests are passing. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

…d up release logic to avoid warnings in logs. Better implementation of isFlushChunk. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

…nstream using a semaphore and volatiles. Ensure onComplete is called at most once. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

ljnelson · 2021-02-26T22:16:41Z

webserver/jersey/src/main/java/io/helidon/webserver/jersey/ResponseWriter.java

                req.next();
            });
            return new OutputStream() {
                @Override
-                public void write(int b) throws IOException {
+                public void write(int b) {


(This could be a static class.)

This could be OutputStream.nullOutputStream();

ljnelson · 2021-02-26T22:22:27Z

webserver/jersey/src/main/java/io/helidon/webserver/jersey/ResponseWriter.java

+            while (len > 0) {
+                if (byteBuf == null) {
+                    awaitRequest();
+                    byteBuf = PooledByteBufAllocator.DEFAULT.buffer(BYTEBUF_DEFAULT_SIZE);


@spericas This was what I was asking about earlier today. This will not make use of direct buffers, and perhaps that is just fine. On the other hand, if you use ByteBufAllocator.DEFAULT you'll get some perhaps useful behavior.

Sorry; I got slightly confused here. PooledByteBufAllocator will indeed use direct buffers. I guess my comment is more: if you simply use ByteBufAllocator.DEFAULT you pick up whatever default behavior Netty wants in the future.

But If we use ByteBufAllocator today, are we guaranteed to get a pooled buffer now? After all, that is the whole point of this PR.

If you see https://github.com/netty/netty/blob/1529ef1794e0a6654fa4334fd979b769d6940e61/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L77-L93 you can see what it does. In terms of guarantees, there is very little documentation so effectively no contract as to what you can expect or depend upon.

webserver/webserver/src/main/java/io/helidon/webserver/ByteBufDataChunk.java

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

romain-grecourt

How about merging ByteBufDataChunk and ByteBufRequestChunk ?

spericas · 2021-03-01T20:48:19Z

How about merging ByteBufDataChunk and ByteBufRequestChunk ?

That class includes the reference queue's logic. Perhaps we can connect the two at some point, but that's a task for a different day in my view.

ljnelson · 2021-03-03T18:52:19Z

webserver/webserver/src/main/java/io/helidon/webserver/ByteBufDataChunk.java

+    @Override
+    public int remaining() {
+        int remaining = 0;
+        for (ByteBuf byteBuf : byteBufs) {


If you're going to do stuff with multiple ByteBuf instances, consider Netty's built-in CompositeByteBuf. It may or may not suit your needs.

ljnelson

LGTM; I made a note about Netty's built-in CompositeByteBuf.

* Initial prototype based on Netty buffers. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Revised implementation of ByteBufDataChunk and ResponseWriter. All unit tests are passing. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Removed test. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Checkstyle problems. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Ensure proper release of buffers. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Renamed DataChunkImpl as we now have multiple implementations. Cleaned up release logic to avoid warnings in logs. Better implementation of isFlushChunk. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Fixed checkstyle. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Fixed checkstyle. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Copyright in module-info. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Lazy creation of DataChunkOutputStream publisher. Guard access to downstream using a semaphore and volatiles. Ensure onComplete is called at most once. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Implement old method in new ByteBufDataChunk class for testing purposes. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Use a static create() method instead of a public constructor. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

…io#2805) * Initial prototype based on Netty buffers. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Revised implementation of ByteBufDataChunk and ResponseWriter. All unit tests are passing. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Removed test. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Checkstyle problems. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Ensure proper release of buffers. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Renamed DataChunkImpl as we now have multiple implementations. Cleaned up release logic to avoid warnings in logs. Better implementation of isFlushChunk. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Fixed checkstyle. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Fixed checkstyle. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Copyright in module-info. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Lazy creation of DataChunkOutputStream publisher. Guard access to downstream using a semaphore and volatiles. Ensure onComplete is called at most once. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Implement old method in new ByteBufDataChunk class for testing purposes. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com> * Use a static create() method instead of a public constructor. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

spericas added 7 commits February 16, 2021 14:18

Initial prototype based on Netty buffers.

5c2f627

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Revised implementation of ByteBufDataChunk and ResponseWriter. All un…

126ec58

…it tests are passing. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Removed test.

fe0311e

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Checkstyle problems.

60e5345

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Ensure proper release of buffers.

ab2fcb5

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Merge branch 'master' into response-writer-performance

775afb4

Renamed DataChunkImpl as we now have multiple implementations. Cleane…

43719c8

…d up release logic to avoid warnings in logs. Better implementation of isFlushChunk. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

spericas self-assigned this Feb 25, 2021

spericas added webserver enhancement New feature or request labels Feb 25, 2021

spericas added this to the 2.3.0 milestone Feb 25, 2021

spericas added 4 commits February 25, 2021 14:58

Fixed checkstyle.

61e252a

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Fixed checkstyle.

557f78b

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Copyright in module-info.

3af74fd

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Lazy creation of DataChunkOutputStream publisher. Guard access to dow…

31a2b32

…nstream using a semaphore and volatiles. Ensure onComplete is called at most once. Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

spericas changed the title ~~Implementation of ResponseWriter using pooled Netty buffers~~ WIP: Implementation of ResponseWriter using pooled Netty buffers Feb 26, 2021

Implement old method in new ByteBufDataChunk class for testing purposes.

6286bd7

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

spericas changed the title ~~WIP: Implementation of ResponseWriter using pooled Netty buffers~~ Implementation of ResponseWriter using pooled Netty buffers Feb 26, 2021

spericas requested review from romain-grecourt, ljnelson and olotenko February 26, 2021 22:09

ljnelson reviewed Feb 26, 2021

View reviewed changes

romain-grecourt reviewed Feb 26, 2021

View reviewed changes

webserver/webserver/src/main/java/io/helidon/webserver/ByteBufDataChunk.java Outdated Show resolved Hide resolved

Use a static create() method instead of a public constructor.

f530913

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

romain-grecourt reviewed Mar 1, 2021

View reviewed changes

Merge branch 'master' into response-writer-performance

1958af7

romain-grecourt approved these changes Mar 3, 2021

View reviewed changes

ljnelson reviewed Mar 3, 2021

View reviewed changes

ljnelson approved these changes Mar 3, 2021

View reviewed changes

spericas merged commit bc4f4e8 into helidon-io:master Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of ResponseWriter using pooled Netty buffers #2805

Implementation of ResponseWriter using pooled Netty buffers #2805

spericas commented Feb 25, 2021 •

edited

Loading

ljnelson Feb 26, 2021

danielkec Mar 9, 2021

ljnelson Feb 26, 2021

ljnelson Feb 26, 2021

spericas Mar 1, 2021

ljnelson Mar 2, 2021

romain-grecourt left a comment

spericas commented Mar 1, 2021

ljnelson Mar 3, 2021

ljnelson left a comment

Implementation of ResponseWriter using pooled Netty buffers #2805

Implementation of ResponseWriter using pooled Netty buffers #2805

Conversation

spericas commented Feb 25, 2021 • edited Loading

ljnelson Feb 26, 2021

Choose a reason for hiding this comment

danielkec Mar 9, 2021

Choose a reason for hiding this comment

ljnelson Feb 26, 2021

Choose a reason for hiding this comment

ljnelson Feb 26, 2021

Choose a reason for hiding this comment

spericas Mar 1, 2021

Choose a reason for hiding this comment

ljnelson Mar 2, 2021

Choose a reason for hiding this comment

romain-grecourt left a comment

Choose a reason for hiding this comment

spericas commented Mar 1, 2021

ljnelson Mar 3, 2021

Choose a reason for hiding this comment

ljnelson left a comment

Choose a reason for hiding this comment

spericas commented Feb 25, 2021 •

edited

Loading