Skip to content

async_hooks: move to lazy destroy hook registration in AsyncResource

Rodrigo Muino Tomonari requested to merge github/fork/puzpuzpuz/async-resource-lazy-destroy into master

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: https://github.com/petkaantonov/bluebird/pull/1472#issuecomment-352104689

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

Merge request reports