add local CPU profile collection #11

nolanmar511 · 2017-10-09T23:23:37Z

No description provided.

src/profilers/cpu-profiler.ts

+import {serializeCpuProfile} from './profile-serializer';
+const cpuProfiler = require('bindings')('cpu_profiler');
+
+export class CpuProfiler {


ofrobots

Left an initial set of comments. PTAL.

Side note: Most people in the Node.js community will be confused by the phrasing 'Wall Profile', specially in configuration. People are familiar with the V8 CPU Profiler that is also available through dev tools. I understand the difference in terminology that the cloud profiler is using, but in the node community 'wall profiler' will need a lot of explanation for people to understand.

src/profilers/profile-serializer.ts

@@ -0,0 +1,145 @@
+import {perftools} from '../profile';


src/profilers/profile-serializer.ts

+  }
+
+  // clears fields used to build a profile.
+  private reset() {


src/profilers/wall-profiler.ts

@@ -0,0 +1,44 @@
+import {perftools} from '../profile';


src/profilers/wall-profiler.ts

+  // True when the profiler is actively profiling.
+  private profiling: boolean;
+
+  constructor(samplingInterval: number) {


src/profilers/profile-serializer.ts

+   * stack - the stack trace to the current node.
+   */
+  private serializeNode(node: WallProfileNode, stack: Stack) {
+    let that = this;


src/profilers/wall-profiler.ts

+        }, profileDuration);
+      });
+    }
+    return Promise.reject(new Error('already profiling with WallProfiler'));


src/profilers/wall-profiler.ts

+
+  // Collects a profile for the duration, in milliseconds, specified by
+  // profileDuration.
+  // Returns a promise which will resolve to the collected profile.


src/profilers/wall-profiler.ts

+      that.profiling = true;
+      wallProfiler.startProfiling('', true);
+      return new Promise(function(resolve) {
+        setTimeout(function() {


test/test-profile-serializer.ts

@@ -0,0 +1,135 @@
+import {perftools} from '../src/profile';


package.json

@@ -5,7 +5,7 @@
  "main": "build/src/index.js",
  "scripts": {
    "install": "node-gyp rebuild",
-    "test": "true",
+    "test": "mocha build/test/test-*.js",


aalexand · 2017-10-12T19:49:04Z

@ofrobots We could keep the "CPU profiler" terminology in the API here but we'll still need to send these profiles as of "wall" profiling type in the Cloud Profiler speak to be consistent with the data agents for other languages produce. Those use "CPU" profiling type to describe profiles where samples represent the actual CPU time spent by the program.

src/profilers/wall-profiler.ts

-    return Promise.reject(new Error('already profiling with WallProfiler'));
+    this.profiling = true;
+    wallProfiler.startProfiling('', true);
+    return delay(profileDuration).then(() => {


nolanmar511 · 2017-10-12T22:12:05Z

PTAL
(still trying to figure out why gts check fails, which is why travis fails)

codecov · 2017-10-14T21:09:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5dd69cf). Click here to learn what that means.
The diff coverage is 98.33%.

@@            Coverage Diff            @@
##             master      #11   +/-   ##
=========================================
  Coverage          ?   98.41%           
=========================================
  Files             ?        6           
  Lines             ?      126           
  Branches          ?        7           
=========================================
  Hits              ?      124           
  Misses            ?        2           
  Partials          ?        0

Impacted Files	Coverage Δ
test/profile-for-test.ts	`100% <100%> (ø)`
src/profilers/time-profiler.ts	`100% <100%> (ø)`
test/test-time-profiler.ts	`100% <100%> (ø)`
test/test-profile-serializer.ts	`100% <100%> (ø)`
src/profilers/profile-serializer.ts	`96.42% <96.42%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dd69cf...54efc86. Read the comment docs.

nolanmar511 · 2017-10-16T15:34:51Z

PTAL

src/profilers/profile-serializer.ts

+// A stack of function UIDs.
+type Stack = Array<number>;
+
+// Converts v8 Profile into profile with profile format used by Stackdriver


src/profilers/profile-serializer.ts

+    period: sampleInterval
+  };
+
+  /* Adds samples from a node and it's children to the fields tracking


src/profilers/profile-serializer.ts

+      samples.push(sample);
+    }
+    for (let child of node.children) {
+      serializeNode(child, stack);


src/profilers/profile-serializer.ts

+  function getLocation(node: TimeProfileNode): perftools.profiles.Location {
+    const id = node.callUid;
+    if (locationMap.has(id)) {
+      return locationMap.get(id) as perftools.profiles.Location;


src/profilers/profile-serializer.ts

+  }
+
+  function getLocation(node: TimeProfileNode): perftools.profiles.Location {
+    const id = node.callUid;


src/profilers/time-profiler.ts

+
+  // Returns true if the TimeProfiler is currently profiling and false
+  // otherwise.
+  isRunning(): boolean {


src/profilers/time-profiler.ts

+    return serializeTimeProfile(result, this.samplingInterval);
+  }
+
+  // Returns true if the TimeProfiler is currently profiling and false


src/profilers/profile-serializer.ts

+    return f;
+  }
+
+  function getSampleValueType(): perftools.profiles.ValueType {


bindings/time-profiler.cc

@@ -19,7 +19,7 @@

 using namespace v8;

-Local<Value> TranslateCpuProfileNode(const CpuProfileNode* node) {
+Local<Value> TranslateTimeProfileNode(const CpuProfileNode* node) {
  // TODO: Implement unimplemented interface


src/profilers/time-profiler.ts

+      throw new Error('already profiling with TimeProfiler');
+    }
+    this.profiling = true;
+    timeProfiler.startProfiling('', true);


ofrobots

This is looking good. From what I can see the main outstanding issue is the comment from @aalexand about recursion.

bindings/time-profiler.cc

@@ -19,7 +19,7 @@

 using namespace v8;

-Local<Value> TranslateCpuProfileNode(const CpuProfileNode* node) {
+Local<Value> TranslateTimeProfileNode(const CpuProfileNode* node) {
  // TODO: Implement unimplemented interface


src/profilers/profile-serializer.ts

+  }
+
+  function getLocation(node: TimeProfileNode): perftools.profiles.Location {
+    const id = node.callUid;


nolanmar511 · 2017-10-18T00:10:32Z

PTAL

src/profilers/profile-serializer.ts

+   * @param stack - the stack trace to the current node.
+   */
+  function serializeNode(root: TimeProfileNode) {
+    let nodes = [root];


src/profilers/profile-serializer.ts

-      let node = nodes.pop();
-      let stack = stacks.pop();
-      if (node !== undefined && stack !== undefined) {
+    let entries: {node: TimeProfileNode, stack: number[]}[] = [];


nolanmar511 · 2017-10-19T14:43:17Z

PTAL

src/profilers/profile-serializer.ts

@@ -0,0 +1,145 @@
+import {perftools} from '../profile';


src/profilers/profile-serializer.ts

+import {perftools} from '../profile';
+import {getIndexOrAdd} from '../util';
+import {TimeProfile, TimeProfileNode} from '../v8-types';
+// A stack of function UIDs.


src/profilers/profile-serializer.ts

+import {perftools} from '../profile';
+import {getIndexOrAdd} from '../util';
+import {TimeProfile, TimeProfileNode} from '../v8-types';
+// A stack of function UIDs.


src/profilers/profile-serializer.ts

+}
+
+/**
+ * Converts v8 Profile into profile with profile format used by Stackdriver


src/profilers/profile-serializer.ts

+  let samples: Array<perftools.profiles.Sample> = [];
+  let locations: Array<perftools.profiles.Location> = [];
+  let functions: Array<perftools.profiles.Function> = [];
+  let locationMap: Map<number, perftools.profiles.Location> = new Map();


src/profilers/profile-serializer.ts

+      name: name,
+      systemName: name,
+      filename: getIndexOrAdd(node.scriptResourceName || '(unknown)', strings)
+      // start_line


src/profilers/profile-serializer.ts

+    if (f !== undefined) {
+      return f;
+    }
+    const name = getIndexOrAdd(node.functionName || '(anonymous)', strings);


src/profilers/time-profiler.ts

+   * @param durationMillis - time in milliseconds for which to collect profile.
+   */
+  async profile(durationMillis: number): Promise<perftools.profiles.IProfile> {
+    const runName = 'stackdriver-profiler-' + Date.now() + '-' + Math.random();


src/v8-types.ts

@@ -16,21 +16,21 @@

 // Type Definitions based on implementation in bindings/

-export interface CpuProfile {
+export interface TimeProfile {
  /** Time in nanoseconds at which profile was stopped. */
  endTime: number;


src/v8-types.ts

  /** Time in nanoseconds at which profile was stopped. */
  endTime: number;
-  topDownRoot: CpuProfileNode;
+  topDownRoot: TimeProfileNode;
  /** Time in nanoseconds at which profile was started. */
  startTime: number;


googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 9, 2017

nolanmar511 force-pushed the cpu-profiler branch from da5fdf7 to 2563c01 Compare October 9, 2017 23:24

nolanmar511 commented Oct 9, 2017

View reviewed changes

src/profilers/cpu-profiler.ts Outdated

import {serializeCpuProfile} from './profile-serializer';

const cpuProfiler = require('bindings')('cpu_profiler');

export class CpuProfiler {

This comment was marked as spam.

Sign in to view

nolanmar511 force-pushed the cpu-profiler branch 3 times, most recently from 30f1459 to a960905 Compare October 10, 2017 17:12

add local cpu profile collection

080bc14

nolanmar511 force-pushed the cpu-profiler branch 3 times, most recently from 6e39349 to 5c0b1b2 Compare October 11, 2017 14:45

Rename CPU profiler to wall profiler

e4a68c9

nolanmar511 force-pushed the cpu-profiler branch from 5c0b1b2 to e4a68c9 Compare October 11, 2017 14:56

nolanmar511 requested a review from ofrobots October 12, 2017 18:06

ofrobots reviewed Oct 12, 2017

View reviewed changes

Addressed comments

0976a56

nolanmar511 force-pushed the cpu-profiler branch from 2878931 to 0976a56 Compare October 12, 2017 21:08

ofrobots reviewed Oct 12, 2017

View reviewed changes

src/profilers/wall-profiler.ts Outdated

return Promise.reject(new Error('already profiling with WallProfiler'));

this.profiling = true;

wallProfiler.startProfiling('', true);

return delay(profileDuration).then(() => {

This comment was marked as spam.

Sign in to view

address comments

971ad36

nolanmar511 force-pushed the cpu-profiler branch from f03d9b8 to 1be0b8c Compare October 12, 2017 22:10

fix test

94842e0

nolanmar511 force-pushed the cpu-profiler branch 6 times, most recently from b6ea041 to d525007 Compare October 12, 2017 23:51

Rename wall profiler to time profiler

7f2d979

nolanmar511 force-pushed the cpu-profiler branch from d525007 to 7f2d979 Compare October 12, 2017 23:58

nolanmar511 force-pushed the cpu-profiler branch from 701d195 to 7515fd7 Compare October 14, 2017 21:04

add code coverage, and update tests

b12b092

nolanmar511 force-pushed the cpu-profiler branch from 7515fd7 to b12b092 Compare October 14, 2017 21:05

remove unused import

1a3ef77

nolanmar511 force-pushed the cpu-profiler branch from 12045b4 to 1a3ef77 Compare October 16, 2017 17:18

aalexand reviewed Oct 16, 2017

View reviewed changes

nolanmar511 added 2 commits October 16, 2017 14:03

address comments

bfdfc10

address comments

7153b5d

ofrobots reviewed Oct 17, 2017

View reviewed changes

make serializeNode non-recursive

c0d645d

remove TODO

085e932

ofrobots reviewed Oct 18, 2017

View reviewed changes

src/profilers/profile-serializer.ts Outdated

* @param stack - the stack trace to the current node.

*/

function serializeNode(root: TimeProfileNode) {

let nodes = [root];

This comment was marked as spam.

Sign in to view

nolanmar511 force-pushed the cpu-profiler branch 3 times, most recently from e258e4f to bbe21f8 Compare October 18, 2017 14:56

use entry in serializeNode

1735b3e

nolanmar511 force-pushed the cpu-profiler branch from bbe21f8 to 1735b3e Compare October 18, 2017 15:58

switch from simple-mock to sinon

7be7874

ofrobots reviewed Oct 19, 2017

View reviewed changes

src/profilers/profile-serializer.ts Outdated

let node = nodes.pop();

let stack = stacks.pop();

if (node !== undefined && stack !== undefined) {

let entries: {node: TimeProfileNode, stack: number[]}[] = [];

This comment was marked as spam.

Sign in to view

make Entry interface

54efc86

nolanmar511 force-pushed the cpu-profiler branch from f4b9ce0 to 54efc86 Compare October 19, 2017 14:29

ofrobots approved these changes Oct 19, 2017

View reviewed changes

nolanmar511 merged commit 1bb807e into googleapis:master Oct 19, 2017

aalexand reviewed Oct 19, 2017

View reviewed changes

nolanmar511 mentioned this pull request Oct 19, 2017

Efficiency of profile-serializer #15

Closed

nolanmar511 deleted the cpu-profiler branch December 21, 2017 19:04

add local CPU profile collection #11

add local CPU profile collection #11

Conversation

nolanmar511 commented Oct 9, 2017

This comment was marked as spam.

ofrobots left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

aalexand commented Oct 12, 2017

This comment was marked as spam.

nolanmar511 commented Oct 12, 2017

codecov bot commented Oct 14, 2017 • edited Loading

Codecov Report

nolanmar511 commented Oct 16, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

nolanmar511 commented Oct 18, 2017

This comment was marked as spam.

This comment was marked as spam.

nolanmar511 commented Oct 19, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment •

edited

Loading

codecov bot commented Oct 14, 2017 •

edited

Loading