Jetty 13.0.x buffers

[lorban]

Phase 1

• Buffers implementation
• SslConnection conversion

Phase 2

• EndPoints conversion
• Introduction of gathering target

Phase 3

• HTTP/2 conversion
• FCGI conversion

Phase 4

• HTTP/1 conversion
• Content.Sink.write() API conversion
• Introduction of transfer-to target

[gregw]

Some more javadoc in the interfaces would help conceptualise your intent before too much more is implemented.

[gregw]

why do these return WritableBuffer ?

[lorban]

Because it felt to me like both ought to be done right before re-trying to fill the buffer. If that assumption turns out to be false, we should make those methods stay in read mode and return void.

Regarding compact(), there is also a feeling that this should be done in toWritable(), at least when there is nothing to be read.

Nothing set in stone yet.

[gregw]

Can there be a common interface that provides these methods?

[lorban]

For the two position() and capacity(), we could but I feel like this wouldn't be very useful.

For remaining(), I have a growing sense that this method should be replaced by a pair of remainingForRead() and remainingForWrite() methods as I've been caught a couple of times calling remaining() on a read buffer when actually the remaining fillable capacity was needed; i.e.: it feels too easy to mess that up, both those methods are cheap to implement and would make things more obvious.

[gregw]

I think using "size" and "space" would be better that "remainingForRead" and "remainingForWrite". The term "remaining" doesn't mean much on a buffer that can be read and written to.
We already use size, space and capacity in our code!

[gregw]

is this 'toReadable' or 'asReadable'? Some javadoc would help
Is it expected to be zero copy? Can a writable with 1 ref be converted to a readable so it cannot be written again?

[lorban]

toReadable() and toWritable() are the equivalent of BufferUtil.flipToFlush()/BufferUtil.flipToFill(): change the indices but then return this as a different type. Regarding naming (to vs as) you know I'm not very opinionated, but I do not want us to spend too much time discussing this, at least for now.

I did not consider retainability so far, but maybe we should make ReadableBuffer.toWritable() throw when the retain counter is > 1? This feels like the right thing to do: when a ReadableBuffer is stored as a member variable, we could test if it's retain counter is > 1 and release + acquire a new buffer when that's true?

[gregw]

We have been able to hide the flip stuff in the implementation of org.eclipse.jetty.io.RetainableByteBuffer.FixedCapacity so we should aim to do the same.

If we make it an explicit flip in the API like this, then what happens if somebody calls both toWritable() and toReadable() - does that imply a copy so you can get them both?

This is a REALLY important abstraction to get write - when does a copy happen if ever???

[lorban]

Each time you call toWritable() or toReadable(), you just flip the buffer: there is no copy, no slicing, just a buffer flip and a cast.

This obviously mean that when you call ReadableBuffer.toWritable(), the existing ReadableBuffer variable should not be used anymore as it refers to a buffer in write mode. Most methods (like getXyz()) will catch that and throw, while some other (capacity(), position(), remaining()) will work but do the wrong thing.

While modifying SslConnection, it became apparent that you only need a WritableBuffer for a very short duration: for filling it from the socket or from SSLEngine.wrap(), while you may stay with a ReadableBuffer for a relatively long period: until the user reads the Request data, or until you can write the Response to the socket. Based on this, a pattern seems to have emerged:

• store the "non-empty buffers at rest" as ReadableBuffers in member variables
• null out the member variables in the methods that are working with the buffers
• flip the buffers to WritableBuffer within a very short code block then flip them back to ReadableBuffers at the end of the code block so the local WritableBuffer variables get out of scope
• store the non-empty buffers back into the member variables at the end, or release the empty ones

The above seems to work elegantly, the buffer mode is now enshrined in the type system and we enforce that within a certain scope, a buffer is meant to only be read from or only written to.

[gregw]

Each time you call toWritable() or toReadable(), you just flip the buffer: there is no copy, no slicing, just a buffer flip and a cast.

explicit flips in the code are a real step backwards. The buffer wrapper should just know what mode it is in and carry the third pointer necessary.
Having modal buffers, even with a return, is a real -1. There is no point having them, just use ByteBuffer if you want to track modes yourself.

Having readonly or immutable buffers is a different thing - as they are not modes, but permissions. The state of the internal pointers should have nothing to do with permissions.

[sbordet]

@gregw problems with RBB:

• getByteBuffer()
• Bugs and confusion using DynamicCapacity, tries to do too much, inconsistent API, etc.
• long / int mismatch for size() vs remaining(), etc.

getByteBuffer() is the worst offender. Removing it allows for e.g. transfer-to optimizations.

[gregw]

@sbordet just being the devils advocate here, as I'm not really opposed to a new buffer abstraction. But we really need to be clear of the aims of any new buffer abstraction.

I don't disagree with with the RBB issues you list, but why cannot they be fixed within the current abstractions rather than create a new one (with all the unintended consequences that will entail)

• getByteBuffer()

I agree that this is a problematic method. But why not just add a transfer-to method to RBB?

• Bugs and confusion using DynamicCapacity, tries to do too much, inconsistent API, etc.

This class could easily be split into different Aggregators and Accumulators to avoid the do-too-much

• long / int mismatch for size() vs remaining(), etc.

Fixable. Probably best just to deprecate/remove all the ByteBuffer styled methods like remaining()

[sbordet]

I agree that getByteBuffer() is a problematic method. But why not just add a transfer-to method to RBB?

We kind of have that already, RBB.writeTo(), but it is using the wrong abstraction, Sink, while it should have used a lower-level abstraction such as ReadableBuffer.Target.
Sink should have been high-level and have write(boolean, RBB, Callback), so that we could hide a transfer-to behind the RBB parameter.
But then we need a low-level Target that eventually accesses the ByteBuffers that can do single, gather and transfer-to writes.
The current proposal is going into that direction: change Sink.write() signature, and introduce Target.

Also, getByteBuffer() is problematic WRT:

• Throws if many buffers are coalesced into one but don't fit (e.g. large buffers that overflow int)
• Ownership of the raw ByteBuffer: it becomes unclear if getByteBuffer() transfers the ownership or not.
• @lorban do we have more?

It might really be that the "new" RBB is quite similar to the old, with only long APIs, no access to the internal ByteBuffers and a revised writeTo() method.

[lorban]

In my view, having a proper writeTo() abstraction (yes, using Sink was a mistake), totally getting rid of getByteBuffer(), and changing Content.Sink.write() to not take a NIO ByteBuffer anymore are the three new killer features of this rework.

DynamicCapacity having a overly confusing API and exposing methods that use int instead of long also are hindrances but to a lesser extent. Nevertheless, cleaning that is also a welcome addition.

Embarking in that grand buffer surgery had the benefit of uncovering everything we hid under the carpet (willingly or not) over the years, but the conclusion is that a proper writeTo() + no more getByteBuffer() + Content.Sink.write(RBB) is the real gain we're looking for, so we could quite easily retrofit all the work done so far into a single auto-flipping RBB with a cleaned-up API.

We can continue discussing the details (split Read/Write APIs, RBB or not, ...) as IMHO those are secondary and orthogonal to the bulk of the work which is about modifying all the code that uses NIO ByteBuffer.

[gregw]

@lorban I agree that the approach of totally getting rid of getByteBuffer() is the right way to proceed at this point. Once the grand surgery is completed, we can look at it and see exactly how different it is to RBB - renaming back to RBB and adding deprecated methods has the benefit of it being an API migration rather than a revolution (and avoiding breaking too many Handlers), but then a clean start with legacy mode support might be a better way forward for long term. So totally agree the final API is secondary to the bulk of the work.

[gregw]

love the name... I guess Spring is taken :)

