Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add option to collect cpu usage from processes #13

Merged
merged 4 commits into from Mar 22, 2018
Merged

Add option to collect cpu usage from processes #13

merged 4 commits into from Mar 22, 2018

Conversation

RMacfarlane
Copy link
Contributor

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

@RMacfarlane RMacfarlane self-assigned this Mar 16, 2018
@RMacfarlane RMacfarlane requested a review from Tyriar March 16, 2018 21:46
Tyriar
Tyriar requested changes Mar 17, 2018
View reviewed changes
typings/windows-process-tree.d.ts Outdated
@@ -15,6 +15,7 @@ declare module 'windows-process-tree' {
pid: number,
name: string,
memory?: number,
cpu?: number,
Copy link
Member

Choose a reason for hiding this comment

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

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.

test.js Outdated
getProcessTree(process.pid, (tree) => {
assert.equal(tree.name, 'node.exe');
assert.equal(tree.pid, process.pid);
assert.notEqual(tree.cpu, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

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

src/process.cc Outdated
return kt.QuadPart + ut.QuadPart;
}

void GetCpuUsage(ProcessInfo process_info[1024], uint32_t* process_count, BOOL first_pass) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@RMacfarlane RMacfarlane mentioned this pull request Mar 20, 2018
Closed
3 tasks
RMacfarlane
RMacfarlane commented Mar 20, 2018
View reviewed changes
src/process.h Outdated
@@ -18,11 +25,14 @@ struct ProcessInfo {

enum ProcessDataFlags {
NONE = 0,
MEMORY = 1
MEMORY = 1,
CPU = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove CPU flag

Tyriar
Tyriar requested changes Mar 20, 2018
View reviewed changes
lib/index.ts Outdated
const requestQueue = [];
let cpuUsageRequestInProgress = false;

const requestQueue = {
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/index.ts Outdated
// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/index.ts Outdated
* @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
Copy link
Member

Choose a reason for hiding this comment

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

Indentation, we should add an editor config file :)

lib/index.ts Outdated
callback: callback
});

if (!cpuUsageRequestInProgress) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this blocking flag

src/process.cc Outdated
FILETIME sysIdleTime, sysKernelTime, sysUserTime;
if (GetProcessTimes(hProcess, &creationTime, &exitTime, &kernelTime, &userTime)
&& GetSystemTimes(&sysIdleTime, &sysKernelTime, &sysUserTime)) {
if (first_pass) {
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

src/process.cc

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

Choose a reason for hiding this comment

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

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

src/process.cc Outdated
}

ULONGLONG GetTotalTime(const FILETIME* kernelTime, const FILETIME* userTime) {
ULARGE_INTEGER kt, ut;
Copy link
Member

Choose a reason for hiding this comment

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

Comment to explain function

@Tyriar Tyriar added this to the 0.2.0 milestone Mar 20, 2018
Tyriar
Tyriar reviewed Mar 22, 2018
View reviewed changes
src/addon.cc
@@ -6,6 +6,7 @@
#include <nan.h>
#include "process.h"
#include "worker.h"
#include <cmath>
Copy link
Member

Choose a reason for hiding this comment

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

Is this being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it imports isnan

Tyriar
Tyriar approved these changes Mar 22, 2018
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

LGTM after the 2 remaining comments, great job 😃

typings/windows-process-tree.d.ts Outdated
pid: number;
name: string;
memory?: number;
children: IProcessTreeNode[];
}

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

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

Choose a reason for hiding this comment

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

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

@RMacfarlane RMacfarlane merged commit 2d6a504 into master Mar 22, 2018
@RMacfarlane 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@RMacfarlane RMacfarlane

Labels
None yet
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RMacfarlane @Tyriar