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: only set up hooks if used #13177

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@addaleax
Member

addaleax commented May 23, 2017

Splitting this out from #13000 to have it get reviewed on its own. I think it makes sense but I’d like some kind of confirmation. (Motivation was that setting up Promise hooks for async_wrap is likely to have some kind of noticeable performance impact, probably even once we have the internal fields, so we might not want to have that unconditionally.)

@nodejs/diagnostics

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

@addaleax addaleax added this to the 8.0.0 milestone May 23, 2017

@matthewloring matthewloring referenced this pull request May 23, 2017

Closed

async_wrap,src: promise hook integration #13000

3 of 4 tasks complete
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen approved these changes May 24, 2017 edited

LGTM. There are quite a few places where we require('async_hooks'), so this seems like a good optimization.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

Landed in 410b141

Member

addaleax commented May 25, 2017

Landed in 410b141

@addaleax addaleax closed this May 25, 2017

@addaleax addaleax deleted the addaleax:async-hooks-lazy branch May 25, 2017

addaleax added a commit that referenced this pull request May 25, 2017

async_hooks: only set up hooks if used
PR-URL: #13177
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request May 25, 2017

async_hooks: only set up hooks if used
PR-URL: #13177
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request May 28, 2017

async_hooks: only set up hooks if used
PR-URL: #13177
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

assuming don't land, let me know if it needs to be included for future async_hooks backport (in which case switch to lts-watch)

Member

MylesBorins commented Jul 17, 2017

assuming don't land, let me know if it needs to be included for future async_hooks backport (in which case switch to lts-watch)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jul 17, 2017

Member

@MylesBorins I think we’d want to backport most of async_hooks as it is in v8.x/master if we do, not backport the individual changes, so dont-land seems fine to me, just with different semantics? (I am relabelling to lts-watch though, as requested)

Member

addaleax commented Jul 17, 2017

@MylesBorins I think we’d want to backport most of async_hooks as it is in v8.x/master if we do, not backport the individual changes, so dont-land seems fine to me, just with different semantics? (I am relabelling to lts-watch though, as requested)

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Jul 18, 2017

Member

I've removed the lts-watch from this PR and added it to the main AsyncHooks PR #12892.

Member

AndreasMadsen commented Jul 18, 2017

I've removed the lts-watch from this PR and added it to the main AsyncHooks PR #12892.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment