-
Notifications
You must be signed in to change notification settings - Fork 30k
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
tls: simplify write mechanism #17883
Conversation
Related failure here: https://ci.nodejs.org/job/node-test-commit-linux/15238/nodes=fedora24/tapResults/ 😢 |
@apapirovski I couldn’t reproduce locally in a couple thousand runs … I’ll wait until CI has freed up a bit, then run stress tests to verify it’s a problem with this PR (if the rest of the CI doesn’t already point that out) |
@apapirovski That would make a lot of sense! I’ll try that tomorrow or so – don’t want to rush in #17863 since it’s holiday season for a lot of people … :) Edit: |
src/tls_wrap.cc
Outdated
@@ -120,20 +117,18 @@ TLSWrap::~TLSWrap() { | |||
|
|||
|
|||
void TLSWrap::MakePending() { | |||
write_item_queue_.MoveBack(&pending_write_items_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove MoveBack() from util.h and util-inl.h now, this is the only place that uses it.
src/tls_wrap.cc
Outdated
// This can be skipped in the error case because no further writes | ||
// would succeed anyway. | ||
for (; i < buffers.size(); ++i) | ||
pending_cleartext_input_.push_back(buffers[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending_cleartext_input_.insert(pending_cleartext_input_.end(), &buffers[i], buffers.end());
?
src/tls_wrap.cc
Outdated
@@ -648,7 +640,7 @@ int TLSWrap::DoWrite(WriteWrap* w, | |||
|
|||
// No errors, queue rest | |||
for (; i < count; i++) | |||
clear_in_->Write(bufs[i].base, bufs[i].len); | |||
pending_cleartext_input_.push_back(bufs[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise.
src/tls_wrap.cc
Outdated
@@ -120,20 +117,18 @@ TLSWrap::~TLSWrap() { | |||
|
|||
|
|||
void TLSWrap::MakePending() { | |||
write_item_queue_.MoveBack(&pending_write_items_); | |||
write_callback_scheduled_ = true; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well, I wouldn't object to removing MakePending()
and inlining write_callback_scheduled_ = true
at its two or three call sites. It arguably obscures more than it abstracts.
b6b0a22
to
7d6c54e
Compare
@@ -171,11 +157,10 @@ class TLSWrap : public AsyncWrap, | |||
StreamBase* stream_; | |||
BIO* enc_in_; | |||
BIO* enc_out_; | |||
crypto::NodeBIO* clear_in_; | |||
std::vector<uv_buf_t> pending_cleartext_input_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a std::list be more appropriate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu Why? uv_buf_t
is a pretty small structure, so what I would guess is that the overhead of possibly having a small number of them being unused is more or less negligible compared to allocating a list node for every buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the variations I tried with http2, the performance difference is inconsequential. Using std::vector
here is just fine.
The TLS implementation previously had two separate queues for WriteWrap instances, one for currently active and one for finishing writes (i.e. where the encrypted output is being written to the underlying socket). However, the streams implementation in Node doesn’t allow for more than one write to be active at a time; effectively, the only possible states were that: - No write was active. - The first write queue had one item, the second one was empty. - Only the second write queue had one item, the first one was empty. To reduce overhead and implementation complexity, remove these queues, and instead store a single `WriteWrap` pointer and keep track of whether its write callback should be called on the next invocation of `InvokeQueued()` or not (which is what being in the second queue previously effected).
The TLS implementation previously kept a separate buffer for incoming pieces of data, into which buffers were copied before they were up for writing. This removes this buffer, and replaces it with a simple list of `uv_buf_t`s: - The previous implementation copied all incoming data into that buffer, both allocating new storage and wasting time with copy operations. Node’s streams/net implementation already has to make sure that the allocated memory stays fresh until the write is finished, since that is what libuv streams rely on anyway. - The fact that a separate kind of buffer, `crypto::NodeBIO` was used, was confusing: These `BIO` instances are only used to communicate with openssl’s streams system otherwise, whereas this one was purely for internal memory management. - The name `clear_in_` was not very helpful.
7d6c54e
to
4cbd571
Compare
The TLS implementation previously had two separate queues for WriteWrap instances, one for currently active and one for finishing writes (i.e. where the encrypted output is being written to the underlying socket). However, the streams implementation in Node doesn’t allow for more than one write to be active at a time; effectively, the only possible states were that: - No write was active. - The first write queue had one item, the second one was empty. - Only the second write queue had one item, the first one was empty. To reduce overhead and implementation complexity, remove these queues, and instead store a single `WriteWrap` pointer and keep track of whether its write callback should be called on the next invocation of `InvokeQueued()` or not (which is what being in the second queue previously effected). PR-URL: #17883 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
The TLS implementation previously kept a separate buffer for incoming pieces of data, into which buffers were copied before they were up for writing. This removes this buffer, and replaces it with a simple list of `uv_buf_t`s: - The previous implementation copied all incoming data into that buffer, both allocating new storage and wasting time with copy operations. Node’s streams/net implementation already has to make sure that the allocated memory stays fresh until the write is finished, since that is what libuv streams rely on anyway. - The fact that a separate kind of buffer, `crypto::NodeBIO` was used, was confusing: These `BIO` instances are only used to communicate with openssl’s streams system otherwise, whereas this one was purely for internal memory management. - The name `clear_in_` was not very helpful. PR-URL: #17883 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
This does not land cleanly on v9.x, could it be manually backported? |
This should apply cleanly after #17650 is backported |
The TLS implementation previously had two separate queues for WriteWrap instances, one for currently active and one for finishing writes (i.e. where the encrypted output is being written to the underlying socket). However, the streams implementation in Node doesn’t allow for more than one write to be active at a time; effectively, the only possible states were that: - No write was active. - The first write queue had one item, the second one was empty. - Only the second write queue had one item, the first one was empty. To reduce overhead and implementation complexity, remove these queues, and instead store a single `WriteWrap` pointer and keep track of whether its write callback should be called on the next invocation of `InvokeQueued()` or not (which is what being in the second queue previously effected). PR-URL: #17883 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
The TLS implementation previously kept a separate buffer for incoming pieces of data, into which buffers were copied before they were up for writing. This removes this buffer, and replaces it with a simple list of `uv_buf_t`s: - The previous implementation copied all incoming data into that buffer, both allocating new storage and wasting time with copy operations. Node’s streams/net implementation already has to make sure that the allocated memory stays fresh until the write is finished, since that is what libuv streams rely on anyway. - The fact that a separate kind of buffer, `crypto::NodeBIO` was used, was confusing: These `BIO` instances are only used to communicate with openssl’s streams system otherwise, whereas this one was purely for internal memory management. - The name `clear_in_` was not very helpful. PR-URL: #17883 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
tls: refactor write queues away
The TLS implementation previously had two separate queues for
WriteWrap instances, one for currently active and one for
finishing writes (i.e. where the encrypted output is being written
to the underlying socket).
However, the streams implementation in Node doesn’t allow for
more than one write to be active at a time; effectively,
the only possible states were that:
To reduce overhead and implementation complexity, remove these
queues, and instead store a single
WriteWrap
pointer andkeep track of whether its write callback should be called
on the next invocation of
InvokeQueued()
or not(which is what being in the second queue previously effected).
tls: remove cleartext input data queue
The TLS implementation previously kept a separate buffer for
incoming pieces of data, into which buffers were copied
before they were up for writing.
This removes this buffer, and replaces it with a simple list
of
uv_buf_t
s:that buffer, both allocating new storage and wasting time
with copy operations. Node’s streams/net implementation
already has to make sure that the allocated memory stays
fresh until the write is finished, since that is what
libuv streams rely on anyway.
crypto::NodeBIO
was used, was confusing: These
BIO
instances areonly used to communicate with openssl’s streams system
otherwise, whereas this one was purely for internal
memory management.
clear_in_
was not very helpful.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tls