New ChannelBuffer type proposed

Frederic Bregier fredbregier at free.fr
Tue Sep 1 14:51:20 EDT 2009


Just a short add on slice()
You're right, I should implement a specific slice(start, length) version
since it is legal to have readerIndex at 0 and writerIndex at length (still
keeping the extra bytes if any).

Give me a day to propose you this (it should be very easy)...

Once I've done that, I let you decide if it is good enough to replace the
current Component (which I feel a good solution since it will optimize a bit
more its already optimized current implementation) and the two methods in
ChannelBuffers for wrappedBuffer.

Cheers,
Frederic


Frederic Bregier wrote:
> 
> Hi Trustin,
> 
> First, no you didn't discourage me... ;-)
> 
> In the AggregateCB, you don't have to read everything to see what I
> propose.
> There is a 
> // XXX under that point: similar implementation of CompositeChannelBuffer
> at line 344 that specifies that special implementation stops here.
> Under that everything should be similar to CompositeChannelBuffer, except
> reference to Aggregate instead of Composite and toString() which
> integrates the number of slices.
> 
> What I suggest is not really to add a new type, but perhaps (if it is ok
> for you) to enhance the default Composite version by get some ideas from
> this Aggregate one. You've picked up some already as you state in your
> answer.
> 
> The main idea is to remove as many as possible slices that are already
> read when it is legal to do so. Mainly it is at two stages: construction
> and discard
> 
> So here are the main modifications:
> 
> In wrapped method from ChannelBuffers:
> - public static ChannelBuffer wrappedCheckedBuffer(ChannelBuffer buffer)
> Instead of 
>      return buffer.slice();
> doing
>      ChannelBuffer[] buffers = { buffer };
>      return new AggregateChannelBuffer(buffers);// not buffer.slice();
> Since it gives a chance if the buffer is itself an Aggregate to remove
> already read slices.
> 
> - public static ChannelBuffer wrappedBuffer(ChannelBuffer... buffers) {
> Instead of 
>         case 1:
>             if (buffers[0].readable()) {
>                 return wrappedBuffer(buffers[0]);
>             }
>             break;
> doing
>         case 1:
>             if (buffers[0].readable()) {
>                 return new AggregateChannelBuffer(buffers);
>             }
>             break;
> Again to give a chance to remove unused slices if already an aggregate
> one.
> 
> - public AggregateChannelBuffer(ChannelBuffer... buffers)
> The constructor tryes to keep only usable part from buffers like slice()
> does except that it takes care too about already aggregate buffer. In the
> Composite version, Composite buffers would be just added as sub-slice, so
> having slice of slice of slice... if you use the constructor several times
> with the same buffer again and again like:
> ChannelBuffer buffer = wrappedBuffer(buffer1, buffer2);
> loop
>     buffer = wrappedBuffer(buffer, otherbuffer);
>     read some from buffer
> 
> Then buffer will be something like: (if I'm correct of course)
>     buffer -> slice[0]-> (slice[0]-> (slice[0] -> buffer1+slice[1] ->
> buffer2)+slice[1]-> otherbuffer(1))
>              -> slice[1] -> otherbuffer(2)
> While Aggregate will give
>     buffer -> slice[0] -> buffer1
>              -> slice[1] -> buffer2
>              -> slice[2] -> otherbuffer(1)
>              -> slice[3] -> otherbuffer(2)
> and even if buffer1 and buffer2 are read when aggregation of
> otherbuffer(s) comes up
>     buffer -> slice[0] -> otherbuffer(1)
>              -> slice[1] -> otherbuffer(2)
> 
> - public void writeBytes(ChannelBuffer src, int srcIndex, int length)
> Again tries to add this buffer by using useful slice if it is an aggregate
> one.
> In fact, I'm not sure it is useful to do something specific here...
> 
> - public void discardReadBytes() 
> Of course, like constructor, do the real job wanted here...
> 
> 
> In fact, for the slice(), I didn't do any specific optimization since it
> produces a SlicedChannelBuffer.
> Perhaps doing something close to discard could be an optimization too as
> you wrote ?
> 
> 
> So it seems you get it. Now, I probably missed something (several tries
> and test to remove some bugs from time to time).
> WDYT?
> 
> Cheers,
> Frederic
> 
> 
> 
> Trustin Lee wrote:
>> 
>> Hi Frederic,
>> 
>> Now I've got some time to think about this new buffer type finally. :)
>> 
>> If I understood correctly, you want to improve the performance of
>> ChannelBuffer.discardReadBytes().  Am I right?
>> 
>> If so, I think we can achieve this by either:
>> 
>> 1) Relaxing the definition of ChannelBuffer.discardReadBytes() so that
>> you can remove the reference to the discardable component instead of
>> expensive memory copy:
>> 
>>   ChannelBuffer composite = ...;
>>   .. read some ..
>>   composite.discardReadBytes(); // Release the reference to the
>>                                 // discardable bytes - capacity is
>>                                 // decreased.
>> 
>> 2) Optimizing the implementation of CompositeChannelBuffer.slice() so
>> that it returns the slice that does not refer to unused components:
>> 
>>   ChannelBuffer composite = ...;
>>   .. read some ..
>>   composite = composite.slice(); // Unreadable components are
>>                                  // released.
>> 
>> The first change needs more thoughts but the second one can be applied
>> as we don't need to break backward compatibility at all.
>> 
>> Both changes could be made as they are complimentary, and once done, we
>> don't need AggregatedChannelBuffer, or am I missing out something?
>> 
>> BTW I hope my suggestion did not discourage you.  I just want to make
>> sure to find the most efficient way with least effort and least change.
>> 
>> Trustin
>> 
>> On Sun, 23 Aug 2009 06:34:17 +0900 (KST)
>> Frederic Bregier <fredbregier at free.fr> wrote:
>>> 
>>> Hi Trustin and all,
>>> 
>>> I've playing a bit with the Http codec and I found that having an
>>> aggregate version of the channelBuffer, almost like the
>>> CompositeChannelBuffer one, but that differs in such way that:
>>> 
>>> - when an aggregation is done (at construction for instance), only
>>> non empty buffer are kept.
>>> - when some aggregate buffer is passed at constructor argument, they
>>> are parsed from their slices too.
>>> 
>>> The main idea is to keep the advantage of the Composite but:
>>> - the memory consumption is lower by discarding unsuseful
>>> channelBuffer
>>> - the speed where no copy is necessary is the same than in Composite
>>> - only one level of buffer most of the time is keeped (in fact 2:
>>> each slice is a Slice ChannelBuffer which points to the real previous
>>> ChannelBuffer)
>>> 
>>> Therefore, it is quite efficient to do:
>>> 
>>> ChannelBuffer mybuffer = ChannelBuffers.EMPTY_BUFFER;
>>> mybuffer = AggregateChannelBuffer.wrappedCheckedBuffer(mybuffer,
>>> otherbuffer, ...);
>>> while (true) {
>>>     consumeSomeBytes(mybuffer);
>>>     mybuffer = AggregateChannelBuffer.wrappedCheckedBuffer(mybuffer,
>>> newlyProduceBuffer);
>>> }
>>> 
>>> since there is a chance that the number of buffers in the mybuffer
>>> will stay low in memory since there are consumed.
>>> In the HttpDataDecoder, the FileUpload is one good example.
>>> 
>>> Not all methods are already optimzed. For instance, it is possible to
>>> optimize:
>>> - writeBytes(ChannelBuffer, ...) methods
>>> I try to implement in the joined implementation the method
>>> writeBytes(ChannelBuffer, int, int) (the others will use it).
>>> 
>>> Note that AggregateCB has no write capability by default (due to the
>>> slice command used to add CB from constructor), like with CompositeCB
>>> too (at least, if I understand well). 
>>> However, getting such function could help the previous code as the
>>> following (simpler) but could be dizzy if the user is not warned
>>> enough -he could believe that other write operations are possible- :
>>> 
>>> ChannelBuffer mybuffer = ChannelBuffers.EMPTY_BUFFER;
>>> mybuffer = AggregateChannelBuffer.wrappedCheckedBuffer(mybuffer,
>>> otherbuffer, ...);
>>> while (true) {
>>>     consumeSomeBytes(mybuffer);
>>>     mybuffer.writeBytes(newlyProduceBuffer);
>>> }
>>> 
>>> Perhaps better would be to create a new method addBytes(ChannelBuffer)
>>> instead of writeBytes, which is more exact.
>>> 
>>>     mybuffer.addBytes(newlyProduceBuffer);
>>> 
>>> http://n2.nabble.com/file/n3496606/AggregateChannelBuffer.java
>>> AggregateChannelBuffer.java 
>>> 
>>> However, this is a proposition, perhaps I'm wrong...
>>> Of course any idea or enhancement... ;-)
>>> 
>>> Cheers,
>>> Frederic
>>> 
>>> -----
>>> Hardware/Software Architect
>> 
>> 
>> 
>> -- 
>> Trustin Lee, http://gleamynode.net/
>> _______________________________________________
>> netty-dev mailing list
>> netty-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/netty-dev
>> 
>> 
> 
> 


-----
Hardware/Software Architect
-- 
View this message in context: http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3562359.html
Sent from the Netty Developer Group mailing list archive at Nabble.com.


More information about the netty-dev mailing list