[jira] Resolved: (DIRMINA-328) org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient accumulation strategy

["Trustin Lee (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/DIRMINA-328?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Trustin Lee resolved DIRMINA-328.
---------------------------------

       Resolution: Fixed
    Fix Version/s: 1.0.2
         Assignee: Trustin Lee

Thank you for the patch!  It has been applied to all branches.

> org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient accumulation strategy
> ------------------------------------------------------------------------------------------------
>
>                 Key: DIRMINA-328
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-328
>             Project: MINA
>          Issue Type: Improvement
>          Components: Filter
>    Affects Versions: 1.0.2, 1.1.0, 2.0.0-M1
>         Environment: I have only studied the implementation thus far, and environment isn't a factor in this analysis.
>            Reporter: Steven E. Harris
>         Assigned To: Trustin Lee
>            Priority: Minor
>             Fix For: 1.0.2
>
>         Attachments: CumulativeProtocolDecoder.java.patch
>
>
> 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.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira