[Repost] New ChannelBuffer type proposed
Trustin Lee (이희승)
trustin at gmail.com
Tue Sep 29 06:05:52 EDT 2009
No problem. I will take care of it when I merge. It's not a big deal. :)
— Trustin Lee, http://gleamynode.net/
On Tue, Sep 29, 2009 at 6:43 PM, Frederic Bregier <fredbregier at free.fr> wrote:
>
> No, I'm not so fast, but I spend some times one Netty those days ("before
> production testing" is starting those days).
> However, I'm not able to make the change into the branch except this evening
> (no access from my job except mailing list). So do you mind if you take care
> of those changes ?
>
>
> On the http2 step, OK, I will wait for the 3.2.0.ALPHA1 in trunk in order to
> have the correct dependencies with the new branch.
>
> Cheers,
> Frederic
>
>
> Trustin Lee wrote:
>>
>> 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
>>>
>>
>> _______________________________________________
>> 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-tp3496606p3734984.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