tls: group chunks into TLS segments

[mildsunrise]

TLSWrap::DoWrite() now concatenates data chunks and makes a single call to SSL_write(). Grouping data into a single segment:

• reduces network overhead: by factors of even 2 or 3 in usages like http2 or form-data
• improves security: segment lengths can reveal lots of info, i.e. with form-data, how many fields are sent and the approximate length of every individual field and its headers
• reduces encryption overhead: a quick benchmark showed a ~30% CPU time decrease for an extreme case, see request body sent in separate SSL segment (bug at 9.6.0) #27573 (comment)

Fixes: #27573

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included ← are tests needed?
• documentation is changed or added ← does this apply?
• commit message follows commit guidelines

[mildsunrise]

test-http2-large-write-destroy is broken... I checked and there seems to be a bug in http2, I've opened #27863.

[mildsunrise]

Benchmarks seem good!

                                                             confidence improvement accuracy (*)    (**)    (***)
 tls/convertprotocols.js n=1                                                 4.43 %       ±9.84% ±13.14%  ±17.18%
 tls/convertprotocols.js n=50000                                             0.05 %       ±7.38%  ±9.83%  ±12.80%
 tls/secure-pair.js size=1024 securing='SecurePair' dur=5           ***     92.54 %       ±3.08%  ±4.13%   ±5.44%
 tls/secure-pair.js size=1024 securing='TLSSocket' dur=5            ***    105.94 %       ±3.16%  ±4.26%   ±5.64%
 tls/secure-pair.js size=1048576 securing='SecurePair' dur=5                -0.01 %       ±1.53%  ±2.04%   ±2.67%
 tls/secure-pair.js size=1048576 securing='TLSSocket' dur=5                  0.45 %       ±1.63%  ±2.17%   ±2.83%
 tls/secure-pair.js size=2 securing='SecurePair' dur=5              ***   2236.19 %      ±20.65% ±27.83%  ±36.94%
 tls/secure-pair.js size=2 securing='TLSSocket' dur=5               ***   2779.25 %      ±26.17% ±35.26%  ±46.81%
 tls/throughput.js size=1024 type='asc' dur=5                       ***     58.21 %       ±2.84%  ±3.82%   ±5.07%
 tls/throughput.js size=1024 type='buf' dur=5                       ***     64.49 %       ±3.20%  ±4.30%   ±5.70%
 tls/throughput.js size=1024 type='utf' dur=5                       ***     63.08 %       ±0.82%  ±1.10%   ±1.43%
 tls/throughput.js size=1048576 type='asc' dur=5                             0.27 %       ±0.43%  ±0.57%   ±0.75%
 tls/throughput.js size=1048576 type='buf' dur=5                      *      0.43 %       ±0.37%  ±0.50%   ±0.65%
 tls/throughput.js size=1048576 type='utf' dur=5                      *      0.37 %       ±0.30%  ±0.40%   ±0.52%
 tls/throughput.js size=2 type='asc' dur=5                          ***    446.80 %       ±4.08%  ±5.49%   ±7.27%
 tls/throughput.js size=2 type='buf' dur=5                          ***   1506.14 %       ±9.31% ±12.55%  ±16.65%
 tls/throughput.js size=2 type='utf' dur=5                          ***    467.60 %       ±3.63%  ±4.89%   ±6.46%
 tls/tls-connect.js dur=5 concurrency=1                                      0.50 %       ±4.82% ±21.82% ±188.62%
 tls/tls-connect.js dur=5 concurrency=10                             NA     -2.18 %           NA      NA       NA

Maybe too good? I'm worried I'm missing something...
Edit: They're all mostly throughput tests, so it makes sense.

[Trott]

I suppose this is blocked until the bug in http2 is fixed?

[Trott]

Benchmark results: 😮

[mildsunrise]

I suppose this is blocked until the bug in http2 is fixed?

Hmm, we can mark the test as flaky (which seems to be the case, i.e. it depends on a race condition) until the bug is fixed in http2. Or we can wait until it's fixed, and then rebase this PR to make CI green.

[Trott]

/ping @nodejs/crypto

[indutny]

LGTM, if CI is green. Great PR!

[mildsunrise]

I've done some changes (basically, add a debug message if there's no data).
Once #27891 has landed, I'll rebase and this should be ready.

[ryzokuken]

Thanks

[Trott]

(My main hesitation has been that the benchmark results seem too good to be true!)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23428/

[Trott]

parallel/test-http2-large-write-destroy seems to be timing out a lot now? Optimistically, perhaps improved performance here is exposing a race condition in the test? Pessimistically, perhaps this means there's a bug in this change?

[mildsunrise]

It's a race in the test, see the discussion at #27863.

The test (at the server) writes data and then destroys the stream, and (at the client) it just waits for close without starting data flow.

before -> stream is destroyed before written data can be sent, so the client emits close and we're done.

after -> write is done synchronously, then the stream is destroyed. client stream can't be destroyed if there's pending data to be read, so it times out

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23442/

[mildsunrise]

I've just rebased, this should now pass CI ^^

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23444/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23445/

[ryzokuken]

Landed in 28ec14f 🎉

[addaleax]

I think this has also addressed #20263 – i.e., we may need/want to backport this to v10.x, so I’m adding the label.

@jmendeth Awesome work!

[mildsunrise]

@addaleax I wasn't aware of #20263, I'll look into it. I noticed yesterday that Node 8 does some kind of buffering for TLSSocket, but not for HTTP/2 for example. Backporting to v10 would be great

[addaleax]

@jmendeth Yeah, I only made the connection after this landed – #20263 isn’t about TLSSocket, but about us switching to implementing SecurePair on top of TLSSocket (because they ultimately provide the same functionality).

And regarding the backport, there seems to be quite a large number of conflicts when cherry-picking the commit here directly, so it’s probably going to require a manual one. (There is a guide for how to do that here, if you would like to do so yourself – the tl;dr is, cherry-picking the commit, resolving conflicts and opening a PR against v10.x-staging.) We do have a general rule of a commit being released in the Current release line for 2 weeks before it gets released in LTS, so there’s no rush and it might be a good idea to wait and see if we can backport other TLS-related changes first (mostly @sam-github’s #25713 and my #26843).

[mildsunrise]

Oh, nice! I volunteer to open a backport PR of these three PRs if it's useful?

[addaleax]

@jmendeth I’ve opened #27967, but it would be awesome if you could do a backport for this one after that PR lands? :)

[mildsunrise]

Okay!