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.

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

Inspector: Track async stacks only when necessary #16308

Merged
merged 2 commits into from
Oct 29, 2017

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 18, 2017

This is an alternative to #16260. It depends on a back port of an upstream fix (included), but solves the problem much more elegantly & comprehensively.

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.

In practical terms, this means that we still get async stack tracking right from startup when --inspect-brk is used for debuggers that enable async stacks (DevTools does this by default, and I think WebStorm and others as well).

When using --inspect, we no longer enable async stack tracking by default. The inspector client can request async stack tracking at the time of the connection. Any async resources already created will not have stack tracking. I believe this is the right behavior (see #16180). Users needing async stack tracking right from startup can use --inspect-brk.

Fixes: #16180

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

deps:v8, inspector

/cc @nodejs/v8-inspector
CI: https://ci.nodejs.org/job/node-test-pull-request/10836/ https://ci.nodejs.org/job/node-test-pull-request/10862/ https://ci.nodejs.org/job/node-test-pull-request/10865/ https://ci.nodejs.org/job/node-test-pull-request/11030/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1000/ https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1001/

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 18, 2017
@ofrobots
Copy link
Contributor Author

@nodejs/v8 PTAL as well. The back port adds a vtable entry. This should be ABI compatible by virtue of being the last virtual function.

@@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 6
#define V8_MINOR_VERSION 1
#define V8_BUILD_NUMBER 534
#define V8_PATCH_LEVEL 42
#define V8_PATCH_LEVEL 43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that V8 6.2 has landed, we can update the embedder string instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ofrobots ofrobots added the inspector Issues and PRs related to the V8 inspector protocol label Oct 19, 2017
@ofrobots
Copy link
Contributor Author

Looking at the CI, the added test crashes on Windows VS2017. There is suspicion that it might be related to #15558.

@refack
Copy link
Contributor

refack commented Oct 19, 2017

@ofrobots
Copy link
Contributor Author

Curiously, in the stress test it didn't fail at all. I can reproduce this with ~50% intermittency on a vs2017 build on a local windows machine. I am fairly confident that this is related to #15558. Getting rid of the session.disconnect from the test makes it pass reliably.

@refack
Copy link
Contributor

refack commented Oct 20, 2017

@ofrobots
Copy link
Contributor Author

I have disabled part of the test affected by flakiness from #15558. New CI: https://ci.nodejs.org/job/node-test-pull-request/10865/

if (result.IsEmpty()) {
FatalError(
"node::inspector::Agent::ToggleAsyncHook",
"Cannot toggle Inspector's AsyncHook, please report this.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four space indent (line continuation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

MylesBorins pushed a commit that referenced this pull request Oct 31, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#48705}

PR-URL: nodejs/node#16308
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#48705}

PR-URL: nodejs/node#16308
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#48705}

PR-URL: nodejs/node#16308
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jan 15, 2018
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#48705}

PR-URL: nodejs#16308
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 16, 2018
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#48705}

PR-URL: nodejs#16308
Backport-PR-URL: nodejs#16413
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 22, 2018
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#48705}

PR-URL: nodejs#16308
Backport-PR-URL: nodejs#16413
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit to targos/node that referenced this pull request Feb 7, 2018
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#48705}

PR-URL: nodejs#16308
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
Original commit message:
  [inspector] added V8InspectorClient::maxAsyncCallStackDepthChanged

  [email protected]

  Bug: none
  Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: I0fa10978266feb3c3907ce1f3386ae7a34a33582
  Reviewed-on: https://chromium-review.googlesource.com/726490
  Reviewed-by: Dmitry Gozman <[email protected]>
  Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#48705}

PR-URL: #16308
Backport-PR-URL: #16413
Refs: v8/v8@b1cd96e
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async stack tracking should not be enabled by default
10 participants