Add import of v8 heap allocation profile #170

vmarchaud · 2018-10-04T12:55:47Z

First PR here, but first thanks for the awesome tool !

We plan to integrate it into our product (https://pm2.io/) to visualize all kind of profile that we can take out of NodeJS, for this we needed to be able to render heap allocation that are done by V8 so here it is !

Sorry for not opening an issue first, we just wanted to see if we were able to implement it before bothering you

Regards

coveralls · 2018-10-04T13:00:57Z

Coverage increased (+0.6%) to 44.482% when pulling b0d4589 on vmarchaud:vmarchaud/import-v8-heap into d78d20e on jlfwong:master.

jlfwong

Wooooaaaah cool! Thanks for doing this and nice work figuring out where to put the necessary code!

Before this can land, in addition to the code review comments that need resolving, I need a few things:

Instructions for how to record a heap allocation profile like this to be placed into the PR description (I'll use that to update the wiki page: https://github.com/jlfwong/speedscope/wiki/Importing-from-Node.js)
A minimal checked in example of a heap allocation profile placed in this directory within a subdirectory for the relevant node version https://github.com/jlfwong/speedscope/tree/master/sample/profiles/node
Leveraging the checked in allocation profile in 2, a test which imports the profile and asserts a snapshot of the contents, like this: https://github.com/jlfwong/speedscope/blob/master/src/import/v8proflog.test.ts

I also don't use yarn for this project, so if you could please remove the yarn.lock file from this PR, that would be much appreciated!

jlfwong · 2018-10-04T16:03:45Z

src/import/index.ts

@@ -103,6 +104,9 @@ async function _importProfileGroup(dataSource: ProfileDataSource): Promise<Profi
  } else if (fileName.endsWith('.v8log.json')) {
    console.log('Importing as --prof-process v8 log')
    return toGroup(importFromV8ProfLog(JSON.parse(contents)))
+  } else if (fileName.endsWith('.heapprofile')) {


I try to make every kind of profile import even if it has the incorrect extension by using heuristics. Can you please add a sensible heuristic to the list below for the second pass?

Maybe something like:

'head' in parsed && 'callFrame' in parsed['head']

I went for 'head' in parsed && 'selfSize' in parsed['head'] because it's more specific to the heap profile format

jlfwong · 2018-10-04T16:05:15Z

src/import/v8heapalloc.ts

+  }
+  computeId(chromeProfile.head)
+
+  // compute the total size


A small stylistic thing: I try to start all my comments with a capitalized letter

jlfwong · 2018-10-04T16:08:37Z

src/import/v8heapalloc.ts

@@ -0,0 +1,97 @@
+import {Profile, FrameInfo, StackListProfileBuilder} from '../lib/profile'


Is there a URL you can add to a comment here that provides documentation on this file format? Or at least a link that explains how to record this kind of profile?

jlfwong · 2018-10-04T16:10:50Z

src/import/v8heapalloc.ts

+  const total = computeTotalSize(chromeProfile.head)
+
+  // compute all stacks by taking each last node and going upward
+  const stacks: HeapProfileNode[][] = Array.from(nodeById.values()).map(currentNode => {


It's a pretty small thing, but I generally use for loops rather than map for a slight performance boost. (https://jsperf.com/map-vs-for-loop-performance/6)

So something like:

const stacks: HeapProfileNode[][] = [] for (let currentNode of nodeById.values()) { ... }

If I'm misunderstanding the perf implications here, please let me know!

I personally prefer to use native methods of Array for readability but i switched the logic to use for as requested

jlfwong · 2018-10-04T16:12:58Z

src/import/v8heapalloc.ts

+    stack.push(currentNode)
+    // while we found a parent
+    while (true) {
+      if (typeof currentNode.parent !== 'number') break


I think this is correct, but I found it a bit misleading to read. I think what this is checking is if the parent is undefined or not, but it shouldn't be possible or parent to be any type other than number or undefined. I think the idiomatic way of doing that would be:

if (currentParent.parent == null) break

(intentionally using == so that it captures both undefined and null, but still not 0, because JavaScript is a craaazy language)

true, i switched to a null check but with a strict comparaison (see my comment bellow)

jlfwong · 2018-10-04T16:13:54Z

src/import/v8heapalloc.ts

+    while (true) {
+      if (typeof currentNode.parent !== 'number') break
+      const parent = nodeById.get(currentNode.parent)
+      if (parent === undefined) break


In this case I would just do if (!parent) break, since any node returned by nodeById.get(...) will not be falsy. if (parent != null) would also be fine.

Performance wise, it's better to check against the type you need than relying on V8 adding the check for 0/null/undefined for you
Since any Map return undefined if no entry is found, i believe we can check only for undefined

jlfwong · 2018-10-04T16:17:37Z

src/import/v8heapalloc.ts

+      const parent = nodeById.get(currentNode.parent)
+      if (parent === undefined) break
+      // push the parent at the begining of the stack
+      stack.unshift(parent)


Weird! I would've expected doing repeated stack.push(parent) followed by a stack.reverse() at the end to be more efficient than doing repeated unshift calls, but it looks like that's wrong? https://jsperf.com/array-unshift-vs-array-push-and-array-reverse. I would've thought that arrays would be implemented as vectors, making unshift an O(n) operation, but it looks like it's optimized really well somehow.

jlfwong · 2018-10-04T16:18:54Z

src/import/v8heapalloc.ts

+      if (typeof currentNode.parent !== 'number') break
+      const parent = nodeById.get(currentNode.parent)
+      if (parent === undefined) break
+      // push the parent at the begining of the stack


typo: "beginning"

jlfwong · 2018-10-04T16:22:59Z

src/import/v8heapalloc.ts

+
+  const profile = new StackListProfileBuilder(total)
+
+  for (let i = 0; i < stacks.length; i++) {


I think you can just do for (let stack of stacks) { ... } here. TypeScript will compile it down to roughly the same thing, and it doesn't look like you need the index i anywhere else in the loop.

vmarchaud · 2018-10-05T09:50:49Z

First round of fix, i will the documentation/test later in the day

vmarchaud · 2018-10-05T12:41:54Z

Importing a Heap Profile from the Chrome inspector

Like the CPU Profiling, you can use the Chrome Inspector to record an heap allocation profile, just launch your process with the --inspect flag and then go the Memory section :

Click the start button to start recording a profile.

Now, use your application like you want to and click on the Stop button.

Click on "save" to save the recorded profile. Save it with the default .heapprofile file extension.

You should be able to open that file in https://www.speedscope.app/.

To view it offline, you'll need to install speedscope via npm:

npm install -g speedscope

Then you should be able to open it like so:

speedscope /path/to/profile.heapprofile

vmarchaud · 2018-10-05T13:20:42Z

I'm not sure to understand why the test are failing, the CI only log :

This is likely because this test is run in a continuous integration (CI) environment in which snapshots are not written by default.

Do you have any idea why ?

jlfwong

Sweet! I think all that's left is to run npm run jest -- -u on this branch & push the result. Once that's done, CI should pass, and I can merge and deploy the result. Thanks for following through with PR feedback so quickly

jlfwong · 2018-10-05T18:32:50Z

src/import/v8heapalloc.test.ts

+import {checkProfileSnapshot} from '../lib/test-utils'
+
+test('importV8HeapAlloc from Chrome', async () => {
+  await checkProfileSnapshot('./sample/profiles/Chrome/69/Heap-20181005T144546.heapprofile')


The way these tests work is by generating output, then comparing against output that's checked into the repository. To seed the initial data, you'll need to run npm run jest -- -u, which should create local snapshot files that you'll need to check into the repository. I believe that should result in CI passing.

CI is currently failing because jest is run in a mode which does not write snapshots to disk, it only compares them. It doesn't find a corresponding snapshot on disk to compare against since none was checked in, resulting in a test failure.

See: https://jestjs.io/docs/en/snapshot-testing

jlfwong · 2018-10-05T18:35:45Z

src/import/v8heapalloc.ts

+import {getOrInsert} from '../lib/utils'
+import {ByteFormatter} from '../lib/value-formatters'
+
+/**


This is perfect! Thanks for writing this :)

vmarchaud · 2018-10-08T11:37:49Z

And done ! thanks for the help for generating the snapshot.

Just to tell you also, i'm working to support an old cpu profile format of v8 (mainly outputted by https://github.com/hyj1991/v8-profiler-node8), which is the current format we use ourselves, so an PR will arrive this week i believe ;)

jlfwong · 2018-10-08T16:56:31Z

@vmarchaud thanks for the hard work! This is now merged, published to npm, and deployed to https://www.speedscope.app/. I also updated the documentation here: https://github.com/jlfwong/speedscope/wiki/Importing-from-Node.js#importing-a-heap-profile

Just to tell you also, i'm working to support an old cpu profile format of v8 (mainly outputted by https://github.com/hyj1991/v8-profiler-node8), which is the current format we use ourselves, so an PR will arrive this week i believe ;)

Sounds good to me 😄

As said on #170, i added the support for the old format used by https://github.com/hyj1991/v8-profiler-node8 (which is currently used for pm2.io).

Add import of v8 heap allocation profile

6ac972f

jlfwong reviewed Oct 4, 2018

View reviewed changes

chore: address PR comments

873d5d3

vmarchaud added 3 commits October 5, 2018 14:49

chore: add sample / doc / tests

700a7ff

chore: fix lint

8021713

chore: fix sample path for node

bbc6df8

jlfwong reviewed Oct 5, 2018

View reviewed changes

tests: add snapshot for v8 heap profile

b0d4589

jlfwong merged commit 4b292b2 into jlfwong:master Oct 8, 2018

vmarchaud mentioned this pull request Oct 9, 2018

Import v8 cpu profile (old format) #177

Merged

jlfwong pushed a commit that referenced this pull request Dec 4, 2018

Import v8 cpu profile (old format) (#177)

8cddf3f

As said on #170, i added the support for the old format used by https://github.com/hyj1991/v8-profiler-node8 (which is currently used for pm2.io).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add import of v8 heap allocation profile #170

Add import of v8 heap allocation profile #170

vmarchaud commented Oct 4, 2018

coveralls commented Oct 4, 2018 •

edited

jlfwong left a comment

jlfwong Oct 4, 2018

vmarchaud Oct 5, 2018

jlfwong Oct 4, 2018

jlfwong Oct 4, 2018

jlfwong Oct 4, 2018

vmarchaud Oct 5, 2018

jlfwong Oct 4, 2018

vmarchaud Oct 5, 2018

jlfwong Oct 4, 2018

vmarchaud Oct 5, 2018

jlfwong Oct 4, 2018

jlfwong Oct 4, 2018

jlfwong Oct 4, 2018

vmarchaud commented Oct 5, 2018

vmarchaud commented Oct 5, 2018 •

edited

vmarchaud commented Oct 5, 2018

jlfwong left a comment

jlfwong Oct 5, 2018

jlfwong Oct 5, 2018

vmarchaud commented Oct 8, 2018

jlfwong commented Oct 8, 2018

		@@ -0,0 +1,97 @@
		import {Profile, FrameInfo, StackListProfileBuilder} from '../lib/profile'


		const profile = new StackListProfileBuilder(total)

		for (let i = 0; i < stacks.length; i++) {

Add import of v8 heap allocation profile #170

Add import of v8 heap allocation profile #170

Conversation

vmarchaud commented Oct 4, 2018

coveralls commented Oct 4, 2018 • edited

jlfwong left a 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

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

Choose a reason for hiding this comment

vmarchaud commented Oct 5, 2018

vmarchaud commented Oct 5, 2018 • edited

Importing a Heap Profile from the Chrome inspector

vmarchaud commented Oct 5, 2018

jlfwong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud commented Oct 8, 2018

jlfwong commented Oct 8, 2018

coveralls commented Oct 4, 2018 •

edited

vmarchaud commented Oct 5, 2018 •

edited