Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
9936df1
Add mark&reset methods and canUseByteBuffer&getByteBuffer methods to …
voidzcy Aug 17, 2020
aa93e1d
Default implementations.
voidzcy Aug 17, 2020
454e224
Wire new methods for forwarder
voidzcy Aug 17, 2020
3f47d5e
Support mark&reset and retrieving content via ByteBuffer for netty.
voidzcy Aug 17, 2020
b3c9974
Implementation for composite.
voidzcy Aug 17, 2020
2ede525
Define interface for accessing readable content via ByteBuffers.
voidzcy Aug 17, 2020
3dca312
Implement mark&reset for simple readable buffers.
voidzcy Aug 17, 2020
6249353
Use HasByteBuffer interface for accesing input stream's backing ByteB…
voidzcy Aug 17, 2020
29dce67
Eliminate the length argument for retrieving the ByteBuffer.
voidzcy Aug 17, 2020
22ea6c3
Do no require netty buffer to be direct from API's perspective.
voidzcy Aug 17, 2020
53e347c
Use Deque operations to avoid unncessary moves.
voidzcy Aug 17, 2020
27801fe
Make a list of ByteBuffers up-front instead of a running iterator.
voidzcy Aug 17, 2020
eb71a68
Add getByteBufferSupported method for HasByteBuffer so that it can be…
voidzcy Aug 17, 2020
5d3c657
It's not necessary to implement getByteBuffer for ByteReadbaleBufferW…
voidzcy Aug 17, 2020
9fd8d3c
Add test coverage for mark&reset and getByteBuffer for generic ByteBu…
voidzcy Aug 17, 2020
e3afe50
Add test coverage for netty's special get NIO bytebuffer operation.
voidzcy Aug 17, 2020
e2fdd07
Skip test for operations not supported by okhttp.
voidzcy Aug 17, 2020
033270b
Add test coverage for BufferInputStream with getByteBuffer operation.
voidzcy Aug 17, 2020
0622d51
Add test using a known-length input stream with getByteBuffer operati…
voidzcy Aug 17, 2020
0e8caee
Modify test method name.
voidzcy Aug 17, 2020
ba4e91b
Add test coverage for mark&reset and getByteBuffer for CompositeReada…
voidzcy Aug 17, 2020
69618b2
Add getByteBuffer support for ByteReadableBufferWrapper.
voidzcy Aug 20, 2020
437857d
Only pull ByteBuffers when message is large.
voidzcy Aug 21, 2020
1363505
Run ByteBuffer codepath only in Java 9+.
voidzcy Aug 28, 2020
c46eb73
Slight improvement for avoiding array creation if not necessary.
voidzcy Aug 28, 2020
7b4e070
Merge branch 'master' of github.com:grpc/grpc-java into impl/zero_cop…
voidzcy Aug 28, 2020
772b3ba
Change ReadableBuffer#canUseByteBuffer to hasByteBuffer.
voidzcy Sep 1, 2020
b1c99e5
Removed unnecessary reset.
voidzcy Sep 1, 2020
692076c
Simplify checking runtime java version.
voidzcy Sep 1, 2020
10c13b8
Add ExperimentalApi annotation.
voidzcy Sep 1, 2020
f13c165
Rename ReadableBuffer#hasByteBuffer to getByteBufferSupported.
voidzcy Sep 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Eliminate the length argument for retrieving the ByteBuffer.
  • Loading branch information
voidzcy committed Aug 17, 2020
commit 29dce6730c349e4db625c0be271a12f9eac21d2e
11 changes: 7 additions & 4 deletions api/src/main/java/io/grpc/HasByteBuffer.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@
public interface HasByteBuffer {

/**
* Gets a {@link ByteBuffer} containing up to {@code length} bytes of the content, or {@code
* null} if has reached end of the content.
* @param length the maximum number of bytes to contain in returned {@link ByteBuffer}.
* Gets a {@link ByteBuffer} containing some bytes of the content next to be read, or {@code
* null} if has reached end of the content. The number of bytes contained in the returned buffer
* is implementation specific. Calling this method does not change the position of the input
* stream. The returned buffer's content should not be modified, but the position, limit, and
* mark may be changed. Operations for changing the position, limit, and mark of the returned
* buffer does not affect the position, limit, and mark of this input stream.
*/
@Nullable
ByteBuffer getByteBuffer(int length);
ByteBuffer getByteBuffer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the API cleaner how about having this return null for not supported and a (constant) empty ByteBuffer for EOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having this return null for not supported and a (constant) empty ByteBuffer for EOS

The semantics of returning null for operation not supported isn't strong enough. Most existing Java APIs (e.g.,java.nio.ByteBuffer uses hasArray() and array()) throw an UnsupportedOperationException for unsupported optional APIs. I think the current approach is cleaner.

}
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public boolean canUseByteBuffer() {
}

@Override
public ByteBuffer getByteBuffer(int length) {
public ByteBuffer getByteBuffer() {
throw new UnsupportedOperationException();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,11 @@ public boolean canUseByteBuffer() {

@Nullable
@Override
public ByteBuffer getByteBuffer(int length) {
public ByteBuffer getByteBuffer() {
if (readableBuffers.isEmpty()) {
return null;
}
ReadableBuffer buffer = readableBuffers.peek();
return buffer.getByteBuffer(Math.min(length, buffer.readableBytes()));
return readableBuffers.peek().getByteBuffer();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ public boolean canUseByteBuffer() {

@Nullable
@Override
public ByteBuffer getByteBuffer(int length) {
return buf.getByteBuffer(length);
public ByteBuffer getByteBuffer() {
return buf.getByteBuffer();
}

@Override
Expand Down
18 changes: 9 additions & 9 deletions core/src/main/java/io/grpc/internal/ReadableBuffer.java
Original file line number Diff line number Diff line change
Expand Up @@ -143,19 +143,19 @@ public interface ReadableBuffer extends Closeable {
boolean canUseByteBuffer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe better name would be hasByteBuffer()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, go as you like. I don't have a strong preference.


/**
* Gets a {@link ByteBuffer} that contains up to {@code length} bytes of this buffer's content,
* or {@code null} if this buffer has been exhausted. The position of this buffer is unchanged
* after calling this method. The returned buffer's content should not be modified, but the
* position, limit, and mark may be changed. Operations for changing the position, limit, and
* mark of the returned {@link ByteBuffer} does not affect the position, limit, and mark of
* this buffer. {@link ByteBuffer}s returned by this method have independent position, limit
* and mark. This is an optional method, so callers should first check {@link #canUseByteBuffer}.
* Gets a {@link ByteBuffer} that contains some bytes of the content next to be read, or {@code
* null} if this buffer has been exhausted. The number of bytes contained in the returned buffer
* is implementation specific. The position of this buffer is unchanged after calling this
* method. The returned buffer's content should not be modified, but the position, limit, and
* mark may be changed. Operations for changing the position, limit, and mark of the returned
* buffer does not affect the position, limit, and mark of this buffer. Buffers returned by this
* method have independent position, limit and mark. This is an optional method, so callers
* should first check {@link #canUseByteBuffer}.
*
* @param length the maximum number of bytes to contain in returned {@link ByteBuffer}.
* @throws UnsupportedOperationException the buffer does not support this method.
*/
@Nullable
ByteBuffer getByteBuffer(int length);
ByteBuffer getByteBuffer();

/**
* Closes this buffer and releases any resources.
Expand Down
14 changes: 12 additions & 2 deletions core/src/main/java/io/grpc/internal/ReadableBuffers.java
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,16 @@ public void mark() {
public void reset() {
bytes.reset();
}

@Override
public boolean canUseByteBuffer() {
return true;
}

@Override
public ByteBuffer getByteBuffer() {
return (ByteBuffer) bytes;
}
}

/**
Expand Down Expand Up @@ -397,8 +407,8 @@ private static class ByteBufferInputStream extends BufferInputStream implements

@Nullable
@Override
public ByteBuffer getByteBuffer(int length) {
return buffer.getByteBuffer(length);
public ByteBuffer getByteBuffer() {
return buffer.getByteBuffer();
}
}

Expand Down
4 changes: 2 additions & 2 deletions netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ public boolean canUseByteBuffer() {
}

@Override
public ByteBuffer getByteBuffer(int length) {
return buffer.nioBuffers(buffer.readerIndex(), length)[0];
public ByteBuffer getByteBuffer() {
return buffer.nioBuffers()[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach could be pretty heavy on garbage, especially if underlying buffer is composite. I'd suggest at least doing buffer.nioBufferCount() == 1 ? buffer.nioBuffer() : buffer.nioBuffers()[0].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sound fair. Improved.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,11 +287,7 @@ public boolean hasNext() {
if (buffer != null) {
return true;
}
try {
buffer = ((HasByteBuffer) stream).getByteBuffer(stream.available());
} catch (IOException e) {
throw new RuntimeException(e);
}
buffer = ((HasByteBuffer) stream).getByteBuffer();
return buffer != null;
}

Expand Down