crypto: add optional callback to crypto.sign and crypto.verify

[panva]

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

• crypto.sign(algorithm, data, key[, callback])
• crypto.verify(algorithm, data, key, signature[, callback])

[tniessen]

Is there room for benchmarks? I can imagine two aspects:

1. How much slower is async signing compared to sync signing? (Both sequentially)
2. How much faster is async signing compared to sync signing? (Async in parallel)

So we'd expect drastic performance improvements in the second case, I assume?

[panva]

I can benchmark locally for sure, but would need help setting up the benchmark in CI.

[panva]

So we'd expect drastic performance improvements in the second case, I assume?

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.

Details

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: 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

[mcollina]

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 also depends on how many CPUs are available on the machine.

[jasnell]

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 :-)

[panva]

@panva ... here's the updated patch for dsaEncoding with the necessary adjustments... If I can get a moment later this afternoon I'll push to your branch if you don't beat me to it...

Thank you, i'll re-push your patch.

[jasnell]

LGTM for all the parts that I didn't contribute to. Will need another sign off to cover those

[panva]

rebased to hopefully get a clean CI.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36596/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36600/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36603/

[jasnell]

Landed in 25985d6