feat: use inspector for heap profiles #236

psx95 · 2022-11-26T04:05:40Z

For heap profiling, replaces the native C++ addon with calls to Node's inspector module. This uses the same V8::CpuProfiler under the hood but communicates over the chrome devtools protocol.

A similar PR has been raised to determine viability of using Node's inspector module for time profiling.

Testing

Able to produce heap profiles which can be viewed using the pprof tool.
All unit tests are passing.

Caveats

Although no the stackDepth parameter to the heap profile is still preserved here to avoid breaking changes, this parameter is a no-op now. Stack depth will default to 128. This is based on the native source code of the V8 heap profiler.
The memory usage number seem a bit off when compared to what was generated by the native C++ addon. There are images of flame graphs attached to highlight this difference.

Flame graph generated using the in-built inspector

Flame graph generated using the C++ addon

The graphs look similar, but there are differences, mostly due to the serializeHeapProfile task. When using the in-built inspector, this process takes about 1.55MB, while in the implementation that used C++ addon, this process took 0.81MB.

This accounts for most difference b/w the memory usage reported by the different profilers.

conventional-commit-lint-gcf · 2022-11-26T04:05:47Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

nolanmar511 · 2022-11-28T17:00:22Z

ts/src/heap-profiler-inspector.ts

+
+export function startSamplingHeapProfiler(
+  heapIntervalBytes: number,
+  stackDepth: number


Please remove "stack depth", since it's no longer in use.

nolanmar511 · 2022-11-28T17:02:34Z

ts/src/heap-profiler-inspector.ts

+      {heapIntervalBytes},
+      (err: Error | null): void => {
+        if (err !== null) {
+          console.error(`Error starting heap sampling: ${err}`);


Nothing should be logged within this repo (cloud-profiler-nodejs will log errors occurring; logging anything directly here will either result in things being double logged, or prevent users from being able to control what is logged)

nolanmar511 · 2022-11-28T17:05:10Z

ts/src/heap-profiler.ts

@@ -84,17 +83,17 @@ export function profile(
 * started with different parameters, this throws an error.
 *
 * @param intervalBytes - average number of bytes between samples.
- * @param stackDepth - maximum stack depth for samples collected.
+ * @param stackDepth - maximum stack depth for samples collected. This is currently no-op.
+ *                     Default stack depth of 128 will be used. Kept to avoid making breaking change.


I'd favor making the breaking change here (over keeping behavior which no longer works).

nolanmar511 · 2022-11-28T21:56:44Z

Kokoro system tests have failed with the following (suggesting that maybe the heap profile isn't being written out successfully any more):

+ pprof -filefunctions -top -nodecount=2 heap.pb.gz
+ grep 'busyLoop.*src/busybench.js'
+ tee
heap.pb.gz: parsing profile: empty input file
pprof: failed to fetch any source profiles

aabmass · 2022-12-01T16:29:53Z

ts/src/heap-profiler-inspector.ts

+
+/**
+ * Need to create this interface since the type definitions file for node inspector
+ * at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/inspector.d.ts


You might open an issue in that repo and link it here

Good point, will do that.

aabmass · 2022-12-01T16:33:22Z

ts/src/heap-profiler-inspector.ts

+          result.profile as CompatibleSamplingHeapProfile;
+        resolve(
+          translateToAllocationProfileNode(
+            compatibleHeapProfile.head,


Nit since these are the only two fields in compatibleHeapProfile, just pass it directly here

aabmass · 2022-12-01T16:36:27Z

ts/src/heap-profiler-inspector.ts

+    children: [],
+  };
+
+  const children: AllocationProfileNode[] = new Array<AllocationProfileNode>(


node.children.map() should give the same result without all the boilerplate?

aabmass · 2022-12-01T16:41:02Z

ts/src/heap-profiler-inspector.ts

+  nodeId: number,
+  samples: SamplingHeapProfileSample[]
+): SamplingHeapProfileSample[] {
+  const filtered = samples.filter((sample: SamplingHeapProfileSample) => {


I'd create a map in the outer scope or closure of the recursive function to do this lookup in constant time as this is n^2 now. Check out my PR for cpu time if you don't what i mean

aabmass · 2022-12-01T16:50:03Z

ts/src/heap-profiler-inspector.ts

+  return filtered;
+}
+
+function createAllocationsFromSamplesForNode(


Couldnt this be done once in linear time for all nodes, since the samples already have node ID? Group by node ID then for each group, group by byte sizes being equal?

aabmass · 2022-12-01T18:02:07Z

ts/src/heap-profiler.ts

    );
  }
  heapIntervalBytes = intervalBytes;
-  heapStackDepth = stackDepth;
-  startSamplingHeapProfiler(heapIntervalBytes, heapStackDepth);
+  startSamplingHeapProfiler(heapIntervalBytes, stackDepth);


This function will return before the promise completes as it's fire and forget with this promise, might be why the test is failing

psx95 requested review from aabmass and nolanmar511 November 26, 2022 04:05

psx95 self-assigned this Nov 26, 2022

feat: use inspector for heap profiles

23eab92

psx95 force-pushed the migrate-heap-inspector branch from 75d2335 to 23eab92 Compare November 26, 2022 04:08

nolanmar511 reviewed Nov 28, 2022

View reviewed changes

aabmass reviewed Dec 1, 2022

View reviewed changes

aabmass mentioned this pull request Sep 17, 2024

Pre-built binaries for newer node versions #300

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use inspector for heap profiles #236

feat: use inspector for heap profiles #236

psx95 commented Nov 26, 2022

conventional-commit-lint-gcf bot commented Nov 26, 2022

nolanmar511 Nov 28, 2022

nolanmar511 Nov 28, 2022

nolanmar511 Nov 28, 2022

nolanmar511 commented Nov 28, 2022

aabmass Dec 1, 2022

psx95 Dec 1, 2022

aabmass Dec 1, 2022

aabmass Dec 1, 2022

aabmass Dec 1, 2022

aabmass Dec 1, 2022

aabmass Dec 1, 2022

feat: use inspector for heap profiles #236

Are you sure you want to change the base?

feat: use inspector for heap profiles #236

Conversation

psx95 commented Nov 26, 2022

Testing

Caveats

Flame graph generated using the in-built inspector

Flame graph generated using the C++ addon

conventional-commit-lint-gcf bot commented Nov 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment