benchmark: add secure-pair benchmark #20344

alexfernandez · 2018-04-26T22:53:26Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Adds a benchmark to compare tls.createSecurePair() with new tls.TLSSocket(). The first option is deprecated, while the second is the recommended way forward for securing existing server sockets. A proxy server is placed between the client and the server to evidence any performance issues with socket events.

Results v8.11.1

tls.createSecurePair() with Node.js v8.11.1 is quite faster than new tls.TLSSocket():

 $ node benchmark/tls/secure-pair.js 
tls/secure-pair.js size=2 securing="SecurePair" dur=5: 15.94602188948082
tls/secure-pair.js size=1024 securing="SecurePair" dur=5: 2,060.951341694148
tls/secure-pair.js size=1048576 securing="SecurePair" dur=5: 3,234.913943348519
tls/secure-pair.js size=2 securing="TLSSocket" dur=5: 3.1348338992549807
tls/secure-pair.js size=1024 securing="TLSSocket" dur=5: 1,020.9719597316911
tls/secure-pair.js size=1048576 securing="TLSSocket" dur=5: 3,704.526017262844

Deprecation warnings have been removed from the output. Test machine is a dual-core Lenovo X1 Carbon with Ubuntu 17.10.

There is a performance regression using new tls.TLSSocket() for small packet sizes:

For 2 bytes throughput goes from 16 bytes per iteration to around 3.
For 1 KB throughput goes from 2 KB per iteration to 1 KB.
For 1 MB throughput actually increases, which is to be expected since there are less streams to pipe there.

The results point to an inefficiency in small packets, and experimentally it appears that while SecurePair buffers data, TLSSocket doesn't: the number of bytes per iteration is quite similar to the number of bytes per packet sent.

Results Using `master`

Also, a performance regression has been reported in #20263 for v10.0.0, in which removes the old legacy code for tls.createSecurePair() using instead new tls.TLSSocket() underneath. So using the latest in master (node v11.0.0-pre):

 $ ./node benchmark/tls/secure-pair.js 
tls/secure-pair.js size=2 securing="SecurePair" dur=5: 3.612850011228876
tls/secure-pair.js size=1024 securing="SecurePair" dur=5: 1,073.6013989026228
tls/secure-pair.js size=1048576 securing="SecurePair" dur=5: 3,801.1694627588176
tls/secure-pair.js size=2 securing="TLSSocket" dur=5: 3.1985467121541213
tls/secure-pair.js size=1024 securing="TLSSocket" dur=5: 1,023.6285765524207
tls/secure-pair.js size=1048576 securing="TLSSocket" dur=5: 4,079.423755357186

In master there is no performance gap between SecurePair and TLSSocket; instead they are both in the low end of the range seen with v8.11.1.

As requested by @addaleax, this benchmark should work reliably on all systems since it does not vary with the number of cores or indeed with CPU load (within reason).

Results With #20287

As requested by @benjamingr, applying #20287 by @apapirovski gives very similar numbers as without it. I don't have an explanation as to why it does not improve as expected.

 $ ./node benchmark/tls/secure-pair.js 
tls/secure-pair.js size=2 securing="SecurePair" dur=5: 3.4185875822957215
tls/secure-pair.js size=1024 securing="SecurePair" dur=5: 1,144.471090251163
tls/secure-pair.js size=1048576 securing="SecurePair" dur=5: 3,647.8361764068845
tls/secure-pair.js size=2 securing="TLSSocket" dur=5: 2.833589476787596
tls/secure-pair.js size=1024 securing="TLSSocket" dur=5: 1,026.9533974461256
tls/secure-pair.js size=1048576 securing="TLSSocket" dur=5: 3,879.9676141790346

Deployment Notes

PR should be safe to merge since it only adds a benchmark.

Trott · 2018-04-26T22:55:22Z

@nodejs/benchmarking @nodejs/crypto

apapirovski · 2018-04-26T22:56:30Z

