[Repost] New ChannelBuffer type proposed
Frederic Bregier
fredbregier at free.fr
Wed Sep 30 08:49:13 EDT 2009
Hi Trustin,
You're right again. wrappedBuffer could be reverted even to the original.
However I've found a bug when I made the change on my local copy.
In fact not really a bug but a different interpretation on the API but which
tends to be a bug ... in the JUnit test case.
- In slice(), the returned buffer is the sliced version where capacity =
readableBytes() from the original one (not the original capacity).
- In wrappedBuffer(buffer), the returned buffer is claimed to be a exact
copy while sharing all the data (any modification on the source lead in
modification on the wrapped one and reverse).
Now in the Junit, the first thing that is done is (in newBuffer method):
buffer = wrappedBuffer(arrayOfBuffers); where capacity is 18450
buffer.writerIndex(4096);
buffer = wrappedBuffer(buffer); *** note here there is the difference
between API and code ***
asserEquals(4096, buffer.capacity());
asserEquals(4096, buffer.readableBytes());
assertFalse(buffer.writable());
Since wrapped called slice in the second call (1 buffer), buffer is supposed
to be of capacity 4096 as of readableBytes and not writable (that's how is
the current CompositeChannelBuffer implementation).
But according to the API of wrappedBuffer, its capacity could be 18450, its
readableBytes as 4096 and being writable.
To fix this (still having the same behaviour than previous
COmpositeChannelBuffer), there are two things:
1) a fix on the new CompositeChannelBuffer (I will commit it this evening)
in the slice method:
The idea is to restrict the new CompositeChannelBuffer to the specified
slice and not still having the extra bytes as before (which is wrong
according to the API).
The fix is ready, just need full internet access...
Of course, I will revert too the ChannelBuffers method to the original.
2) fix the API doc for wrappedBuffer on ChannelBuffer where it is specified
that only readable part is keeped, not the writable part.
WDYT ?
Frederic
Trustin Lee wrote:
>
> 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
>>
>
> _______________________________________________
> 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-tp3496606p3742462.html
Sent from the Netty Developer Group mailing list archive at Nabble.com.
More information about the netty-dev
mailing list