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: recent large performance regressions #31739

Closed
mscdex opened this issue Feb 11, 2020 · 1 comment
Closed

crypto: recent large performance regressions #31739

mscdex opened this issue Feb 11, 2020 · 1 comment
Labels
crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js.

Comments

@mscdex
Copy link
Contributor

mscdex commented Feb 11, 2020

  • Version: master
  • Platform: linux
  • Subsystem: crypto

While working on a system that automatically and regularly runs benchmarks on master and compares the results between builds, I found some large crypto performance regressions between f368210 and 93c4c1a. Here are the crypto benchmarks that showed a greater than 10% regression (numeric values below are the % change with "***" confidence):

'aes-gcm-throughput': {
   'cipher="aes-128-gcm" len=1024 n=500': -45.12,
   'cipher="aes-128-gcm" len=4096 n=500': -29.89,
   'cipher="aes-128-gcm" len=16384 n=500': -13.18,
   'cipher="aes-192-gcm" len=1024 n=500': -44.51,
   'cipher="aes-192-gcm" len=4096 n=500': -30.73,
   'cipher="aes-192-gcm" len=16384 n=500': -10.44,
   'cipher="aes-256-gcm" len=1024 n=500': -44.13,
   'cipher="aes-256-gcm" len=4096 n=500': -27.38,
   'cipher="aes-256-gcm" len=16384 n=500': -11.74 },
'hash-stream-creation': {
   'algo="sha256" api="legacy" len=2 out="hex" type="asc" writes=500': -68.41,
   'algo="sha256" api="legacy" len=1024 out="hex" type="asc" writes=500': -60.25,
   'algo="sha256" api="legacy" len=2 out="binary" type="asc" writes=500': -68.58,
   'algo="sha256" api="legacy" len=1024 out="binary" type="asc" writes=500': -60.51,
   'algo="sha256" api="legacy" len=2 out="buffer" type="asc" writes=500': -64.06,
   'algo="sha256" api="legacy" len=1024 out="buffer" type="asc" writes=500': -57.33,
   'algo="sha256" api="legacy" len=2 out="hex" type="utf" writes=500': -69.56,
   'algo="sha256" api="legacy" len=1024 out="hex" type="utf" writes=500': -58.65,
   'algo="sha256" api="legacy" len=2 out="binary" type="utf" writes=500': -70.01,
   'algo="sha256" api="legacy" len=1024 out="binary" type="utf" writes=500': -58.88,
   'algo="sha256" api="legacy" len=2 out="buffer" type="utf" writes=500': -65.12,
   'algo="sha256" api="legacy" len=1024 out="buffer" type="utf" writes=500': -56.01,
   'algo="sha256" api="legacy" len=2 out="hex" type="buf" writes=500': -67.46,
   'algo="sha256" api="legacy" len=1024 out="hex" type="buf" writes=500': -58.93,
   'algo="sha256" api="legacy" len=2 out="binary" type="buf" writes=500': -67.74,
   'algo="sha256" api="legacy" len=1024 out="binary" type="buf" writes=500': -58.98,
   'algo="sha256" api="legacy" len=2 out="buffer" type="buf" writes=500': -62.85,
   'algo="sha256" api="legacy" len=1024 out="buffer" type="buf" writes=500': -55.95,
   'algo="md5" api="legacy" len=2 out="hex" type="asc" writes=500': -68.92,
   'algo="md5" api="legacy" len=1024 out="hex" type="asc" writes=500': -65.12,
   'algo="md5" api="legacy" len=102400 out="hex" type="asc" writes=500': -11.61,
   'algo="md5" api="legacy" len=2 out="binary" type="asc" writes=500': -69.65,
   'algo="md5" api="legacy" len=1024 out="binary" type="asc" writes=500': -65.85,
   'algo="md5" api="legacy" len=102400 out="binary" type="asc" writes=500': -11.46,
   'algo="md5" api="legacy" len=2 out="buffer" type="asc" writes=500': -64.73,
   'algo="md5" api="legacy" len=1024 out="buffer" type="asc" writes=500': -61.71,
   'algo="md5" api="legacy" len=102400 out="buffer" type="asc" writes=500': -11.4,
   'algo="md5" api="legacy" len=2 out="hex" type="utf" writes=500': -70.09,
   'algo="md5" api="legacy" len=1024 out="hex" type="utf" writes=500': -63.91,
   'algo="md5" api="legacy" len=2 out="binary" type="utf" writes=500': -70.61,
   'algo="md5" api="legacy" len=1024 out="binary" type="utf" writes=500': -63.95,
   'algo="md5" api="legacy" len=2 out="buffer" type="utf" writes=500': -65.73,
   'algo="md5" api="legacy" len=1024 out="buffer" type="utf" writes=500': -60.23,
   'algo="md5" api="legacy" len=2 out="hex" type="buf" writes=500': -67.93,
   'algo="md5" api="legacy" len=1024 out="hex" type="buf" writes=500': -63.9,
   'algo="md5" api="legacy" len=102400 out="hex" type="buf" writes=500': -11.59,
   'algo="md5" api="legacy" len=2 out="binary" type="buf" writes=500': -68.48,
   'algo="md5" api="legacy" len=1024 out="binary" type="buf" writes=500': -64.36,
   'algo="md5" api="legacy" len=102400 out="binary" type="buf" writes=500': -11.55,
   'algo="md5" api="legacy" len=2 out="buffer" type="buf" writes=500': -63.59,
   'algo="md5" api="legacy" len=1024 out="buffer" type="buf" writes=500': -60.13,
   'algo="md5" api="legacy" len=102400 out="buffer" type="buf" writes=500': -11.73 },
'hash-stream-throughput': {
   'algo="sha1" api="legacy" len=2 type="asc" writes=500': -35.71,
   'algo="sha1" api="legacy" len=1024 type="asc" writes=500': -24.66,
   'algo="sha1" api="legacy" len=2 type="utf" writes=500': -41.67,
   'algo="sha1" api="legacy" len=1024 type="utf" writes=500': -21.52,
   'algo="sha1" api="legacy" len=2 type="buf" writes=500': -33.78,
   'algo="sha1" api="legacy" len=1024 type="buf" writes=500': -22.71,
   'algo="sha256" api="legacy" len=2 type="asc" writes=500': -36.48,
   'algo="sha256" api="legacy" len=1024 type="asc" writes=500': -18.78,
   'algo="sha256" api="legacy" len=2 type="utf" writes=500': -41.03,
   'algo="sha256" api="legacy" len=1024 type="utf" writes=500': -16.56,
   'algo="sha256" api="legacy" len=2 type="buf" writes=500': -33.76,
   'algo="sha256" api="legacy" len=1024 type="buf" writes=500': -17.47,
   'algo="sha512" api="legacy" len=2 type="asc" writes=500': -36.95,
   'algo="sha512" api="legacy" len=1024 type="asc" writes=500': -20.98,
   'algo="sha512" api="legacy" len=2 type="utf" writes=500': -42.2,
   'algo="sha512" api="legacy" len=1024 type="utf" writes=500': -18.5,
   'algo="sha512" api="legacy" len=2 type="buf" writes=500': -34.29,
   'algo="sha512" api="legacy" len=1024 type="buf" writes=500': -19.05 },

The culprits seem to be at least e559842 and possibly also d016b9d.

/cc @ronag

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js. labels Feb 11, 2020
@ronag
Copy link
Member

ronag commented Feb 11, 2020

e559842 could be a cause if crypto related code uses writable/readable.

d016b9d I'm very sceptical whether it could/should have any performance impact in a hot path.

ronag added a commit to nxtedition/node that referenced this issue Feb 11, 2020
nodejs@e559842
made writable/readable computed with a legacy mode if the properties
are written to.

LazyTransform still unecessarily wrote to these properties causing a performance
regression.

Fixes: nodejs#31739
@ronag ronag closed this as completed in 9cbf6af Feb 13, 2020
codebytere pushed a commit that referenced this issue Feb 17, 2020
e559842
made writable/readable computed with a legacy mode if the properties
are written to.

LazyTransform still unecessarily wrote to these properties causing a
performance regression.

Fixes: #31739

PR-URL: #31742
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this issue Mar 15, 2020
e559842
made writable/readable computed with a legacy mode if the properties
are written to.

LazyTransform still unecessarily wrote to these properties causing a
performance regression.

Fixes: #31739

PR-URL: #31742
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this issue Mar 17, 2020
e559842
made writable/readable computed with a legacy mode if the properties
are written to.

LazyTransform still unecessarily wrote to these properties causing a
performance regression.

Fixes: #31739

PR-URL: #31742
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this issue Mar 30, 2020
e559842
made writable/readable computed with a legacy mode if the properties
are written to.

LazyTransform still unecessarily wrote to these properties causing a
performance regression.

Fixes: #31739

PR-URL: #31742
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants