Revert lazy profile-tree changes to mitigate CpuProfile::Delete crash (#329)

* Revert "use stop and collect on time profiler (#305)"

This reverts commit 85f2457.

* Revert "Switch heap profiling to use lazy allocation profile method by default (#281)"

This reverts commit fb3d75d.

* Touchup for TS6 changes introduced after the reverted commits.

Specifically enforcing TS2883 under `composite: true`: any
function whose inferred return type names a non-imported
type must be annotated explicitly.

Author: szegedi

README.md

@@ -97,14 +97,10 @@ Install [`pprof`][npm-url] with `npm` or add to your `package.json`.
         pprof -http=: heap.pb.gz
         ```
 
-    * Collecting a heap profile with V8 allocation profile format:
+    * Collecting a heap profile with  V8 allocation profile format:
         ```javascript
-          const profile = pprof.heap.v8Profile(pprof.heap.convertProfile);
+          const profile = await pprof.heap.v8Profile();
         ```
-        `v8Profile` accepts a callback and returns its result. Allocation nodes
-        are only valid during the callback, so copy/transform what you need
-        before returning. `heap.convertProfile` performs that conversion during
-        the callback, and `heap.profile()` uses it under the hood.
 
 [build-image]: https://github.com/Datadog/pprof-nodejs/actions/workflows/build.yml/badge.svg?branch=main
 [build-url]: https://github.com/Datadog/pprof-nodejs/actions/workflows/build.yml

bindings/profilers/heap.cc

@@ -488,6 +488,23 @@ NAN_METHOD(HeapProfiler::StopSamplingHeapProfiler) {
   }
 }
 
+// Signature:
+// getAllocationProfile(): AllocationProfileNode
+NAN_METHOD(HeapProfiler::GetAllocationProfile) {
+  auto isolate = info.GetIsolate();
+  std::unique_ptr<v8::AllocationProfile> profile(
+      isolate->GetHeapProfiler()->GetAllocationProfile());
+  if (!profile) {
+    return Nan::ThrowError("Heap profiler is not enabled.");
+  }
+  v8::AllocationProfile::Node* root = profile->GetRootNode();
+  auto state = PerIsolateData::For(isolate)->GetHeapProfilerState();
+  if (state) {
+    state->OnNewProfile();
+  }
+  info.GetReturnValue().Set(TranslateAllocationProfile(root));
+}
+
 // mapAllocationProfile(callback): callback result
 NAN_METHOD(HeapProfiler::MapAllocationProfile) {
   if (info.Length() < 1 || !info[0]->IsFunction()) {
@@ -579,6 +596,7 @@ NAN_MODULE_INIT(HeapProfiler::Init) {
       heapProfiler, "startSamplingHeapProfiler", StartSamplingHeapProfiler);
   Nan::SetMethod(
       heapProfiler, "stopSamplingHeapProfiler", StopSamplingHeapProfiler);
+  Nan::SetMethod(heapProfiler, "getAllocationProfile", GetAllocationProfile);
   Nan::SetMethod(heapProfiler, "mapAllocationProfile", MapAllocationProfile);
   Nan::SetMethod(heapProfiler, "monitorOutOfMemory", MonitorOutOfMemory);
   Nan::Set(target,

bindings/profilers/heap.hh

@@ -30,6 +30,10 @@ class HeapProfiler {
   // stopSamplingHeapProfiler()
   static NAN_METHOD(StopSamplingHeapProfiler);
 
+  // Signature:
+  // getAllocationProfile(): AllocationProfileNode
+  static NAN_METHOD(GetAllocationProfile);
+
   // Signature:
   // mapAllocationProfile(callback): callback result
   static NAN_METHOD(MapAllocationProfile);

bindings/profilers/wall.cc

@@ -929,6 +929,28 @@ v8::ProfilerId WallProfiler::StartInternal() {
   return result.id;
 }
 
+NAN_METHOD(WallProfiler::Stop) {
+  if (info.Length() != 1) {
+    return Nan::ThrowTypeError("Stop must have one argument.");
+  }
+  if (!info[0]->IsBoolean()) {
+    return Nan::ThrowTypeError("Restart must be a boolean.");
+  }
+
+  bool restart = info[0].As<Boolean>()->Value();
+
+  WallProfiler* wallProfiler =
+      Nan::ObjectWrap::Unwrap<WallProfiler>(info.This());
+
+  v8::Local<v8::Value> profile;
+  auto err = wallProfiler->StopImpl(restart, profile);
+
+  if (!err.success) {
+    return Nan::ThrowTypeError(err.msg.c_str());
+  }
+  info.GetReturnValue().Set(profile);
+}
+
 // stopAndCollect(restart, callback): callback result
 NAN_METHOD(WallProfiler::StopAndCollect) {
   if (info.Length() != 2) {
@@ -1087,6 +1109,20 @@ Result WallProfiler::StopCore(bool restart, ProfileBuilder&& buildProfile) {
   return {};
 }
 
+Result WallProfiler::StopImpl(bool restart, v8::Local<v8::Value>& profile) {
+  return StopCore(restart,
+                  [&](const v8::CpuProfile* v8_profile,
+                      bool hasCpuTime,
+                      int64_t nonJSThreadsCpuTime,
+                      ContextsByNode* contextsByNodePtr) {
+                    profile = TranslateTimeProfile(v8_profile,
+                                                   includeLines_,
+                                                   contextsByNodePtr,
+                                                   hasCpuTime,
+                                                   nonJSThreadsCpuTime);
+                  });
+}
+
 Result WallProfiler::StopAndCollectImpl(bool restart,
                                         v8::Local<v8::Function> callback,
                                         v8::Local<v8::Value>& result) {
@@ -1121,6 +1157,7 @@ NAN_MODULE_INIT(WallProfiler::Init) {
                    SetContext);
 
   Nan::SetPrototypeMethod(tpl, "start", Start);
+  Nan::SetPrototypeMethod(tpl, "stop", Stop);
   Nan::SetPrototypeMethod(tpl, "stopAndCollect", StopAndCollect);
   Nan::SetPrototypeMethod(tpl, "dispose", Dispose);
   Nan::SetPrototypeMethod(tpl,

bindings/profilers/wall.hh

@@ -157,6 +157,7 @@ class WallProfiler : public Nan::ObjectWrap {
   v8::ProfilerId StartInternal();
   template <typename ProfileBuilder>
   Result StopCore(bool restart, ProfileBuilder&& buildProfile);
+  Result StopImpl(bool restart, v8::Local<v8::Value>& profile);
   Result StopAndCollectImpl(bool restart,
                             v8::Local<v8::Function> callback,
                             v8::Local<v8::Value>& result);
@@ -188,6 +189,7 @@ class WallProfiler : public Nan::ObjectWrap {
 
   static NAN_METHOD(New);
   static NAN_METHOD(Start);
+  static NAN_METHOD(Stop);
   static NAN_METHOD(StopAndCollect);
   static NAN_METHOD(V8ProfilerStuckEventLoopDetected);
   static NAN_METHOD(Dispose);

bindings/translate-heap-profile.cc

@@ -41,6 +41,30 @@ class HeapProfileTranslator : ProfileTranslator {
 #undef X
 
  public:
+  v8::Local<v8::Value> TranslateAllocationProfile(
+      v8::AllocationProfile::Node* node) {
+    v8::Local<v8::Array> children = NewArray(node->children.size());
+    for (size_t i = 0; i < node->children.size(); i++) {
+      Set(children, i, TranslateAllocationProfile(node->children[i]));
+    }
+
+    v8::Local<v8::Array> allocations = NewArray(node->allocations.size());
+    for (size_t i = 0; i < node->allocations.size(); i++) {
+      auto alloc = node->allocations[i];
+      Set(allocations,
+          i,
+          CreateAllocation(NewNumber(alloc.count), NewNumber(alloc.size)));
+    }
+
+    return CreateNode(node->name,
+                      node->script_name,
+                      NewInteger(node->script_id),
+                      NewInteger(node->line_number),
+                      NewInteger(node->column_number),
+                      children,
+                      allocations);
+  }
+
   v8::Local<v8::Value> TranslateAllocationProfile(Node* node) {
     v8::Local<v8::Array> children = NewArray(node->children.size());
     for (size_t i = 0; i < node->children.size(); i++) {
@@ -118,6 +142,11 @@ std::shared_ptr<Node> TranslateAllocationProfileToCpp(
   return new_node;
 }
 
+v8::Local<v8::Value> TranslateAllocationProfile(
+    v8::AllocationProfile::Node* node) {
+  return HeapProfileTranslator().TranslateAllocationProfile(node);
+}
+
 v8::Local<v8::Value> TranslateAllocationProfile(Node* node) {
   return HeapProfileTranslator().TranslateAllocationProfile(node);
 }

bindings/translate-heap-profile.hh

@@ -39,4 +39,7 @@ std::shared_ptr<Node> TranslateAllocationProfileToCpp(
     v8::AllocationProfile::Node* node);
 
 v8::Local<v8::Value> TranslateAllocationProfile(Node* node);
+v8::Local<v8::Value> TranslateAllocationProfile(
+    v8::AllocationProfile::Node* node);
+
 }  // namespace dd

ts/src/heap-profiler-bindings.ts

@@ -37,6 +37,10 @@ export function stopSamplingHeapProfiler() {
   profiler.heapProfiler.stopSamplingHeapProfiler();
 }
 
+export function getAllocationProfile(): AllocationProfileNode {
+  return profiler.heapProfiler.getAllocationProfile();
+}
+
 export function mapAllocationProfile<T>(
   callback: (root: AllocationProfileNode) => T,
 ): T {

ts/src/heap-profiler.ts

@@ -17,6 +17,7 @@
 import {Profile} from 'pprof-format';
 
 import {
+  getAllocationProfile,
   mapAllocationProfile,
   startSamplingHeapProfiler,
   stopSamplingHeapProfiler,
@@ -33,6 +34,20 @@ import {isMainThread} from 'worker_threads';
 let enabled = false;
 let heapIntervalBytes = 0;
 let heapStackDepth = 0;
+
+/*
+ * Collects a heap profile when heapProfiler is enabled. Otherwise throws
+ * an error.
+ *
+ * Data is returned in V8 allocation profile format.
+ */
+export function v8Profile(): AllocationProfileNode {
+  if (!enabled) {
+    throw new Error('Heap profiler is not enabled.');
+  }
+  return getAllocationProfile();
+}
+
 /**
  * Collects a heap profile when heapProfiler is enabled. Otherwise throws
  * an error.
@@ -44,13 +59,35 @@ let heapStackDepth = 0;
  * @param callback - function to convert the heap profiler to a converted profile
  * @returns <T> converted profile
  */
-export function v8Profile<T>(callback: (root: AllocationProfileNode) => T): T {
+export function v8ProfileV2<T>(
+  callback: (root: AllocationProfileNode) => T,
+): T {
   if (!enabled) {
     throw new Error('Heap profiler is not enabled.');
   }
   return mapAllocationProfile(callback);
 }
 
+/**
+ * Collects a profile and returns it serialized in pprof format.
+ * Throws if heap profiler is not enabled.
+ *
+ * @param ignoreSamplePath
+ * @param sourceMapper
+ */
+export function profile(
+  ignoreSamplePath?: string,
+  sourceMapper?: SourceMapper,
+  generateLabels?: GenerateAllocationLabelsFunction,
+): Profile {
+  return convertProfile(
+    v8Profile(),
+    ignoreSamplePath,
+    sourceMapper,
+    generateLabels,
+  );
+}
+
 export function convertProfile(
   rootNode: AllocationProfileNode,
   ignoreSamplePath?: string,
@@ -93,12 +130,12 @@ export function convertProfile(
  * @param sourceMapper
  * @param generateLabels
  */
-export function profile(
+export function profileV2(
   ignoreSamplePath?: string,
   sourceMapper?: SourceMapper,
   generateLabels?: GenerateAllocationLabelsFunction,
 ): Profile {
-  return v8Profile(root => {
+  return v8ProfileV2(root => {
     return convertProfile(root, ignoreSamplePath, sourceMapper, generateLabels);
   });
 }

ts/src/index.ts

@@ -34,6 +34,7 @@ export const time = {
   profile: timeProfiler.profile,
   start: timeProfiler.start,
   stop: timeProfiler.stop,
+  profileV2: timeProfiler.profileV2,
   getContext: timeProfiler.getContext,
   setContext: timeProfiler.setContext,
   runWithContext: timeProfiler.runWithContext,
@@ -49,6 +50,7 @@ export const heap = {
   start: heapProfiler.start,
   stop: heapProfiler.stop,
   profile: heapProfiler.profile,
+  profileV2: heapProfiler.profileV2,
   convertProfile: heapProfiler.convertProfile,
   v8Profile: heapProfiler.v8Profile,
   monitorOutOfMemory: heapProfiler.monitorOutOfMemory,