async_hooks: move to lazy destroy hook registration in AsyncResource

[puzpuzpuz]

Adds a check into AsyncResource constructor, so that registerDestroyHook only gets called when requireManualDestroy is false and there are no active destroy hooks (previously the check only included requireManualDestroy value). As below benchmark shows this eliminates a certain overhead related with additional tracking of these objects as a part of GC cycle.

This scenario is important considering AsyncLocalStorage API which only uses init hooks.

Important note. Obviously, this PR changes the behavior in the following edge case: previously when an AsyncResource was created (but not yet destroyed) before enabling the first destroy hook, the hook would be called for the resource. With this change it's not called.

Related discussion thread: petkaantonov/bluebird#1472 (comment)

cc @addaleax @vdeturckheim @Qard @Flarna (sorry for such long CC list, but I think all of you might be interested)

Benchmarks

Before this change:

$ ./node benchmark/async_hooks/gc-tracking.js 
async_hooks/gc-tracking.js method="trackingEnabled" n=1000000: 3,338,788.791796874
async_hooks/gc-tracking.js method="trackingDisabled" n=1000000: 79,598,226.36048095

After this change:

$ ./node benchmark/async_hooks/gc-tracking.js 
async_hooks/gc-tracking.js method="trackingEnabled" n=1000000: 73,493,075.92334495
async_hooks/gc-tracking.js method="trackingEnabledWithDestroyHook" n=1000000: 2,497,582.277915421
async_hooks/gc-tracking.js method="trackingDisabled" n=1000000: 79,170,025.7848857

Note: trackingEnabledWithDestroyHook scenario was added as a replacement for the old trackingEnabled scenario.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[puzpuzpuz]

@vdeturckheim is it too much to ask you to take a look at this PR?

[vdeturckheim]

@puzpuzpuz it's on the list rn, but I need a bit more time as I have a lot to do in the next few days.

[puzpuzpuz]

@puzpuzpuz it's on the list rn, but I need a bit more time as I have a lot to do in the next few days.

I didn't know that. Thanks for the clarification!

[vdeturckheim]

We will probably need to rebase this part over https://github.com/nodejs/node/pull/32514/files

[puzpuzpuz]

Thanks for the heads up!

As that PR is now merged, I have rebased this PR and slightly changed this new sentence in the AsyncResource doc, so it's more consistent with #32514.

[nodejs-github-bot]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30474/ (:heavy_check_mark:)

[Flarna]

Landed in 561dda2