Use CodedInputStream.newInstance(ByteBuffer) for non-segmented messages

[ejona86]

This is to improve the decode performance of small messages (<16 KB) to avoid a message-sized allocation and copy.

Protobuf already has an optimized code path for decoding a direct ByteBuffer. We currently copy to a byte[], but that could be avoided because small messages will commonly be in a single ByteBuffer.

This will require adding a new interface for retrieving the ByteBuffer from the InputStream. I'm uncertain whether the interface should support returning multiple ByteBuffers.

[amorken]

Hi!

For my own entertainment's sake I've hacked up a patch for this, and to my amazement, the tests seem to pass and the qps benchmark runs fine. It needs a little polish before I can push it to GitHub without shaming myself, though, so I'm just wondering if there's interest in an outside contribution here before I do the cleanup and write javadoc and stuff?

While I'm a happy user (as an ex-googler I'm glad to see Son of Stubby on GitHub! =), I haven't contributed back yet, so I am probably not going to get everything right the first time around, but if you have the patience and inclination to point me in the right direction me I'll be happy to try...

Thanks,
-AndersM

[ejona86]

@amorken, yeah! We welcome contributions and this is something that someone else can pick up relatively easily. The process may be slowed down by the API addition to get ByteBuffer from InputStream, but you should be able to mark it @ExperimentalApi which also a bit less API review.

[ejona86]

@amorken, if you want, you can send a PR before it is fully polished, just noting what still needs to be done. Then we can make sure there is some level of agreement on the overall approach.

[carl-mastrangelo]

FYI @amorken I was going to trash the ProtoLiteUtils decoder and just use plain old CIS.newInstance once proto 3.4 is released. It made some changes that largely make the custom decoding logic in there useless. If you send out a PR, be mindful that the logic will be changing there within the next month or so.

[amorken]

OK, thanks, @ejona86. =)

I didn't find time to touch the code yesterday, but I figured I'd test it a bit more before proceeding. I'm not exactly sure which, if any, of the benchmarks, are relevant to this, but the TransportTest looked somewhat applicable. I realize that the benchmarks intentionally try to avoid benchmarking protobuf, though. =)

I dumped the summaries in https://gist.github.com/amorken/533908010f20a40487456af0218a9568 - I ran a couple of tests with and without using CodedInputStream.newInstance(ByteBuffer) - baseline.txt is a recent master checkout, and nio-buffers.txt is the same with the patch. Both runs were on an otherwise mostly quiet physical machine.

There does seem to be a modest improvement, so I'll push a change and send a PR. At least it doesn't seem to regress.

Thanks for the headsup @carl-mastrangelo - the change as a whole, including the change to ProtoLiteUtils, is modest, so I don't think I'm wasting that much effort here.

[ejona86]

Yeah, those benchmarks do show a pretty consistent 300-400µs improvement. I don't know what's "wrong" with direct=false, transport=NETTY but I'm not too worried. If you increase the message size to be larger, you may see bigger benefits.

In an ideal world, a microbenchmark would be nice, but I think that would require using some internal/Netty internals and would probably bit-rot or stop reflecting reality pretty easily.

[anuraaga]

While randomly scanning through issues here, happened to find this one related to an active topic of mine :)

FWIW, I've been using UnsafeDirectNioDecoder in similar code (https://github.com/line/armeria/blob/master/grpc/src/main/java/com/linecorp/armeria/internal/grpc/GrpcMessageMarshaller.java#L191) and found it to be a significant performance regression when the proto includes a significant number of strings (not uncommon for many APIs, e.g. a search API); for these, copying to a byte[] first is always faster.

I have the fix within protobuf here, recommend waiting for it to land before looking into decoding ByteBuffer directly - protocolbuffers/protobuf#3824

[ejona86]

@anuraaga, thanks for the notification. I hadn't seen that issue.