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

[ssl] Use ring buffer in ssl transport layer #14060

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
7 participants
@euroelessar
Copy link
Contributor

euroelessar commented Jan 18, 2018

BIO_pair is implemented on top ring buffer which is more efficient compared to current BIO_mem approach.

See #14058

@thelinuxfoundation

This comment has been minimized.

Copy link

thelinuxfoundation commented Jan 18, 2018

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@euroelessar euroelessar changed the title [ssl] Reduce number of copies in ssl transport layer [ssl] Use ring buffer in ssl transport layer Jan 18, 2018

@euroelessar

This comment has been minimized.

Copy link
Contributor Author

euroelessar commented Jan 18, 2018

Performed cla signup process (hint for bot)

@euroelessar

This comment has been minimized.

Copy link
Contributor Author

euroelessar commented Jan 18, 2018

@jboeuf @justinburke can you have a look please?

@jboeuf

This comment has been minimized.

Copy link
Contributor

jboeuf commented Jan 18, 2018

I like the idea. I will let @jiangtaoli2016 do the review.

SSL_free(ssl);
if (into_ssl != nullptr) BIO_free(into_ssl);
if (from_ssl != nullptr) BIO_free(into_ssl);

This comment has been minimized.

Copy link
@SaveTheRbtz

@euroelessar euroelessar force-pushed the euroelessar:boringssl-use-bio-pair branch from df02eec to be4f7c6 Jan 19, 2018

@jiangtaoli2016
Copy link
Contributor

jiangtaoli2016 left a comment

Looks good overall. Minor comments.

@@ -96,17 +96,15 @@ struct tsi_ssl_server_handshaker_factory {
typedef struct {
tsi_handshaker base;
SSL* ssl;
BIO* into_ssl;
BIO* from_ssl;
BIO *network_io;

This comment has been minimized.

Copy link
@jiangtaoli2016

jiangtaoli2016 Jan 19, 2018

Contributor

Please use "BIO* network_io;" to be consistent with the existing coding style.

@@ -1062,8 +1061,8 @@ static tsi_result ssl_handshaker_create_frame_protector(
* cannot call anything else but destroy on the handshaker after this call. */

This comment has been minimized.

Copy link
@jiangtaoli2016

jiangtaoli2016 Jan 19, 2018

Contributor

Please also update comment that transfer ownership of ssl and network_io to frame protector.

@euroelessar euroelessar force-pushed the euroelessar:boringssl-use-bio-pair branch from be4f7c6 to 1722640 Jan 20, 2018

@euroelessar

This comment has been minimized.

Copy link
Contributor Author

euroelessar commented Jan 20, 2018

@jiangtaoli2016 thanks, I've addressed comments

@jiangtaoli2016

This comment has been minimized.

Copy link
Contributor

jiangtaoli2016 commented Jan 23, 2018

@euroelessar Thanks for revision. Looks good to me. Let's wait for test result.

@grpc-testing

This comment has been minimized.

Copy link

grpc-testing commented Jan 23, 2018

****************************************************************

libgrpc.so

     VM SIZE                                              FILE SIZE
 ++++++++++++++ GROWING                                ++++++++++++++
  +0.0%     +64 [None]                                 +3.38Ki  +0.1%
  +0.1%     +16 src/core/tsi/ssl_transport_security.cc     +16  +0.1%
      +4.5%     +14 [Unmapped]                                 +14  +4.5%
       +34%     +14 ssl_protector_destroy                      +14   +34%
       +29%      +9 ssl_handshaker_destroy                      +9   +29%

  +0.0%     +80 TOTAL                                  +3.39Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

Copy link

grpc-testing commented Jan 23, 2018

[trickle] No significant performance differences
@grpc-testing

This comment has been minimized.

Copy link

grpc-testing commented Jan 23, 2018

[microbenchmarks] No significant performance differences

@jiangtaoli2016 jiangtaoli2016 merged commit 633add8 into grpc:master Jan 23, 2018

29 checks passed

Artifact Build Linux (internal CI) Internal CI build successful
Details
Artifact Build MacOS (internal CI) Internal CI build successful
Details
Artifact Build Windows (internal CI) Internal CI build successful
Details
Asan C (internal CI) Internal CI build successful
Details
Asan C++ (internal CI) Internal CI build successful
Details
Basic Tests C Linux [dbg] (internal CI) Internal CI build successful
Details
Basic Tests C Linux [opt] (internal CI) Internal CI build successful
Details
Basic Tests C++ Linux [dbg] (internal CI) Internal CI build successful
Details
Basic Tests C++ Linux [opt] (internal CI) Internal CI build successful
Details
Basic Tests MacOS [dbg] (internal CI) Internal CI build successful
Details
Basic Tests MacOS [opt] (internal CI) Internal CI build successful
Details
Basic Tests Multi-language Linux (internal CI) Internal CI build successful
Details
Basic Tests Windows [dbg] (internal CI) Internal CI build successful
Details
Basic Tests Windows [opt] (internal CI) Internal CI build successful
Details
Bazel Build Basic (Internal CI) Internal CI build successful
Details
Distribution Tests Linux (standalone subset) Internal CI build successful
Details
Distribution Tests Windows (standalone subset) Internal CI build successful
Details
Interop Cloud-to-Cloud Tests (internal CI) Internal CI build successful
Details
Interop Cloud-to-Prod Tests (internal CI) Internal CI build successful
Details
Microbenchmarks Diff (internal CI) Internal CI build successful
Details
Msan C (internal CI) Internal CI build successful
Details
Portability Tests Linux [Build Only] (internal CI) Internal CI build successful
Details
Portability Tests Windows [Build Only] (internal CI) Internal CI build successful
Details
Sanity Checks (internal CI) Internal CI build successful
Details
Trickle Diff (internal CI) Internal CI build successful
Details
Tsan C (internal CI) Internal CI build successful
Details
Tsan C++ (internal CI) Internal CI build successful
Details
UBsan C (internal CI) Internal CI build successful
Details
cla/linuxfoundation euroelessar authorized
Details

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.