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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

puzpuzpuz
Copy link
Member

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 Issues and PRs related to the async hooks subsystem. label Mar 22, 2020
@puzpuzpuz
Copy link
Member Author

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

@vdeturckheim
Copy link
Member

@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 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
vdeturckheim approved these changes Apr 2, 2020
View reviewed changes
doc/api/async_hooks.md Outdated
@@ -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

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

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 April 3, 2020 07:09
@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 5, 2020 •
edited by addaleax
Loading

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

@Flarna
Copy link
Member

Flarna commented Apr 6, 2020

Landed in 561dda2

@Flarna Flarna closed this Apr 6, 2020
Flarna pushed a commit that referenced this pull request Apr 6, 2020
@puzpuzpuz @Flarna
async_hooks: move to lazy destroy hook registration in AsyncResource
561dda2 
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@puzpuzpuz @BethGriggs
async_hooks: move to lazy destroy hook registration in AsyncResource
0d9798d 
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
@puzpuzpuz @targos
async_hooks: move to lazy destroy hook registration in AsyncResource
b632231 
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
Merged
targos pushed a commit that referenced this pull request Apr 22, 2020
@puzpuzpuz @targos
async_hooks: move to lazy destroy hook registration in AsyncResource
28abbfd 
PR-URL: #32429
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@targos targos mentioned this pull request Apr 22, 2020
Merged
@puzpuzpuz puzpuzpuz mentioned this pull request Jun 5, 2020
Closed
@puzpuzpuz puzpuzpuz mentioned this pull request Jul 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flarna Flarna Flarna approved these changes

@vdeturckheim vdeturckheim vdeturckheim approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@puzpuzpuz @vdeturckheim @nodejs-github-bot @Flarna