Details
Minor Description
I've just started writing a MINA protocol decoder extending the class
org.apache.mina.filter.codec.CumulativeProtocolDecoder, and I noticed
some potential pathological behavior with its current implementation
of its storeRemainingInSession() method:
,----
| private void storeRemainingInSession(ByteBuffer buf, IoSession session) |
| {
| ByteBuffer remainingBuf = ByteBuffer.allocate( buf.remaining() );
| remainingBuf.setAutoExpand( true );
| remainingBuf.put( buf );
| session.setAttribute( BUFFER, remainingBuf );
| }
`---- |
The ByteBuffer passed as the first argument is either the buffer
supplied to the decode() method, or it's buffer stored in the
IoSession by a previous call to decode(), allocated and owned by
CumulativeProtocolDecoder.
The current implementation unconditionally allocates a fresh buffer
and copies any unconsumed data from the current buffer in use to the
new buffer, then stores this new buffer for use on a subsequent call
to decode().
Consider two abberrant cases: when the doDecode() method consumes only
one byte and returns false, and when it consumes all but one byte and
returns false. Assume the buffer to be decoded had N bytes
remaining.
In the first case, a new buffer gets allocated of size N - 1, with N -
1 bytes copied, and the current buffer is abandoned. That's a lot of
spurious allocation with a potential reduction in buffer capacity for
the next pass.
In the second case, a new buffer gets allocated of size 1, with 1 byte
copied, and the current buffer is abandoned. We now have a capacity of
only 1 in the new buffer, having tossed away the larger capacity of
the previous buffer. This new buffer will certainly have to grow to
accommodate any new content, causing even more reallocation.
I suggest we take advantage of the ByteBuffer.compact() method to
attempt to preserve capacity and avoid reallocation when possible. Of
course, the same amount of bytes must be copied with either approach.
My proposed implementations assume that we're not allowed to hang on
to the buffer supplied to our ProtocolDecoder.decode() call. First,
here's one that checks the session attribute again to see if we're
using a buffer we've previously allocated ourselves:
,----
| private void storeRemainingInSession(ByteBuffer buf, IoSession session) |
| { |
| ByteBuffer sessionBuf = ( ByteBuffer ) session.getAttribute( BUFFER ); |
| if ( sessionBuf != null ) |
| { | // We're reusing our own buffer. | assert( buf == sessionBuf ); | buf.compact(); | } |
| else |
| { | // We're using a supplied buffer, so we need to copy it. | final ByteBuffer remainingBuf = ByteBuffer.allocate( buf.capacity() ); | remainingBuf.setAutoExpand( true ); | remainingBuf.put( buf ); | session.setAttribute( BUFFER, remainingBuf ); | } |
| } `---- |
Alternately, we could keep a boolean flag around from the beginning of
the decode() method the first time we check the session attribute, and
only call storeRemainingInSession() if we need to copy a buffer we
didn't allocate:
,----
| public void decode( IoSession session, ByteBuffer in, |
| ProtocolDecoderOutput out ) throws Exception |
| { |
| boolean usingSessionBuffer = true; |
| ByteBuffer buf = ( ByteBuffer ) session.getAttribute( BUFFER ); |
| // if we have a session buffer, append data to that otherwise |
| // use the buffer read from the network directly |
| if( buf != null ) |
| { | buf.put( in ); | buf.flip(); | } |
| else |
| {
| usingSessionBuffer = false;
| buf = in;
| }
`---- |
...
,----
| if ( buf.hasRemaining() ) |
| { | if ( usingSessionBuffer ) | buf.compact(); | else | storeRemainingInSession( buf, session ); | } |
| else |
| {
| if ( usingSessionBuffer )
| session.removeAttribute( BUFFER );
| }
`---- |
I think the second approach is better. Find a patch attached.