As requested by @benjamingr, applying #20287 by @apapirovski gives very similar numbers as without it. I don't have an explanation as to why it does not improve as expected.

I don't think it should've improved. I'm not surprised at all that it's unchanged... (I tried to make a comment stating as much in the thread.)

alexfernandez · 2018-04-26T23:13:18Z

@apapirovski You are right, but I thought that it might at least affect the results for TLSSocket? Sorry for any mistakes, it is hard for me to follow the C++ code (and the whole process is a bit overwhelming for a newbie).

apapirovski · 2018-04-26T23:21:29Z

@alexfernandez It's hard to know from the runs above. These benchmarks can vary 20% between a run so one needs to run them a good number of times to get any significant data. But either way, if this benchmark uses drain (I haven't had a chance to look at the code) then it won't really be affected by the other PR.

I'm working on some other related stuff that should be more reliable in terms of getting a performance gain but it'll take a while to get it finished.

jasnell · 2018-04-27T14:20:33Z

benchmark/tls/secure-pair.js

+  server = net.createServer(onRedirectConnection);
+  server.listen(REDIRECT_PORT, () => {
+    proxy = net.createServer(onProxyConnection);
+    proxy.listen(common.PORT, function() {


minor nit... would be good to be consistent with using either () => {} or function() {} rather than mixing.

Changed in commit 6ba84cf. As a bonus, removed all var inherited from benchmark/tls/throughput.js.

Refs: #20263

benjamingr

Thanks for doing this! LGTM

tniessen · 2018-04-27T15:34:34Z

@alexfernandez The commit is not associated with your GitHub account, you might want to fix this before this gets merged.

alexfernandez · 2018-04-27T15:41:38Z

@tniessen That should be fixed now?

BridgeAR · 2018-04-29T14:07:27Z

benchmark/tls/secure-pair.js

+const common = require('../common.js');
+const bench = common.createBenchmark(main, {
+  dur: [5],
+  securing: ['SecurePair', 'TLSSocket'],


Please add 'securing=SecurePair' to ./test/sequential/test-benchmark-tls.js in the array with the other options there. Otherwise the test will run twice the duration it has to.

@BridgeAR Done in commit 487a327.

alexfernandez · 2018-05-03T22:03:39Z

Is there anything I can do to move this PR along? Thanks!

jasnell · 2018-05-03T22:32:34Z

@alexfernandez ... not really, we need to do a CI run and get it landed. Let's ask @BridgeAR to give one final sign off for his request. It should get landed in the few days.

tniessen · 2018-05-04T14:11:04Z

benchmark/tls/secure-pair.js

+    conn.pipe(serverPair.encrypted);
+    serverPair.encrypted.pipe(conn);
+    serverPair.on('error', (error) => {
+      throw new Error('Pair error: ' + error);


This could use a template literal instead.

Done in commit c17c783.

tniessen · 2018-05-04T14:11:13Z

benchmark/tls/secure-pair.js

+  function secureTLSSocket(conn, client) {
+    const serverSocket = new tls.TLSSocket(conn, options);
+    serverSocket.on('error', (e) => {
+      throw new Error('Socket error: ' + e);


Done in commit c17c783.

tniessen · 2018-05-04T14:14:05Z

benchmark/tls/secure-pair.js

+  }
+
+  function securePair(conn, client) {
+    const serverCtxt = tls.createSecureContext(options);


Micro nit: We usually abbreviate context as ctx within our codebase so I'd prefer serverCtx for consistency.

Done in commit 53cd71f.

alexfernandez · 2018-05-09T10:56:31Z

@jasnell Three sign-offs, waiting for the CI run. Thanks!

tniessen · 2018-05-09T11:54:55Z

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

alexfernandez · 2018-05-09T15:38:29Z

Apparently the CI run, although I have no clue as to why 😞 Can you please help?

tniessen · 2018-05-11T21:19:12Z

Failures look unrelated, let's try again: https://ci.nodejs.org/job/node-test-pull-request/14818/

apapirovski · 2018-05-14T16:44:20Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14866/

apapirovski · 2018-05-16T04:55:18Z

Landed in c346cb6.

Thank you @alexfernandez and congrats on becoming a Contributor! 🎉

PR-URL: #20344 Refs: #20263 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

alexfernandez · 2018-05-16T07:56:48Z

Thanks @apapirovski and everyone else involved for all the good work! Now for the performance regression on #20263 😄

PR-URL: #20344 Refs: #20263 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. tls Issues and PRs related to the tls subsystem. labels Apr 26, 2018

alexfernandez changed the title ~~Added secure-pair benchmark.~~ benchmark: added secure-pair benchmark Apr 26, 2018

jasnell reviewed Apr 27, 2018

View reviewed changes

jasnell approved these changes Apr 27, 2018

View reviewed changes

benchmark: (tls) add benchmark for legacy TLS secure pair

6ba84cf

Refs: #20263

benjamingr approved these changes Apr 27, 2018

View reviewed changes

alexfernandez changed the title ~~benchmark: added secure-pair benchmark~~ benchmark: add secure-pair benchmark Apr 27, 2018

BridgeAR reviewed Apr 29, 2018

View reviewed changes

Add securing=securePair to benchmark options.

487a327

BridgeAR requested a review from tniessen May 4, 2018 14:04

tniessen approved these changes May 4, 2018

View reviewed changes

Alex Fernández added 2 commits May 5, 2018 17:51

Use template literals.

c17c783

Use serverCtx for consistency.

53cd71f

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 14, 2018

apapirovski closed this May 16, 2018

alexfernandez mentioned this pull request May 16, 2018

Performance Regression: tls.createSecurePair() Not Buffering Connections in v10 #20263

Closed

addaleax mentioned this pull request May 22, 2018

v10.2.0 proposal #20724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

benchmark: add secure-pair benchmark #20344

benchmark: add secure-pair benchmark #20344

alexfernandez commented Apr 26, 2018 •

edited

Loading

Trott commented Apr 26, 2018

apapirovski commented Apr 26, 2018

alexfernandez commented Apr 26, 2018

apapirovski commented Apr 26, 2018

jasnell Apr 27, 2018

alexfernandez Apr 27, 2018 •

edited

Loading

benjamingr left a comment

tniessen commented Apr 27, 2018

alexfernandez commented Apr 27, 2018

BridgeAR Apr 29, 2018

alexfernandez Apr 30, 2018

alexfernandez commented May 3, 2018

jasnell commented May 3, 2018

tniessen May 4, 2018

alexfernandez May 5, 2018

tniessen May 4, 2018

alexfernandez May 5, 2018

tniessen May 4, 2018

alexfernandez May 5, 2018

alexfernandez commented May 9, 2018

tniessen commented May 9, 2018

alexfernandez commented May 9, 2018

tniessen commented May 11, 2018

apapirovski commented May 14, 2018

apapirovski commented May 16, 2018

alexfernandez commented May 16, 2018

benchmark: add secure-pair benchmark #20344

benchmark: add secure-pair benchmark #20344

Conversation

alexfernandez commented Apr 26, 2018 • edited Loading

Checklist

Results v8.11.1

Results Using master

Results With #20287

Deployment Notes

Trott commented Apr 26, 2018

apapirovski commented Apr 26, 2018

alexfernandez commented Apr 26, 2018

apapirovski commented Apr 26, 2018

Choose a reason for hiding this comment

alexfernandez Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

tniessen commented Apr 27, 2018

alexfernandez commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfernandez commented May 3, 2018

jasnell commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfernandez commented May 9, 2018

tniessen commented May 9, 2018

alexfernandez commented May 9, 2018

tniessen commented May 11, 2018

apapirovski commented May 14, 2018

apapirovski commented May 16, 2018

alexfernandez commented May 16, 2018

alexfernandez commented Apr 26, 2018 •

edited

Loading

Results Using `master`

alexfernandez Apr 27, 2018 •

edited

Loading