Skip to content

Commit

Permalink
tls: reduce memory copying and number of BIO buffer allocations
Browse files Browse the repository at this point in the history
Avoid copying buffers before passing to SSL_write if there
are zero length buffers involved.  Only copy the data when
the buffer has a non zero length.

Send a memory allocation hint to the crypto BIO about how much
memory will likely be needed to be allocated by the next call
to SSL_write.  This makes a single allocation rather than the BIO
allocating a buffer for each 16k TLS segment written.  This
solves a problem with large buffers written over TLS triggering
V8's GC.

PR-URL: #31499
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
rustyconover authored and codebytere committed Mar 30, 2020
1 parent 2bf9a2d commit 54395c6
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 5 deletions.
2 changes: 1 addition & 1 deletion benchmark/tls/throughput.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const common = require('../common.js');
const bench = common.createBenchmark(main, {
dur: [5],
type: ['buf', 'asc', 'utf'],
size: [2, 1024, 1024 * 1024]
size: [2, 1024, 1024 * 1024, 4 * 1024 * 1024, 16 * 1024 * 1024]
});

const fixtures = require('../../test/common/fixtures');
Expand Down
7 changes: 7 additions & 0 deletions src/node_crypto_bio.cc
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,13 @@ void NodeBIO::TryAllocateForWrite(size_t hint) {
kThroughputBufferLength;
if (len < hint)
len = hint;

// If there is a one time allocation size hint, use it.
if (allocate_hint_ > len) {
len = allocate_hint_;
allocate_hint_ = 0;
}

Buffer* next = new Buffer(env_, len);

if (w == nullptr) {
Expand Down
16 changes: 16 additions & 0 deletions src/node_crypto_bio.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,21 @@ class NodeBIO : public MemoryRetainer {
return length_;
}

// Provide a hint about the size of the next pending set of writes. TLS
// writes records of a maximum length of 16k of data plus a 5-byte header,
// a MAC (up to 20 bytes for SSLv3, TLS 1.0, TLS 1.1, and up to 32 bytes
// for TLS 1.2), and padding if a block cipher is used. If there is a
// large write this will result in potentially many buffers being
// allocated and gc'ed which can cause long pauses. By providing a
// guess about the amount of buffer space that will be needed in the
// next allocation this overhead is removed.
inline void set_allocate_tls_hint(size_t size) {
constexpr size_t kThreshold = 16 * 1024;
if (size >= kThreshold) {
allocate_hint_ = (size / kThreshold + 1) * (kThreshold + 5 + 32);
}
}

inline void set_eof_return(int num) {
eof_return_ = num;
}
Expand Down Expand Up @@ -164,6 +179,7 @@ class NodeBIO : public MemoryRetainer {
Environment* env_ = nullptr;
size_t initial_ = kInitialBufferLength;
size_t length_ = 0;
size_t allocate_hint_ = 0;
int eof_return_ = -1;
Buffer* read_head_ = nullptr;
Buffer* write_head_ = nullptr;
Expand Down
30 changes: 26 additions & 4 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ void TLSWrap::ClearIn() {
AllocatedBuffer data = std::move(pending_cleartext_input_);
crypto::MarkPopErrorOnReturn mark_pop_error_on_return;

crypto::NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(data.size());
int written = SSL_write(ssl_.get(), data.data(), data.size());
Debug(this, "Writing %zu bytes, written = %d", data.size(), written);
CHECK(written == -1 || written == static_cast<int>(data.size()));
Expand Down Expand Up @@ -699,8 +700,15 @@ int TLSWrap::DoWrite(WriteWrap* w,

size_t length = 0;
size_t i;
for (i = 0; i < count; i++)
size_t nonempty_i = 0;
size_t nonempty_count = 0;
for (i = 0; i < count; i++) {
length += bufs[i].len;
if (bufs[i].len > 0) {
nonempty_i = i;
nonempty_count += 1;
}
}

// We want to trigger a Write() on the underlying stream to drive the stream
// system, but don't want to encrypt empty buffers into a TLS frame, so see
Expand Down Expand Up @@ -744,20 +752,34 @@ int TLSWrap::DoWrite(WriteWrap* w,
crypto::MarkPopErrorOnReturn mark_pop_error_on_return;

int written = 0;
if (count != 1) {

// It is common for zero length buffers to be written,
// don't copy data if there there is one buffer with data
// and one or more zero length buffers.
// _http_outgoing.js writes a zero length buffer in
// in OutgoingMessage.prototype.end. If there was a large amount
// of data supplied to end() there is no sense allocating
// and copying it when it could just be used.

if (nonempty_count != 1) {
data = env()->AllocateManaged(length);
size_t offset = 0;
for (i = 0; i < count; i++) {
memcpy(data.data() + offset, bufs[i].base, bufs[i].len);
offset += bufs[i].len;
}

crypto::NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length);
written = SSL_write(ssl_.get(), data.data(), length);
} else {
// Only one buffer: try to write directly, only store if it fails
written = SSL_write(ssl_.get(), bufs[0].base, bufs[0].len);
uv_buf_t* buf = &bufs[nonempty_i];
crypto::NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len);
written = SSL_write(ssl_.get(), buf->base, buf->len);

if (written == -1) {
data = env()->AllocateManaged(length);
memcpy(data.data(), bufs[0].base, bufs[0].len);
memcpy(data.data(), buf->base, buf->len);
}
}

Expand Down

0 comments on commit 54395c6

Please sign in to comment.