perf: coalesce buffer instead of calling SSL_write() for each slice.

source/common/network/connection_impl.cc

@@ -324,9 +324,9 @@ void ConnectionImpl::write(Buffer::Instance& data, bool end_stream) {
     // TODO(mattklein123): All data currently gets moved from the source buffer to the write buffer.
     // This can lead to inefficient behavior if writing a bunch of small chunks. In this case, it
     // would likely be more efficient to copy data below a certain size. VERY IMPORTANT: If this is
-    // ever changed, read the comment in Ssl::ConnectionImpl::doWriteToSocket() VERY carefully.
-    // That code assumes that we never change existing write_buffer_ chain elements between calls
-    // to SSL_write(). That code will have to change if we ever copy here.
+    // ever changed, read the comment in SslSocket::doWrite() VERY carefully. That code assumes that
+    // we never change existing write_buffer_ chain elements between calls to SSL_write(). That code
+    // might need to change if we ever copy here.
     write_buffer_->move(data);
 
     // Activating a write event before the socket is connected has the side-effect of tricking

source/common/ssl/ssl_socket.cc

@@ -16,7 +16,6 @@ namespace Ssl {
 
 SslSocket::SslSocket(Context& ctx, InitialState state)
     : ctx_(dynamic_cast<Ssl::ContextImpl&>(ctx)), ssl_(ctx_.newSsl()) {
-  SSL_set_mode(ssl_.get(), SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
   if (state == InitialState::Client) {
     SSL_set_connect_state(ssl_.get());
   } else {
@@ -153,57 +152,48 @@ Network::IoResult SslSocket::doWrite(Buffer::Instance& write_buffer, bool end_st
     }
   }
 
-  uint64_t original_buffer_length = write_buffer.length();
+  uint64_t bytes_to_write;
+  if (bytes_to_retry_) {
+    bytes_to_write = bytes_to_retry_;
+    bytes_to_retry_ = 0;
+  } else {
+    bytes_to_write = std::min(write_buffer.length(), static_cast<uint64_t>(16384));
+  }
+
   uint64_t total_bytes_written = 0;
-  bool keep_writing = true;
-  while ((original_buffer_length != total_bytes_written) && keep_writing) {
-    // Protect against stack overflow if the buffer has a very large buffer chain.
-    // TODO(mattklein123): See the comment on getRawSlices() for why we have to also check
-    // original_buffer_length != total_bytes_written during loop iteration.
+  while (bytes_to_write) {
     // TODO(mattklein123): As it relates to our fairness efforts, we might want to limit the number
     // of iterations of this loop, either by pure iterations, bytes written, etc.
-    const uint64_t MAX_SLICES = 32;
-    Buffer::RawSlice slices[MAX_SLICES];
-    uint64_t num_slices = write_buffer.getRawSlices(slices, MAX_SLICES);
-
-    uint64_t inner_bytes_written = 0;
-    for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) {
-      // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call
-      // it again with the same parameters. Most implementations keep track of the last write size.
-      // In our case we don't need to do that because: a) SSL_write() will not write partial
-      // buffers. b) We only move() into the write buffer, which means that it's impossible for a
-      // particular chain to increase in size. So as long as we start writing where we left off we
-      // are guaranteed to call SSL_write() with the same parameters.
-      int rc = SSL_write(ssl_.get(), slices[i].mem_, slices[i].len_);
-      ENVOY_CONN_LOG(trace, "ssl write returns: {}", callbacks_->connection(), rc);
-      if (rc > 0) {
-        inner_bytes_written += rc;
-        total_bytes_written += rc;
-      } else {
-        int err = SSL_get_error(ssl_.get(), rc);
-        switch (err) {
-        case SSL_ERROR_WANT_WRITE:
-          keep_writing = false;
-          break;
-        case SSL_ERROR_WANT_READ:
-        // Renegotiation has started. We don't handle renegotiation so just fall through.
-        default:
-          drainErrorQueue();
-          return {PostIoAction::Close, total_bytes_written, false};
-        }
 
+    // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call
+    // it again with the same parameters. This is done by tracking last write size, but not write
+    // data, since linearize() will return the same undrained data anyway.
+    ASSERT(bytes_to_write <= write_buffer.length());
+    int rc = SSL_write(ssl_.get(), write_buffer.linearize(bytes_to_write), bytes_to_write);
+    ENVOY_CONN_LOG(trace, "ssl write returns: {}", callbacks_->connection(), rc);
+    if (rc > 0) {
+      ASSERT(rc == static_cast<int>(bytes_to_write));
+      total_bytes_written += rc;
+      write_buffer.drain(rc);
+      bytes_to_write = std::min(write_buffer.length(), static_cast<uint64_t>(16384));
+    } else {
+      int err = SSL_get_error(ssl_.get(), rc);
+      switch (err) {
+      case SSL_ERROR_WANT_WRITE:
+        bytes_to_retry_ = bytes_to_write;
         break;
+      case SSL_ERROR_WANT_READ:
+      // Renegotiation has started. We don't handle renegotiation so just fall through.
+      default:
+        drainErrorQueue();
+        return {PostIoAction::Close, total_bytes_written, false};
       }
-    }
 
-    // Draining must be done within the inner loop, otherwise we will keep getting the same slices
-    // at the beginning of the buffer.
-    if (inner_bytes_written > 0) {
-      write_buffer.drain(inner_bytes_written);
+      break;
     }
   }
 
-  if (total_bytes_written == original_buffer_length && end_stream) {
+  if (write_buffer.length() == 0 && end_stream) {
     shutdownSsl();
   }
 

source/common/ssl/ssl_socket.h

@@ -55,6 +55,7 @@ class SslSocket : public Network::TransportSocket,
   bssl::UniquePtr<SSL> ssl_;
   bool handshake_complete_{};
   bool shutdown_sent_{};
+  uint64_t bytes_to_retry_{};
   mutable std::string cached_sha_256_peer_certificate_digest_;
   mutable std::string cached_url_encoded_pem_encoded_peer_certificate_;
 };