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

[v8.x backport] inspector: Track async stack only when necessary #16590

Closed
wants to merge 2 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 29, 2017

This is the 8.x backport of #16308 as the version of V8 in 8.x is slightly older than master and some changes were necessary.

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

deps:v8, inspector

R= @nodejs/v8 @nodejs/v8-inspector

CI: https://ci.nodejs.org/job/node-test-pull-request/11104/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1025/

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Oct 29, 2017
@ofrobots ofrobots added inspector Issues and PRs related to the V8 inspector protocol dont-land-on-v4.x labels Oct 29, 2017
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

@ofrobots looks like this conflicts with something I just landed, would you mind taking a look?

diff:

<<<<<<< HEAD
  // Enable tracking of async stack traces
  if (!enable_async_hook_function_.IsEmpty()) {
    Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
    auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
    if (result.IsEmpty()) {
      FatalError(
        "node::InspectorAgent::StartIoThread",
        "Cannot enable Inspector's AsyncHook, please report this.");
    }
  }

||||||| parent of 2069010344... inspector: track async stacks when necessary
  // Enable tracking of async stack traces
  if (!enable_async_hook_function_.IsEmpty()) {
    Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
    auto context = parent_env_->context();
    auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
    if (result.IsEmpty()) {
      FatalError(
        "node::InspectorAgent::StartIoThread",
        "Cannot enable Inspector's AsyncHook, please report this.");
    }
  }

=======
>>>>>>> 2069010344... inspector: track async stacks when necessary

@gibfahn gibfahn force-pushed the v8.x-staging branch 7 times, most recently from 97c2301 to ab0d7a6 Compare October 31, 2017 00:16
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]>
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 <[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
Copy link
Contributor Author

@gibfahn Conflicts resolved.

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: #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]>
@MylesBorins
Copy link
Contributor

landed in ab0d7a6...2ea25ad

@ofrobots ofrobots deleted the backport-16308 branch October 31, 2017 03:42
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.

4 participants