Skip to content

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

Merged
jiangtaoli2016 merged 1 commit intogrpc:masterfrom
euroelessar:boringssl-use-bio-pair
Jan 23, 2018
Merged

[ssl] Use ring buffer in ssl transport layer#14060
jiangtaoli2016 merged 1 commit intogrpc:masterfrom
euroelessar:boringssl-use-bio-pair

Conversation

@euroelessar
Copy link
Copy Markdown
Contributor

@euroelessar 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
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Contributor Author

euroelessar commented Jan 18, 2018

Performed cla signup process (hint for bot)

@euroelessar
Copy link
Copy Markdown
Contributor Author

euroelessar commented Jan 18, 2018

@jboeuf @justinburke can you have a look please?

@jboeuf jboeuf requested a review from jiangtaoli2016 January 18, 2018 22:44
@jboeuf
Copy link
Copy Markdown
Contributor

jboeuf commented Jan 18, 2018

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

SaveTheRbtz
SaveTheRbtz reviewed Jan 19, 2018
View reviewed changes
src/core/tsi/ssl_transport_security.cc Outdated
Copy link
Copy Markdown

@SaveTheRbtz SaveTheRbtz Jan 19, 2018

Choose a reason for hiding this comment

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

image

@euroelessar euroelessar force-pushed the boringssl-use-bio-pair branch from df02eec to be4f7c6 Compare January 19, 2018 21:42
jiangtaoli2016
jiangtaoli2016 reviewed Jan 19, 2018
View reviewed changes
Copy link
Copy Markdown

@jiangtaoli2016 jiangtaoli2016 left a comment

Choose a reason for hiding this comment

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

Looks good overall. Minor comments.

src/core/tsi/ssl_transport_security.cc Outdated
Copy link
Copy Markdown

@jiangtaoli2016 jiangtaoli2016 Jan 19, 2018

Choose a reason for hiding this comment

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

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

src/core/tsi/ssl_transport_security.cc Outdated
Copy link
Copy Markdown

@jiangtaoli2016 jiangtaoli2016 Jan 19, 2018

Choose a reason for hiding this comment

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

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

@euroelessar euroelessar force-pushed the boringssl-use-bio-pair branch from be4f7c6 to 1722640 Compare January 20, 2018 00:57
@euroelessar
Copy link
Copy Markdown
Contributor Author

euroelessar commented Jan 20, 2018

@jiangtaoli2016 thanks, I've addressed comments

@jiangtaoli2016
Copy link
Copy Markdown

jiangtaoli2016 commented Jan 23, 2018

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

@grpc-testing
Copy link
Copy Markdown

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
Copy link
Copy Markdown

grpc-testing commented Jan 23, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jan 23, 2018 

[microbenchmarks] No significant performance differences

@jiangtaoli2016 jiangtaoli2016 merged commit 633add8 into grpc:master Jan 23, 2018
@jiangtaoli2016 jiangtaoli2016 mentioned this pull request May 16, 2018
Closed
@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.

Reviewers

2 more reviewers

@SaveTheRbtz SaveTheRbtz SaveTheRbtz left review comments

@jiangtaoli2016 jiangtaoli2016 jiangtaoli2016 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiangtaoli2016 jiangtaoli2016

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@euroelessar @thelinuxfoundation @jboeuf @jiangtaoli2016 @grpc-testing @SaveTheRbtz @kokoro-team