Hi Steven,
On 12/22/06, Steven E. Harris <[EMAIL PROTECTED]> wrote:
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().
You are right. It's ineffective.
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.
We actually had been using compact() for every case, and someone found it is
ineffective for the exactly opposite case, so we accepted his patch. Now,
you came up with hybrid solution, which is best! :)
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. Please let me know your
thoughts, and if you'd like a patch to try.
Me, too. Could you provide a real patch file if you are interested in
contributing to MINA? (Actually you are already contributing. :) I can
apply the changes you proposed by myself, but I guess you already made the
change to our code so you could generate a patch with just one command. If
so, please open a JIRA issue, and attach your patch. (Don't forget to check
the radio button that helps you accept our contribution policy without
faxing a license agreement document and waiting for its confirmation for a
few days until the patch gets accepted.)
Thank you in advance,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP key fingerprints:
* E167 E6AF E73A CBCE EE41 4A29 544D DE48 FE95 4E7E
* B693 628E 6047 4F8F CFA4 455E 1C62 A7DC 0255 ECA6
