tls: simplify write mechanism#17883
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added c++ 
MemberAuthor
addaleax commented Dec 27, 2017
Contributor
apapirovski commented Dec 27, 2017
Related failure here: https://ci.nodejs.org/job/node-test-commit-linux/15238/nodes=fedora24/tapResults/ 😢 |
MemberAuthor
addaleax commented Dec 27, 2017
@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) |
Contributor
apapirovski commented Dec 27, 2017
MemberAuthor
@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: |
bnoordhuis approved these changes Dec 27, 2017
src/tls_wrap.cc Outdated
Member
There was a problem hiding this comment.
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
Member
There was a problem hiding this comment.
pending_cleartext_input_.insert(pending_cleartext_input_.end(), &buffers[i], buffers.end());?
src/tls_wrap.cc Outdated
src/tls_wrap.cc Outdated
Member
There was a problem hiding this comment.
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.
jasnell approved these changes Dec 27, 2017
addaleaxforce-pushed the tls-simplify-writes branch from b6b0a22 to 7d6c54eCompare
tniessen approved these changes Dec 28, 2017
src/tls_wrap.h Outdated
Member
There was a problem hiding this comment.
Would a std::list be more appropriate here?
MemberAuthor
There was a problem hiding this comment.
@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?
Member
There was a problem hiding this comment.
based on the variations I tried with http2, the performance difference is inconsequential. Using std::vector here is just fine.
MemberAuthor
addaleax commented Jan 2, 2018
addaleax added author ready 
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.
addaleaxforce-pushed the tls-simplify-writes branch from 7d6c54e to 4cbd571CompareMemberAuthor
addaleax commented Jan 7, 2018
addaleax removed the author ready addaleax added a commit that referenced this pull request Jan 7, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this pull request Jan 7, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Contributor
MylesBorins commented Jan 9, 2018
This does not land cleanly on v9.x, could it be manually backported? |
MemberAuthor
addaleax commented Jan 10, 2018
This should apply cleanly after #17650 is backported |
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
WriteWrappointer 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_ts: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::NodeBIOwas used, was confusing: These
BIOinstances 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