perf: improve isAsyncFunction

[Uzlopak]

I was playing around and realized that isAsyncFunction is using utils.inspect and startsWitht. So I tested the benchmark/validate and got

MONGOOSE_DEV=1 node validate
invalid x 24,186 ops/sec ±0.62% (92 runs sampled)
valid x 76,020 ops/sec ±0.59% (92 runs sampled)

So I replaced startsWith with indexOf which is about 10 times faster than startsWith and benchmarked again:

MONGOOSE_DEV=1 node validate
invalid x 24,283 ops/sec ±3.05% (94 runs sampled)
valid x 76,428 ops/sec ±0.50% (95 runs sampled)

No performance gain, despite we know that indexOf is about 10 x faster indicates that we have a different bottleneck, utils.inspect.

So I thought: Ok, there is a call to utils.inspect which is basically stringifying the function to just check if the string begins with [AsyncFunction:

So I was like: We get non-primitive values for the isAsyncFunction-check. So I can use a WeakMap to cache the information if it is an async Function or not. And then I benchmarked again:

MONGOOSE_DEV=1 node validate
invalid x 27,056 ops/sec ±0.67% (91 runs sampled)
valid x 104,265 ops/sec ±0.66% (93 runs sampled)

Nice gain.

To make sure, that dont get an Error when passing a primitive type, I added a check if we pass a primitive type and benched again:

MONGOOSE_DEV=1 node validate
invalid x 27,390 ops/sec ±0.51% (93 runs sampled)
valid x 104,177 ops/sec ±0.70% (95 runs sampled)

To compare how close we are if we are basically skipping the check, as the benchmark is not using async functions, i patched isAsyncFunction to return false.

MONGOOSE_DEV=1 node validate
invalid x 26,246 ops/sec ±0.83% (90 runs sampled)
valid x 107,461 ops/sec ±0.77% (91 runs sampled)

So with this PR we are avoiding the bottleneck very good.

So from 76k to 104k is not bad.. Also compared to #11298 where we started with 46k in mongoose 6.1.10 for the valid path, this means we basically doubled our performance for validate from mongoose 6.2.0.

[Uzlopak]

@AbdelrahmanHafez
Please also have a look at this

[Uzlopak]

With the improved benchmark from #11415
before:

before:

MONGOOSE_DEV=1 node validate
invalid async x 21,179 ops/sec ±0.71% (84 runs sampled)
valid async x 41,408 ops/sec ±1.34% (83 runs sampled)
invalid sync x 24,553 ops/sec ±0.76% (93 runs sampled)
valid sync x 76,212 ops/sec ±0.67% (92 runs sampled)

after:

 MONGOOSE_DEV=1 node validate
invalid async x 21,100 ops/sec ±0.80% (83 runs sampled)
valid async x 42,095 ops/sec ±1.59% (81 runs sampled)
invalid sync x 27,730 ops/sec ±0.59% (93 runs sampled)
valid sync x 105,097 ops/sec ±0.68% (95 runs sampled)

As you can see it improves the valid sync path significantly.

[vkarpov15]

My only concern with this PR is the caching. We try to avoid caching because we don't want to unexpectedly cause extra memory usage. Granted this is less of a concern with WeakMap(), but I'd prefer if we didn't do caching here.

Potential alternative approach to avoid the overhead of inspect():

const asyncFunctionProto = Object.getPrototypeOf(async function() {});

Object.getPrototypeOf(v) === asyncFunctionProto; // true for async functions, false for regular functions

[Uzlopak]

@vkarpov15
Nice trick!

Added unit tests.

For my curiosity I checked what happens if the function returns a Promise, and it is of course false. inspect also just knows that it is a sync function.

[AbdelrahmanHafez]

LGTM, thanks! 👍

[vkarpov15]

Thanks 👍