Skip to content

Commit

Permalink
src,test: multiple fixes in heap profile code
Browse files Browse the repository at this point in the history 
Better handling of state in `HeapSnapshotStor` to avoid possible crashes
and race conditions.
Make the test reliable by adding a JS callback to notify Heap Profile
completions.

PR-URL: #92
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
@santigimeno
santigimeno committed Mar 15, 2024
1 parent 78f5eca commit d383711
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 58 deletions.
48 changes: 26 additions & 22 deletions src/nsolid/nsolid_heap_snapshot.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,16 @@ int NSolidHeapSnapshot::StartTrackingHeapObjects(
Snapshot::snapshot_proxy_sig proxy) {
uint64_t thread_id = envinst->thread_id();
nsuv::ns_mutex::scoped_lock lock(&in_progress_heap_snapshots_);
uint64_t profile_id = in_progress_timers_.fetch_add(
1, std::memory_order_relaxed);
uint64_t profile_id =
in_progress_timers_.fetch_add(1, std::memory_order_relaxed);
// We can not trigger this command if there is already a snapshot in progress
auto it = threads_running_snapshots_.emplace(
thread_id,
HeapSnapshotStor{ redacted, true, profile_id, proxy, std::move(data) });
HeapSnapshotStor{ redacted,
kFlagIsTrackingHeapObjects,
profile_id,
proxy,
std::move(data) });
if (it.second == false) {
return UV_EEXIST;
}
Expand Down Expand Up @@ -62,9 +66,8 @@ int NSolidHeapSnapshot::GetHeapSnapshot(SharedEnvInst envinst,
return UV_EEXIST;
}

uint64_t profile_id = in_progress_timers_.fetch_add(
1,
std::memory_order_relaxed);
uint64_t profile_id =
in_progress_timers_.fetch_add(1, std::memory_order_relaxed);

int status = RunCommand(envinst,
CommandType::Interrupt,
Expand All @@ -76,7 +79,7 @@ int NSolidHeapSnapshot::GetHeapSnapshot(SharedEnvInst envinst,
thread_id,
HeapSnapshotStor{
redacted,
false,
kFlagNone,
profile_id,
proxy,
internal::user_data(data, deleter)});
Expand All @@ -90,8 +93,10 @@ int NSolidHeapSnapshot::StopTrackingHeapObjects(SharedEnvInst envinst) {
nsuv::ns_mutex::scoped_lock lock(&in_progress_heap_snapshots_);
auto it = threads_running_snapshots_.find(thread_id);
// Make sure there is a snapshot running
if (it == threads_running_snapshots_.end())
if (it == threads_running_snapshots_.end() ||
!(it->second.flags & kFlagIsTrackingHeapObjects)) {
return UV_ENOENT;
}

int er = RunCommand(envinst,
CommandType::Interrupt,
Expand All @@ -111,7 +116,7 @@ int NSolidHeapSnapshot::StopTrackingHeapObjectsSync(SharedEnvInst envinst) {
HeapSnapshotStor& stor = it->second;
// If this condition is reached. This was called by EnvList::RemoveEnv.
// It wants to stop any pending snapshot w/ tracking heap object.
if (!stor.is_tracking_heapobjects_) {
if (!(stor.flags & kFlagIsTrackingHeapObjects) || stor.flags & kFlagIsDone) {
// If no pending trackers, just do nothing
// There are not peding snapshots with trackers
return UV_ENOENT;
Expand Down Expand Up @@ -142,10 +147,7 @@ int NSolidHeapSnapshot::StopTrackingHeapObjectsSync(SharedEnvInst envinst) {
snapshot->Serialize(&stream);
}

ASSERT_EQ(stor.is_tracking_heapobjects_, true);
profiler->StopTrackingHeapObjects();
stor.is_tracking_heapobjects_ = false;


// At this point, the snapshot is fully serialized
stor.cb(0, snapshot_str.c_str(), stor.data.get());
Expand Down Expand Up @@ -176,7 +178,7 @@ void NSolidHeapSnapshot::stop_tracking_heap_objects(
HeapSnapshotStor& stor = it->second;
// If this condition is reached. This was called by EnvList::RemoveEnv.
// It wants to stop any pending snapshot w/ tracking heap object.
if (!stor.is_tracking_heapobjects_) {
if (!(stor.flags & kFlagIsTrackingHeapObjects) || stor.flags & kFlagIsDone) {
// If no pending trackers, just do nothing
// There are not peding snapshots with trackers
return;
Expand All @@ -191,9 +193,10 @@ void NSolidHeapSnapshot::stop_tracking_heap_objects(

const v8::HeapSnapshot* snapshot = profiler->TakeHeapSnapshot();
if (snapshot == nullptr) {
stor.cb(heap_profiler::HEAP_SNAPSHOT_FAILURE,
std::string(),
stor.data.get());
QueueCallback(snapshot_cb,
thread_id,
heap_profiler::HEAP_SNAPSHOT_FAILURE,
std::string());
} else {
DataOutputStream<uint64_t, v8::HeapSnapshot> stream(
[](std::string snapshot_str, uint64_t* tid) {
Expand All @@ -207,15 +210,15 @@ void NSolidHeapSnapshot::stop_tracking_heap_objects(
snapshot->Serialize(&stream);
}

ASSERT_EQ(stor.is_tracking_heapobjects_, true);
profiler->StopTrackingHeapObjects();
stor.is_tracking_heapobjects_ = false;

// Work around a deficiency in the API. The HeapSnapshot object is const
// but we cannot call HeapProfiler::DeleteAllHeapSnapshots() because that
// invalidates _all_ snapshots, including those created by other tools.
const_cast<v8::HeapSnapshot*>(snapshot)->Delete();
}

stor.flags |= kFlagIsDone;
}

void NSolidHeapSnapshot::start_tracking_heapobjects(
Expand All @@ -231,7 +234,7 @@ void NSolidHeapSnapshot::start_tracking_heapobjects(
ASSERT(it != snapshotter->threads_running_snapshots_.end());

HeapSnapshotStor& stor = it->second;
ASSERT_EQ(stor.is_tracking_heapobjects_, true);
ASSERT(stor.flags & kFlagIsTrackingHeapObjects);

envinst->isolate()->GetHeapProfiler()->StartTrackingHeapObjects(
trackAllocations);
Expand Down Expand Up @@ -266,7 +269,7 @@ void NSolidHeapSnapshot::take_snapshot_timer(SharedEnvInst envinst,
return;
}

ASSERT_EQ(stor.is_tracking_heapobjects_, true);
ASSERT(stor.flags & kFlagIsTrackingHeapObjects);

// Give control back to the V8 thread
int er = RunCommand(envinst,
Expand Down Expand Up @@ -320,16 +323,17 @@ void NSolidHeapSnapshot::take_snapshot(SharedEnvInst envinst,
}

// A snapshot requested via `StopTrackingHeapObjects` or timer
if (stor.is_tracking_heapobjects_) {
if (stor.flags & kFlagIsTrackingHeapObjects) {
profiler->StopTrackingHeapObjects();
stor.is_tracking_heapobjects_ = false;
}

// Work around a deficiency in the API. The HeapSnapshot object is const
// but we cannot call HeapProfiler::DeleteAllHeapSnapshots() because that
// invalidates _all_ snapshots, including those created by other tools.
const_cast<v8::HeapSnapshot*>(snapshot)->Delete();
}

stor.flags |= kFlagIsDone;
}

void NSolidHeapSnapshot::snapshot_cb(uint64_t thread_id,
Expand Down
8 changes: 7 additions & 1 deletion src/nsolid/nsolid_heap_snapshot.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,15 @@ namespace nsolid {

class NSolidHeapSnapshot {
public:
enum HeapSnapshotFlags {
kFlagNone = 0,
kFlagIsTrackingHeapObjects = 1 << 0,
kFlagIsDone = 1 << 1
};

struct HeapSnapshotStor {
bool redacted;
bool is_tracking_heapobjects_;
uint32_t flags;
uint64_t snapshot_id;
Snapshot::snapshot_proxy_sig cb;
internal::user_data data;
Expand Down
99 changes: 84 additions & 15 deletions test/addons/nsolid-track-heap-objects/binding.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,77 @@
#include <assert.h>
#include <map>

using v8::Context;
using v8::Function;
using v8::FunctionCallbackInfo;
using v8::Global;
using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::NewStringType;
using v8::Number;
using v8::String;
using v8::Value;

std::map<uint64_t, std::string> snapshots;

static void got_snapshot(int status,
std::string snapshot,
uint64_t thread_id) {
assert(status == 0);
snapshots[thread_id] += snapshot;
struct Stor {
std::string profile;
Global<Function> cb;
};

std::map<uint64_t, Stor> profiles;
uv_mutex_t profiles_map_lock;

static void profile_cb(node::nsolid::SharedEnvInst envinst, int status) {
uint64_t thread_id = node::nsolid::GetThreadId(envinst);
uv_mutex_lock(&profiles_map_lock);
auto it = profiles.find(thread_id);
assert(it != profiles.end());
Isolate* isolate = Isolate::GetCurrent();
HandleScope scope(isolate);
Local<Context> context = isolate->GetCurrentContext();
Context::Scope context_scope(context);
Local<Function> cb = it->second.cb.Get(isolate);
Local<Value> argv[] = {
Integer::New(isolate, status),
String::NewFromUtf8(
isolate,
it->second.profile.c_str(),
NewStringType::kNormal).ToLocalChecked()
};

profiles.erase(it);
uv_mutex_unlock(&profiles_map_lock);
cb->Call(context, Undefined(isolate), 2, argv);
}

static void started_profiler(int status, std::string json, uint64_t thread_id) {
static void got_profile(int status,
std::string profile,
uint64_t thread_id) {
assert(status == 0);
uv_mutex_lock(&profiles_map_lock);
auto it = profiles.find(thread_id);
assert(it != profiles.end());
if (profile.empty()) {
if (it->second.cb.IsEmpty()) {
profiles.erase(it);
} else {
node::nsolid::SharedEnvInst envinst = node::nsolid::GetEnvInst(thread_id);
assert(0 == node::nsolid::RunCommand(envinst,
node::nsolid::CommandType::EventLoop,
profile_cb,
status));
}
} else {
it->second.profile += profile;
}

uv_mutex_unlock(&profiles_map_lock);
}

static void StartTrackingHeapObjectsBinding(
const FunctionCallbackInfo<Value>& args) {
v8::HandleScope handle_scope(args.GetIsolate());
HandleScope handle_scope(args.GetIsolate());
// thread_id
assert(args[0]->IsUint32());
// Redacted heap snapshot
Expand All @@ -35,25 +85,45 @@ static void StartTrackingHeapObjectsBinding(
assert(args[2]->IsBoolean());
// Stop after this many milliseconds
assert(args[3]->IsNumber());
if (args.Length() > 4) {
assert(args[4]->IsFunction());
}

uint64_t thread_id = args[0].As<Number>()->Value();
bool redacted = args[1]->BooleanValue(args.GetIsolate());
bool track_allocations = args[2]->BooleanValue(args.GetIsolate());
uint64_t duration = static_cast<uint64_t>(
args[3]->NumberValue(args.GetIsolate()->GetCurrentContext()).FromJust());

uv_mutex_lock(&profiles_map_lock);
auto pair = profiles.emplace(thread_id, Stor());
if (args.Length() > 4) {
pair.first->second.cb.Reset(args.GetIsolate(), args[4].As<Function>());
}
uv_mutex_unlock(&profiles_map_lock);

int ret = node::nsolid::Snapshot::StartTrackingHeapObjects(
node::nsolid::GetEnvInst(thread_id),
redacted,
track_allocations,
duration,
started_profiler,
got_profile,
thread_id);
if (ret != 0) {
if (pair.second) {
uv_mutex_lock(&profiles_map_lock);
profiles.erase(thread_id);
uv_mutex_unlock(&profiles_map_lock);
}
} else {
assert(pair.second);
}

args.GetReturnValue().Set(Integer::New(args.GetIsolate(), ret));
}

static void StopTrackingHeapObjects(const FunctionCallbackInfo<Value>& args) {
v8::HandleScope handle_scope(args.GetIsolate());
HandleScope handle_scope(args.GetIsolate());
// thread_id
assert(args[0]->IsUint32());

Expand All @@ -66,7 +136,7 @@ static void StopTrackingHeapObjects(const FunctionCallbackInfo<Value>& args) {

static void StopTrackingHeapObjectsSync(
const FunctionCallbackInfo<Value>& args) {
v8::HandleScope handle_scope(args.GetIsolate());
HandleScope handle_scope(args.GetIsolate());
// thread_id
assert(args[0]->IsUint32());

Expand All @@ -78,9 +148,7 @@ static void StopTrackingHeapObjectsSync(
}

static void at_exit_cb() {
for (const auto& pair : snapshots) {
assert(pair.second.size() > 0);
}
uv_mutex_destroy(&profiles_map_lock);
}

NODE_MODULE_INIT(/* exports, module, context */) {
Expand All @@ -91,6 +159,7 @@ NODE_MODULE_INIT(/* exports, module, context */) {
exports, "stopTrackingHeapObjectsSync", StopTrackingHeapObjectsSync);
node::nsolid::SharedEnvInst envinst = node::nsolid::GetLocalEnvInst(context);
if (node::nsolid::IsMainThread(envinst)) {
assert(0 == uv_mutex_init(&profiles_map_lock));
atexit(at_exit_cb);
}
}
Loading

0 comments on commit d383711

Please sign in to comment.