You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "Steven E. Harris (JIRA)" <ji...@apache.org> on 2006/12/22 19:33:21 UTC
[jira] Created: (DIRMINA-328)
org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient
accumulation strategy
org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient accumulation strategy
------------------------------------------------------------------------------------------------
Key: DIRMINA-328
URL: http://issues.apache.org/jira/browse/DIRMINA-328
Project: MINA
Issue Type: Improvement
Components: Filter
Affects Versions: 1.0.2, 1.1, 2.0
Environment: I have only studied the implementation thus far, and environment isn't a factor in this analysis.
Reporter: Steven E. Harris
Priority: Minor
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: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (DIRMINA-328)
org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient
accumulation strategy
Posted by "Steven E. Harris (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/jira/browse/DIRMINA-328?page=all ]
Steven E. Harris updated DIRMINA-328:
-------------------------------------
Attachment: CumulativeProtocolDecoder.java.patch
Patch implementing second proposal described in the problem description.
> org.apache.mina.filter.codec.CumulativeProtocolDecoder uses an inefficient accumulation strategy
> ------------------------------------------------------------------------------------------------
>
> Key: DIRMINA-328
> URL: http://issues.apache.org/jira/browse/DIRMINA-328
> Project: MINA
> Issue Type: Improvement
> Components: Filter
> Affects Versions: 1.1, 2.0, 1.0.2
> Environment: I have only studied the implementation thus far, and environment isn't a factor in this analysis.
> Reporter: Steven E. Harris
> Priority: Minor
> 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: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira