Perf regression on crypto.verify #72
Comments
|
Richard Lau gave some hints to see OpenSSL move from 1.1.1 to 3.0.x, if investigation of the JS code didn't bring any hint about the issue, I think it's worth taking a look at this. |
|
That would be my first guess as well. You could build from source before and after nodejs/node#38512 and run the benchmark with the respective binaries to test this hypothesis. Or simply use v17.0.0-nightly2021100135dc3861cd and v18.0.0-nightly2021110186099a375a representing the commit range 35dc3861cd...86099a375a for a less accurate but probably still helpful result. |
|
Might be worth checking if it's one of the known upstream OpenSSL 3 performance issues: Otherwise, if you are comfortable rebuilding Node.js you could try dynamically linking against external OpenSSL, e.g. this would compile against dynamically linked OpenSSL 1.1.1n: ./configure --verbose --shared-openssl --shared-openssl-includes=/opt/openssl-1.1.1n/include/ --shared-openssl-libpath=/opt/openssl-1.1.1n/lib/where |
|
Hey, I didn't have time to build the nodejs with OpenSSL 1.1 but @NewEraCracker did and it seems to improve the performance accords #84 (comment). Is this problem likely to be related to OpenSSL, if so is there anything that can be done? |
|
Besides fixing it upstream. I don't think so. |
|
I dug into this a little bit on the surface. For testing I used an M1 MacBook Pro, node v16.20.0 and v18.16.0 installed via nvm and the benchmark scripts from nodejs/node#48267 These benchmarks test the oneshot crypto.sign and crypto.verify operations with
I can definitely observe a regression across the board for all verify operation throughputs between v16 and v18, but not as severe as 5x everywhere, it varies per how the key is passed in. For the sign operation some configs have no regression at all. What is interesting though is that such severe regression (e.g. rsa verify 28k ops/s down to 7k ops/s) is only present when the keys are passed as pem or der, when passed as jwk or a KeyObject a much less severe regression is observed. IIRC the two severely impacted key formats have their own unique code path in cc @tniessen My results (verify) |
|
@panva Did you have a chance to test 18.x against OpenSSL 1.1.1? So far, we have suspected that the difference in performance is due to the different OpenSSL versions. |
This comment was marked as outdated.
This comment was marked as outdated.
I have built v18.x with OpenSSL 1.1.1 now, it (OpenSSL 3.x) is indeed the problem at hand for the slowdowns in both pem/der parsing and sign/verify. |
|
On Linux I am using this script: https://gist.github.com/NewEraCracker/43533159b9e56b77ce6895f8d0b6d4de to build a custom version with its own OpenSSL and install Node.Js on a custom directory. Maybe you will find it useful. It is based on the Dockerfile in @richardlau comment and then I just kept asking ChatGPT to add more stuff to it. So far I have tested this in three different Linux environments. |
|
Does anyone feel like plotting this to a chart(s)? Verifyv18.16.0 w/ OpenSSL 1.1.1 `benchmark/crypto/oneshot-verify.js`v18.16.0 w/ OpenSSL 3.0.x `benchmark/crypto/oneshot-verify.js`v18.16.0 w/ OpenSSL 3.1.x `benchmark/crypto/oneshot-verify.js`Signv18.16.0 w/ OpenSSL 1.1.1 `benchmark/crypto/oneshot-sign.js`v18.16.0 w/ OpenSSL 3.0.x `benchmark/crypto/oneshot-sign.js`v18.16.0 w/ OpenSSL 3.1.x `benchmark/crypto/oneshot-sign.js`createPublicKey / createPrivateKeyv18.16.0 w/ OpenSSL 1.1.1 `benchmark/crypto/create-keyobject.js`v18.16.0 w/ OpenSSL 3.0.x `benchmark/crypto/create-keyobject.js`v18.16.0 w/ OpenSSL 3.1.x `benchmark/crypto/create-keyobject.js` |
|
ChatGPT did, after massaging the data a bit. The values seem accurate. There is a noticeable performance drop when moving from OpenSSL 1.1.1 to 3.0.x across all operations. However, there is a small improvement when upgrading from 3.0.x to 3.1.x. Verify
Sign
createPublicKey / createPrivateKey
No warranties express or implied. Edit: Percentage values revised manually. |
|
I suggest to hide the above comment because it seems to mesh together the different configs and losing context while doing so, the drop is not like that across the board. |
|
Maybe we have to upgrade to OpenSSL 3.1? |
|
@Uzlopak That's not an option given the current OpenSSL release strategy. |
|
Then we have to wait for OpenSSL 3.2 to land. |
OpenSSL 1.1.1w vs 3.0.12OpenSSL 1.1.1w vs 3.1.3OpenSSL 3.0.12 vs 3.1.3
|
|
Users can already dynamically link against OpenSSL 3.1 if they wish. If we upgrade the statically linked version of OpenSSL to 3.1, then we will be forced to again upgrade (or downgrade) OpenSSL on existing Node.js release lines. I don't know if the OpenSSL team has released information about the 3.2 end of life date, or about backward compatibility of future versions. |
|
I see that openssl released today v3.2.0 beta. Maybe in a month we have stable 3.2.0. Is upgrading to openssl 3.2.0 an acceptable? |
That depends on how long it will be supported by the OpenSSL team. |
AFAIK nothing has been stated yet about 3.2, or what the next LTS after 3.0 will be. https://www.openssl.org/policies/releasestrat.html was last updated in March. They have stated that OpenSSL now follows semver. |
|
My understading of OpenSSL is that minor are not ABI stable. Has anything changed on that front? In other terms, we cannot update OpenSSL because we risk breaking addons. |
|
https://www.openssl.org/policies/general/versioning-policy.html For minor:
The same with Patch Releases.
So according to openssl they will have a stable ABI. |
|
Then if a API/ABI compatible release come out that has a LTS date after our LTS deadline, we can certainly migrate. |
|
@nodejs/tsc I'd like add the following topic to the upcoming TSC agenda: our plan for upgrading OpenSSL. Here's why it's important: If we don't upgrade OpenSSL, our crypto library will remain slow, and we won't get the new features of the 3.x versions. For example, version 3.2 plans to add argon2, but we won't get it because OpenSSL says they won't add new functionality to existing minor versions. The OpenSSL developers say that for versions 3 and up, they'll keep the ABI stable when they update minor or patch versions. So, in theory, it shouldn't cause problems for our users. The only good reason to stick with OpenSSL 3.0 is that it's supposed to be supported for five years as LTS, while 3.x versions only get two years of support. OpenSSL 3.0's end-of-life is in September 2026, while 3.1's is in March 2025. If OpenSSL 3.2 comes out in December 2023, it'll be unsupported by December 2025. Node 18's security support ends on April 30, 2025, which works fine with OpenSSL 3.2. But Node 20's security support ends on April 30, 2026, which is a problem. We'd have to use OpenSSL 3.0 since the time window for OpenSSL 3.2 is too short and downgrading from OpenSSL 3.2 to 3.0 means removal of additional functionality and thus breaking userland. However, if OpenSSL 3.2 comes out on or after April 30, 2024, we can upgrade to it. If we release Node 22 with OpenSSL 3.0 and set the security support to end on April 30, 2027, we'll run into the same issue as with Node 16 because OpenSSL 3.0 becomes unsupported in September 2026. So, the risk here is that our security support might extend beyond the end-of-life of an OpenSSL version with the same ABI as 3.0. For Node 22, we just can hope that OpenSSL releases a 3.x version on or after April 30, 2025, or they declare a 3.x or 4.0 version as LTS before April 30, 2024. This situation isn't ideal because it ties our support to OpenSSL. But it's something we need to discuss. Maybe we should talk to the OpenSSL developers and work out an agreement to make sure our security support aligns with our security end-of-life. Maybe we have to accept that at some point we need to upgrade OpenSSL with breaking ABI and addons would break and need to adapt accordingly. My personal preference is to upgrade OpenSSL and force addons to adapt accordingly, as they would run into the same OpenSSL support issues as node core would do. |
|
One elephant in the room is that Node.js doesn't actually bundle openssl -- it bundles the quic fork of openssl from https://github.com/quictls/openssl. While they have a fork for 3.1, there appear to be issues for 3.2 and later: quictls/quictls#14. |
Why does that need to be on the TSC agenda? GitHub discussions among collaborators and other community members should be the default channel for any decision-making process.
That has always been the case, and we've regularly discussed how to deal with OpenSSL's release strategy.
Let's be realistic. Node.js is one of thousands of dependents of OpenSSL and probably not even among the most important ones. We cannot reasonably ask OpenSSL maintainers to change their release schedule for Node.js, nor can we ask them for commercial support that extends beyond the official end of life. We can adjust the release schedule of Node.js to align with OpenSSL if necessary for security reasons, but we try to avoid that.
From my point of view, a reasonable list of priorities is: security, then reliability/stability, and lastly features/performance. I don't think we should sacrifice the former categories for the latter. As @richardlau mentions, one complication is that we use a fork of OpenSSL. And let's not forget about embedders that link against BoringSSL either (e.g., Electron). We don't want to make things too difficult for them either. |
|
What's the reason for preferring OpenSSL over BoringSSL? |
|
Doesn't the openssl version deserve an issue in nodejs/node? Neither 3.1 nor 3.2 (likely?) solves the regression at hand for which this issue is. The discussion is important but doesn't belong here. |
BoringSSL does not offer LTS.
@panva I'm lost, my reading of #72 (comment) was that at least v3.1 mitigated the problem. Has the root cause of the issue been identified? |
@mcollina Have a look at 1.1.1w vs 3.1.3 please. It is still far from mitigated for most common scenarios. |
From the mentioned issue:
I'm guessing feedback should be left there too on how to move forward. |
In addition to what @mcollina said, Google specifically tells us not to use BoringSSL because they provide no stability guarantees whatsoever. BoringSSL also lacks some features that users of Node.js might rely on. Other embedders such as Electron only link against BoringSSL because they have to, e.g., because Chromium only supports BoringSSL but not OpenSSL. |
|
Others have deemed OpenSSL v3 as not production ready, because it has this super bad performance regression. E.g. microsoft implemented for this reason msquic based on OpenSSL 1 and 3, which they use for their azure instances. Also to what @panva wrote: |
We had no choice but to use OpenSSL 3.x because 1.1.1 is now EOL. We will continue to support linking with 1.1.1 so long as the commercial extended support OpenSSL offers still covers 1.1.1 security releases (privately distributed). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
On NodeJS 16.x,
crypto.verifymade 30k op/s (ref)But on NodeJS 18.x,
crypto.verifydowngrade to 6~7k op/s (ref).I initially discover this on this pr: RafaelGSS/nodejs-bench-operations#31, I already did an initial investigation of the cause, which currently points to
validationFunction, but I think we should look on V8 side to see if something change.It's probable that this slowdown affects
crypto.signtoo, but I didn't have time yet to benchmark.The text was updated successfully, but these errors were encountered: