Fixes #12687 - Buffer reusal in the BufferingResponseListener. #12782
Conversation
sbordet
commented
Feb 9, 2025
sbordet
commented
- Introduced RetainingResponseListener as an efficient alternative to BufferingResponseListener, so there is no need to allocate buffers, just to retain the chunks.
- Fixed bug in RBB.takeByteArray().
- Replaced usages of BufferingResponseListener with RetainingResponseListener.
* Introduced RetainingResponseListener as an efficient alternative to BufferingResponseListener, so there is no need to allocate buffers, just to retain the chunks. * Fixed bug in RBB.takeByteArray(). * Replaced usages of BufferingResponseListener with RetainingResponseListener. Signed-off-by: Simone Bordet <[email protected]>
|
The test failures look related to these changes. |
…eringresponselistener-buffer-management'. Signed-off-by: Simone Bordet <[email protected]>
…ged by the Brotli implementation. As a side effect, now Compression objects must be initialized with a ByteBufferPool. Remove unused Compression.acquireByteBuffer(), leaving only acquireByteBuffer(int). Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
| byte[] array = (!(buffer instanceof Pooled) && !buffer.isRetained() && !buffer.isDirect()) | ||
| ? buffer.getByteBuffer().array() : BufferUtil.toArray(buffer.getByteBuffer()); | ||
|
|
||
| byte[] array = BufferUtil.toArray(buffer.getByteBuffer()); |
There was a problem hiding this comment.
What was the problem with taking a non pooled array without copying?
If it really doesn't work, then the javadoc above regarding the length needs to be updated
There was a problem hiding this comment.
The code was just plain wrong.
It did not account for the offset nor the length of the buffer. Once you do, you have to copy.
There was a problem hiding this comment.
With regards to length, the code was just following the javadoc above, which allows for a longer array than the content to be returned. So if you want to make this method always copy, then you SHOULD update the javadoc.
For the offset, yes it should check that the offset is zero (as it mostly is).
So either update the javadoc OR add the zero offset check. Don't do neither.
| chunk.retain(); | ||
| accumulator.add(chunk); |
There was a problem hiding this comment.
It would be better to use the pattern below, so that the RBB can be configured (or heuristically) decide if a copy or a retain is better. This class might then be better named AccumulatingResponseListener.
| chunk.retain(); | |
| accumulator.add(chunk); | |
| accumulator.append(chunk); |
There was a problem hiding this comment.
I don't want to copy, I want exactly retain+add to a list.
And I don't want any heuristic to decide for me.
I already have the chunk, there is no need to copy, by allocating another buffer, likely of the wrong capacity, to hold copied bytes that don't need to be copied.
And that would require a ByteBufferPool or otherwise be extremely inefficient by allocating buffers to copy into that are not pooled.
I only use DynamicCapacity for takeByteArray(), but I really just need a List, which is why DynamicCapacity was configured to not do any heuristic.
There was a problem hiding this comment.
How you know that you don't want to copy? If you are given 100 x 1 bytes chunks, each coming in 32KB buffer, then it is probably better to copy rather than retain. Or retain the first and then use its space to aggregate others (in some circumstances).
The whole point of RBB is that it removes the need to codify such behaviours in multiple places all over the code base whenever you interact with the RBB. Instead, the behaviour can be specified once at the time of construction and then all interactions with the RBB can use simple methods that will work with whatever algorithm is used.
If you want the default to always retain, then construct the RBB in the default constructor to do so. You then don't need to hard code the algorithm here and it can easily be changed by simply using a different RBB.
Note also, that if RBB needs a pool, it can discover one from the passed RBB if it is a pooled one already.
In short, there are many optimizations/smarts built into RBB that might be of use and there is no reason to bypass them and assume specific behaviour in multiple places in your code. If you want a specific algorithm, then specify that in the constructor that constructs the RBB, not in each method that interacts with the RBB.
In fact, rather than having RetainingResponseListener and BufferingResponseListener as two different classes, they should just be the one class with two different constructors, as all the difference in behaviour can be encoded into the RBB: always copy; always retain; or heuristically decide between the two options. Using RBB properly will avoid the need for a whole new class!
| byte[] array = (!(buffer instanceof Pooled) && !buffer.isRetained() && !buffer.isDirect()) | ||
| ? buffer.getByteBuffer().array() : BufferUtil.toArray(buffer.getByteBuffer()); | ||
|
|
||
| byte[] array = BufferUtil.toArray(buffer.getByteBuffer()); |
There was a problem hiding this comment.
With regards to length, the code was just following the javadoc above, which allows for a longer array than the content to be returned. So if you want to make this method always copy, then you SHOULD update the javadoc.
For the offset, yes it should check that the offset is zero (as it mostly is).
So either update the javadoc OR add the zero offset check. Don't do neither.
| chunk.retain(); | ||
| accumulator.add(chunk); |
There was a problem hiding this comment.
How you know that you don't want to copy? If you are given 100 x 1 bytes chunks, each coming in 32KB buffer, then it is probably better to copy rather than retain. Or retain the first and then use its space to aggregate others (in some circumstances).
The whole point of RBB is that it removes the need to codify such behaviours in multiple places all over the code base whenever you interact with the RBB. Instead, the behaviour can be specified once at the time of construction and then all interactions with the RBB can use simple methods that will work with whatever algorithm is used.
If you want the default to always retain, then construct the RBB in the default constructor to do so. You then don't need to hard code the algorithm here and it can easily be changed by simply using a different RBB.
Note also, that if RBB needs a pool, it can discover one from the passed RBB if it is a pooled one already.
In short, there are many optimizations/smarts built into RBB that might be of use and there is no reason to bypass them and assume specific behaviour in multiple places in your code. If you want a specific algorithm, then specify that in the constructor that constructs the RBB, not in each method that interacts with the RBB.
In fact, rather than having RetainingResponseListener and BufferingResponseListener as two different classes, they should just be the one class with two different constructors, as all the difference in behaviour can be encoded into the RBB: always copy; always retain; or heuristically decide between the two options. Using RBB properly will avoid the need for a whole new class!
...compression-brotli/src/main/java/org/eclipse/jetty/compression/brotli/BrotliCompression.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
