protobuf: zero copy into protobuf #7250
Conversation
…g underlying ByteBuffers
…ByteBuffer operation.
…ation supports it.
voidzcy
force-pushed
the
impl/zero_copy_to_protobuf
branch
from
fe6c132 to
94b5124
Compare
| * @throws UnsupportedOperationException the buffer does not support this method | ||
| * @throws IndexOutOfBoundsException if required bytes are not readable | ||
| */ | ||
| List<ByteBuffer> readByteBuffers(int length); |
There was a problem hiding this comment.
How about just returning null here instead of separate boolean method? This would also be more efficient for composite case since it means not having to iterate over the buffers an additional time.
| if (buffer.readableBytes() < length) { | ||
| throw new IndexOutOfBoundsException(); | ||
| } | ||
| List<ByteBuffer> res = Arrays.asList(buffer.nioBuffers(buffer.readerIndex(), length)); |
There was a problem hiding this comment.
Here would be preferable to do:
List<ByteBuffer> res = buffer.nioBufferCount() == 1
? Collections.singletonList(buffer.nioBuffer(buffer.readableIndex(), length)
: Arrays.asList(buffer.nioBuffers(buffer.readerIndex(), length));| public List<ByteBuffer> readByteBuffers(int length) { | ||
| if (buffer.readableBytes() < length) { | ||
| throw new IndexOutOfBoundsException(); | ||
| } |
There was a problem hiding this comment.
Return empty list here if length == 0?
| // messages. | ||
| if (stream instanceof ByteBufferReadable) { | ||
| cis = CodedInputStream.newInstance( | ||
| ((ByteBufferReadable) stream).readByteBuffers(size)); |
There was a problem hiding this comment.
Seems the protobuf code doesn't do this itself so would be better to handle (most common) singleton case separately:
List<ByteBuffer> bufs = ((ByteBufferReadable) stream).readByteBuffers(size);
cis = bufs.size() == 1 ? CodedInputStream.newInstance(bufs.get(0)) : CodedInputStream.newInstance(bufs);| * bytes can be read. | ||
| */ | ||
| @Nullable | ||
| Iterable<ByteBuffer> readByteBuffers(int length); |
There was a problem hiding this comment.
Maybe return Collections.emptyList() instead of null?
| int readLength = length; | ||
| if (buffer.readableBytes() <= length) { | ||
| readLength = buffer.readableBytes(); | ||
| buffers.poll(); |
| @Override | ||
| public List<ByteBuffer> readByteBuffers(int length) { | ||
| checkReadable(length); | ||
| readableBytes -= length; |
There was a problem hiding this comment.
Maybe better to decrement this within the loop so that the buffer state remains consistent in case of runtime exception?
Also how about a special case (should be common):
if (buffers.size() == 1) {
return (readableBytes == 0 ? buffers.poll() : buffers.peek()).readByteBuffers(length);
}…he responsibility of releasing resources after data access.
…release data when done.
|
Close in favor of #7330. |
2 participants
TODO: tests and Javadoc enhancement.