Use native promisify utility when possible

[brandon93s]

Opening to get the conversation started on leveraging native promisify when the executing process supports it. Updates use native promisify when available and none of the pify options imply functionality it cannot support. Performance testing may inform whether these changes are worth while or not.

Resolves #41

Todo:

• Investigate what is failing in the optimization-tests in Node 8. Initial glances indicate it could be an issue with v8-natives. Other tests are passing.

[brandon93s]

"naive" performance testing:

const sut = require('./index')
const iterations = 10000
const sleep = (fn) => { setTimeout(fn, 100) }
const average = arr => arr.reduce( ( p, c ) => p + c, 0 ) / arr.length;    
const native = sut(sleep)
const pify = sut(sleep, { FORCE_DISABLE_NATIVE: true }) // option added for performance testing

async function timed(fn){
    const hrstart = process.hrtime()
    await Promise.all([...Array(iterations).keys()].map(() => fn() )) // simulate `iteration` sleeps 
    const hrend = process.hrtime(hrstart)
    return hrend[1]/1000000
}

async function test() {
    const pifyResults = await Promise.all([...Array(100).keys()].map(() => timed(pify)))    
    const nativeResults = await Promise.all([...Array(100).keys()].map(() => timed(native)))
    
    console.log(`Native Average Execution: ${average(nativeResults)} ms`)
    console.log(`Pify Average Execution: ${average(pifyResults)} ms`)    
}

test()

Results:

Native Average Execution: 409.61466827000027 ms
Pify Average Execution: 530.97985807 ms

Native is a consistent winner between test runs. Note that this is a bit of an extreme test however. The current test is spinning up 10,000 promises that each sleep for 100 ms. This process is repeated 100 times and the average execution time is calculated.

[kevva]

You can do all these with util.promisify using https://nodejs.org/api/util.html#util_custom_promisified_functions.

[brandon93s]

You can, but you don't gain the performance benefits of util.promisify. The underlying implementation would still be the pify method with a bit more setup required.

Current:

if (isCustom) {
     return getPifyImplementation(fn);
}

Using custom:

if (isCustom) {
     const promiseFn = return getPifyImplementation(fn);
     fn[util.promisify.custom] = promiseFn;
     return promisify(fn);
}

// promisify(fn) === promiseFn  => true

[sindresorhus]

One problem I didn't think of when opening #41 is that the result of some core methods can change when using util.promisify. For example, with child_process.spawn:

If this method is invoked as its util.promisify()ed version, it returns a Promise for an object with stdout and stderr properties. In case of an error, a rejected promise is returned, with the same error object given in the callback, but with an additional two properties stdout and stderr.

One solution would be for pify to do the same, by using https://nodejs.org/api/util.html#util_custom_promisified_functions if available, but that would be a breaking change, so not sure it's worth it.

[brandon93s]

Closing this out for now. The trade-offs do not seem worth the changes at this time:

• Minimal performance benefit
• Added code complexity
• Difficult to concisely support edge scenarios