Easy and performant copy of the ByteBuffer

• Key: DIRMINA-165
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Trustin Lee
• Reporter: Federico Bonelli
• Affects Version/s: 0.8.0, 0.8.1, 0.8.2, 0.9.0
• Fix Version/s: 0.9.3

Description

Until now if you wish to broadcast a message you must create by your own the bytebuffer copies, it would be better to provide a ByteBuffer.asReadOnly() method that create a copy that share the data with the original ByteBuffer

Comments

[Federico Bonelli]

In the MINA byteBuffer.asReadOnly() I create a new mina DefaultByteBuffer container and get into that the nio.buffer copy and the reference to the original mina bytebuffer, and a boolean flag that says if the DefaultByteBuffer wrap a readOnly copy or a real writable nio.bytebuffer. I do an aquire() of the original MINA byteBuffer.
(Errata Corrige: instead of the flag I have used the reference to the original buffer: if it is null I think that it is not a copy)

So when the MINA ByteBuffer copy is released, the release method just look at the flag and release the original mina bytebuffer.
The MINA bytebuffer copy _must_ be not pooled obviusly.
 
In this way there is no need to make a copy of the data, but only a new nio.bytebuffer that use the same byte[] or direct array, and the original ByteBuffer will be released only when all the copies will be already written to the channel.

This implementation do not work well with the expand features.
All the stuff I have added are marked with a "DIRMINA 165" comment.

[Federico Bonelli]

The ByteBuffer version I have published is not a good solution, I have modified it so I have the last version and an echo TCP server that does broadcast with the messages received.

Somebody who have the power can remove here the ByteBuffer so I can publish the right version with the example?

[Niklas Therning]

The original attachment has been removed

[Federico Bonelli]

Thanks a lot Niklas 8)
This is the MINA broadcast test server, I have just modified the echoserver, the code is very trivial.
Tested and well-working

[Federico Bonelli]

And this is the modified (and working) ByteBuffer, with the asReadOnly() method.
See the hubserver for an example of the right usage.

[Trustin Lee]

Your patch is very error-prone. What about this?

* Provide all methods like duplicate, asReadOnlyBuffer, slice, ...
* To invoke these methods, 'pooled' property must be false.
* 'pooled' property of all derived buffers and its parent buffer must be 'false' and cannot be changed anymore.

In case of broadcasting only, we could provide more effective solution as we discussed in DIRMINA-42.

WDYT?

[Niklas Therning]

I've already implemented slice() but haven't committed it yet. I've also added a slice(offset, length) method which uses the passed in values instead of the current postion and limit. I think it will be quite useful. I haven't thought of checking pooled == false. I will have to add that before committing.

slice() becomes:

public ByteBuffer slice()
{
  assertNotPooled(); // Throws IllegalStateException if pooled == true
  return wrap( buf().slice() );
}

Does this look OK? If it does the rest of the methods (duplicate(), asReadOnlyBuffer()) could be implemented like this.

[Niklas Therning]

There could also be a ByteBuffer.unpool() method which returns an unpooled copy of the buffer. If the buffer isn't pooled it will simply return itself:

[Federico Bonelli]

I agree that buffers need all the duplicate-like methods, but what about pooling them?

If all the buffers copied _and the parent too_ have to be unpooled, it may increase a lot the time of allocation for direct buffers, especially in case of a massive use of copies that could be very light for CPU.

It is certainly better than all the copies have pooled = false, and could not be changed, but the parent can be pooled, I think, after that all the copies have been released.
WDYT?

[Trustin Lee]

Will fix this in 0.9.3 due to high demand

[Trustin Lee]

Another solution:

Wrap underlying NIO buffer with UnexpandableByteBuffer (MINA buffer). The eventual MINA buffer we use wraps UnexpandableByteBuffer. In other words, we can solve this problem by maintaining reference count in two-level.

[Trustin Lee]

I realized the solution I mentioned above was not really easy to implement after wasting an hour. :(

Federico's solution doesn't resolve the problem in the following scenario:

    buf = ...;
    dupBuf = buf.duplicate();
    buf.release();
    newBuf = ByteBuffer.allocate( ... ); // newBuf can be the same instance with buf
    dupBuf.release(); // This will lead us to a big mess.

[Trustin Lee]

It's hard to duplicate buffers safely with autoExpand.

There are two solutions AFAIK:

* Force 'autoExpand' property to be 'false' always, once duplication is invoked.

In this workaround, reference count is shared among the original buffer and the derived ones. It becomes hard to detect wrong number of releases spreaded over multiple duplicate buffers.

* Force both 'pooled' and 'autoExpand' property to be 'false' always, once duplication is invoked.

In this workaround, there's no need to manage a referemce count at all.

[Federico Bonelli]

I agree about keeping autoExpand "false" after a duplication.

Personally I thought a lot about this problem, and I suggest having a refCount object, shared between all the copies and the originalBuffer (I consider the original as a copy like the others), and having an unshared refCount, used privately by each copy.
SharedRefCount should be an object wich has a reference to the original nio.bytebuffer.
Then, when we create a copy, its own refCount is set to 1 by default, and we should give the copy the sharedRefCount increased by 1.
When an acquire() or release() method is invoked it will increase/decrease the private refCount and the sharedRefCount.

In this way, whenever calling a release(),and the privateRefCount goes to zero, we check if the sharedRefCount is higher than zero, and we simply discard the underlying nio.bytebuffer (because it is a copy).
Instead, if the sharedRefCount is zero, we discard, as usual, our nio.bytebuffer, and then we pool the original nio.ByteBuffer, obtained from our sharedRefCount.

This way of doing should be effective with duplicate() and asReadOnly() methods, because we are sure we don't pool any of the copied nio.bytebuffers that are dangerous to be used, and we manage the privateRefCounts properly.

WDYT?

[Robert Greig]

I think (?) I agree with Federico. I don't see why we need to lose the pooling of the original buffer in the case where you slice the buffer, although obviously you need to prevent auto expand.

In the implementation I did as a quick test, I created a subclass of DefaultByteBuffer - I called it SlicedByteBuffer - which had its own reference count and held a reference to the original (parent) byte buffer it came from. When you call slice on a DefaultByteBuffer, that increment the reference count and returns a SlicedByteBuffer. Calling release() on the SlicedByteBuffer decrements the slice's reference count and once that reaches zero it calls release() on the parent.

So in Trustin's example:

    buf = ...;
    dupBuf = buf.duplicate(); // this increments the ref count of buf and returns a subclass that has different release() implementation
    buf.release(); // ref count is still 1 so buffer is not released into the pool
    newBuf = ByteBuffer.allocate( ... ); // this is now fine
    dupBuf.release(); // this releases both the dupBuf and buf instances

Obviously SlicedByteBuffer cannot be pooled nor can it autoexpand.

[Trustin Lee]

I checked in the fix. I think I added sufficient test cases to ByteBufferTest:

http://svn.apache.org/viewcvs?rev=387793&view=rev

Please review the fix and try to break it. :)