process.cpuUsage() - implementation, doc, tests

[pmuellr]

Checklist

• tests and code linting passes (make -j4 lint test)
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

current planned commit message

process: add cpuUsage()

The cpuUsage() function will return user and system CPU usage timings for the
current process.

Drive-by fix to process.hrtime() doc to align with preferred non-usage of "you"
in function description.

Affected core subsystem(s)

process module

Description of change

add the function cpuUsage() to process

[pmuellr]

See discussion from original PR #5565

[Fishrock123]

const please :)

[addaleax]

Hm, is there any concrete advantage to splitting up the 64-bit numbers into two lanes and reassembling them in JS? Like, wouldn’t using a Float64Array + doubles be a bit easier and have the same results?

[eljefedelrodeodeljefe]

nit: const

[eljefedelrodeodeljefe]

Awesome. LGTM with nits.

[pmuellr]

@addaleax I copied the style of hrtime() for cpuUsage(). I think it may have been possible to combine all the numeric bits into a single large int, because the cpu usage values are to the microsecond precision, instead of nanosecond (like hrtime()). Figured it would be better to copy the style instead of doing something radically different.

[addaleax]

@pmuellr Sounds reasonable – was just wondering because it seemed a little odd :)

[mhdawson]

I'd suggest a CI run so that we confirm uv_getrusage is fully supported across all platforms(should be). Otherwise we might need to add info in that respect to the docs. I can't tell if you can do that so I launched one for you:

https://ci.nodejs.org/job/node-test-pull-request/2243/

[eljefedelrodeodeljefe]

Should be fine. On Windows only those two fields this PR is using are populated. The unices share the same function, but don't populate same results for some fields. nit: The inline comment could be rephrased, omitting the part that uv_getrusage is exposed.

[Fishrock123]

cc @mscdex who appears to have primarily reviewed the previous PR.

[mscdex]

I didn't really review the last PR, I was mostly just seeing if there was interest in having a separate .metrics or similarly-named namespace for memory, cpu, other resource usage.

Just briefly looking over this PR, I'm wondering if it might just be better to drop the extra * 1000 used to keep the same units as hrtime(). I think it would help in the case if a lot of CPU time is used by the process, where it would reach Number.MAX_SAFE_INTEGER much quicker than without the extra multiplication. hrtime() uses nanoseconds because it actually has that kind of resolution, but these CPU usage values do not.

[jasnell]

Just a style nit...

if (_cpuUsage(cpuValues) !== 0)
  throw new Error('unable to obtain cpu usage time');

[jasnell]

I'd be +1 on not having the additional * 1000 but don't think it's critical.

[jasnell]

definitely use const wherever possible :-)

[jasnell]

Generally LGTM with a few nits.

[Fishrock123]

@pmuellr I'd probably just make the commit message something like

process: add new `cpuUsage()` method

[pmuellr]

@mhdawson that build is green, so guess it passes muster?

• https://ci.nodejs.org/job/node-test-pull-request/2243/

[pmuellr]

@eljefedelrodeodeljefe

The inline comment could be rephrased, omitting the part that uv_getrusage is exposed.

My usual excuse here - I copied from Hrtime() just above it :-)

• https://github.com/pmuellr/node/blob/c9aae2dfad942b60a6d887830319cdd20ec47897/src/node.cc#L2200

[pmuellr]

@mscdex

I'm wondering if it might just be better to drop the extra * 1000 used to keep the same units as hrtime(). I think it would help in the case if a lot of CPU time is used by the process, where it would reach Number.MAX_SAFE_INTEGER much quicker than without the extra multiplication. hrtime() uses nanoseconds because it actually has that kind of resolution, but these CPU usage values do not.

I think the argument comes down to aligning w/hrtime()'s nano-second resolution, to presumably make life easier for users, or to stay true to the rusage() precision or just optimize things a bit.

I'm siding with making life easier for users.

[mscdex]

@pmuellr What does aligning with hrtime() output really gain us though? Are you expecting users to pass values from cpuUsage() to hrtime() or vice-versa?

[pmuellr]

I've added the proposed commit message in the header of this PR. Currently:

---

process: add cpuUsage()

The cpuUsage() function will return user and system CPU usage timings for the
current process.

Drive-by fix to process.hrtime() doc to align with preferred non-usage of "you"
in function description.

[pmuellr]

@mscdex ah, yes, forgot to mention that

One expected uses of cpuUsage() would be to get the % of cpu this process is currently using. You can do that by dividing a cpuUsage() diff from a hrtime() diff. Example:

'use strict'

var startTime  = process.hrtime()
var startUsage = process.cpuUsage()

// spin the CPU for 500 milliseconds
var now = Date.now()
while (Date.now() - now < 500)

var elapTime = process.hrtime(startTime)
var elapUsage = process.cpuUsage(startUsage)

var elapTimeMS = secNSec2ms(elapTime)
var elapUserMS = secNSec2ms(elapUsage.user)
var elapSystMS = secNSec2ms(elapUsage.system)
var cpuPercent = Math.round(100 * (elapUserMS + elapSystMS) / elapTimeMS)

console.log('elapsed time ms:  ', elapTimeMS)
console.log('elapsed user ms:  ', elapUserMS)
console.log('elapsed system ms:', elapSystMS)
console.log('cpu percent:      ', cpuPercent)

function secNSec2ms (secNSec) {
  return secNSec[0] * 1000 + secNSec[1] / 1000000
}

Whenever you're dealing with hrtime(), it seems you will likely be wanting a single number to deal with, which you can determine the precision of yourself. In this case, it converted the tuples into millisecond values. Reused the same conversion function for both hrtime() and cpuUsage() values.

[mscdex]

@pmuellr Right, but what I meant was users are not going to be doing process.cpuUsage(startTime) or process.hrtime(startUsage.user).

I think I am still -1 on artificially adjusting the cpu usage values.

[pmuellr]

@mscdex yes, correct, you'd never mix/match values from cpuUsage() and hrtime() to get diff values.

Still think there's value in having a shared model for these, so you can have a single (tuple) -> value conversion fn that works with both.

But it's not a deal-breaker for me or anything.

[trevnorris]

Comments should be capitalized and punctuated.

[santigimeno]

Extra space before =

[pmuellr]

Thanks for the comments @santigimeno .

Pushed a new commit.

Using assert.notStrictEqual didn't seem to make sense to me here, as the intent was to check for null || undefined, so ... would have expected maybe assert.notEqual. But looking at the test as a whole, realized is Number.isFinite is already checking null/undefined, and that I wasn't actually checking result in the first place, so removed the two field asserts, replaced with an assert(result) instead.

[santigimeno]

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2430/

[jasnell]

CI looks good except for unrelated build bot failure (passed previously on that bot). Landing...

[jasnell]

Landed in 52cb410. Thanks @pmuellr !

[pmuellr]

Thanks everyone! Great way to end the week.