make ByteAccumulator recyclable to fix issue #5499 #5520

leonchen83 · 2020-10-28T04:41:36Z

this PR let the ByteAccumulator recyclable. after invoke ByteAccumulator.transferTo method
we can invoke ByteAccumulator.recycle method to reuse byte[] via ByteAccumulator.newByteArray method

Signed-off-by: Baoyi Chen chen.bao.yi@qq.com

lachlan-roberts · 2020-10-28T07:10:53Z

@leonchen83 Thanks for the PR!

The ECA check has failed validation for some reason, take a look here for more info:
https://www.eclipse.org/jetty/documentation/current/contributing-patches.html

leonchen83 · 2020-10-28T07:32:40Z

@lachlan-roberts
I do some change just right now to fix test failure and check style, please be informed

joakime

Remove the ThreadLocal.

Also, fix your ECA, otherwise we cannot merge this.

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java

.../main/java/org/eclipse/jetty/websocket/common/extensions/compress/DeflateFrameExtension.java

.../java/org/eclipse/jetty/websocket/common/extensions/compress/PerMessageDeflateExtension.java

gregw

@leonchen83 @lachlan-roberts I think this is more or less the right approach, but I think more can be done as there is still a fair bit of copying, plus we might end up with lots of max size buffers in the list.

A couple of ideas....

The algorithm in copyChunk could be:

if the byte array passed is the last one returned by newByteArray
- If there are no chunks in the list OR there is not enough free space in last item in list
  - just add that array to the list.
  - do not let the buffer be returned again by newByteArray
- else there is room in the last buffer in the list
  - copy the data into the free space in the last entry.
  - allow the passed buffer to be returned again by next call to newByteArray
else
- throw IllegalArgumentException

The other thing that could greatly improve this class is to pass the BufferPool to it, so that rather than creation byte arrays, it creates a ByteBuffers with arrays. If the final list only has a single entry, then it can be used as a ByteBuffer rather than with a transferTo

@lachlan-roberts can you work with @leonchen83 to improve this... net necessary as per my suggestions, but do consider them.

leonchen83 · 2020-10-28T09:40:18Z

@joakime
hi, thanks for review code.
I think it is necessary to use ThreadLocal<ByteAccumulator> accumulator
I want to create only 1 ByteAccumulator object per thread. so we can reuse that ByteAccumulator(reuse in thread level) via recycle method
It's not related about websocket thread model.

gregw · 2020-10-28T09:45:52Z

@lachlan-roberts is it possible/easy to associate a ByteAccumulator with a connection?

joakime

Fix your ECA.
Don't make unrelated changes to this PR please.

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java

joakime · 2020-10-28T09:54:12Z

@joakime
hi, thanks for review code.
I think it is necessary to use ThreadLocal<ByteAccumulator> accumulator
I want to create only 1 ByteAccumulator object per thread. so we can reuse that ByteAccumulator(reuse in thread level) via recycle method
It's not related about websocket thread model.

First, fix your ECA. (I cannot stress this enough, don't ignore this).

Test this on slow connections, with larger messages split over many frames, on busy ThreadPools and you'll start to see why we don't use ThreadLocal ourselves.

leonchen83 · 2020-10-28T09:56:34Z

@joakime
hi, thanks for review code.
I think it is necessary to use ThreadLocal<ByteAccumulator> accumulator
I want to create only 1 ByteAccumulator object per thread. so we can reuse that ByteAccumulator(reuse in thread level) via recycle method
It's not related about websocket thread model.

First, fix your ECA. (I cannot stress this enough, don't ignore this).

Test this on slow connections, with larger messages split over many frames, on busy ThreadPools and you'll start to see why we don't use ThreadLocal ourselves.

Hi I'm digging how to fix ECA, sorry but maybe need a little time.

joakime · 2020-10-28T13:15:01Z

@lachlan-roberts is it possible/easy to associate a ByteAccumulator with a connection?

The Extensions, like these Compress Extensions, are already 1 unique instance per connection.
Since the ByteAccumulator in this PR is only used in the Extensions, isn't that effectively the same thing as a ByteAccumulator per connection already?

gregw · 2020-10-28T17:12:03Z

The Extensions, like these Compress Extensions, are already 1 unique instance per connection.
Since the ByteAccumulator in this PR is only used in the Extensions, isn't that effectively the same thing as a ByteAccumulator per connection already?

Ah yes! so no need to pool the BAs in a thread local or anywhere else. Just create one in the connection and the buffers will be reused at least within that connection. A good simple saving without the risk of sharing buffers between connections.

joakime · 2020-10-28T17:24:26Z

The Extensions, like these Compress Extensions, are already 1 unique instance per connection.
Since the ByteAccumulator in this PR is only used in the Extensions, isn't that effectively the same thing as a ByteAccumulator per connection already?

Ah yes! so no need to pool the BAs in a thread local or anywhere else. Just create one in the connection and the buffers will be reused at least within that connection. A good simple saving without the risk of sharing buffers between connections.

And if you have 2 extensions present that both need a ByteAccumulator then the current non-ThreadLocal version is still preferred.

leonchen83 · 2020-10-29T04:41:04Z

Hi
@gregw @joakime @lachlan-roberts
I can't find a way to create ByteAccumulator in non-ThreadLocal way. could you help me to fix that code in this PR?

today I changed following code in this PR

change self define ByteArray to java standard ByteBuffer
optimize recycle mehtod that avoid retain too many elements.
optimize CompressExtension.decompress mehtod so that avoid copy not full byte[] to chunk

lachlan-roberts · 2020-10-29T05:28:47Z

@leonchen83 instead of using the ThreadLocal you can store the ByteAccumulator as a field on the CompressExtension, this way it can used and recycled for all frames on a single websocket connection.

leonchen83 · 2020-10-29T06:36:56Z

@lachlan-roberts
Hi I have done the ThreadLocal fix. please help review the code

lachlan-roberts

We also have access to the ByteBufferPool from CompressExtension so this could be used in the ByteAccumulator.

I think you could pass in the bufferPool in the constructor, then the newByteBuffer method could use the bufferPool, and all the buffers should be released back to the pool when it is recycled.

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java

leonchen83 · 2020-10-29T07:55:11Z

@lachlan-roberts
pass in the bufferPool to ByteAccumulator 's constructor. should change the declaration of constructor, that could break compatibility. you mean add another constructor to ByteAccumulator ?

lachlan-roberts · 2020-10-29T08:58:59Z

@joakime would you consider ByteAccumulator public API? Do we need to worry about compatibility for this.
It might be difficult to use the ByteBufferPool and reduce the copying without at least changing some of the method signatures.

leonchen83 · 2020-10-29T09:37:17Z

@lachlan-roberts
I do some change right now. please help review

lachlan-roberts

I still think there are some problems with this approach. We should discuss further with @gregw and @joakime before continuing.

...on/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/ByteAccumulator.java

lachlan-roberts · 2020-10-29T10:09:48Z

...on/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/ByteAccumulator.java

-        System.arraycopy(buf, offset, copy, 0, length);
-
-        chunks.add(copy);
+        chunks.add(ByteBuffer.wrap(buf, offset, length));


This copyChunk(byte[] buf, int offset, int length) method doesn't really work with the ByteBufferPool. This ByteBuffer will eventually be returned to the pool but we have created it right here. I think this will give an error if it is used.

...on/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/ByteAccumulator.java

leonchen83 · 2020-10-29T10:57:27Z

@lachlan-roberts
yes, I totally agree to further discuss this PR before continuing.

leonchen83 · 2020-10-30T04:46:09Z

after fix
the memory allocation as following

this PR let the ByteAccumulator recyclable. after invoke ByteAccumulator.transferTo method we can invoke ByteAccumulator.recycle method to reuse byte[] via ByteAccumulator.newByteBuffer method Signed-off-by: Baoyi Chen <chen.bao.yi@qq.com>

gregw

This still needs more thought/work.
The bytebuffer pools are really very sensitive as ifa buffer is put in there incorrectly it can have bad security implications, so we need something that is really REALLY certain.

I'm thinking that perhaps we should write a new ByteBufferAccumulator class from scratch rather than try to make the current one less allocating. We can then come up with a much better API contract to ensure that we don't mess up the pools.

gregw · 2020-11-03T16:45:32Z

...on/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/ByteAccumulator.java

+    }
+
+    public void copyChunk(ByteBuffer buf)
+    {


I think we need to check that the buf here was the one last returned from newByteBuffer

gregw · 2020-11-03T16:46:34Z

...on/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/ByteAccumulator.java

+
+        if (buf.hasRemaining())
+        {
+            chunks.add(buf);


If we always add to the chunks list, then we can end up with a lot of buffers in the list, all max size, but each with just a few bytes in them.

lachlan-roberts · 2020-11-05T05:24:27Z

@leonchen83 I will pull in your commits into a new branch and combine it with my efforts on PR #5536.
I will open a new PR and ask you to review and also test how much improves the memory allocations for you.

leonchen83 · 2020-11-05T06:12:20Z

@lachlan-roberts
thank you for your help. if done, I m grad to review that code.

lachlan-roberts · 2020-11-05T06:47:18Z

This work is being continued in PR #5574.

leonchen83 mentioned this pull request Oct 28, 2020

Improve temporary buffer usage for WebSocket PerMessageDeflate. #5499

Closed

leonchen83 force-pushed the jetty-9.4.x branch from ccb3582 to e2d1eae Compare October 28, 2020 05:07

lachlan-roberts self-requested a review October 28, 2020 06:57

leonchen83 force-pushed the jetty-9.4.x branch from 57141b4 to 054e18b Compare October 28, 2020 07:29

leonchen83 force-pushed the jetty-9.4.x branch 2 times, most recently from 10051a6 to 103653f Compare October 28, 2020 09:13

joakime requested changes Oct 28, 2020

View reviewed changes

gregw requested changes Oct 28, 2020

View reviewed changes

joakime requested changes Oct 28, 2020

View reviewed changes

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java Show resolved Hide resolved

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java Outdated Show resolved Hide resolved

leonchen83 force-pushed the jetty-9.4.x branch from f0273e2 to ae95f6d Compare October 28, 2020 10:13

leonchen83 force-pushed the jetty-9.4.x branch from 3822961 to 4530d9c Compare October 28, 2020 16:12

leonchen83 force-pushed the jetty-9.4.x branch 2 times, most recently from c04a934 to a98265c Compare October 29, 2020 04:33

leonchen83 force-pushed the jetty-9.4.x branch from 1fc4f56 to 479883f Compare October 29, 2020 06:33

lachlan-roberts requested changes Oct 29, 2020

View reviewed changes

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java Show resolved Hide resolved

.../src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java Outdated Show resolved Hide resolved

leonchen83 force-pushed the jetty-9.4.x branch from e131c7b to 018722f Compare October 29, 2020 08:15

leonchen83 force-pushed the jetty-9.4.x branch from 5a87f3b to 8b72f13 Compare October 29, 2020 09:36

lachlan-roberts requested changes Oct 29, 2020

View reviewed changes

leonchen83 force-pushed the jetty-9.4.x branch 2 times, most recently from 95f7c04 to 0b22a92 Compare October 29, 2020 10:55

leonchen83 force-pushed the jetty-9.4.x branch 2 times, most recently from dd0c63c to 64a4738 Compare October 30, 2020 04:32

leonchen83 force-pushed the jetty-9.4.x branch 5 times, most recently from 2da50c1 to 7cbe501 Compare October 30, 2020 15:58

Fix issue jetty#5499

1f5b446

this PR let the ByteAccumulator recyclable. after invoke ByteAccumulator.transferTo method we can invoke ByteAccumulator.recycle method to reuse byte[] via ByteAccumulator.newByteBuffer method Signed-off-by: Baoyi Chen <chen.bao.yi@qq.com>

leonchen83 force-pushed the jetty-9.4.x branch from 1e39446 to 1f5b446 Compare October 30, 2020 16:09

leonchen83 requested review from gregw, lachlan-roberts and joakime November 2, 2020 01:01

gregw requested changes Nov 3, 2020

View reviewed changes

gregw mentioned this pull request Nov 4, 2020

Improve Jetty 10 websocket temporary buffer usage #5536

Closed

lachlan-roberts mentioned this pull request Nov 5, 2020

Issue #5499 - Reduce buffer allocations and copying from ByteAccumulator #5574

Merged

lachlan-roberts closed this Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ByteAccumulator recyclable to fix issue #5499 #5520

make ByteAccumulator recyclable to fix issue #5499 #5520

leonchen83 commented Oct 28, 2020

lachlan-roberts commented Oct 28, 2020

leonchen83 commented Oct 28, 2020

joakime left a comment

gregw left a comment

leonchen83 commented Oct 28, 2020 •

edited

gregw commented Oct 28, 2020

joakime left a comment

joakime commented Oct 28, 2020

leonchen83 commented Oct 28, 2020

joakime commented Oct 28, 2020

gregw commented Oct 28, 2020

joakime commented Oct 28, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts commented Oct 29, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts left a comment

leonchen83 commented Oct 29, 2020

lachlan-roberts commented Oct 29, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts left a comment

lachlan-roberts Oct 29, 2020

leonchen83 commented Oct 29, 2020

leonchen83 commented Oct 30, 2020

gregw left a comment

gregw Nov 3, 2020

gregw Nov 3, 2020

lachlan-roberts commented Nov 5, 2020

leonchen83 commented Nov 5, 2020

lachlan-roberts commented Nov 5, 2020

make ByteAccumulator recyclable to fix issue #5499 #5520

make ByteAccumulator recyclable to fix issue #5499 #5520

Conversation

leonchen83 commented Oct 28, 2020

lachlan-roberts commented Oct 28, 2020

leonchen83 commented Oct 28, 2020

joakime left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

leonchen83 commented Oct 28, 2020 • edited

gregw commented Oct 28, 2020

joakime left a comment

Choose a reason for hiding this comment

joakime commented Oct 28, 2020

leonchen83 commented Oct 28, 2020

joakime commented Oct 28, 2020

gregw commented Oct 28, 2020

joakime commented Oct 28, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts commented Oct 29, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts left a comment

Choose a reason for hiding this comment

leonchen83 commented Oct 29, 2020

lachlan-roberts commented Oct 29, 2020

leonchen83 commented Oct 29, 2020

lachlan-roberts left a comment

Choose a reason for hiding this comment

lachlan-roberts Oct 29, 2020

Choose a reason for hiding this comment

leonchen83 commented Oct 29, 2020

leonchen83 commented Oct 30, 2020

gregw left a comment

Choose a reason for hiding this comment

gregw Nov 3, 2020

Choose a reason for hiding this comment

gregw Nov 3, 2020

Choose a reason for hiding this comment

lachlan-roberts commented Nov 5, 2020

leonchen83 commented Nov 5, 2020

lachlan-roberts commented Nov 5, 2020

leonchen83 commented Oct 28, 2020 •

edited