async_hooks: move to lazy destroy hook registration in AsyncResource#32429
Closed
puzpuzpuz wants to merge 1 commit intonodejs:masterfrom
Closed
async_hooks: move to lazy destroy hook registration in AsyncResource#32429puzpuzpuz wants to merge 1 commit intonodejs:masterfrom
puzpuzpuz wants to merge 1 commit intonodejs:masterfrom
Conversation
Flarna
approved these changes
Mar 24, 2020
Member
Author
|
@vdeturckheim is it too much to ask you to take a look at this PR? |
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. |
Member
Author
I didn't know that. Thanks for the clarification! |
vdeturckheim
approved these changes
Apr 2, 2020
Member
There was a problem hiding this comment.
We will probably need to rebase this part over https://github.com/nodejs/node/pull/32514/files
Member
Author
There was a problem hiding this comment.
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.
61f8496 to
65a44dd
Compare
Collaborator
Member
|
Landed in 561dda2 |
Flarna
pushed a commit
that referenced
this pull request
Apr 6, 2020
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
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
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 22, 2020
PR-URL: #32429 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a check into
AsyncResourceconstructor, so thatregisterDestroyHookonly gets called whenrequireManualDestroyisfalseand there are no activedestroyhooks (previously the check only includedrequireManualDestroyvalue). 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
AsyncLocalStorageAPI which only usesinithooks.Important note. Obviously, this PR changes the behavior in the following edge case: previously when an
AsyncResourcewas created (but not yet destroyed) before enabling the firstdestroyhook, 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:
After this change:
Note:
trackingEnabledWithDestroyHookscenario was added as a replacement for the oldtrackingEnabledscenario.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes