test,inspector: add heap allocation tracker test by addaleax · Pull Request #26089 · nodejs/node · GitHub

This provides coverage for the InspectorTimer instances
created as part of heap allocation tracking.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2597/pipeline

This provides coverage for the `InspectorTimer` instances created as part of heap allocation tracking.

CI: https://ci.nodejs.org/job/node-test-pull-request/20777/

Btw, @eugeneo @nodejs/v8 any ideas for what to do about this?

I’ll try to look into the Windows failures (unless they are the same as what @joyeecheung saw in #26006 and she already has something figured out?)

Edit: Possible diff for addressing this:

diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc
index 1bcf65134f..25b6270e41 100644
--- a/src/inspector/main_thread_interface.cc
+++ b/src/inspector/main_thread_interface.cc
@@ -224,11 +224,6 @@ MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop,
                                          v8::Platform* platform)
                                          : agent_(agent), isolate_(isolate),
                                            platform_(platform) {
-  main_thread_request_.reset(new AsyncAndInterface(uv_async_t(), this));
-  CHECK_EQ(0, uv_async_init(loop, &main_thread_request_->first,
-                            DispatchMessagesAsyncCallback));
-  // Inspector uv_async_t should not prevent main loop shutdown.
-  uv_unref(reinterpret_cast<uv_handle_t*>(&main_thread_request_->first));
 }

 MainThreadInterface::~MainThreadInterface() {
@@ -253,7 +248,6 @@ void MainThreadInterface::Post(std::unique_ptr<Request> request) {
   bool needs_notify = requests_.empty();
   requests_.push_back(std::move(request));
   if (needs_notify) {
-    CHECK_EQ(0, uv_async_send(&main_thread_request_->first));
     if (isolate_ != nullptr && platform_ != nullptr) {
       std::shared_ptr<v8::TaskRunner> taskrunner =
         platform_->GetForegroundTaskRunner(isolate_);
diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h
index a7d9f8a3c9..1c644144cc 100644
--- a/src/inspector/main_thread_interface.h
+++ b/src/inspector/main_thread_interface.h
@@ -105,7 +105,6 @@ class MainThreadInterface {
   Agent* const agent_;
   v8::Isolate* const isolate_;
   v8::Platform* const platform_;
-  DeleteFnPtr<AsyncAndInterface, CloseAsync> main_thread_request_;
   std::shared_ptr<MainThreadHandle> handle_;
   std::unordered_map<int, std::unique_ptr<Deletable>> managed_objects_;
 };
diff --git a/src/node_worker.cc b/src/node_worker.cc
index f38b187c18..feda1d7210 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -150,7 +150,8 @@ class WorkerThreadData {

     isolate->Dispose();

-    // Need to run the loop one more time to close the platform's uv_async_t
+    // Need to run the loop twice to close the platform's uv_async_t
+    uv_run(&loop_, UV_RUN_ONCE);
     uv_run(&loop_, UV_RUN_ONCE);

     CheckedUvLoopClose(&loop_);

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: nodejs#26089 Refs: nodejs#26006

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

New CI: https://ci.nodejs.org/job/node-test-pull-request/20846/

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20889/

Landed in cfcbfc0

This provides coverage for the `InspectorTimer` instances created as part of heap allocation tracking. PR-URL: #26089 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

This provides coverage for the `InspectorTimer` instances created as part of heap allocation tracking. PR-URL: #26089 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax requested a review from eugeneo February 14, 2019 00:25

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 14, 2019

test,inspector: add heap allocation tracker test

9397b12

This provides coverage for the `InspectorTimer` instances created as part of heap allocation tracking.

addaleax force-pushed the inspector-heap-alloc-track-test branch from 9362d0d to 9397b12 Compare February 14, 2019 00:26

addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Feb 14, 2019

eugeneo approved these changes Feb 14, 2019

View reviewed changes

jasnell approved these changes Feb 14, 2019

View reviewed changes

addaleax commented Feb 14, 2019

View reviewed changes

addaleax mentioned this pull request Feb 15, 2019

src: remove inspector main_thread_request_ field #26137

Closed

2 tasks

addaleax added a commit to addaleax/node that referenced this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop

06e09f7

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

addaleax mentioned this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop #26138

Closed

2 tasks

addaleax added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 16, 2019

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 19, 2019

addaleax closed this Feb 20, 2019

addaleax deleted the inspector-heap-alloc-track-test branch February 20, 2019 15:59

BridgeAR mentioned this pull request Feb 26, 2019

v11.11.0 proposal #26322

Merged

+                // TODO(addaleax): Using `{ reportProgress: true }` crashes the process
+                // because the progress indication event would mean calling into JS while
+                // a heap snapshot is being taken, which is forbidden.
+                // What can we do about that?

test,inspector: add heap allocation tracker test by addaleax · Pull Request #26089 · nodejs/node · GitHub

Uh oh!

Uh oh!

test,inspector: add heap allocation tracker test - #26089

test,inspector: add heap allocation tracker test#26089
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:inspector-heap-alloc-track-test

addaleax commented Feb 14, 2019

Uh oh!

nodejs-github-bot commented Feb 14, 2019

Uh oh!

addaleax commented Feb 14, 2019

Uh oh!

addaleax Feb 14, 2019

Uh oh!

addaleax commented Feb 15, 2019 •
edited
Loading

Uh oh!

Uh oh!

addaleax commented Feb 17, 2019

Uh oh!

addaleax commented Feb 19, 2019

Uh oh!

addaleax commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

test,inspector: add heap allocation tracker test by addaleax · Pull Request #26089 · nodejs/node · GitHub

Uh oh!

Uh oh!

test,inspector: add heap allocation tracker test - #26089

test,inspector: add heap allocation tracker test#26089addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:inspector-heap-alloc-track-testaddaleax/node:inspector-heap-alloc-track-testCopy head branch name to clipboard

Conversation

addaleax commented Feb 14, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 14, 2019

Uh oh!

addaleax commented Feb 14, 2019

Uh oh!

addaleax Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 17, 2019

Uh oh!

addaleax commented Feb 19, 2019

Uh oh!

addaleax commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

test,inspector: add heap allocation tracker test#26089
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:inspector-heap-alloc-track-test

addaleax commented Feb 15, 2019 •
edited
Loading