[Repost] New ChannelBuffer type proposed
Trustin Lee (이희승)
trustin at gmail.com
Tue Sep 29 05:29:37 EDT 2009
The new patch looks great. You are fast :)
Sure. Please feel free to create another branch as this branch is
going to be merged pretty soon.
Cheers!
— Trustin Lee, http://gleamynode.net/
On Tue, Sep 29, 2009 at 6:26 PM, Frederic Bregier <fredbregier at free.fr> wrote:
>
> 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.
>
> _______________________________________________
> netty-dev mailing list
> netty-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/netty-dev
>
More information about the netty-dev
mailing list