[Repost] New ChannelBuffer type proposed
Trustin Lee (이희승)
trustin at gmail.com
Wed Sep 30 03:28:27 EDT 2009
Thanks a lot - that was fast again. :)
BTW, if the specified buffer is CompositeChannelBuffer, wouldn't
calling slice() return the optimized CompositeChannelBuffer? You
improved CompositeChannelBuffer.slice(), so calling slice() on a
composite buffer and creating a new CompositeChannelBuffer() with an
existing composite buffer should have the same effect.
If this is true, we didn't need to modify ChannelBuffers at all. :-D
Am I missing something here?
— Trustin Lee, http://gleamynode.net/
On Wed, Sep 30, 2009 at 2:56 PM, Frederic Bregier <fredbregier at free.fr> wrote:
>
> Hi Trustin,
>
> I suppose that the current fix take your time (I understand), so as I said
> that I can have the time the evening, I made it ... this morning before
> going at work... ;-)
> I test it again with JUnit and it continues to be OK.
>
> So you don't have to patch it before merging.
>
> Cheers,
> Frederic
>
>
> Trustin Lee wrote:
>>
>> 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
>>>
>>
>> _______________________________________________
>> 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-tp3496606p3740900.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