★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/32429Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: move to lazy destroy hook registration in AsyncResource #32429

Closed

Conversation

Copy link
Member

@puzpuzpuz puzpuzpuz commented Mar 22, 2020

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

@nodejs-github-bot nodejs-github-bot added the async_hooks label Mar 22, 2020
@puzpuzpuz
Copy link
Member Author

@puzpuzpuz puzpuzpuz commented Mar 26, 2020

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

@vdeturckheim
Copy link
Member

@vdeturckheim vdeturckheim commented Mar 26, 2020

@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
Copy link
Member Author

@puzpuzpuz puzpuzpuz commented Mar 26, 2020

@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!

@@ -685,6 +685,8 @@ asyncResource.triggerAsyncId();
object is garbage collected. This usually does not need to be set (even if
`emitDestroy` is called manually), unless the resource's `asyncId` is
retrieved and the sensitive API's `emitDestroy` is called with it.
When set to `false`, automatic `emitDestroy` behavior will only take
Copy link
Member

@vdeturckheim vdeturckheim Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@puzpuzpuz puzpuzpuz Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@puzpuzpuz puzpuzpuz force-pushed the async-resource-lazy-destroy branch from 61f8496 to 65a44dd Compare Apr 3, 2020
@Flarna Flarna added the author ready label Apr 3, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 5, 2020

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 5, 2020 •

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

@Flarna
Copy link
Member

@Flarna Flarna commented Apr 6, 2020

Landed in 561dda2

@Flarna Flarna closed this Apr 6, 2020
Flarna pushed a commit that referenced this issue Apr 6, 2020
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
BethGriggs pushed a commit that referenced this issue Apr 7, 2020
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
targos pushed a commit that referenced this issue Apr 12, 2020
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
targos pushed a commit that referenced this issue Apr 22, 2020
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks author ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants