test,inspector: add heap allocation tracker test #26089
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
eugeneo
approved these changes
Feb 14, 2019
jasnell
approved these changes
Feb 14, 2019
addaleax
commented
Feb 14, 2019
| // 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? |
There was a problem hiding this comment.
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_); |
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Feb 15, 2019
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
2 tasks
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Feb 15, 2019
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
2 tasks
addaleax
added
blocked
addaleax
added
the
author ready
|
Landed in cfcbfc0 |
addaleax
added a commit
that referenced
this pull request
Feb 20, 2019
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
added a commit
that referenced
this pull request
Feb 21, 2019
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>
Merged
rvagg
pushed a commit
that referenced
this pull request
Feb 28, 2019
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This provides coverage for the
InspectorTimerinstancescreated as part of heap allocation tracking.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes