Skip to content
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

Update grpc 1.46.5 #593

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Update grpc 1.46.5 #593

merged 10 commits into from
Oct 27, 2022

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Oct 24, 2022

1.46.x is the last patch that support C++-11. So update to this version first.

1.46.x has removed epollex engine, which causes regression. And 1.46.x also introduce transparent retry, which can hurt client streaming throughput. The benchmark is run with transparent retry disabled.

Case Diff (epollex) Diff (epoll1)
rust_generic_async_streaming_ping_pong -31.44% 0.06%
rust_protobuf_async_streaming_ping_pong -32.17% 0.85%
rust_protobuf_async_unary_ping_pong -27.87% -1.00%
rust_protobuf_sync_to_async_unary_ping_pong -24.59% -1.49%
rust_protobuf_async_unary_qps_unconstrained 1.91% -3.09%
rust_protobuf_async_streaming_qps_unconstrained 4.45% -1.43%
rust_protobuf_async_streaming_from_client_unconstrained -7.01% -6.04%
rust_protobuf_async_unary_ping_pong_1MB -2.40% 0.67%

@BusyJay BusyJay marked this pull request as ready for review October 27, 2022 09:21
@tabokie
Copy link
Member

tabokie commented Oct 27, 2022

The benchmark results are a little confusing. I assume "Diff (engine)" is the latency comparison between two versions with the specific engine type. Then why can we still test epollex when it is removed from 1.46.5?

@BusyJay
Copy link
Member Author

BusyJay commented Oct 27, 2022

The diff is throughput difference. Diff (epollex) compares 1.46.5(epoll1) with 1.44.0(epollex), diff (epoll1) compares 1.46.5(epoll1) with 1.44.0(epoll1). Testing both two results to show that major regression is from engine change.

Giving that there is a high risk of regression, but my benchmark of current TiKV using 1.44.0 and epoll1 has no obvious regression, and even some improvement under certain benchmark type(like bank). That is also part of reason why I want to upgrade grpc and check for more serious benchmark types.

let mut creds = ServerCredentials::from_raw(
grpcio_sys::grpc_ssl_server_credentials_create_with_options(opt),
);
creds._fetcher = Some(Box::from_raw(fetcher_wrap_ptr));
Copy link
Member

Choose a reason for hiding this comment

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

_fetcher isn't fully gated with _secure?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, grpc c core will not delete it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, you mean whether it can be accessed in "credentials.rs". Yes, it can. The file is also feature gated by "_secure".

tabokie
tabokie previously approved these changes Oct 27, 2022
Signed-off-by: Jay Lee <[email protected]>
@BusyJay BusyJay merged commit 4d9c61a into tikv:master Oct 27, 2022
@BusyJay BusyJay deleted the update-grpc-1.46 branch October 27, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants