[Repost] New ChannelBuffer type proposed
Frederic Bregier
fredbregier at free.fr
Tue Sep 29 05:26:32 EDT 2009
Hi Trustin,
Yes the simple wrappefBuffer(ChannelBuffer) (only with one ChannelBuffer
argument is concerned) returns a CompositeChannelBuffer. I was feeling that
it could be better than the default slice which was the default:
@@ -306,7 +306,8 @@
*/
public static ChannelBuffer wrappedBuffer(ChannelBuffer buffer) {
if (buffer.readable()) {
- return buffer.slice();
+ ChannelBuffer[] buffers = { buffer };
+ return new CompositeChannelBuffer(buffers);
} else {
return EMPTY_BUFFER;
}
I see one reason to keep this: if the given ChannelBuffer is itself a
CompositeChannelBuffer, the result is optimized (less memory since some part
can be removed into the wrapped one) and still a CompositeChannelBuffer.
However, if not a CompositeChannelBuffer, I agree that it could be simply a
buffer.slice().
Keeping this prevents some bad composition like:
wrapped(wrapped(wrapped(wrapped(buffer1), buffer2)))
giving -> [slice(slice((slice(buffer1),buffer2)))]
but giving -> [slice(buffer1),buffer2]
So perhaps something like:
public static ChannelBuffer wrappedBuffer(ChannelBuffer buffer) {
if (buffer.readable()) {
- return buffer.slice();
+ if (buffer instanceof CompositeChannelBuffer) {
+ ChannelBuffer[] buffers = { buffer };
+ return new CompositeChannelBuffer(buffers);
+ } else {
+ return buffer.slice();
+ }
} else {
return EMPTY_BUFFER;
}
But then perhaps revert the following change:
@@ -363,7 +364,7 @@
break;
case 1:
if (buffers[0].readable()) {
- return wrappedBuffer(buffers[0]);
+ return new CompositeChannelBuffer(buffers);
}
break;
default:
WDYT ?
Also, would you like that I create too another branch for the proposed http2
renamed in http (body post encoder and decoder) that was related to this
work too ?
Cheers,
Frederic
Trustin Lee wrote:
>
> I see a problem with ChannelBuffers.wrappedBuffer(). It's not a big
> problem as we can fix it easily though. The problem is that
> ChannelBuffers.wrappedBuffer(ChannelBuffer) will create a
> CompositeChannelBuffer even when the specified buffer is not a
> composite buffer.
>
> Due to the latest regression I've put into 3.1.4, I think I have to
> release 3.1.5 soon. And then I will merge your work to the trunk and
> proceed to 3.2.0.ALPHA1.
>
> Thanks!
>
> — Trustin Lee, http://gleamynode.net/
>
>
>
> On Sun, Sep 27, 2009 at 4:33 PM, Frederic Bregier <fredbregier at free.fr>
> wrote:
>>
>> Hi Trustin,
>>
>> I've just create the "aggregate" branch. It includes the modified
>> CompositeChannelBuffer, ChannelBuffers (correction of wrapped methods) ,
>> and
>> the extension of the existing two JUnit test case (BigEndianComposite and
>> LittleEndianComposite ChannelBufferTest).
>>
>> I've rerun the Junit test and all seem ok.
>>
>> Cheers,
>> Frederic
>>
>>
>> Frederic Bregier wrote:
>>>
>>> Hi Trustin,
>>>
>>> Ok with the new branch.
>>> I will create it named "aggregate" in order to know for what reason it
>>> is
>>> there, if you're ok.
>>>
>>> Cheers,
>>> Frederic
>>>
>>>
>>> Trustin Lee wrote:
>>>>
>>>> Hi Frederic,
>>>>
>>>> On Fri, Sep 25, 2009 at 2:14 PM, Frederic Bregier <fredbregier at free.fr>
>>>> wrote:
>>>>>
>>>>> Hi Trustin,
>>>>>
>>>>> I'm agree that we could replace CompositeChannelBuffer with this one
>>>>> (so
>>>>> not
>>>>> introducing a new name).
>>>>> I had done like this in order to allow anyone to test it without
>>>>> removing
>>>>> CCB from Netty.
>>>>> Now that you think it is ok, I will propose you the new code for CCB
>>>>> using
>>>>> the one from the Aggregate.
>>>>>
>>>>> For wrappedCheckedBuffer, in fact, it was strictly in the same mind
>>>>> that
>>>>> for
>>>>> CCB, that is to say that I did not wanted to remove the default
>>>>> wrappedBuffer from ChannelBuffers. But of course, I would propose you
>>>>> the
>>>>> patch for ChannelBuffers such that wrappedBuffer take care of this
>>>>> without
>>>>> introducing any new method...
>>>>>
>>>>> Give me a day or two (at least before the end of the week-end), and I
>>>>> will
>>>>> post a new set of java code (replacement for CompositeChannelBuffer
>>>>> and
>>>>> its
>>>>> Junit test, and the patch for ChannelBuffers).
>>>>> Are you ok with this ?
>>>>
>>>> Sure. Sounds great!
>>>>
>>>>> I could also directly patch on trunk but if you don't mind, I would
>>>>> prefer
>>>>> you check a last time before comiting, ok ?
>>>>
>>>> Let's create a new temporary branch. I still need to maintain 3.1.x
>>>> branch and this feature should go to 3.2. WDYT?
>>>>
>>>> Thanks!
>>>>
>>>> — Trustin Lee, http://gleamynode.net/
>>>>
>>>>>
>>>>> Cheers,
>>>>> Frederic
>>>>>
>>>>>
>>>>>
>>>>> Trustin Lee wrote:
>>>>>>
>>>>>> Hi Frederic,
>>>>>>
>>>>>> The code looks OK, but I'm still not sure if we need to make
>>>>>> wrappedCheckedBuffer() public. We could modify
>>>>>> ChannelBuffers.wrappedBuffer() to recognize composite buffers using
>>>>>> instanceof.
>>>>>>
>>>>>> Also, if your implementation works in the same way with
>>>>>> CompositeChannelBuffer, then we could simply replace it with your
>>>>>> buffer type and rename it to CompositeChannelBuffer, rather than
>>>>>> introducing a new name?
>>>>>>
>>>>>> Thanks for your patience
>>>>>>
>>>>>> — Trustin Lee, http://gleamynode.net/
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Sep 10, 2009 at 5:13 AM, Frederic Bregier
>>>>>> <fredbregier at free.fr>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Trustin,
>>>>>>>
>>>>>>> I've found a bug in the slice implementation when start is not at 0.
>>>>>>> So I correct this and add a last line in the testDiscardReadBytes
>>>>>>> method
>>>>>>> of
>>>>>>> the Junit test (in Aggregate since it override the default one from
>>>>>>> Abstract
>>>>>>> Junit test since this original test could be buggy for the test of
>>>>>>> not
>>>>>>> equality of extra bytes as I wrote in previous mail).
>>>>>>>
>>>>>>> Here are the two files:
>>>>>>> http://n2.nabble.com/file/n3613619/AggregateChannelBuffer.java
>>>>>>> AggregateChannelBuffer.java
>>>>>>> http://n2.nabble.com/file/n3613619/AggregateChannelBufferTest.java
>>>>>>> AggregateChannelBufferTest.java
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Frederic
>>>>>>>
>>>>>>>
>>>>>>> Frederic Bregier wrote:
>>>>>>>>
>>>>>>>> Hi Trustin,
>>>>>>>>
>>>>>>>> Ok I write and test the slice functionality (using Junit).
>>>>>>>> It seems OK.
>>>>>>>>
>>>>>>>> I still feel like writeBytes should be left as the default
>>>>>>>> implementation
>>>>>>>> (ie the one from AbstractChannelBuffer) since the setBytes method
>>>>>>>> should
>>>>>>>> do the trick as usual.
>>>>>>>> I test by using the default implementation, and all tests pass
>>>>>>>> too...
>>>>>>>> So, except if you think that writing by slice from source could be
>>>>>>>> useful
>>>>>>>> (if the source is an aggregate one), I think this method could be
>>>>>>>> removed
>>>>>>>> from Aggregate (so not implementing in the modified version of
>>>>>>>> Component
>>>>>>>> if ok) so letting the default one be called.
>>>>>>>> I put a comment on this method.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Frederic
>>>>>>>>
>>>>>>>> http://n2.nabble.com/file/n3565134/AggregateChannelBuffer.java
>>>>>>>> AggregateChannelBuffer.java
>>>>>>>> http://n2.nabble.com/file/n3565134/AggregateChannelBufferTest.java
>>>>>>>> AggregateChannelBufferTest.java
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -----
>>>>>>> Hardware/Software Architect
>>>>>>> --
>>>>>>> View this message in context:
>>>>>>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3613619.html
>>>>>>> Sent from the Netty Developer Group mailing list archive at
>>>>>>> Nabble.com.
>>>>>>> _______________________________________________
>>>>>>> netty-dev mailing list
>>>>>>> netty-dev at lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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-tp3496606p3710508.html
>>>>> Sent from the Netty Developer Group mailing list archive at
>>>>> Nabble.com.
>>>>>
>>>>> _______________________________________________
>>>>> netty-dev mailing list
>>>>> netty-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> 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-tp3496606p3719936.html
>> Sent from the Netty Developer Group mailing list archive at Nabble.com.
>>
>> _______________________________________________
>> netty-dev mailing list
>> netty-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>
>
> _______________________________________________
> 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-tp3496606p3734901.html
Sent from the Netty Developer Group mailing list archive at Nabble.com.
More information about the netty-dev
mailing list