-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
core: implement resourceUsage() #1568
Conversation
resource_usage->Set(env->voluntary_context_switches(), | ||
Number::New(env->isolate(), rusage.ru_nvcsw)); | ||
resource_usage->Set(env->involuntary_context_switches(), | ||
Number::New(env->isolate(), rusage.ru_nivcsw)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of the second argument seems off by one space.
LGTM with comments. Maybe this can get one more pair of eyeballs, @iojs/collaborators? |
|
||
Returns the resource usage measures | ||
|
||
Note: Not all resource usage measures are available on Windows. Fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know exactly which fields aren't available on Windows? If so, you could call them out.
libuv already provides the `uv_getrusage` function to get the resource mesaures for the current process. This changes implements the new `process.resourceUsage()` function to make use of it. An example output looks like: { userCpuTimeUsedSec: 0, userCpuTimeUsedMs: 159729, systemCpuTimeUsedSec: 0, systemCpuTimeUsedMs: 30597, maxResidentSetSize: 20426752, sharedMemSize: 0, integralUnsharedDataSize: 0, integralUnsharedStackSize: 0, pageReclaims: 3002, pageFaults: 2250, swaps: 0, blockInputOperations: 0, blockOutputOperations: 0, ipcMessagesSent: 55, ipcMessagesReceived: 54, signalsReceived: 0, voluntaryContextSwitches: 92, involuntaryContextSwitches: 374 } Not supported fields on windows are zero.
@bnoordhuis @cjihrig thanks for your response! I removed the whitespace from the second argument. |
LGTM A question for everyone: do we want to deviate from the UNIX rusage field names? I'd say 'yes' because the UNIX names are pretty cryptic and don't look like idiomatic JS identifiers. |
@bnoordhuis Using different names sounds appropriate. |
+1 to more JS-like names. |
Can we make the fields not-present on Windows instead? Would be a much better experience. |
Easy enough. We just need to identify the fields that don't exist on Windows, |
If the fields aren't present on Windows, would that make the behavior different to e.g |
Hmm, that is true. It would be more consistent with |
I'd prefer unix field names with
becauseLongCamelCasedIdentifiersAreUnreadable If you still want to use those, maybe borrow some identifiers from here (just something I found searching for At least they are shorter. |
If it uses js-style identifiers, please document the mapping to the underlying getrusage fields, so that its possible to find reference documentation. Should do the same either way for Windows, so its possible to look up more information for Windows, as well. |
@sam-github you mean something like a comment behind the example output:
@rlidwka thanks! I thought that the unix names are somehow cryptic. (In case of JS names: take @domenic it's not only ``os.loadavg() |
I guess I don't have strong enough feelings to override the precedent set for other os.* things. It seems nicer if they're absent but it's not like I have some program in mind that would benefit from the change. |
I would prefer if functions that cannot be meaningfully used do not exist, as opposed to returning garbage. |
+1 to keeping the properties on Windows and just setting them to 0. If nothing else it prevents the need to have to branch on |
I agree with @mscdex. |
how about returning -1 instead of 0? I agree that keeping properties is the way to go. |
@jbergstroem -1 could look like an error code or an error that happened. 0 has the advantage that you could e.g. perform calculations with them and you don't get a negative result for the values. |
Copy libuv/libuv#342 |
Agree with @jbergstroem that 0 is a mediocre "not supported" indicator. Why not |
@bnoordhuis think this is cool to land? seems it was on track before some last minute bikeshedding occurred. It LGTM. |
@@ -0,0 +1,23 @@ | |||
var assert = require('assert'); | |||
var common = require('../common'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter is probably going to complain that this should have 'use strict'
at the top and that the imports should be const
.
What are the fields that are not supported on Windows? If it's more than just a few of them then I'm -0 on this I think. |
@rvagg, over 50% fields are missing for windows in libuv, even though there is a Windows98-days helpers library (PDH) provided by win CRT till date for measuring low-level CPU related performance stats etc., which apparently emits all the information required to match Unix implementation (else how would game devs pull off so many projects on windows in the past and current century?). I have no dev experience with it therefore this request is blocking: libuv/libuv#342. Can anyone take a stab? Maybe guys from https:/Microsoft/node get excited to chip in on this one; @jianchun. |
I'm not a fan then, it's going to look second-class, the @nodejs/platform-windows opinions? |
I agree, we shouldn't encourage people to write non-portable code. |
I have added libuv/libuv#342 to our queue of Windows fixes. Preferably, this PR would wait for that feature to be fully implemented in libuv on all platforms. If this cannot wait, I would recommend specifying what is supported on which platform in the docs. Since some of these fields might be platform-specific anyway, I think leaving them undefined where they are not implemented makes more sense than setting them to 0. It makes writing portable code easier. |
@orangemocha @rvagg @bnoordhuis ... what do we want to do with this one? Still pending? |
@jasnell Still pending, I think. libuv/libuv#342 is still open. I don't expect anyone from the libuv team to work on that anytime soon though, it would have to come from outside. |
@jasnell still planning to work on libuv/libuv#342 |
I could work on the windows part. I guess there won't be windows equivalents for the whole struct. So if we want to go further with it, we should be planning on windows specific docs. |
libuv/libuv#342 is closed. Should we remove the |
Removed the label. The PR needs a rebase, though. |
Since the repo that this PR is against is now listed as |
libuv already provides the
uv_getrusage
function to get theresource mesaures for the current process.
This changes implements the new
process.resourceUsage()
functionto make use of it.
An example output looks like:
Not supported fields on windows are zero.