Skip to content

Conversation

@ofrobots
Copy link
Contributor

@ofrobotsofrobots 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-botnodejs-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
ContributorAuthor

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

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
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

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

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

@ofrobots
Copy link
ContributorAuthor

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

@ofrobots
Copy link
ContributorAuthor

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

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
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

ofrobots added a commit to ofrobots/node that referenced this pull request Oct 31, 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@{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]>
ofrobots added a commit to ofrobots/node 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. PR-URL: nodejs#16308Fixes: nodejs#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]>
MylesBorins pushed a commit that referenced this pull request Oct 31, 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} Backport-PR-URL: #16590 PR-URL: #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]>
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: #16308Fixes: #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]>
@gibfahngibfahn 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#16308Fixes: 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#16308Fixes: 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#16308Fixes: 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocollib / srcIssues 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

@ofrobots@refack@bnoordhuis@eugeneo@jasnell@TimothyGu@targos@MylesBorins@gibfahn@nodejs-github-bot