Add option to collect cpu usage from processes #13

RMacfarlane · 2018-03-16T21:46:51Z

Calculates the cpu usage of a process as the change in kernel + user time the process has used over the change in kernel + user time the system has used after sleeping for a second.

Currently this is being calculated for all processes when the flag is set instead of just the processes under rootId, as I was having trouble serializing the structure of cpu information back and forth between javascript and C++.

Tyriar · 2018-03-17T22:00:14Z

typings/windows-process-tree.d.ts

 		pid: number,
 		name: string,
 		memory?: number,
+		cpu?: number,


I think the API should look something like this (after the TS change is merged):

export interface ProcessCpuTreeNode extends ProcessTreeNode { cpu: number }; export function getProcessCpuUsage(tree: ProcessTreeNode, callback: (tree: ProcessCpuTreeNode): void;

That way we don't need to complicate getProcessTree and everything is separated nicely. Also the small amount of time needed to cross the native boundary doesn't matter since this is a long running async operation.

Tyriar · 2018-03-17T22:01:22Z

test.js

+    getProcessTree(process.pid, (tree) => {
+      assert.equal(tree.name, 'node.exe');
+      assert.equal(tree.pid, process.pid);
+      assert.notEqual(tree.cpu, undefined);


Maybe checking typeof cpu and 0 <= cpu <= 100 would be a better check?

Tyriar · 2018-03-17T22:02:49Z

src/process.cc

+  return kt.QuadPart + ut.QuadPart;
+}
+
+void GetCpuUsage(ProcessInfo process_info[1024], uint32_t* process_count, BOOL first_pass) {


I think this needs some comments explaining how the CPU time calculation works.

RMacfarlane · 2018-03-20T20:50:14Z

src/process.h

  NONE = 0,
-  MEMORY = 1
+  MEMORY = 1,
+  CPU = 2


Remove CPU flag

Tyriar · 2018-03-20T20:44:09Z

lib/index.ts

-const requestQueue = [];
+let cpuUsageRequestInProgress = false;
+
+const requestQueue = {


We should type this so it's clear what they do.

Tyriar · 2018-03-20T20:46:32Z

lib/index.ts

+  // This prevents too many requests from being made, there is also a crash that
+  // can occur when performing multiple calls to CreateToolhelp32Snapshot at
+  // once.
+  if (!requestInProgress) {


We should refactor this to share code with getProcessTree, I think we need to pass in a queue and a filter function

Tyriar · 2018-03-20T20:46:43Z

lib/index.ts

+ * @param callback The callback to use with the returned list of processes
+ */
+export function getProcessCpuUsage(processList: IProcessInfo[], callback: (tree: IProcessCpuInfo[]) => void): void {
+    // Push the request to the queue


Indentation, we should add an editor config file :)

Tyriar · 2018-03-20T20:48:59Z

lib/index.ts

+      callback: callback
+    });
+
+    if (!cpuUsageRequestInProgress) {


I'm not sure we need this blocking flag

Tyriar · 2018-03-20T20:54:06Z

src/process.cc

+  FILETIME sysIdleTime, sysKernelTime, sysUserTime;
+  if (GetProcessTimes(hProcess, &creationTime, &exitTime, &kernelTime, &userTime)
+    && GetSystemTimes(&sysIdleTime, &sysKernelTime, &sysUserTime)) {
+      if (first_pass) {


Indentation

Tyriar · 2018-03-20T20:55:50Z

src/process.cc

+
+  FILETIME creationTime, exitTime, kernelTime, userTime;
+  FILETIME sysIdleTime, sysKernelTime, sysUserTime;
+  if (GetProcessTimes(hProcess, &creationTime, &exitTime, &kernelTime, &userTime)


We may want to handle when the if condition is false (cpu = undefined?)

Tyriar · 2018-03-20T20:57:54Z

src/process.cc

+}
+
+ULONGLONG GetTotalTime(const FILETIME* kernelTime, const FILETIME* userTime) {
+  ULARGE_INTEGER kt, ut;


Comment to explain function

Tyriar · 2018-03-22T16:58:13Z

src/addon.cc

 #include <nan.h>
 #include "process.h"
 #include "worker.h"
+#include <cmath>


Is this being used?

Yeah, it imports isnan

Tyriar

LGTM after the 2 remaining comments, great job 😃

Tyriar · 2018-03-22T17:00:16Z

typings/windows-process-tree.d.ts


  export function getProcessTree(rootPid: number, callback: (tree: IProcessTreeNode) => void, flags?: ProcessDataFlag): void;
+
+  export function getProcessList(rootPid: number, callback: (processList: IProcessInfo[]) => void, flags?: ProcessDataFlag): void;


Let's start filling in the JS Doc here as the API is getting a little more complex

READY : Update node-gyp before npm install

RMacfarlane self-assigned this Mar 16, 2018

RMacfarlane requested a review from Tyriar March 16, 2018 21:46

Tyriar requested changes Mar 17, 2018

View reviewed changes

Rachel Macfarlane added 2 commits March 19, 2018 10:44

Add option to collect cpu usage from processes

e43e24f

Add getProcessCpuUsage as separate call

94469f5

RMacfarlane force-pushed the rmacfarlane/cpu branch from d377ac3 to 94469f5 Compare March 20, 2018 18:42

RMacfarlane mentioned this pull request Mar 20, 2018

Add support for getting process memory and cpu usage information #12

Closed

3 tasks

RMacfarlane commented Mar 20, 2018

View reviewed changes

src/process.h Outdated

NONE = 0,

MEMORY = 1

MEMORY = 1,

CPU = 2

Copy link

Contributor Author

RMacfarlane Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove CPU flag

Tyriar requested changes Mar 20, 2018

View reviewed changes

Tyriar added this to the 0.2.0 milestone Mar 20, 2018

Small fixes and refactor in index.ts

c9de2cb

Tyriar reviewed Mar 22, 2018

View reviewed changes

Tyriar approved these changes Mar 22, 2018

View reviewed changes

Add JSDoc comments to the interface

fc51ca5

RMacfarlane merged commit 2d6a504 into master Mar 22, 2018

RMacfarlane deleted the rmacfarlane/cpu branch March 22, 2018 17:58

vmihdal pushed a commit to vmihdal/vscode-windows-process-tree that referenced this pull request Aug 13, 2021

Merge pull request microsoft#13 from volodymyrmigdal/master

852efdc

READY : Update node-gyp before npm install


		export function getProcessTree(rootPid: number, callback: (tree: IProcessTreeNode) => void, flags?: ProcessDataFlag): void;

		export function getProcessList(rootPid: number, callback: (processList: IProcessInfo[]) => void, flags?: ProcessDataFlag): void;

Add option to collect cpu usage from processes #13

Add option to collect cpu usage from processes #13

Uh oh!

Conversation

RMacfarlane commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants