RetainableByteBuffer as mutable #11801

gregw · 2024-05-16T08:09:03Z

Opening this PR against 12.1.x rather than 12.0.x, replacing #11598

I'm just going to summarize the objectives of this PR:

Homogenize the APIs used for buffer manipulation in utils, aggregators, accumulators and buffers. Currently we have several different naming conventions and semantics (e.g. is the buffer consumed or not).
Reduce the API impedance between key abstractions. Buffers from Content.Sources should be compatible with buffers from ByteBufferPools and both should be easily written to Content.Sinks
Separate the read-only APIs from read-write APIs. The API used should indicate if buffer mutation is permitted or not.
protect mutable APIs from being applied to retained buffers
Avoid or hide modal APIs. Abstract away from the BB fill/flush mode issues so that these are rarely a concern for most buffer manipulations.
Where possible, internal implementations should leave BBs in the appropriate mode to avoid excess flipping
Remove unnecessary copies and encourage retention where possible and desirable.
Create common mechanisms that could allow heuristics decisions regarding copy vs retain, so that common usage does not need to make a hard coded choice.
Compatible with current buffer and API usage. We want evolution not revolution.
Avoid the complaints about BufferUtil and flipToFill and flipToFlush

Small tweaks to the RBB API to make the concept more uniform throughout the codebase.

Too much noise in this PR. Will make another PR for it.

…1/10541/chunkAsRBB

gregw · 2024-05-27T02:50:13Z

@lorban Can you quickly go through and resolve any questions that you think are well enough answered?

lorban · 2024-05-28T13:17:59Z

@gregw the only point of contention I see is the behavior of accumulator.add() w.r.t the Retainable contract. I don't mind making an exception to the rules if that's properly javadoc'ed, as you did.

But this caught me by surprise, and I assume it could again in the future, more so if we multiply the exceptions. So I think there should be some hint in the method's name that it's not playing by the rules to suggest the caller of that API to go read the javadoc.

How about renaming add() to addPreRetained() or something like that? And maybe put it in bold at the top of the javadoc that an exception to the Retainable contract is being made?

gregw · 2024-05-29T21:19:21Z

But this caught me by surprise, and I assume it could again in the future, more so if we multiply the exceptions. So I think there should be some hint in the method's name that it's not playing by the rules to suggest the caller of that API to go read the javadoc.

@lorban I too was caught by surprise that we already have some accumulators that have this semantic behind a method called append, so this is not new behaviour.

How about renaming add() to addPreRetained() or something like that? And maybe put it in bold at the top of the javadoc that an exception to the Retainable contract is being made?

I'm open to change the name, but I don't like addPreRetained, @sbordet has referred to it as offer so we could use that name?

# Conflicts: # jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java # jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java

gregw · 2024-06-11T01:55:58Z

@sbordet @lorban please don't forget this. Let's keep reviewing it and discussing it.
Can you at least resolve any conversations that I have answered... or indicate that the answer is not sufficient and there is more work to do.

gregw · 2024-06-19T23:39:21Z

@sbordet @lorban I intend to merge this PR to 12.1.x tomorrow, unless there are any objections.

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

lorban

There are a few nits left (see my comments) and I took care of a few TODOs related to usages of the deprecated ByteBufferPool.Accumulator, otherwise this looks like a good step forward.

lorban · 2024-06-20T11:26:51Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java

@@ -581,14 +581,19 @@ private Pool.Entry<RetainableByteBuffer> evict()
        }
    }

-    private static class Buffer extends AbstractRetainableByteBuffer
+    private static class PooledBuffer extends RetainableByteBuffer.Pooled


Almost only ArrayByteBufferPool makes use of this class so it could be made private for that purpose.
The only other usage is DynamicCapacity.takeByteArray() which prevents getting the array if the buffer is pooled, but that check contrasts with getByteBuffer() which does no such check.

I don't like very much this class being exposed as part of the buffer API,

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

lorban · 2024-06-20T15:52:39Z

I also noticed that tests inheriting from org.eclipse.jetty.http3.tests.AbstractClientServerTest leak buffers both on the client and the server, but that also happens without these changes so I reverted my change that adds the leak tracker.

gregw · 2024-06-24T06:29:01Z

@lorban I think this is good to merge and get some prime time usage!?!?! no? 12.1.x will not be final for a while, so there is still time to make changes

lorban

I agree to get this merged into 12.1.x.

There still are things that need to be cleaned up, and others that may be done differently but this is going to the right direction, already improves the current state and already is a large and complex PR.

gregw · 2024-06-25T06:55:49Z

@lorban I have merged. Can you open issues for the things you want us to investigate more

lorban · 2024-06-25T10:17:39Z

@gregw I've opened the 5 enhancements listed above. AFAICS, we can consider this RBB refactoring is done once those are tackled.

gregw added 30 commits May 16, 2024 17:41

Make chunk a RBB

4abb57b

Small tweaks to the RBB API to make the concept more uniform throughout the codebase.

Make chunk a RBB

974d4c4

Small tweaks to the RBB API to make the concept more uniform throughout the codebase.

revert EventsHandler change

74ce7c2

WIP

5fd2543

updates from review

6992609

updates from review

4776471

more tests

5723732

inline asReadOnly

67c302b

Added Appendable as a replacement for both Accumulator and Aggregator

010267e

Better slice implementation

d18c93a

protect from modification if retained

ecdc2de

avoid flip-flopping in append loops

d29fb05

long methods

b0fb6cd

use some appends

ecaf1b6

javadoc

f17395f

Fixed test

521dccd

appendable wrap

ec022ff

detailString

0d6a1c7

Revert NON_POOLING change

df3bf77

Too much noise in this PR. Will make another PR for it.

Testing Fixed

a5cd43f

Limit usage to buffer size requests

34620d7

Simplified hierarchy and naming

f7cf481

Simplified hierarchy and naming

4c1670d

more testing

4a726fb

improved test

eb1e17a

disable leaky tests

91e9561

removed redundant NetworkBuffer class

595323c

Better heuristic for retaining buffers

1cb541e

Updated BufferedResponseHandler

4442c49

Use RBB in BAEP

4481998

gregw added 2 commits May 23, 2024 14:50

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…

ab59f9b

…1/10541/chunkAsRBB

fixes for review updates

022b83f

Merge branch 'jetty-12.1.x' into fix/jetty-12.1/10541/chunkAsRBB

2bcbdc3

# Conflicts: # jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java # jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java

gregw requested a review from lorban June 11, 2024 01:56

replace usages of ByteBufferPool.Accumulator with RetainableByteBuffe…

4627f75

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

lorban requested changes Jun 20, 2024

View reviewed changes

lorban added 3 commits June 20, 2024 16:17

replace usages of ByteBufferPool.Accumulator with RetainableByteBuffe…

e1a7e79

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

replace usages of ByteBufferPool.Accumulator with RetainableByteBuffe…

d6dc39a

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

replace usages of ByteBufferPool.Accumulator with RetainableByteBuffe…

db00da7

…r.DynamicCapacity Signed-off-by: Ludovic Orban <lorban@bitronix.be>

updates from review

54738d9

gregw requested a review from lorban June 21, 2024 00:43

lorban approved these changes Jun 24, 2024

View reviewed changes

gregw merged commit 36538d6 into jetty-12.1.x Jun 24, 2024
10 checks passed

gregw deleted the fix/jetty-12.1/10541/chunkAsRBB branch June 24, 2024 22:12

olamy mentioned this pull request Sep 29, 2024

Jetty 12.1.x add Eclipse dash too usagee #12325

Merged

olamy mentioned this pull request Oct 17, 2024

jetty 12.1.x merge 12.0 #12400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RetainableByteBuffer as mutable #11801

RetainableByteBuffer as mutable #11801

gregw commented May 16, 2024

gregw commented May 27, 2024

lorban commented May 28, 2024

gregw commented May 29, 2024

gregw commented Jun 11, 2024

gregw commented Jun 19, 2024

lorban left a comment

lorban Jun 20, 2024

lorban commented Jun 20, 2024

gregw commented Jun 24, 2024

lorban left a comment

gregw commented Jun 25, 2024

lorban commented Jun 25, 2024

RetainableByteBuffer as mutable #11801

RetainableByteBuffer as mutable #11801

Conversation

gregw commented May 16, 2024

gregw commented May 27, 2024

lorban commented May 28, 2024

gregw commented May 29, 2024

gregw commented Jun 11, 2024

gregw commented Jun 19, 2024

lorban left a comment

Choose a reason for hiding this comment

lorban Jun 20, 2024

Choose a reason for hiding this comment

lorban commented Jun 20, 2024

gregw commented Jun 24, 2024

lorban left a comment

Choose a reason for hiding this comment

gregw commented Jun 25, 2024

lorban commented Jun 25, 2024