Make Content.Chunk a RetainableByteBuffer

[gregw]

A minimal set of changes extracted from #11094 and variants in fix/jetty-12/10541/byteBufferAccumulator3 and fix/jetty-12/10541/byteBufferAccumulator4 in an attempt to find a rough consensus on some core API changes.

[gregw]

@sbordet please don't review saying "this method is not used". Just imagine that it is used and review on the basis of asking is it the right API in the write location.

In fix/jetty-12/10541/byteBufferAccumulator4 I'm extending this with the goodness of byteBufferAccumulator2, but in a Mutable API.

[gregw]

See #11599 for the Mutable API

[lorban]

I like the cleanups, and I also like where this is leading us.

[lorban]

Mental note: asRetainableByteBuffer() should have been called toRetainableByteBuffer(): as prefix for modifying the presentation (wrap/unwrap) and to prefix for anything implying havier work like mem copies.

[gregw]

I've always thought that as prefix should be used when returning a different view of the same object. The to prefix should be used when making a new object from the current one.

So this could be thought of as a to as it is creates a whole new object.... but that object also mutates the original object (by consuming all its input), so it is kind of a view onto the original object.... if the resulting RBB delays reading the source until it knows how it is going to be used, then it is really is a view onto the source. So as works as well.

I'm enough on the fence not to disrupt things by changing the name at this point.

[lorban]

There is one design issue that needs to be addressed: do we stick to int for everything related to the RBB's size, do we move to long or do we postpone this decision to later?

[gregw]

Very good question.... kind of depends if this is a 12.0.x thing or a 12.1.x thing. If the later, we can go long! Let's leave this unresolved for a bit and think about it.

[gregw]

As this is a new method, I have made this one long. But probably should change all to being longs.

[lorban]

I don't think toString() should be delegated.

[gregw]

How about with a formatted wrapper?

[lorban]

Exposing this constructor just for testing looks a bit dangerous IMHO. I'd prefer to use the default ctor and add a couple of retain() calls in tests.

[gregw]

I've removed most... there are still a few I need to review

[gregw]

@lorban thanks for the review. I've fixed most, but will ponder some for a while....

[sbordet]

@gregw I had a look with @lorban and below my thoughts.

I can see the opportunity to have a common super-interface for RBB and Chunk, so that we can have accumulators/aggregators (AAs) that can deal with both.

Speaking with @lorban I could see a ray of light when analyzing RBB.writeTo(Content.Sink sink, boolean last, Callback callback), but I don't think it's right yet, especially because it has a boolean last parameter that would be implicit for Chunk (and so it would clash with the one of the Chunk).

The use cases around are:

1. BBP.Accumulator: used for generation, with the semantic of a list: add a bunch of RBBs, until ready to write them out to EndPoint.write(), which requires a ByteBuffer[]. Its usage is add(), add(), add(), getList().toArray().
2. Aggregation: used in Servlets (but kinda irrelevant here), and in WebSocket for text messages. The semantic is add+copyBB because the BB is copied into a Utf8StringBuilder. Its usage is add+copy, add+copy, takeTotalCopy().
3. Accumulation with Callback: used in WebSocket for binary messages. The semantic is add(), but deals with BB, not RBB, because that's what Frame.getPayload() returns. Its usage is add(), add(), add(), takeTotalCopy().

I could not find any use case where a "composite" RBB is ever used as a composite.
The only implementation is the private ContentSourceRetainableByteBuffer.Accumulator, which is only used by Content.Source.asRetainableByteBuffer(), which is not used.

Converting a Source to an RBB to then exploit RBB.writeTo(Sink...) seems weird because we already have Content.copy(Source, Sink).
I can see perhaps an hypothetical use case for a composite RBB.writeTo(Sink), but none in practice so far, so I'm still asking myself if this is really needed.

To me, Chunk is read-only for users, despite obviously exposing a BB.
So it is not good that it now has a clear() method inherited from RBB (with the BufferUtil semantic, which is different from the BB semantic that otherwise this class exposes).

I see Chunk as a public API to be used by applications (probably 90% of usages), and RBB as a more private API used by implementation, or applications that really want to bind to Jetty low level APIs (10% of usages).

However, I can see that having a common super-interface for RBB and Chunk could be useful, but I think should be minimal:

interface Retainable {
  interface WithByteBuffer extends Retainable {
    BB getByteBuffer();
    void writeToSink(Sink, Callback);
}

Note that writeToSink() does not have the boolean last parameter because "lastness" is a concept only for Chunk, not for RBB.
Furthermore, "lastness" is a concept of the caller, not of the composite RWBB (nor the RBB), as explained below.

RWBB would allow implementations of AAs that work with RBB and Chunk.
These AAs have an API that returns the whole AA as a composite Retainable.WithByteBuffer (or RBB), which would then perhaps allow the use of writeToSink().

How would that work?

WebSocket receives Frames with a naked BB. It can be wrapped into an RBB and accumulated (forces the wrapping which is technically unnecessary).
Request/MultiPart receive Source, which can be easily copied to a Sink via Content.copy(). But let's assume we read and accumulate the Chunks.
A scenario could be that I would like to write N WebSocket frames or N MultiPart parts as JSON object elements of a JSON array to a Sink, let's say a PathContentSink.

But we cannot, because the caller controls the "lastness", not each individual composite RWBB.

// Write to file a list of AAs seen as RWBBs.
void writeToPath(Path file, List<RWBB> rwbbs, Callback finished) {
  Sink sink = Sink.from(file);

  sink.write(false, "[", NOOP);

  // Must use IteratingCallback, below pseudo-code.
  for (RWBB rwbb : rwbbs) {
    // Controls "lastness", but copies data in the getBB() call.
    sink.write(false, rwbb.getBB(), Callback.from(rwbb.release())).block();

    // This does not control "lastness", but avoids data copy.
//    rwbb.writeTo(sink, Callback.from(rwbb.release());

    // This overrides "lastness", and avoids data copy, but may be confusing.
//    rwbb.writeTo(sink, false, Callback.from(rwbb.release());

    sink.write(false, ",", NOOP);
  }

  // Finish and close the file.
  sink.write(true, "]", finished);
}

So, RWBB.writeTo(Sink, Callback) does not really work because it does not allow control of "lastness".
But RWBB.writeTo(Sink, boolean, Callback) also does not really work because it would be confusing for Chunks, that have their own "lastness".
We can specify in the javadocs that one overrides the other, but it is prone to errors: Chunk.EOF.writeTo(sink, false, NOOP) -- is it right, or it is a bug?

In summary, I am not convinced by the extent of this PR, as it impacts too much.

I can see a driver force for a smaller PR introducing RWBB and AAs that works with RBB and Chunk, but the writeTo() problem (the boolean last parameter) is there, which defeats the usability of a composite RBB, which itself seem to have no existing use case (we don't need it), just some hypothetical (which I'm not denying, but I would go with a smaller PR first, and then if the hypothetical use case really shows up, it would be possible to add a default method to RWBB).

Unless I missed, and the hypothetical use case is already there 😀

Lastly, I would avoid to add BB wrapper methods that have the BufferUtil semantic at all costs (e.g. RBB.clear(), appendTo(), isFull(), etc.).
Also, changing the BB semantic with our own would be confusing (e.g. get(byte[] bytes, int offset, int length)).
Just use getBB() and then the ByteBuffer APIs.

[gregw]

Just use getBB() and then the ByteBuffer APIs.

@sbordet by saying this you indicate that you still do not understand the intent of this PR.

Yes I know there are few users of the composite RBB in this PR. That is because I've kept it minimal.

You've spent a lot of time taking about other ways it could be done and why they wouldn't work, but you have not really discussed this PR itself, which does work better than what we have today.

I need some time to digest all that you wrote to try to work out why you are not understanding and see if I can find a better way to explain.

[gregw]

@gregw I had a look with @lorban and below my thoughts.

@sbordet Thanks for the long detailed analysis, but I think I see a few things that you've got wrong in your analysis that lead you down some rabbit holes... so you ended up reviewing the wrong rabbit I think.

I can see the opportunity to have a common super-interface for RBB and Chunk, so that we can have accumulators/aggregators (AAs) that can deal with both.

Note also lots of cleanup in this PR with methods like isRetained on Retained that allow a more unified behavior that including for H2 Data abstraction.

Speaking with @lorban I could see a ray of light when analyzing RBB.writeTo(Content.Sink sink, boolean last, Callback callback), but I don't think it's right yet, especially because it has a boolean last parameter that would be implicit for Chunk (and so it would clash with the one of the Chunk).

They are different lasts, so there is no clash.

The last in a chunk is indicating the last buffer in a sequence from a source. The last in the writeTo is the same as the last in a sink write and indicates the last outgoing buffer. Only in a pure echo app are they the same. Just because I'm putting a last chunk into a buffer/sink, does not mean that is the last data I'm putting/writing. I might be doing a multipart, so after the last chunk I need to put/write a boundary and then more parts, each potentially having a last chunk.

When a chunk is appended to a RBB, it is done so as a RBB and not as a chunk. So the last status of the chunk is irrelevant, even if the chunk is retained. For all we know, a retained chunk is being retained just for a small slice of its data, which might not represent the last byte of that chunk.

The lasts are independent and there is no clash.

The use cases around are: ...

Don't get obsessed by the existing use-cases. They have all been formed by legacy code, mostly written before we had the option of retaining and even now with the API impedance we sometimes accumulating instead of aggregating.

Ultimately if the source data is retainable, we will rarely ever want to aggregate. That should only ever be done if there is no suitable retainable available. This PR makes the retainable for the data more available, but it explicitly has not modified the use-cases. I've done some more use-cases in #11599, but even that is probably legacy stuff.

Fundamentally, I don't think we really should know/care about Aggregators vs Accumulators, as we should accumulate when we can and aggregate if not possible... maybe even a mix of those if we have mixed sources. The only decisions the code should be making are: do I want to buffer? Do I want my buffer to grow to hold the whole content, or just parts?, Do I have a size limit?

Ultimately the RBB code can then make an internal decision about aggregation vs accumulation. But we have more work to do on this to identify the usecases and to perhaps come up with the correct heuristics. Note that once we start retaining buffers all the way through, we might need to consider the efficiency of small retains. For example if we read a request into a 32KB buffer and then end up reading on 2KB of data and then retaining for only 10B, the protocol layer is currently getting a new buffer to continue reading into and will will have 32KB mostly empty buffer help by the application (this is a risk now, just more so with this PR). The RBB might decide to aggregate if it is only 10B. Or perhaps the protocol layer should continue with the buffer, using only the remaining 30KB?

So there is lots more work to do in this area, and heuristics in RBB might not be able to solve all of it. But this PR at least removes the API impedance so we can retain easily if we want to. It's no good thinking we are safe from these issues because our API impedance makes it harder to do.

I could not find any use case where a "composite" RBB is ever used as a composite. The only implementation is the private ContentSourceRetainableByteBuffer.Accumulator, which is only used by Content.Source.asRetainableByteBuffer(), which is not used.

This PR doesn't have the use-cases. I actually wrote a comment to you in this PR saying "Please don't review saying: this is not used" but then deleted it, thinking it was too rude. I've unhidden the comment now. See it at the top of the PR :)

Converting a Source to an RBB to then exploit RBB.writeTo(Sink...) seems weird because we already have Content.copy(Source, Sink). I can see perhaps an hypothetical use case for a composite RBB.writeTo(Sink), but none in practice so far, so I'm still asking myself if this is really needed.

I agree that doing this directly would be strange. I think this would only be useful if there is some software layer in-between that takes only RBBs and you only have a Source, so you convert.... then later on the layer does a writeTo of that retained RBB and other content. However, our APIs should be plug-and-play, with simple adaptions between our core concepts. In short RBB.writeTo should only ever be used to access a source when the caller has no idea that the RBB is backed by a source.

To me, Chunk is read-only for users, despite obviously exposing a BB. So it is not good that it now has a clear() method inherited from RBB (with the BufferUtil semantic, which is different from the BB semantic that otherwise this class exposes).

Clear is a read operation! Clear simply moves the position to the limit, and is equivalent to skip(remaining()). Hmmm using the words Mutable and Immutable are probably wrong when applied to RBB, because a read-only RBB is not immutable, as content can be consumed from it... just that content cannot be added nor mutated.

I see Chunk as a public API to be used by applications (probably 90% of usages), and RBB as a more private API used by implementation, or applications that really want to bind to Jetty low level APIs (10% of usages).

Well it started out that way, but we are now exposing ByteBufferPool as one of the Components available via a Request. We want to encourage applications to use pooled buffers and they come as RBBs. If we were to write an application that buffers a lot, we'd use RBBs from that pool and adapt chunks to be retained where possible. Now RBBs are public, we should allow our users to do the same easily without API impedance.

However, I can see that having a common super-interface for RBB and Chunk could be useful, but I think should be minimal: ...

There is no point having a common interface unless there is usable API on it. Calling getByteBuffer() needs to be treated as an anti-pattern:

• It takes you to a read-write API, so we lose our distinction between readable and read-write APIs that this PR is the first step moving towards
• It forces any accumulation to be resolved and thus copies to be made.
• It is int size when we can have long size

Note that writeToSink() does not have the boolean last parameter because "lastness" is a concept only for Chunk, not for RBB. Furthermore, "lastness" is a concept of the caller, not of the composite RWBB (nor the RBB), as explained below.

Different last!

WebSocket receives Frames with a naked BB. It can be wrapped into an RBB and accumulated (forces the wrapping which is technically unnecessary). ....

Naked BB come in two forms: ones backed by a BBPool of some time; and locally allocated ones. So wrapping a RBBs makes sense for the former (although better to get to any RBB it came from), and wrapping is not too expensive for the later, especially as it will provided isRetained so the caller can know if they are able to re-use their naked BB or not.

But we cannot, because the caller controls the "lastness", not each individual composite RWBB.

Different last!
Anyway, you are now reviewing your API suggestion, which is not this PR.

In summary, I am not convinced by the extent of this PR, as it impacts too much.

What are the impacts? What existing APIs are changed by this PR in a dangerous/bad/impactful way?

I'm actually thinking this is more of a 12.1.x change, but really there is nothing much in this PR that could not go into 12.0.x Rather than review your own ideas/variants can you review this actual PR and say what the impacts actually are?

Unless I missed, and the hypothetical use case is already there 😀

ARGH! You do this to me all the time! I do a significant API refactor and include all the updated usages. You say: PR is too big and impossible to review. I split the PR into the minimal change and later PRs to use that change. You say: the API change is not motivated/used! Hence my hidden (now unhidden) comment saying please don't do that.

Let's step back and not worry so much about the actual use-cases in the code, other than as approximate examples. Then let's design a really good buffer abstraction that:

• has low API impedance from source, to buffer to sink
• separates read-only from read-write APIs
• encourages retention where possible and thus reduces copies.
• is compatible with current buffer usage

Once we have that buffer abstraction, we can increase its usage over time.

Lastly, I would avoid to add BB wrapper methods that have the BufferUtil semantic at all costs (e.g. RBB.clear(), appendTo(), isFull(), etc.). Also, changing the BB semantic with our own would be confusing (e.g. get(byte[] bytes, int offset, int length)). Just use getBB() and then the ByteBuffer APIs.

The method getBB is an anti-pattern and should be avoided at all costs. This whole 3rd attempt at solving this problem has been motivated by your insistence that we could not possible add 2 append method signatures to RBB as they would then appear on chunk and it is read-only. So if getBB is our primary API, then we are using an API that is read-write and has many many methods. So you can't now just say use BB API and that contradicts your "cannot be read-write" insistence!

So can you please have another look at this PR for what it is and not what you think it could/should be. I'm not saying it is perfect, but I'd really like to know what specifically in this PR you don't like... not what you don't like about your own ideas/variations on it.

This PR/issue is not the highest priority and is almost certainly a 12.1.x thing, but let's not drop it, as at the very least there are some good cleanups in this PR even if we go no further. Let's hangout about it soon.

[gregw]

I'm just going to summarize the objectives of this crusade!

• 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

[lorban]

What I generally like:

• The writeTo() pattern as the less we have to call getByteBuffer() the better.
• The append() pattern for the same reason as above.
• The split between RBB and RBB.Mutable to make it clearer when some API is meant to consume or produce data.
• The strong abstraction of the Appender / Aggregator.

What makes me cautions:

• The multiplication of append() overloads that takes byte[] with and without offset, ByteBuffer, RBB and so on, as it makes the API less concise. Potential ditto for writeTo().
• The fact that RBB and Content.Sink aren't in the same module so we cannot design an API where RBB exposes API using Content.Sink.
• Chunk becoming a RBB subclass looks both elegant and unnatural. I have to ponder that a bit more as I have no objection besides a bad gut feeling.

If we're considering those changes for 12.1, I'm thinking we should charge ahead as it is the perfect time to make changes to semi-internal APIs.

[gregw]

Closing in favour of a new PR with less history....

[gregw]

replaced by #11801