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

Inspector: Track async stacks only when necessary #16308

Merged
merged 2 commits into from
Oct 29, 2017
Merged

Inspector: Track async stacks only when necessary #16308

merged 2 commits into from
Oct 29, 2017

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 18, 2017 •
edited
Loading

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 ofrobots mentioned this pull request Oct 18, 2017
Closed
2 tasks
@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.

targos
targos reviewed Oct 19, 2017
View reviewed changes
deps/v8/include/v8-version.h Outdated
@@ -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

Stressing
built with VS2017: https://ci.nodejs.org/job/node-stress-single-test/1458/nodes=win2012r2-mp-vs2017/
built with VS2015: https://ci.nodejs.org/job/node-stress-single-test/1459/nodes=win2012r2-mp-vs2015/

@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

Stress on Windows2008 gives 64% fails - https://ci.nodejs.org/job/node-stress-single-test/1460/nodes=win2008r2-mp-vs2017/console

@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/

bnoordhuis
bnoordhuis approved these changes Oct 20, 2017
View reviewed changes
src/inspector_agent.cc Outdated
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
@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 <[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
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@ofrobots @Qard
deps: V8: backport b1cd96e from upstream
f15189d 
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
@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 <[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
@ofrobots @Qard
deps: V8: backport b1cd96e from upstream
08d9c86 
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
@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 <[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
@ofrobots @addaleax
deps: V8: backport b1cd96e from upstream
d8564af 
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
@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 <[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
@ofrobots @targos
deps: V8: backport b1cd96e from upstream
0a3283e 
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
@ofrobots @gibfahn
deps: V8: backport b1cd96e from upstream
1d1975a 
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
@ofrobots @gibfahn
deps: V8: backport b1cd96e from upstream
b150338 
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
@ofrobots @MylesBorins
deps: V8: backport b1cd96e from upstream
55d386c 
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
@ofrobots @gibfahn
deps: V8: backport b1cd96e from upstream
711f344 
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]>
@Zorono Zorono mentioned this pull request Oct 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

@eugeneo eugeneo eugeneo approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

async stack tracking should not be enabled by default
10 participants
@ofrobots @refack @bnoordhuis @eugeneo @jasnell @TimothyGu @targos @MylesBorins @gibfahn @nodejs-github-bot