WIP #43

[soyuka]

ping @simonepri

Apart from reading procfiles error managment will be just fine as-is.

[simonepri]

Sorry, I didn't have time to work on it. I like the fact you have splitted them in separate files. That's much more clean.

[soyuka]

Sorry, I didn't have time to work on it.

No worries had some time so I took a first step. I'll have a working version by the end of the week but this looks very promising :D.

[soyuka]

Code is now way cleaner. Many more tests. Fix for #47 where start is a Date instance and time is the number of ms of cpu time used. There's still improvement to be done there because elapsed time is not the same as cpu time. I may have computed the start value with Date.now() - cpu time which is not 100% correct. Supports multiple pids on every platform and should be relatively performant!

[simonepri]

I'm not happy with the usage of the proc file for linux when more than one pid is asked.

If I ask you more than one pid then you execute 2 reads. But if those reads for some reasons are delayed by the node.JS event loop, then you gave me in output readings for the two processes taken at two different point of time.

I believe that ps handles this nicely creating reading locks on the files. Also its native implementation should be faster than the nodeJS' readfile.

So my suggestion is to use ps when dealing with multiple processes.

What do you think?

[soyuka]

So my suggestion is to use ps instead when dealing with multiple processes.

This is one of the advantages of file splitting! What about I expose ps, procfile and wmic?

var pidusage = require('pidusage')

pidusage.ps(pids, function (err, data) {
  // do stuff
})

This way you're free to use ps if you want to :).

I believe that ps handles this nicely creating reading locks on the files.

You mean he would be able to read every files at the same moment? With nodejs I doubt that this would indeed be possible. I wonder how they do anyway.

But if those reads for some reasons are delayed by the node.JS event loop for some reason then you gave me in output readings for the two processes taken at two different point of time.

Mhh okay but IMO this is insignificant in terms of CPU/memory usage no? Okay the times won't be computed at the exact same nanosecond but I don't see any issue with that.

Also its native implementation should be faster than the nodeJS' readfile.

Mhh, faster maybe but I really am not sure. Creating a sub process is also heavy, maybe more then just readtime. Especially that these files are accessed a lot.

[simonepri]

I don't see any issue with that.

If you take two readings in different points of time the total CPU usage of the system will not be coherent.

Just try to read ALL the pids of your system and then sum the cpu usage of all of them. The sum MUST be at max cores*100 but if you are under a stress test with many processes chances are high that this sum will be greater if the readings are taken in different points of time.

[soyuka]

CPU usage is always an approximation. I'd be really interested in how ps is able to read every file at the same time though. Going to look into that. Anyway if you're good to go with the above solution lmk.

[simonepri]

What about I expose ps, procfile and wmic?

I don't think that this would make sense. We need to keep the API simple and clean.

What are the advantages of using the proc file? It's really faster than ps? Why should we reinvent the wheel?

I'd be really interested in how ps is able to read every file at the same time though.

I don't think that this would be possible but my hope is that this problem is handled smartly by them. I'll dive into their the code too. I'm curious.

[soyuka]

What are the advantages of using the proc file? It's really faster than ps?

To avoid spawning a new process, it feels dirty to me. Using procfs seems to be the correct way of gathering process data. It should also be accurate. I'd also have wanted to implement sysctl calls on darwin instead of using ps which is just a fallback (and may not be available on some operation system).
It's kinda like if you said "htop is spawning ps" imo.

Btw I think that the ps C code doesn't really care about the time gap you're talking about, at least can't find anything relevant in the code (for example available here or here).

Normally one should go with sys/proc.h anyway (from htop code) and relevant.

I don't think that this would be possible but my hope is that this problem is handled smartly by them.

AFAIK (and I did some research on cpu computation while working on this library), cpu usage is not an exact science and will never be 100% accurate. Even if you had everything right, there are so many things that can happen in a cpu tick no?

[simonepri]

Even if you had everything right, there are so many things that can happen in a cpu tick no?

Yes, right. But I expect that a native implementation is much faster. I don't know if the overhead introduced by the creation of a new process actually removes this gain. We should try with a benchmark.

[soyuka]

Whew tests are finally green on windows, os x and linux :).

We should try with a benchmark.

Oh yes, and I think that I'll definitely do one soon to compare this with multiple processes!

[soyuka]

And the winner is:

# procfile
ok ~109 ms (0 s + 108964948 ns)

# ps
ok ~9.91 ms (0 s + 9906167 ns)

[simonepri]

Wow that's an order of magnitude 😮 The bench was with how many processes?

[soyuka]

The bench was with how many processes?

Every process on my machine (see bench.js) ~248

I really need to study more the ps code this is good shit :P.

[simonepri]

What about performances with a single pid?

[soyuka]

Many thanks for the reviews it's greatly appreciated! Updated according to comments.

[simonepri]

Your welcome! Since I'm not able to work directly on your branch I contribute in that way 😁

[soyuka]

I set up spawn for ps and unit tested wmic as well :)

[simonepri]

I set up spawn for ps

Amazing! But yeah lets benchmark it as well. Ideally since your are processing it "live" it should be faster but you never know. 🙈

[soyuka]

Benching 246 process
NANOBENCH version 2
> /home/abluchet/.nvm/versions/node/v8.1.3/bin/node test/bench.js

# procfile
ok ~70 ms (0 s + 70322060 ns)

# ps
ok ~9.99 ms (0 s + 9991419 ns)

all benchmarks completed
ok ~80 ms (0 s + 80313479 ns)

I don't see any improvement or degradation.

[simonepri]

Then if it's the same let's use the spawn version. It should also use less memory since it spawns only a single process.