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 stack tracking should not be enabled by default #16180

Closed
ofrobots opened this issue Oct 13, 2017 · 10 comments · Fixed by #16308
Closed

async stack tracking should not be enabled by default #16180

ofrobots opened this issue Oct 13, 2017 · 10 comments · Fixed by #16308
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@ofrobots
Copy link
Contributor

#13870 adds support for async stack tracking whenever the inspector agent is enabled. Async stack traces are awesome for debugging, but doing this by automatically when the inspector agent is active has a lot of problems:

Some problematic scenarios I can think of:

  • User wants to attach DevTools in order to profile their app. Enabling async stack traces add overhead that was not present before the profiler was attached. The profile would not be representative of the original program.
  • User wants to take a heap profile and uses the inspector module to take the heap profile. Async stack tracking gets enabled as a side-effect – something that the user did not want (the user is not 'single step debugging').
  • User wants to use the inspector module to build a passive, in-production debugger. User is not interested in the overhead of async stack tracking.

Async stack tracking should have been an opt in. Whether or not the inspector agent is enabled conflates too many use-cases.

Apologies that I didn't catch this issue earlier.

/cc @nodejs/v8-inspector @bajtos @nodejs/diagnostics
@joyeecheung joyeecheung added the inspector Issues and PRs related to the V8 inspector protocol label Oct 13, 2017
@bajtos
Copy link
Contributor

bajtos commented Oct 13, 2017

@ofrobots good points. IIRC, DevTools have a checkbox to enable/disable async stack traces in the debugger. Under the hood, DevTools send Debugger.setAsyncCallStackDepth with a non-zero value (I think it's always 10) to enable async stack traces in V8 and zero to disable them. When the async call stack depth is 0, the V8 AsyncTask* API calls are effectively no-ops, at least it was this way the last time I checked V8 sources.

So I think the only overhead of enabling async stack traces lies in the overhead of Node.js's async_hooks machinery, which is still likely to change the performance profile of the inspected app as you pointed out.

I think it would be best if Node.js could intercept Debugger.setAsyncCallStackDepth commands send by inspector clients (e.g. DevTools front-end) and enable/disable the async_hook depending on whether the front-end requested async stack traces or not.

Thoughts?

@AndreasMadsen
Copy link
Member

I think it would be best if Node.js could intercept Debugger.setAsyncCallStackDepth commands send by inspector clients (e.g. DevTools front-end) and enable/disable the async_hook depending on whether the front-end requested async stack traces or not.

That would be amazing. But we need to read the asyncCallStackDepth on startup otherwise we won't have the full context. Is that possible?

@TimothyGu
Copy link
Member

Interception is totally possible. Reading it at startup isn't as big of a concern, since right now we allow enabling inspection during runtime and tasks started before starting inspection are not tracked anyway.

@refack
Copy link
Contributor

refack commented Oct 14, 2017

As I mentioned in IRC I would consider this semver-minor.

/cc @jkrems, @roblourens, @billti, @prigara, @ulitink, @segrey

@prigara
Copy link

prigara commented Oct 16, 2017

@refack Thanks for the heads up.
Our current plan is to keep async stack traces enabled by default. Right now WebStorm doesn’t support heap profiling with the new protocol, so it shouldn’t be affected by the issues mentioned in the first comment.
Are there any other known issues related to having async stack tracking enabled?

@ofrobots
Copy link
Contributor Author

@prigara The OP mentions a few cases other than heap profiler (e.g. CPU profiling, passive debugging).

IMO, it is not an option to keep by-default stack tracking. I would like to get this fixed before we reach LTS for Node 8.x. I'm looking into intercepting Debugger.setAsyncCallStackDepth as a mechanism to enable async tracking. Perhaps we can add an additional command line flag that allows folks to enable async tracking on process startup.

@ofrobots
Copy link
Contributor Author

Interception of the Debugger.setAsyncCallStackDepth message is somewhat complicated as it has to be implemented in C++.

With --inspect-brk, the message arrives on the wire way too early (before JavaScript has bootstrapped). This means that the interception function cannot be JavaScript. Having to parse JSON on the C++ side in the inspector Agent is starting to look very clunky.

@refack
Copy link
Contributor

refack commented Oct 16, 2017

Are there any other known issues related to having async stack tracking enabled?

@prigara with the hooks enabled, the debugger steps into the hooks scripts for all of the invocations (before and after async operations), and when stepping through the hook invocation script makes the async stack suddenly appear

2017-10-17-debugasync-

@ofrobots I might be wrong, but I think that if the inspector Agent will send Debugger.setAsyncCallStackDepth only after the client has attached, it might be good enough.

@ofrobots ofrobots mentioned this issue Oct 17, 2017
Closed
2 tasks
@ofrobots
Copy link
Contributor Author

PR: #16260

ofrobots added a commit to ofrobots/node that referenced this issue Oct 17, 2017
@ofrobots
inspector: track async stacks when necessary
50c0bc1 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

Fixes: nodejs#16180
@ofrobots ofrobots mentioned this issue Oct 18, 2017
Merged
2 tasks
ofrobots added a commit to ofrobots/node that referenced this issue Oct 29, 2017
@ofrobots
inspector: track async stacks when necessary
5886e20 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

PR-URL: nodejs#16308
Fixes: nodejs#16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
ofrobots added a commit to ofrobots/node that referenced this issue Oct 31, 2017
@ofrobots
inspector: track async stacks when necessary
9b73ae7 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

PR-URL: nodejs#16308
Fixes: nodejs#16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 31, 2017
@ofrobots @MylesBorins
inspector: track async stacks when necessary
2ea25ad 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

Backport-PR-URL: #16590
PR-URL: #16308
Fixes: #16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
@ofrobots @Qard
inspector: track async stacks when necessary
0ee6ed9 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

PR-URL: nodejs/node#16308
Fixes: nodejs/node#16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
@ofrobots @Qard
inspector: track async stacks when necessary
d979598 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

PR-URL: nodejs/node#16308
Fixes: nodejs/node#16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
@ofrobots @addaleax
inspector: track async stacks when necessary
f39a95e 
With this change, we do async stack tracking only when explicitly
requested by the inspector client. This avoids unnecessary overhead
for clients that might not be interested in async stack traces.

PR-URL: nodejs/node#16308
Fixes: nodejs/node#16180
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@Zorono
Copy link

Zorono commented Oct 29, 2019 •
edited
Loading

excuse me, but i am facing the same error recently when i was trying to inspect/debug my NodeJS Code...

[INSPECTOR_ASYNC_STACK_TRACES_NOT_AVAILABLE] Warning: Async stack traces in debugger are not available on 32bit platforms. The feature is disabled.

what about that Fix (#16308) ?? (really my mind is blowing...)

Node.JS version: v10.16.3
NPM version: v6.12.0
Device Operating System: Windows 7 Pro (platform: x32)
i tried inspecting using --inspect arg and npm i node-inspect package and Visual Studio 2019 which i think is using --inspect(if iamn't wrong about the method it is doing it`) but all of them are showing the same warning...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
8 participants
@ofrobots @refack @AndreasMadsen @prigara @bajtos @TimothyGu @joyeecheung @Zorono