Issue #5499 - Reduce buffer allocations and copying from ByteAccumulator

[lachlan-roberts]

Issue #5499

• Add utility classes to jetty-io, ByteBufferOutputStream2 and ByteBufferAccumulator.
• Implement ByteAccumulator with the ByteBufferAccumulator and make it AutoCloseable.
• Add getBuffer method to ByteAccumulator, allowing you to write directly to the internal buffer instead of copying.

[lachlan-roberts]

@gregw @leonchen83 This branch tries to combine the work from PRs #5520 and #5536. I haven't written much testing or javadoc yet, just want to make sure we agree on the API before continuing.

[gregw]

Looking good. I've added a bunch of comments but nothing way out of order... just suggestions really.

[gregw]

Is there really a need to remember the combined ByteBuffer?

[lachlan-roberts]

We need to remember it so that we release it when close() is called.

[gregw]

I think it is wrong for this class to manage the releasing of the combined buffer. Isn't that mostly passed along a chain and will be released when processing is complete. I still lean towards takeBuffer semantics.

[gregw]

I think this behaviour can be implemented in ByteBufferAccumulator with 2 methods:

• toBuffer will create a single buffer of all the contents, it will release any buffers in the list and the list will end up containing just the new single buffer. Any subsequent calls to toBuffer will get the same buffer back. Any calls to add more content will just allocate another buffer in the list. Buffer will be release with close
• takeBuffer will create a single buffer of all the contents, it will release any buffers in the list and clear it. It is the callers responsibility to release the returned buffer.

[lachlan-roberts]

We need to remember it so that we release it when close() is called.

[gregw]

@sbordet can you tune into this PR whilst in draft form? It really has the feel of one of those that I push @lachlan-roberts all the way in one direction, only for you to come in during review with a totally different view point :)

[gregw]

closer!

[gregw]

Suggested change

	public ByteBuffer takeByteBuffer()
	{
	int length = getLength();
	ByteBuffer combinedBuffer = _bufferPool.acquire(length, false);
	for (ByteBuffer buffer : _buffers)
	{
	combinedBuffer.put(buffer);
	}
	return combinedBuffer;
	}
	public ByteBuffer takeByteBuffer()
	{
	int length = getLength();
	switch(length)
	{
	case 0:
	return null; // TODO or empty buffer from pool?
	case 1:
	return _buffers.remove(0);
	default:
	ByteBuffer combinedBuffer = _bufferPool.acquire(length, false);
	for (ByteBuffer buffer : _buffers)
	{
	combinedBuffer.put(buffer);
	_bufferPool.release(buffer);
	}
	_buffers.clear();
	return combinedBuffer;
	}
	}

[lachlan-roberts]

I don't really like the idea of returning null from here, it will introduce null checks everywhere this is used.
How can we get empty buffer from pool, will _bufferPool.aquire(0, false) work, because that's what we are currently doing for 0 length.

[lachlan-roberts]

This also makes it so you can only call takeBuffer() once, but you can call toBuffer() multiple times and get the same buffer.

Why wouldn't we just release these buffers on close() like we would do in other cases?

[gregw]

Because the idea of takeBuffer is that you take the buffer and all it's data, hence it can't be called again.
The intent is that once you call takeBuffer you will pass the resulting buffer along some further chain of events and then only release it once it is free.
freeing the buffer on close makes no sense for most uses, as we want to have a accumulator that lives as long as the connection, during which time it may create many buffers that are taken and passed on.

Happy not to return null.

[gregw]

Suggested change

	_buffers.forEach(_bufferPool::release);
	_buffers.clear();

[gregw]

one final niggle...

[lachlan-roberts]

@sbordet you wanted me to keep these ByteBufferAccumulator classes as internal websocket classes, but not sure how to do this as I can't make them package private as they are needed in multiple packages.

I can also see some potential to use them in other places, for example ByteBufferOutputStream2 could easily be used in MultiPart if we could get the BufferPool there. Should I put them as public websocket classes or can I keep them in jetty-io?

[lorban]

This looks alright to me, but I can't help thinking that this could have been a lot simpler and more efficient if we didn't directly use ByteBuffers but some wrapper around them.

[lorban]

This length accumulation could potentially overflow Integer.MAX_VALUE. We should handle this by either returning a long from here, or detecting the integer overflow and throwing an exception.

[gregw]

Since the accumulation needs to eventually fit into a single buffer, the int should do with overflow detection elsewhere.
An ultimate buffer abstraction could allow for chains of buffers that can be gather written for a long length, but not this abstraction

[gregw]

I'll be +1 once @lorban's overflow concerns are addressed

[leonchen83]

close() method invoked by DeflateFrameExtension.java#L64
and PerMessageDeflateExtension.java#L81
transferTo method no need add extra close method

[lachlan-roberts]

I did this to preserve the behaviour of this class with the previous usage.
Before these changes it was not AutoCloseable, so it would have been used like this with no closing:

ByteAccumulator acc = new ByteAccumulator();
acc.copyChunk(something);
...
acc.transferTo(combinedBuffer);

So in this way we can be more lenient on the usage of ByteAccumulator by supporting the deprecated way of using it, and now we have the replacement ByteBufferAccumulator which is more strict and must be used with an explicit call to close().