Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795573002:
Implement zero-copy SSL buffers. (Closed)
DescriptionImplement zero-copy SSL buffers.
Use the new BoringSSL zero copy interface to avoid allocating new
buffers for each read and write operation and save buffer memory.
BUG=169885
Committed: https://crrev.com/2d92e72b7dbde513405cc79dd648360daec8c2c1
Cr-Commit-Position: refs/heads/master@{#309191}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebase #Patch Set 3 : Address comments #Patch Set 4 : Fixed CHECK failure caused by removing setting offset to 0 #
Messages
Total messages: 21 (5 generated)
haavardm@opera.com changed reviewers: + davidben@chromium.org, rsleevi@chromium.org
A new go for implementing zero-copy buffers using a new BoringSSL api. Ryan: Please review David: FYI
On 2014/12/10 11:18:24, haavardm wrote: > A new go for implementing zero-copy buffers using a new BoringSSL api. > > Ryan: Please review > David: FYI David should take primary here, I'll be FYI. :)
On 2014/12/11 00:48:22, Ryan Sleevi wrote: > On 2014/12/10 11:18:24, haavardm wrote: > > A new go for implementing zero-copy buffers using a new BoringSSL api. > > > > Ryan: Please review > > David: FYI > > David should take primary here, I'll be FYI. :) David: Friendly ping. Can you take over this review?
rsleevi@chromium.org changed reviewers: + agl@chromium.org - davidben@chromium.org
David's out of office (per the updated name) for the holidays, redirecting to Adam. Who may or may not be out of the office as well :)
https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:71: // Default sizes of the internal BoringSSL buffers. // Default size of the internal BoringSSL buffers. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:741: // Let |SSLClientSocketOpenSSL| have ownership over the bio buffers to keep // SSLClientSocketOpenSSL retains ownership of the BIO buffers. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:191: // GrowableIOBuffer is used to keep ownership and setting offset. // GrowableIOBuffer is used to keep ownership and for setting offsets. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:192: scoped_refptr<GrowableIOBuffer> send_buffer_; I'm not sure that a growable IOBuffer is needed here because it never grows by the looks of things. Can a simple IOBuffer be used? Setting the offset is only done just before use, so a simple addition of the offset could replace that.
https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:192: scoped_refptr<GrowableIOBuffer> send_buffer_; On 2014/12/17 19:12:22, agl wrote: > I'm not sure that a growable IOBuffer is needed here because it never grows by > the looks of things. Can a simple IOBuffer be used? Setting the offset is only > done just before use, so a simple addition of the offset could replace that. I used GrowableIOBuffer just for the set offset function (hence the comment). An alternative is DrainableIOBuffer. Might that one is better suited for this (I would only use the offset anyway, but the naming might be better) I'm not sure I see how I can change the offset using the simple IOBuffer. I can't create a new IOBuffer for each send (that would allocate memory) and socket->Read needs an IOBuffer that returns the offset buffer when Data() is called. Also I cannot use the wrapped buffer, as that leaves the socket without a reference/ownership to the data.
LGTM with nits. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1531: DCHECK(status == 1); // Should never fail. DCHECK_EQ https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1534: CHECK(read_buf == reinterpret_cast<uint8_t*>(send_buffer_->data())); CHECK_EQ https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1535: CHECK(buffer_read_offset < static_cast<size_t>(send_buffer_->capacity())); CHECK_LT https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1578: DCHECK(status == 1); // Should never fail. DCHECK_EQ https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1582: CHECK(write_buf == reinterpret_cast<uint8_t*>(recv_buffer_->data())); CHECK_EQ https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1583: CHECK(buffer_write_offset < static_cast<size_t>(recv_buffer_->capacity())); CHECK_LT https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1622: send_buffer_->set_offset(0); Is this set_offset needed? It seems that it would be better done closer to the Read and Write calls, even if that meant that it was duplicated. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1644: recv_buffer_->set_offset(0); Ditto. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:192: scoped_refptr<GrowableIOBuffer> send_buffer_; On 2014/12/17 22:04:53, haavardm wrote: > On 2014/12/17 19:12:22, agl wrote: > > I'm not sure that a growable IOBuffer is needed here because it never grows by > > the looks of things. Can a simple IOBuffer be used? Setting the offset is only > > done just before use, so a simple addition of the offset could replace that. > > I used GrowableIOBuffer just for the set offset function (hence the comment). An > alternative is DrainableIOBuffer. Might that one is better suited for this (I > would only use the offset anyway, but the naming might be better) > > I'm not sure I see how I can change the offset using the simple IOBuffer. I > can't create a new IOBuffer for each send (that would allocate memory) and > socket->Read needs an IOBuffer that returns the offset buffer when Data() is > called. Also I cannot use the wrapped buffer, as that leaves the socket without > a reference/ownership to the data. > Thanks. It was the socket->Read requirement that I missed.
lgtm https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1571: // traffic, this over-subscribed Read()ing will not cause issues. Is this comment still applicable/accurate? Does this still fill |transport_bio_|, or does max_write indicate how much they requested from the BIO? https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1620: int ret = BIO_zero_copy_get_read_buf_done(transport_bio_, bytes_written); What's the contract from BoringSSL if the zero-copy buffers if bytes_written was < max_read from line 1530? Will it handle this appropriately? I would assume so, but wanting to confirm.
PTAL Note that I have not adjusted the default buffer size, as I think that should be done in a separate patch. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:71: // Default sizes of the internal BoringSSL buffers. On 2014/12/17 19:12:22, agl wrote: > // Default size of the internal BoringSSL buffers. Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:741: // Let |SSLClientSocketOpenSSL| have ownership over the bio buffers to keep On 2014/12/17 19:12:22, agl wrote: > // SSLClientSocketOpenSSL retains ownership of the BIO buffers. Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1531: DCHECK(status == 1); // Should never fail. On 2014/12/17 22:29:56, agl wrote: > DCHECK_EQ Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1534: CHECK(read_buf == reinterpret_cast<uint8_t*>(send_buffer_->data())); On 2014/12/17 22:29:56, agl wrote: > CHECK_EQ Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1535: CHECK(buffer_read_offset < static_cast<size_t>(send_buffer_->capacity())); On 2014/12/17 22:29:56, agl wrote: > CHECK_LT Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1571: // traffic, this over-subscribed Read()ing will not cause issues. On 2014/12/17 22:57:10, Ryan Sleevi wrote: > Is this comment still applicable/accurate? Does this still fill > |transport_bio_|, or does max_write indicate how much they requested from the > BIO? I've done no changes to that logic, so max_write is still maximum number of bytes available for write in |transport_bio|. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1620: int ret = BIO_zero_copy_get_read_buf_done(transport_bio_, bytes_written); On 2014/12/17 22:57:10, Ryan Sleevi wrote: > What's the contract from BoringSSL if the zero-copy buffers if bytes_written was > < max_read from line 1530? > > Will it handle this appropriately? I would assume so, but wanting to confirm. Yes, BoringSSL will handle this case. BIO_zero_copy_get_read_buf_done will adjust the internal offset according to bytes_written and transport_bio is unlocked so that BIO_zero_copy_get_read_buf can be called again. To be completely sure I added a few lines to src/third_party/boringssl/src/crypto/bio/bio_test.c here locally (I had only tested partially write using this API). I might upstream. Note that every call to BIO_zero_copy_get_read_buf_done has to be preceded by a call to BIO_zero_copy_get_read_buf. Thus the assumption here is that TransportWriteComplete is never called more than once after a single to socket->Write. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1622: send_buffer_->set_offset(0); On 2014/12/17 22:29:56, agl wrote: > Is this set_offset needed? It seems that it would be better done closer to the > Read and Write calls, even if that meant that it was duplicated. It is not really needed since the offset is always set before the buffer is used. I just found it clean to reset the offset when the buffer goes idle. I think I'll rather remove completely instead as it builds up a expectation that is has to be reset to 0. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1622: send_buffer_->set_offset(0); On 2014/12/17 22:29:56, agl wrote: > Is this set_offset needed? It seems that it would be better done closer to the > Read and Write calls, even if that meant that it was duplicated. Done. https://codereview.chromium.org/795573002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:1644: recv_buffer_->set_offset(0); On 2014/12/17 22:29:56, agl wrote: > Ditto. Done.
The CQ bit was checked by agl@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795573002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
On 2014/12/18 22:05:05, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) I was a bit quick when removing resetting of the offsets. Code wise it was OK, but the checking that the correct buffer has been retrieved from BoringSSL needed some adjustment. PTAL
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795573002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2d92e72b7dbde513405cc79dd648360daec8c2c1 Cr-Commit-Position: refs/heads/master@{#309191} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
