async_hooks: faster AsyncResource#bind()

[bengl]

FunctionPrototypeBind preserves length, and the asyncResource property can just be set normally, avoiding the costly call to ObjectDefineProperties. In the thisArg === undefined case, reducing it to a single ObjectDefinePropertystill has tangible performance gain.

I added a benchmark and here is the output on my machine (the ./node-master result is without this commit, and the ./node result is with it):

$ ./node benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 5.619836901644185e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 5.96856136341294e+99
$ ./node-master benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 1.5451341226758954e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 1.605511858099379e+99

(Expand here for old/incorrect benchmark.)

$ ./node benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 4.6615746235960295e+100
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 5.939287298590602e+100
$ ./node-master benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 1.6248803889798304e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 1.6487041313302313e+99

cc @nodejs/async_hooks

[nodejs-github-bot]

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

[juanarbol]

Lgtm

[bengl]

Looks like I submitted too soon again here.

While FunctionPrototypeBind does preserve length, it's preserving the length of runInAsyncScope, and not the fn in question. It will need to preserve the length the same way as the other code path, but I since I suspect that it will still be significantly faster, I'm going to leave this open in Draft mode until I can clean it up (hopefully in the next day or two).

[bengl]

Setting this as "ready for review" again. It's now always doing an ObjectDefineProperty, but that's still about a 3.7x improvement on my machine.

[mcollina]

LGTM

How did you found this was a bottleneck? :D

[bengl]

@mcollina.bind() is a pretty hot path with a lot of the new changes (and some upcoming ones) in our APM tracing library, so I was investigating it directly.

[bengl]

Here's a benchmark run: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1136/

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[rochdev]

Why not just stop doing this completely? This is only one of the cases needed to preserve the original functionality as the function could also have other properties or symbols attached to it for example, and those wouldn't be copied. Maybe the best approach then would be to copy nothing and leave it to the user to decide what needs copying. Then the implementation will be as fast as it can be and will only become slower if the user decides that copying properties is needed.

[nodejs-github-bot]

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

[bengl]

Closing since #46432 precludes this.