Implement the new Buffer API in terms of the ByteBuf API #12099
Conversation
chrisvest
force-pushed
the
5x-bytebuf-buffer
branch
from
e110a22
to
20c4fa5
Compare
|
|
||
| @Override | ||
| public int writableBytes() { | ||
| return delegate.capacity() - delegate.writerIndex(); |
There was a problem hiding this comment.
| return delegate.capacity() - delegate.writerIndex(); | |
| return delegate.writableBytes(); |
There was a problem hiding this comment.
No, that breaks for read-only buffers. I did try it. 🤷♂️
There was a problem hiding this comment.
in what way ? Because it always return 0 ?
There was a problem hiding this comment.
Yes. In the Buffer interface, it's expected to just be based on the write offset and the capacity.
There was a problem hiding this comment.
I am a bit on the fence here... Generally speaking it seems odd that a ReadOnly buffer would return anything different then 0 for writableBytes.
buffer/src/main/java/io/netty/buffer/api/adaptor/ByteBufBuffer.java
Outdated
Show resolved
Hide resolved
buffer/src/main/java/io/netty/buffer/api/adaptor/ByteBufMemoryManager.java
Outdated
Show resolved
Hide resolved
|
@normanmaurer Addressed your comments. Please take a look. |
|
Looks like a test is running out of memory somewhere. |
Motivation: This will be useful for bridging the gap between the two APIs. Integrators will be able to use this as they migrate their code to the new API bit-by-bit. Modification: Add a complete implementation of the Buffer interface, that is written in terms of the ByteBuf API. The implementation passes all of the Buffer tests. Result: It is now possible to adapt a ByteBuf into a Buffer. This capability joins the ByteBufAdaptor as a sibling. The ByteBufAdaptor converts a Buffer into a ByteBuf, while the new ByteBufBuffer works in the opposite direction, and converts a ByteBuf into a Buffer.
…en if we're copying zero bytes
chrisvest
force-pushed
the
5x-bytebuf-buffer
branch
from
83fa25f
to
977402d
Compare
chrisvest
force-pushed
the
5x-bytebuf-buffer
branch
from
977402d
to
fe33aba
Compare
buffer/src/main/java/io/netty/buffer/api/adaptor/ByteBufMemoryManager.java
Show resolved
Hide resolved
buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java
Outdated
Show resolved
Hide resolved
buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java
Outdated
Show resolved
Hide resolved
| @@ -133,7 +133,7 @@ public void testWithoutUseCacheForAllThreads() { | |||
| /*nHeapArena=*/ 1, | |||
| /*nDirectArena=*/ 1, | |||
| /*pageSize=*/8192, | |||
| /*maxOrder=*/ 11, | |||
| /*maxOrder=*/ 9, | |||
There was a problem hiding this comment.
to reduce memory overhead ?
There was a problem hiding this comment.
Yeah, I was starting to get OOMEs from this test.
There was a problem hiding this comment.
Looks like I have to change the default max order as well. Not clear why this is suddenly such a big problem.
Motivation: A max order of 11 implies a chunk size of 16 MiB, which can be a large investment in memory, and occasionally causes the tests to run out of memory. It is also a large investment on machines with many cores, since the number of arenas are correlated with the number of CPU cores, and each arena that is used will likely allocate a chunk. Modification: Reduce the default max order such that the default chunk size is reduced from 16 MiB to 4 MiB. This potentially increases the number of huge allocations, but I think it's a fair trade-off for Netty's use cases. Result: If we don't need a lot of memory, we'll now use less memory.
It is troublesome to initialise objects at image build time, if they have references to an AbstractReferenceCountedByteBuf, for instance.
This way we make sure to always deallocate the target copy, if the copying fails.
|
@normanmaurer I think this is good now, unless you insist on preserving the ByteBuf behaviour of read-only buffers. |
|
Let's tackle what |
Motivation:
This will be useful for bridging the gap between the two APIs.
Integrators will be able to use this as they migrate their code to the new API bit-by-bit.
Modification:
Add a complete implementation of the Buffer interface, that is written in terms of the ByteBuf API.
The implementation passes all of the Buffer tests.
Result:
It is now possible to adapt a ByteBuf into a Buffer.
This capability joins the ByteBufAdaptor as a sibling.
The ByteBufAdaptor converts a Buffer into a ByteBuf, while the new ByteBufBuffer works in the opposite direction, and converts a ByteBuf into a Buffer.