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: fixes performance regression #31742

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 11, 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

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

@ronag ronag added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem. labels Feb 11, 2020
@ronag ronag requested a review from mscdex February 11, 2020 17:34
@ronag ronag force-pushed the crypto-lazy-transform-perf branch from 6a36d5f to 9a9f9d1 Compare February 11, 2020 17:34
@ronag
Copy link
Member Author

ronag commented 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 force-pushed the crypto-lazy-transform-perf branch from 9a9f9d1 to 81e6995 Compare February 11, 2020 17:37
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 11, 2020

@mscdex
Copy link
Contributor

mscdex commented Feb 12, 2020

It looks like some benchmark parameters were removed from the first Benchmark CI.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/526/console

While it seems this mostly fixes the performance issues, I noticed there are some instances of regressions for some reason:

crypto/cipher-stream.js api='stream' len=1024 type='asc' cipher='AES192' writes=500   ***     -4.07 %       ±1.97%  ±2.65%  ±3.51%
crypto/cipher-stream.js api='stream' len=1024 type='asc' cipher='AES256' writes=500   ***     -4.79 %       ±0.64%  ±0.85%  ±1.11%
crypto/cipher-stream.js api='stream' len=2 type='asc' cipher='AES192' writes=500      ***    -10.08 %       ±3.97%  ±5.35%  ±7.10%
crypto/cipher-stream.js api='stream' len=2 type='asc' cipher='AES256' writes=500      ***     -7.19 %       ±1.04%  ±1.39%  ±1.82%
crypto/cipher-stream.js api='stream' len=2 type='buf' cipher='AES192' writes=500      ***    -10.32 %       ±5.65%  ±7.54%  ±9.86%
crypto/cipher-stream.js api='stream' len=2 type='buf' cipher='AES256' writes=500      ***    -13.26 %       ±5.99%  ±8.07% ±10.69%
crypto/cipher-stream.js api='stream' len=2 type='utf' cipher='AES192' writes=500      ***     -8.51 %       ±1.34%  ±1.80%  ±2.36%

It would be nice if we could figure out the cause of them. I've re-ran the benchmarks to verify and the results are the same.

@ronag
Copy link
Member Author

ronag commented Feb 12, 2020

It looks like some benchmark parameters were removed from the first Benchmark CI.

That was by accident. Resolved since then.

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 12, 2020
@ronag
Copy link
Member Author

ronag commented Feb 12, 2020

While it seems this mostly fixes the performance issues, I noticed there are some instances of regressions for some reason:

Aren't those differences mostly within margin of error?

@mscdex
Copy link
Contributor

mscdex commented Feb 12, 2020

Aren't those differences within margin of error?

Not as far as I understand it. Perhaps @AndreasMadsen can give a more definitive answer though.

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Feb 12, 2020

@Trott: What does "yellow" ci mean? Doesn't seem to re-run on "resume build"?

@addaleax
Copy link
Member

@ronag Yellow means that the only failures were known (i.e. almost always unrelated) flaky tests. I think they should re-run on “resume build”, but landing with a yellow CI is generally okay.

@addaleax
Copy link
Member

addaleax commented Feb 13, 2020

While it seems this mostly fixes the performance issues, I noticed there are some instances of regressions for some reason:

Aren't those differences mostly within margin of error?

@ronag They are not, no – the high level of confidence means that it’s safe to assume that they are real and caused by the changes here. Whether they are acceptable as a regression is another question, though.

(I’m removing author-ready because of that, but if you’d like to go ahead and land this then I’m personally okay with that.)

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2020
@benjamingr
Copy link
Member

I am personally in favor of landing with the changes - including the regressions. Mostly because the regressions aren't big and the benchmarks are very positive.

@ronag
Copy link
Member Author

ronag commented Feb 13, 2020

Landed in 9cbf6af

@ronag ronag closed this Feb 13, 2020
ronag added a commit that referenced this pull request Feb 13, 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto: recent large performance regressions
7 participants