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.

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

process.cpuUsage() - implementation, doc, tests #6157

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@pmuellr
Contributor

pmuellr commented Apr 11, 2016

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

This comment has been minimized.

Contributor

pmuellr commented Apr 11, 2016

See discussion from original PR #5565

@pmuellr pmuellr referenced this pull request Apr 11, 2016

Closed

initial stab at process.cpuUsage() #5565

0 of 4 tasks complete
@Fishrock123

View changes

test/parallel/test-process-cpuUsage.js Outdated
var assert = require('assert');
// the default behavior, return { user: [int, int], system: [int, int]}
var result = process.cpuUsage();

This comment has been minimized.

@Fishrock123

Fishrock123 Apr 11, 2016

Member

const please :)

@addaleax

This comment has been minimized.

Member

addaleax commented Apr 11, 2016

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?

doc/api/process.markdown Outdated
while (Date.now() - now < 500);
console.log(process.cpuUsage(startUsage));
// { user: [ 0, 509627000 ], system: [ 0, 9993000 ] }

This comment has been minimized.

@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 11, 2016

Contributor

nit: const

doc/api/process.markdown Outdated
system code respectively, and may end up being greater than actual elapsed time
if multiple cpu cores are performing work for this process.
You may pass in the result of a previous call to `process.cpuUsage()` to get a

This comment has been minimized.

@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 11, 2016

Contributor

nit: avoid you reference, maybe: process.cpuUsage() can optionally take a previous call as argument to get a diff reading

This comment has been minimized.

@pmuellr

pmuellr Apr 11, 2016

Contributor

Sounds good. Note, I copied off of hrtime() which has the same usage of 'You ...', because that's what I copied from. Happy to do a drive-by fix to that as well ...

This comment has been minimized.

@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 11, 2016

Contributor

That'd be awesome. Hope no-one objects; it would save me a PR, since at some point, we wanted to review the usage those. Thx.

doc/api/process.markdown Outdated
with properties `user` and `system`, whose values are arrays of integers as
`[seconds, nanoseconds]` tuples. These values measure time spent in user and
system code respectively, and may end up being greater than actual elapsed time
if multiple cpu cores are performing work for this process.

This comment has been minimized.

@eljefedelrodeodeljefe
@eljefedelrodeodeljefe

This comment has been minimized.

Contributor

eljefedelrodeodeljefe commented Apr 11, 2016

Awesome. LGTM with nits.

@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 11, 2016

@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

This comment has been minimized.

Member

addaleax commented Apr 11, 2016

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

@mhdawson

This comment has been minimized.

Member

mhdawson commented Apr 11, 2016

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

This comment has been minimized.

Contributor

eljefedelrodeodeljefe commented Apr 11, 2016

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

This comment has been minimized.

Member

Fishrock123 commented Apr 12, 2016

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

@mscdex

This comment has been minimized.

Contributor

mscdex commented Apr 12, 2016

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

View changes

lib/internal/process.js Outdated
process.cpuUsage = function cpuUsage(prevValue) {
const err = _cpuUsage(cpuValues);
if (err !== 0) throw new Error('unable to obtain cpu usage time');

This comment has been minimized.

@jasnell

jasnell Apr 12, 2016

Member

Just a style nit...

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

This comment has been minimized.

Member

jasnell commented Apr 12, 2016

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

@jasnell

View changes

test/pummel/test-process-cpuUsage.js Outdated
require('../common');
var assert = require('assert');
var start = process.cpuUsage();

This comment has been minimized.

@jasnell

jasnell Apr 12, 2016

Member

definitely use const wherever possible :-)

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 12, 2016

Generally LGTM with a few nits.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 12, 2016

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

process: add new `cpuUsage()` method
@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

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

@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

@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 :-)

@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

@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

This comment has been minimized.

Contributor

mscdex commented Apr 12, 2016

@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

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

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

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

@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

This comment has been minimized.

Contributor

mscdex commented Apr 12, 2016

@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

This comment has been minimized.

Contributor

pmuellr commented Apr 12, 2016

@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

View changes

lib/internal/process.js Outdated
// the second element of user/system comes back from libuv as
// microseconds, but we convert to nanoseconds here (* 1000) to
// be consistent with hrtime()

This comment has been minimized.

@trevnorris

trevnorris Apr 12, 2016

Contributor

Comments should be capitalized and punctuated.

argument to the function, to get a diff reading.
```js
const startUsage = process.cpuUsage();

This comment has been minimized.

@santigimeno

santigimeno Apr 29, 2016

Member

Extra space before =

// Ensure that the return value is the expected shape.
function validateResult(result) {
assert(result.user != null);
assert(result.system != null);

This comment has been minimized.

@santigimeno

santigimeno Apr 29, 2016

Member

Maybe use assert.notStrictEqual here.

updates from PR review
* fixed whitespace issue on doc/api/process.md

* improved test in test/parallel/test-process-cpuUsage.js, as below:

changed:

    assert(result.user != null);
    assert(result.system != null);

to:

    assert.notEqual(result, null);

The next tests below the original asserts called Number.isFinite() on the
fields in result, which also return false on null/undefined check, so
the removed asserts were dups.  But realized we weren't actually checking
the result parameter for null/undefined.
@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 29, 2016

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

This comment has been minimized.

Member

santigimeno commented Apr 29, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 29, 2016

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

jasnell added a commit that referenced this pull request Apr 29, 2016

process: add process.cpuUsage() - implementation, doc, tests
Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

PR-URL: #6157
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Apr 29, 2016

Landed in 52cb410. Thanks @pmuellr !

@jasnell jasnell closed this Apr 29, 2016

@pmuellr

This comment has been minimized.

Contributor

pmuellr commented Apr 29, 2016

Thanks everyone! Great way to end the week.

Fishrock123 added a commit that referenced this pull request May 4, 2016

process: add process.cpuUsage() - implementation, doc, tests
Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

PR-URL: #6157
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016

process: add process.cpuUsage() - implementation, doc, tests
Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

PR-URL: nodejs#6157
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
  - Please see our blog post for more info on the security contents of this release:
  - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

Fishrock123 added a commit that referenced this pull request May 5, 2016

2016-05-05, Version 6.1.0 (Current)
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
- Please see our blog post for more info on the security contents of
this release:
- https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
- Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
- Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
- This is set to `100` by default.
- Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
- Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557

@MylesBorins MylesBorins referenced this pull request Jan 6, 2017

Closed

Potential Semver Minor Backports #177

9 of 9 tasks complete

MylesBorins added a commit to MylesBorins/node that referenced this pull request Jan 14, 2017

process: add process.cpuUsage() - implementation, doc, tests
Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

PR-URL: nodejs#6157
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

jasnell added a commit that referenced this pull request Jan 16, 2017

process: add process.cpuUsage() - implementation, doc, tests
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins added a commit that referenced this pull request Jan 24, 2017

process: add process.cpuUsage() - implementation, doc, tests
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins added a commit that referenced this pull request Feb 1, 2017

process: add process.cpuUsage() - implementation, doc, tests
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@sv2 sv2 referenced this pull request Aug 2, 2017

Closed

Add memory and CPU stats #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment