async_hooks: only set up hooks if used - #13177
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. There are quite a few places where we require('async_hooks'), so this seems like a good optimization.
Sorry, something went wrong.
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>
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>
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>
|
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) |
Sorry, something went wrong.
|
@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) |
Sorry, something went wrong.
|
I've removed the lts-watch from this PR and added it to the main AsyncHooks PR #12892. |
Sorry, something went wrong.
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
Affected core subsystem(s)
async_hooks