[lorban]

😃 don't read too much from that name, it was the first one I found that wasn't overloading an existing one like Source.

But please do keep in mind that the Fount / readFrom and Target / writeTo interfaces are still very much WIP. The current form of this API is workable, but it isn't that great so we need to spend time thinking about it too. The two annoyances haunting my mind are:

• should any of this throw any exception at all?
• should Fount.read() return a boolean? If not, we could add an extra WritableBuffer.readFrom() overload that returns a generic T type which would help in SslEndPoint.fill() when unwrap() has to be called and SSLEngineResult has to be returned. At the moment, the SSLEngineResult instance escapes the lambda and is stored in a pre-allocated SSLEngineResult[1] that is quite ugly. But if Fount.read() does not return a boolean, how do we convey that EOF was reached?

[lorban]

Some more javadoc in the interfaces would help conceptualise your intent before too much more is implemented.

Now is probably a good time to add some javadoc to ReadableBuffer and WritableBuffer to try to capture what the current intent is, even if there are still moving parts as it looks like the first feedback I got from this SslConnection experiment seems rather positive and quite a bit of the current API already looks solid.

[sbordet]

Not sure I want clear() to implicitly convert.
Also, JDK clear() on ReadableBuffer does not make much sense.

[sbordet]

Here I think we need to keep the "readable" version of wb as a local variable, and only assign to the field when we exit from this method.

[sbordet]

I'd remove this, as we compact before the networkFill() call anyway, so if we get BUFFER_UNDERFLOW here, we are out of luck and we just throw.

[sbordet]

We should do gathering writes here.

[gregw]

@lorban @sbordet This is some musing of what a 2 class builder style API could look like. I still lean towards keeping this as a RBB API evolution, but happy to consider 2 class solutions.

So here are some examples of how I think a buffer builder pattern could work. It is not that different to ReadableBuffer and WritableBuffer but I think it is more apparent that there could be a lingering relationship between them.
I also think that we can move some of our if (readable.isRetained())... logic into the builder itself, so that if you continue building it can just look at the last retained count of the last built buffer and make decisions based on that.

The first example is of a fully consuming fillAndParse. Note how the builder can encapsulate some of our common buffer reuse strategy:

public void fillAndParse(EndPoint endPoint, Parser parser) throws IOException
    {
        // Get a builder of fixed sized buffers.
        // The buffer builder is AutoCloseable, so EOF, zero-fill and
        // exceptional exits all release any held buffer.
        try (Builder builder = Builder.fixed(__byteBufferPool, true, 8192, 4096))
        {
            while (true)
            {
                // If the builder has no buffer, it acquires one from the pool
                // If it has a buffer that has only 1 reference (the builder), then clear and use it.
                // If the buffer has references but is mostly empty, then build into a slice from the tail, using
                // padding to avoid compression(heuristic needed)
                // else release and acquire a new buffer from the pool
                // This encapsulates our buffer reuse strategies into the builder.
                int filled = builder.fill(endPoint::fill);
                if (filled < 0)
                    return; // EOF: builder closed by try-with-resources; real code also shuts down output.
                if (filled == 0)
                    return; // real code: fillInterested(...) and resume with the builder FIELD intact.

                // Take the filled bytes as an immutable buffer.
                // INTERNALLY: build() does a retain for the emitted buffer: rc=2.
                RetainableBuffer input = builder.build();
                try
                {
                    // Fully consuming parser may take references to slices of the input
                    if (parser.parse(input))
                        return;
                }
                finally
                {
                    // release the built input (may be reused by the builder if parsing did not retain a slice
                    input.release();
                }
            }
        }
    }

And here is a non-consuming parseAndFill example:

    public static class NonConsumingParseAndFill
    {
        private final EndPoint _endPoint;
        private final Parser _parser;
        private final Builder _builder;

        public NonConsumingParseAndFill(EndPoint endPoint, Parser parser)
        {
            _endPoint = endPoint;
            _parser = parser;
            _builder = Builder.fixed(__byteBufferPool, true, 8192, 4096);
        }

        public void onFillable() // also re-entered when the application demands content
        {
            while (true)
            {
                // Parse first; fill only when out of bytes (as HttpConnection does,
                // to avoid unnecessary system calls). build() takes whatever is in
                // the builder: bytes carried from a previous call, or just filled,
                // or none (EMPTY). INTERNALLY: build() retains for the emitted
                // buffer: rc=2.
                RetainableBuffer input = _builder.build();

                // The parser consumes some, none or all of the input.
                // It may reference slices as it goes.
                boolean handle = _parser.parse(input);

                // Always after parsing we give the input back to the builder so it can
                // use any unparsed bytes in the next iteration.
                _builder.append(input); // Builder can detect this is its own buffer and reuse if it wants to
                input.release();

                if (handle)
                {
                    // Pausing the builder is softer than close. It allows any resource that we don't want to hold
                    // during handling to be released.  But if the buffer is retained, it may hold onto it anyway,
                    // hoping that it will be released by the time there is input (heuristic needed).
                    _builder.pause();
                    return;
                }

                try
                {
                    int filled = _builder.fill(_endPoint::fill);
                    if (filled < 0)
                        throw new EofException();
                    if (filled == 0)
                    {
                        // No bytes now: park. A carried partial header stays in the
                        // builder as unbuilt bytes (trim() is then a no-op, nothing is
                        // lost); an empty builder returns its region to the pool.
                        _builder.close();
                        _endPoint.fillInterested(Callback.from(this::onFillable));
                        return;
                    }
                }
                catch (IOException x)
                {
                    _builder.close();
                    return;
                }
            }
        }
    }

This is a buffered framing echo example:

    public static class BufferedFramedEcho
    {
        private final Content.Source _source;
        private final Content.Sink _sink;

        // A chain-of-regions builder: appends retain rather than copy where sensible.
        private final Builder _builder;

        public BufferedFramedEcho(Content.Source source, Content.Sink sink, ByteBufferPool pool)
        {
            _source = source;
            _sink = sink;
            _builder = Builder.accumulating(pool);
        }

        public void iterate()
        {
            while (true)
            {
                Chunk chunk = Chunk.from(_source.read());
                if (chunk == null)
                {
                    _source.demand(this::iterate);
                    return;
                }
                if (Chunk.isFailure(chunk))
                {
                    _builder.close();
                    return;
                }

                boolean last = chunk.isLast();

                // Frame header: generated bytes are COPIED into the builder's current
                // small region (put always copies).
                _builder.putInt((int)chunk.size());

                // Frame payload: NOT copied.
                //
                // INTERNALLY: the accumulating builder decides to keep large content by
                // reference: it retains chunk's storage (its rc increments) and links it
                // into the chain after the header region. A tiny chunk would instead be
                // copied into the header region (so its pooled network buffer recycles
                // immediately); either way the NEXT LINE IS THE SAME, which is the point
                // of the idiom: the caller cannot get ownership wrong by not knowing
                // which choice was made.
                _builder.append(chunk);
                chunk.release(); // pairs the read(); net rc unchanged if retained, decremented if copied.

                if (last)
                {
                    // Emit the frame: a composite of [header-region-slice, content...].
                    RetainableBuffer framed = _builder.build();

                    // The sink's EndPoint-backed override drives built.transferTo(endPoint),
                    // which presents ALL regions to EndPoint.flush(ByteBuffer...) as ONE
                    // gathering write: header and content leave in a single syscall, nothing
                    // is ever coalesced. The regions are exposed only for the duration of
                    // the flush call.
                    framed.writeTo(_sink, last, Callback.from(framed::release, x ->
                    {
                        framed.release();
                        _builder.close();
                    }));

                    return;
                }
            }
        }
    }