crypto: add optional callback to crypto.sign and crypto.verify - #37500
crypto: add optional callback to crypto.sign and crypto.verify#37500panva wants to merge 11 commits into
Conversation
Sorry, something went wrong.
|
Is there room for benchmarks? I can imagine two aspects:
So we'd expect drastic performance improvements in the second case, I assume? |
Sorry, something went wrong.
|
I can benchmark locally for sure, but would need help setting up the benchmark in CI. |
Sorry, something went wrong.
Would you expect a drastic performance improvement in the second case? What I assume is slightly lower performance but having the main thread unblocked... It is what i'm seeing locally. Detailsconst crypto = require('crypto')
const kp = crypto.generateKeyPairSync('rsa', { modulusLength: 2048 * 2 })
const data = crypto.randomBytes(256)
function measureLag(iteration) {
console.time(`measureLag${iteration}`)
setTimeout(() => {
console.timeEnd(`measureLag${iteration}`)
measureLag(iteration + 1) // Recurse
})
}
measureLag(1)
function sign(iteration) {
console.time(`sign${iteration}`)
crypto.sign('sha256', data, kp.privateKey, () => {
console.timeEnd(`sign${iteration}`)
setTimeout(sign.bind(undefined, iteration + 1))
})
}
sign(1)measureLag2: 1.948ms measureLag3: 1.485ms measureLag4: 0.09ms measureLag5: 1.313ms measureLag6: 1.318ms sign2: 4.473ms measureLag7: 0.174ms measureLag8: 1.512ms measureLag9: 1.237ms measureLag10: 1.346ms sign3: 3.778ms measureLag11: 0.971ms measureLag12: 1.43ms measureLag13: 1.394ms measureLag14: 1.092ms measureLag15: 1.165ms sign4: 4.07ms measureLag16: 0.137ms measureLag17: 1.355ms measureLag18: 1.286ms measureLag19: 0.07ms Compared to sync const crypto = require('crypto')
const kp = crypto.generateKeyPairSync('rsa', { modulusLength: 2048 * 2 })
const data = crypto.randomBytes(256)
function measureLag(iteration) {
console.time(`measureLag${iteration}`)
setTimeout(() => {
console.timeEnd(`measureLag${iteration}`)
measureLag(iteration + 1) // Recurse
})
}
measureLag(1)
function sign(iteration) {
console.time(`sign${iteration}`)
crypto.sign('sha256', data, kp.privateKey)
console.timeEnd(`sign${iteration}`)
setTimeout(sign.bind(undefined, iteration + 1))
}
sign(1)measureLag2: 5.191ms sign3: 4.1ms measureLag3: 4.339ms sign4: 3.481ms measureLag4: 4.656ms sign5: 3.729ms measureLag5: 5.178ms sign6: 3.581ms measureLag6: 5ms sign7: 3.87ms measureLag7: 5.056ms sign8: 3.837ms measureLag8: 5.303ms sign9: 3.896ms measureLag9: 5.309ms sign10: 3.703ms measureLag10: 5.043ms sign11: 3.991ms measureLag11: 5.473ms sign12: 3.932ms measureLag12: 5.453ms sign13: 4.012ms measureLag13: 5.523ms sign14: 4.119ms measureLag14: 5.637ms sign15: 4.088ms measureLag15: 5.592ms sign16: 4.07ms measureLag16: 5.476ms sign17: 4.16ms measureLag17: 5.615ms sign18: 4.016ms measureLag18: 5.403ms sign19: 3.961ms measureLag19: 5.187ms |
Sorry, something went wrong.
It also depends on how many CPUs are available on the machine. |
Sorry, something went wrong.
This comment has been minimized.
|
Specifically on the performance bit... it's all relative to what you're testing. Moving the sign/verify operations onto the thread pool will not alter the performance of those operations in any significant way, in fact, if anything it will make those slightly slower. What it does, however, is allow the main event loop to continue turning so it can do other things, so you should see an increase in overall throughput even if the individual operations themselves are not actually faster. That said, you also have to consider all of the other things that may be using the libuv threadpool. File system operations and DNS resolution in particular. If your system is heavy on async file system and crypto, then the individual sign operations may be fairly significantly impacted. Bottom line is that moving to async is not, in itself, a performance silver bullet :-) |
Sorry, something went wrong.
This comment has been minimized.
Thank you, i'll re-push your patch. |
Sorry, something went wrong.
This comment has been minimized.
jasnell
left a comment
There was a problem hiding this comment.
LGTM for all the parts that I didn't contribute to. Will need another sign off to cover those
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
rebased to hopefully get a clean CI. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #37500 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #37500 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update archs files for OpenSSL-1.1.1+quic (James M Snell) #37601 * switch openssl to quictls/openssl (James M Snell) #37601 * doc: * update maintaining-openssl guide (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * add promisified readFile benchmark (Nitzan Uziely) #37608 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * test: * update dom/abort tests (James M Snell) #37693 * fixup test to account for quic openssl version (James M Snell) #37601 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37500 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712 * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712 * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
As discussed in #37218 this begins adding callbacks for one-shot crypto module operations to allow for execution using libuv's threadpool, rather than introducing a new module.
Starting off with