Skip to content

Enhance sinks to make them batchable #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Aug 3, 2020

Conversation

hunterlxt
Copy link
Member

@hunterlxt hunterlxt commented Jun 17, 2020

close #415

Optimize send_all using sink in grpc-rs. By buffering the message to be sent to determine whether there is still message in the stream, and add the buffer_hint flag to indicate follow-up messages.

After a simple test, this does improve the QPS by 50% when you use send_all method.

Before
======== test_client_stream_send_split
QPS: 13626.411713542033
======== test_client_stream_send_all
QPS: 13458.8795676736
======== test_server_stream_send_all
QPS: 21513.004479575207

After
======== test_client_stream_send_split
QPS: 13632.78241828707
======== test_client_stream_send_all
QPS: 20634.743684214573
======== test_server_stream_send_all
QPS: 21282.527286920154

Tcpdump capture file size (only send_all test)
Before:22M
After:2.6M

Related test code: https://github.com/hunterlxt/grpc-rs/blob/xt/bench-sink-over-master/tests-and-examples/examples/hello_world/client.rs

Additional benchmark with standard grpc driver (before vs after)

rust_protobuf_async_streaming_from_client_100msg_unconstrained
QPS: 12058.0
QPS: 41914.1
rust_protobuf_async_streaming_ping_pong
QPS: 18315.0
QPS: 20409.9
rust_protobuf_async_streaming_qps_unconstrained
QPS: 589875.8
QPS: 584359.6

Signed-off-by: Xintao <[email protected]>
@hunterlxt hunterlxt requested review from BusyJay and hicqu June 17, 2020 09:50
@hunterlxt hunterlxt self-assigned this Jun 17, 2020
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from aed81e8 to c6d8441 Compare June 17, 2020 09:51
@BusyJay
Copy link
Member

BusyJay commented Jun 17, 2020

Cool, how does grpcio benchmark say?

Signed-off-by: Xintao <[email protected]>
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from 8ee0b20 to 182dff0 Compare June 17, 2020 12:01
@hunterlxt
Copy link
Member Author

Cool, how does grpcio benchmark say?

The updated results are in the description. BTW, I read the benchmark code, the client does not use the send_all method.

@BusyJay
Copy link
Member

BusyJay commented Jun 18, 2020

If adapted to send_all, how does it performs?

Since now std::future is used, async/await is the preferred style. It will be less useful if the optimization can't work with that.

@hunterlxt
Copy link
Member Author

If adapted to send_all, how does it performs?

Since now std::future is used, async/await is the preferred style. It will be less useful if the optimization can't work with that.

send_all is a method of SinkExt. futures::sink and std::future do not conflict with each other. For users, the method of std::future is not directly used, but the method in SinkExt of futures::sink.

Above is the background. The semantics of send() is to send and ensure that this message is indeed successful. But if the user really wants to send a stream, he should choose the send_all method, and our sink has not been optimized for this.

@BusyJay
Copy link
Member

BusyJay commented Jun 18, 2020

Can you give an example about how to utilize the optimization with async/await?

@hunterlxt
Copy link
Member Author

The previous benchmark in grpc-rs did not implement the stream_from_client bench. I added it and performed the bench. The result has been updated. cc @BusyJay

Signed-off-by: Xintao <[email protected]>
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from f847062 to b90daf3 Compare June 19, 2020 09:29
@hicqu
Copy link
Contributor

hicqu commented Jun 19, 2020

rest LGTM.

Signed-off-by: Xintao <[email protected]>
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from 6f2ddbf to 8926509 Compare June 30, 2020 11:12
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from 630e3d6 to c133a57 Compare July 13, 2020 04:05
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from 80f6eda to 697da61 Compare July 16, 2020 07:01
Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from d4fcc2b to aa12149 Compare July 21, 2020 03:53
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from ac274f8 to 94a5552 Compare July 22, 2020 02:50
@hunterlxt hunterlxt force-pushed the XT/sink-batchable branch from 94a5552 to a60ad06 Compare July 22, 2020 02:51
@hunterlxt
Copy link
Member Author

@BusyJay @hicqu PTAL

Signed-off-by: Xintao <[email protected]>
@hunterlxt hunterlxt merged commit 716f5e8 into tikv:master Aug 3, 2020
@hunterlxt hunterlxt deleted the XT/sink-batchable branch August 3, 2020 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance sinks to make them batchable
3 participants