New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import v8 cpu profile (old format) #177

Open
wants to merge 5 commits into
base: master
from

Conversation

3 participants
@vmarchaud
Contributor

vmarchaud commented Oct 9, 2018

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).

I also re-introduced the (idle) frame, and removed the merge between the gc/ program frame and the previous stack.

As discussed in #176, i believe speedscope need to show what's happening in the main thread at any given time, both of the gc and program frame represent blocking operation, so they should be shown at the top level so user can understand that their code isn't running while they are.

Note that program is actually native code, either V8 or any C++ binding :
Source : https://trac.webkit.org/changeset/138004/webkit

vmarchaud added some commits Oct 4, 2018

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 9, 2018

Coverage Status

Coverage increased (+0.2%) to 44.653% when pulling 85c3a08 on vmarchaud:vmarchaud/import-v8-cpu-old into 0fe0c45 on jlfwong:master.

coveralls commented Oct 9, 2018

Coverage Status

Coverage increased (+0.2%) to 44.653% when pulling 85c3a08 on vmarchaud:vmarchaud/import-v8-cpu-old into 0fe0c45 on jlfwong:master.

@vmarchaud vmarchaud changed the title from Import v8 cpu old to Import v8 cpu profile (old format) Oct 9, 2018

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Oct 9, 2018

Owner

I also re-introduced the (idle) frame, and removed the merge between the gc/ program frame and the previous stack.

As discussed in #176, i believe speedscope need to show what's happening in the main thread at any given time, both of the gc and program frame represent blocking operation, so they should be shown at the top level so user can understand that their code isn't running while they are.

I'm afraid I don't think I agree on either of these fronts. We can certainly discuss, but at a minimum I'd like these to be part of a different PR, since they affect more than what's described in this PR description, and I'd like to avoid interleaving comments about that change with comments about the correctness of the old format import.

Owner

jlfwong commented Oct 9, 2018

I also re-introduced the (idle) frame, and removed the merge between the gc/ program frame and the previous stack.

As discussed in #176, i believe speedscope need to show what's happening in the main thread at any given time, both of the gc and program frame represent blocking operation, so they should be shown at the top level so user can understand that their code isn't running while they are.

I'm afraid I don't think I agree on either of these fronts. We can certainly discuss, but at a minimum I'd like these to be part of a different PR, since they affect more than what's described in this PR description, and I'd like to avoid interleaving comments about that change with comments about the correctness of the old format import.

@vmarchaud

This comment has been minimized.

Show comment
Hide comment
@vmarchaud

vmarchaud Oct 9, 2018

Contributor

Sure i'll do another PR tomorrow

Contributor

vmarchaud commented Oct 9, 2018

Sure i'll do another PR tomorrow

vmarchaud added some commits Oct 9, 2018

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Oct 10, 2018

Owner

Thanks for splitting out #178! I'm pretty busy for the next few days, so as a heads up, it might take a while for me to get to review this & do a release

Owner

jlfwong commented Oct 10, 2018

Thanks for splitting out #178! I'm pretty busy for the next few days, so as a heads up, it might take a while for me to get to review this & do a release

@vmarchaud

This comment has been minimized.

Show comment
Hide comment
@vmarchaud

vmarchaud Oct 12, 2018

Contributor

@jlfwong no problem we deployed a fork with all the fix/modification in our end anyway

Contributor

vmarchaud commented Oct 12, 2018

@jlfwong no problem we deployed a fork with all the fix/modification in our end anyway

@jlfwong

Thanks for another significant contribution!

In addition to the inline comments below, I have a request for the example profile.

I try to keep the profiles as small as possible in order to keep the overall repository size small, as well as to keep the tests running very quickly. Would it be possible to record a smaller example profile by running a very simple JavaScript file? That was the intent of the files in repository, like this one:

https://github.com/jlfwong/speedscope/blob/master/sample/programs/javascript/recursion.js

Before this PR, chrome.test.ts.snap is 521 lines (https://github.com/jlfwong/speedscope/blob/master/src/import/__snapshots__/chrome.test.ts.snap), and after this PR with the current example profile, it's 24000 lines.

@@ -122,11 +124,14 @@ async function _importProfileGroup(dataSource: ProfileDataSource): Promise<Profi
console.log('Importing as Firefox profile')
return toGroup(importFromFirefox(parsed))
} else if (isChromeTimeline(parsed)) {
console.log('Importing as Chrome CPU Profile')
console.log('Importing as Chrome Timeline')

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

Whoops! Nice catch and thanks for fixing this!

@jlfwong

jlfwong Oct 19, 2018

Owner

Whoops! Nice catch and thanks for fixing this!

@@ -264,3 +265,15 @@ export function importFromChromeCPUProfile(chromeProfile: CPUProfile): Profile {
profile.setValueFormatter(new TimeFormatter('microseconds'))
return profile.build()
}
/**

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

Thanks for writing this comment! I think this would more helpfully live at the top of v8cpuFormatter.ts.

I think this could also use a bit of rewording. Something like this:

* This importer handles an old format used by the C++ API of V8. This format is still used by v8-profiler-node8.
* There are two differences between the two formats:
*  - Nodes are a tree in the old format and a flat array in the new format
*  - Weights are timestamps in the old format and deltas in the new format.
*
* For more information, see https://github.com/hyj1991/v8-profiler-node8

Your English is still much, much better than my French 😄

Merci beaucoup pour tout votre contributions! 🇨🇦

@jlfwong

jlfwong Oct 19, 2018

Owner

Thanks for writing this comment! I think this would more helpfully live at the top of v8cpuFormatter.ts.

I think this could also use a bit of rewording. Something like this:

* This importer handles an old format used by the C++ API of V8. This format is still used by v8-profiler-node8.
* There are two differences between the two formats:
*  - Nodes are a tree in the old format and a flat array in the new format
*  - Weights are timestamps in the old format and deltas in the new format.
*
* For more information, see https://github.com/hyj1991/v8-profiler-node8

Your English is still much, much better than my French 😄

Merci beaucoup pour tout votre contributions! 🇨🇦

/**
* Convert the tree based format to the array of nodes like actually
*/
export function chromeTree2nodes(content: OldCPUProfile): CPUProfile {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I generally haven't been using "2" to denote "to" in the code in this repository. I think I'd prefer "chromeTreeToNodes" here.

@jlfwong

jlfwong Oct 19, 2018

Owner

I generally haven't been using "2" to denote "to" in the code in this repository. I think I'd prefer "chromeTreeToNodes" here.

}
/**
* Convert the tree based format to the array of nodes like actually

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I'm not exactly sure what you were trying to express here, but perhaps something like this is what you intended?

/**
 * Convert the old tree-based format to the new flat-array based format
 */
@jlfwong

jlfwong Oct 19, 2018

Owner

I'm not exactly sure what you were trying to express here, but perhaps something like this is what you intended?

/**
 * Convert the old tree-based format to the new flat-array based format
 */
@@ -0,0 +1,61 @@
import {CPUProfile, CPUProfileNode} from './chrome'
export interface OldCPUProfileNode {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I don't think this needs exporting, does it?

Suggested change Beta
export interface OldCPUProfileNode {
interface OldCPUProfileNode {
@jlfwong

jlfwong Oct 19, 2018

Owner

I don't think this needs exporting, does it?

Suggested change Beta
export interface OldCPUProfileNode {
interface OldCPUProfileNode {
return nodes
}
function timestampsToDelta(timestamps: number[], startTime: number): number[] {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I think this would be better named as timestampsToDeltas (with "deltas" being plural)

@jlfwong

jlfwong Oct 19, 2018

Owner

I think this would be better named as timestampsToDeltas (with "deltas" being plural)

}
function timestampsToDelta(timestamps: number[], startTime: number): number[] {
return timestamps.reduce((deltas: number[], timestamp: number, index: number) => {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

It doesn't look to me like you need to use reduce here? Could this be more concisely expressed like so?

return timestamps.map((timestamp, index) => {
  const lastTimestamp = index === 0 ? startTime * 1000000 : timestamps[index - 1]
  return timestamp - lastTimestamp
})
@jlfwong

jlfwong Oct 19, 2018

Owner

It doesn't look to me like you need to use reduce here? Could this be more concisely expressed like so?

return timestamps.map((timestamp, index) => {
  const lastTimestamp = index === 0 ? startTime * 1000000 : timestamps[index - 1]
  return timestamp - lastTimestamp
})
timestamps: number[]
}
function treeToArray(node: OldCPUProfileNode, nodes: CPUProfileNode[]) {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

This is a very minor thing, but I find functions that both mutate their arguments and return a new value confusing. I think this could be avoided like so:

function treeToArray(root: OldCPUProfileNode): CPUProfileNode[] {
  const nodes: CPUProfileNode[] = []
  function visit(node: CPUProfileNode) {
    nodes.push({
      id: node.id,
      callFrame: {
        columnNumber: 0,
        functionName: node.functionName,
        lineNumber: node.lineNumber,
        scriptId: node.scriptId,
        url: node.url,
      },
      hitCount: node.hitCount,
      children: node.children.map(child => child.id),
    })
    nodes.children.forEach(visit)
  }
  visit(root)
  return nodes
}
@jlfwong

jlfwong Oct 19, 2018

Owner

This is a very minor thing, but I find functions that both mutate their arguments and return a new value confusing. I think this could be avoided like so:

function treeToArray(root: OldCPUProfileNode): CPUProfileNode[] {
  const nodes: CPUProfileNode[] = []
  function visit(node: CPUProfileNode) {
    nodes.push({
      id: node.id,
      callFrame: {
        columnNumber: 0,
        functionName: node.functionName,
        lineNumber: node.lineNumber,
        scriptId: node.scriptId,
        url: node.url,
      },
      hitCount: node.hitCount,
      children: node.children.map(child => child.id),
    })
    nodes.children.forEach(visit)
  }
  visit(root)
  return nodes
}
* Convert the tree based format to the array of nodes like actually
*/
export function chromeTree2nodes(content: OldCPUProfile): CPUProfile {
// Care that both startTime and endTime are now in microsecond

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I think the more natural way of saying this would be:

Suggested change Beta
// Care that both startTime and endTime are now in microsecond
// Note that both startTime and endTime are now in microseconds

How would you say this in French? Attention?

@jlfwong

jlfwong Oct 19, 2018

Owner

I think the more natural way of saying this would be:

Suggested change Beta
// Care that both startTime and endTime are now in microsecond
// Note that both startTime and endTime are now in microseconds

How would you say this in French? Attention?

@@ -22,15 +23,15 @@ interface PositionTickInfo {
ticks: number
}
interface CPUProfileCallFrame {
export interface CPUProfileCallFrame {

This comment has been minimized.

@jlfwong

jlfwong Oct 19, 2018

Owner

I don't think this needs to be exported

Suggested change Beta
export interface CPUProfileCallFrame {
interface CPUProfileCallFrame {
@jlfwong

jlfwong Oct 19, 2018

Owner

I don't think this needs to be exported

Suggested change Beta
export interface CPUProfileCallFrame {
interface CPUProfileCallFrame {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment