Skip to content
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

crypto,tls: perf improvements for crypto and tls getCiphers #7225

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 8, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

crypto, tls

Description of change

Improve performance of crypto.getCiphers, getHashes, getCurves
and tls.getCiphers by consolidating filterDuplicates logic, adding
caching of output, and streamlining filterDuplicates implementation.

Benchmarks:

crypto.getCiphers n=1 v6.2.1 = 2559.3, new = 15890 ...... -83.89%
crypto.getCiphers n=5000 v6.2.1 = 3516.3, new = 24203000 ... -99.99%

tls.getCiphers n=1 v6.2.1 = 3405.3, new = 14877 ...... -77.11%
tls.getCiphers n=5000 v6.2.1 = 6074.4, new = 24202000 ... -99.97%

@nodejs/crypto @mscdex

@jasnell jasnell added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. labels Jun 8, 2016
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 8, 2016
@jasnell
Copy link
Member Author

jasnell commented Jun 8, 2016

@bnoordhuis
Copy link
Member

Is this really worth optimizing?

@jasnell
Copy link
Member Author

jasnell commented Jun 8, 2016

It's not a high priority by any means but a perf boost is a perf boost ;-)

@@ -21,15 +22,11 @@ exports.DEFAULT_CIPHERS =

exports.DEFAULT_ECDH_CURVE = 'prime256v1';

var _ciphers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any point in prefixing module-local variable?

@jasnell
Copy link
Member Author

jasnell commented Jun 8, 2016

@indutny ... updated!

@indutny
Copy link
Member

indutny commented Jun 8, 2016

LGTM, if CI is green.

@indutny
Copy link
Member

indutny commented Jun 8, 2016

Thank you.

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Jun 8, 2016
return Object.getOwnPropertyNames(ctx).sort();
};
exports.getCiphers = internalUtil.cachedResult(() => {
return internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: true would look better than 1 for a boolean argument

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2016

LGTM except for one minor nit and if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/2986/

Improve performance of crypto.getCiphers, getHashes, getCurves
and tls.getCiphers by consolidating filterDuplicates logic, adding
caching of output, and streamlining filterDuplicates implementation.

Benchmarks:

crypto.getCiphers n=1    v6.2.1 = 2559.3, new = 15890 ...... -83.89%
crypto.getCiphers n=5000 v6.2.1 = 3516.3, new = 24203000 ... -99.99%

tls.getCiphers    n=1    v6.2.1 = 3405.3, new = 14877 ...... -77.11%
tls.getCiphers    n=5000 v6.2.1 = 6074.4, new = 24202000 ... -99.97%
@jasnell
Copy link
Member Author

jasnell commented Jun 21, 2016

Nit addressed, commits squashed, new CI: https://ci.nodejs.org/job/node-test-pull-request/3039/

jasnell added a commit that referenced this pull request Jun 21, 2016
Improve performance of crypto.getCiphers, getHashes, getCurves
and tls.getCiphers by consolidating filterDuplicates logic, adding
caching of output, and streamlining filterDuplicates implementation.

Benchmarks:

crypto.getCiphers n=1    v6.2.1 = 2559.3, new = 15890 ...... -83.89%
crypto.getCiphers n=5000 v6.2.1 = 3516.3, new = 24203000 ... -99.99%

tls.getCiphers    n=1    v6.2.1 = 3405.3, new = 14877 ...... -77.11%
tls.getCiphers    n=5000 v6.2.1 = 6074.4, new = 24202000 ... -99.97%

PR-URL: #7225
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member Author

jasnell commented Jun 21, 2016

Landed in 6be73fe

@jasnell jasnell closed this Jun 21, 2016
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
Improve performance of crypto.getCiphers, getHashes, getCurves
and tls.getCiphers by consolidating filterDuplicates logic, adding
caching of output, and streamlining filterDuplicates implementation.

Benchmarks:

crypto.getCiphers n=1    v6.2.1 = 2559.3, new = 15890 ...... -83.89%
crypto.getCiphers n=5000 v6.2.1 = 3516.3, new = 24203000 ... -99.99%

tls.getCiphers    n=1    v6.2.1 = 3405.3, new = 14877 ...... -77.11%
tls.getCiphers    n=5000 v6.2.1 = 6074.4, new = 24202000 ... -99.97%

PR-URL: #7225
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Improve performance of crypto.getCiphers, getHashes, getCurves
and tls.getCiphers by consolidating filterDuplicates logic, adding
caching of output, and streamlining filterDuplicates implementation.

Benchmarks:

crypto.getCiphers n=1    v6.2.1 = 2559.3, new = 15890 ...... -83.89%
crypto.getCiphers n=5000 v6.2.1 = 3516.3, new = 24203000 ... -99.99%

tls.getCiphers    n=1    v6.2.1 = 3405.3, new = 14877 ...... -77.11%
tls.getCiphers    n=5000 v6.2.1 = 6074.4, new = 24202000 ... -99.97%

PR-URL: #7225
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

 Conflicts:
	lib/internal/util.js
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@MylesBorins
Copy link
Contributor

@jasnell lts?

@MylesBorins
Copy link
Contributor

this is not landing cleanly so I am going to mark as don't land. Please feel free to backport

@MylesBorins
Copy link
Contributor

/cc @jasnell

@jasnell
Copy link
Member Author

jasnell commented Oct 10, 2016

Not backporting this should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js. tls Issues and PRs related to the tls subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants