tls: simplify write mechanism by addaleax · Pull Request #17883 · nodejs/node

addaleax · 2017-12-27T19:10:44Z

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:
- 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).
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:
- 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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tls

addaleax · 2017-12-27T19:26:43Z

CI: https://ci.nodejs.org/job/node-test-commit/15099/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1178/

apapirovski · 2017-12-27T19:48:00Z

Related failure here: https://ci.nodejs.org/job/node-test-commit-linux/15238/nodes=fedora24/tapResults/ 😢

addaleax · 2017-12-27T19:55:44Z

@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 · 2017-12-27T20:03:29Z

@addaleax It's also very possible this is related to #17863 — maybe you could just land that, rebase and then run the stress test?

addaleax · 2017-12-27T20:07:36Z

@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:
Stress test this PR: https://ci.nodejs.org/job/node-stress-single-test/1589/
Stress test master: https://ci.nodejs.org/job/node-stress-single-test/1590/
Stress test #17863: https://ci.nodejs.org/job/node-stress-single-test/1591/

bnoordhuis · 2017-12-27T23:26:45Z

src/tls_wrap.cc

You can remove MoveBack() from util.h and util-inl.h now, this is the only place that uses it.

bnoordhuis · 2017-12-27T23:32:13Z

src/tls_wrap.cc

pending_cleartext_input_.insert(pending_cleartext_input_.end(), &buffers[i], buffers.end());?

bnoordhuis · 2017-12-27T23:32:34Z

src/tls_wrap.cc

bnoordhuis · 2017-12-27T23:35:33Z

src/tls_wrap.cc

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.

TimothyGu · 2017-12-29T00:22:03Z

src/tls_wrap.h

Would a std::list be more appropriate here?

@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?

based on the variations I tried with http2, the performance difference is inconsequential. Using std::vector here is just fine.

addaleax · 2018-01-02T15:10:20Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/15177/

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.

addaleax · 2018-01-07T21:36:34Z

Landed in d5fa4de, 46f783d

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>

MylesBorins · 2018-01-09T23:49:27Z

This does not land cleanly on v9.x, could it be manually backported?

addaleax · 2018-01-10T15:45:57Z

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>

addaleax added the tls Issues and PRs related to the tls subsystem. label Dec 27, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Dec 27, 2017

bnoordhuis approved these changes Dec 27, 2017

View reviewed changes

jasnell approved these changes Dec 27, 2017

View reviewed changes

addaleax force-pushed the tls-simplify-writes branch from b6b0a22 to 7d6c54e Compare December 28, 2017 13:59

tniessen approved these changes Dec 28, 2017

View reviewed changes

TimothyGu reviewed Dec 29, 2017

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v4.x labels Jan 2, 2018

addaleax added 2 commits January 7, 2018 22:28

addaleax force-pushed the tls-simplify-writes branch from 7d6c54e to 4cbd571 Compare January 7, 2018 21:29

addaleax closed this Jan 7, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2018

addaleax deleted the tls-simplify-writes branch January 7, 2018 21:36

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

addaleax removed the backport-requested-v9.x label Jan 10, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

Uh oh!

Conversation

addaleax commented Dec 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Dec 27, 2017

Uh oh!

apapirovski commented Dec 27, 2017

Uh oh!

addaleax commented Dec 27, 2017

Uh oh!

apapirovski commented Dec 27, 2017

Uh oh!

addaleax commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Dec 29, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 29, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Dec 29, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 2, 2018

Uh oh!

addaleax commented Jan 7, 2018

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

addaleax commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Dec 27, 2017 •

edited

Loading