Allow the message frame to create chunks as large as transport will allow by louiscryan · Pull Request #313 · grpc/grpc-java

louiscryan · 2015-04-17T23:59:39Z

louiscryan · 2015-04-18T00:00:26Z

FYI - This PR is work in progress, initially just demonstrates idea.

nmittler · 2015-04-18T14:30:27Z

@louiscryan One potential issue I see with this is that OutputStreamAdapter which calls writeRaw for every call to write(byte[],int, int), which could lead to a lot of buffer allocations. I guess this won't be a problem in most cases since we'll be using one of the Deferred input streams, which I think would make a single call to write.

It would be interesting to see if the number of allocations increases significantly with this change.

ejona86 · 2015-04-19T04:04:04Z

Note that right now this guarantees two buffer allocations per message: 1 for the header and 1 for the body. That isn't all that hard to fix though, so it can still be useful in seeing how things behave for really large messages.

louiscryan · 2015-04-20T16:27:12Z

@nmittler The buffer allocator can and probably should choose to allocate a buffer with a minimum size in the case where the messages are small to address this issue when we fallback to ByteStreams.copy for the degenerate case.

Similarly the allocator can place an upper bound on the size of the buffer allocated so we don't fragment buffer pools or thrash the heap for very large messages.

The other optimization for more sophisticated bindings is to allow the frame to receive messages that are already in the right buffer format for the transport layer. Right now we only admit byte[] in flushTo but we could let a deferred stream act as a factory for transport buffers

louiscryan · 2015-04-20T17:24:25Z

@ejona86 yes, using one alloc for the header and the message instead of two does make a difference too. For context some numbers from running QpsClient with 16k payloads, 10s warmup, 20s run

baseline --> 8574 qps
one alloc for header & body --> 9082 qps
one alloc for header & body & alloc message len --> 10429 qps

which is a pretty decent improvement

louiscryan · 2015-04-20T21:01:44Z

@ejona86 Ready for review

ejona86 · 2015-04-21T01:06:52Z

integration-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java

Revert the changes to this file?

Unused import is the only change to this file.

ejona86 · 2015-04-21T01:25:04Z

@louiscryan, LGTM, after reverting changes to Http2NettyTest. After reverting that Travis should turn green.

nmittler · 2015-04-21T16:41:39Z

core/src/main/java/io/grpc/transport/MessageFramer.java

Is this comment correct? Shouldn't it be something like "Allocate a buffer that is at least big enough to for this write"?

Not exactly no, see discussion below. Renamed param in interface to 'capacityHint'

louiscryan · 2015-04-21T19:13:27Z

PTAL

ejona86 · 2015-04-21T21:07:07Z

netty/src/main/java/io/grpc/transport/netty/NettyWritableBufferAllocator.java

This should be on the same line as note:

Note:

louiscryan · 2015-04-23T17:20:22Z

PTAL

ejona86 · 2015-04-23T21:57:13Z

@louiscryan, LGTM, after fixing Travis failure (checkstyle failure).

Set upper and lower bounds for Netty & OkHttp allocators based on transport limitations and benchmark results. Fix OkHttp OutboundFlowController to chunk payloads larger than frameWriter.maxDataLength Allow OkHttp to allocate buffers to FrameWriter larger than max DATA length

louiscryan · 2015-04-24T00:37:03Z

Merged. Note that the performance numbers shown above depend on the ability to send larger messages in a single DATA frame and thus need larger flow-control windows. The ability to configure that is coming in a separate PR from @buchgr

louiscryan force-pushed the ThroughputOpt branch from 198d338 to 9bf2d84 Compare April 20, 2015 21:00

ejona86 mentioned this pull request Apr 20, 2015

Propagate explicit flushes through MessageFramer #323

Merged

ejona86 reviewed Apr 21, 2015
View reviewed changes

nmittler reviewed Apr 21, 2015
View reviewed changes

louiscryan force-pushed the ThroughputOpt branch from 9bf2d84 to 2acace3 Compare April 21, 2015 19:12

ejona86 reviewed Apr 21, 2015
View reviewed changes

louiscryan force-pushed the ThroughputOpt branch from ae143fd to 6c97829 Compare April 23, 2015 17:59

louiscryan force-pushed the ThroughputOpt branch from 0a6dd52 to e59930a Compare April 23, 2015 23:44

Louis Ryan added 2 commits April 23, 2015 16:45

Fix missing revert

8eb91d7

louiscryan force-pushed the ThroughputOpt branch from e59930a to 8eb91d7 Compare April 23, 2015 23:52

Fix checkstyle nit

508ae89

louiscryan closed this Apr 24, 2015

This was referenced Apr 28, 2015

MessageFramer produces too small message chunks #312

Closed

Determine upper-bound for Netty buffer sizes used in frame #325

Closed

louiscryan deleted the ThroughputOpt branch April 30, 2015 00:36

ejona86 mentioned this pull request Jul 29, 2016

core,netty: retain buffer in MessageFramer to reuse for subsequent messages #2092

Closed

Conversation

louiscryan commented Apr 17, 2015

Uh oh!

louiscryan commented Apr 18, 2015

Uh oh!

nmittler commented Apr 18, 2015

Uh oh!

ejona86 commented Apr 19, 2015

Uh oh!

louiscryan commented Apr 20, 2015

Uh oh!

louiscryan commented Apr 20, 2015

Uh oh!

louiscryan commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Apr 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louiscryan commented Apr 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louiscryan commented Apr 23, 2015

Uh oh!

ejona86 commented Apr 23, 2015

Uh oh!

louiscryan commented Apr 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants