process: add threadCpuUsage #56467
process: add threadCpuUsage #56467
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
github-actions
bot
removed
the
request-ci
|
What about moving the platform dependent code to libuv? Maybe taking libuv/libuv#3120 to the finish line would be a good idea. |
| return THROW_ERR_THREAD_CPU_USAGE_FAILED(env); | ||
| } | ||
|
|
||
| fields[0] = ((((uint64_t)user_time.dwHighDateTime << 32) | |
There was a problem hiding this comment.
Can we use static cast rather than C cast?
| @@ -148,6 +149,46 @@ function wrapProcessMethods(binding) { | |||
| }; | |||
| } | |||
|
|
|||
| const threadCpuValues = new Float64Array(2); | |||
There was a problem hiding this comment.
I recommend moving this to C++ side and updating it. A similar implementation exist in node url
| // | ||
| // Returns those values as Float64 microseconds in the elements of the array | ||
| // passed to the function. | ||
| static void ThreadCPUUsage(const FunctionCallbackInfo<Value>& args) { |
There was a problem hiding this comment.
Nit: we can add a fast api call for this as well.
|
|
||
| struct rusage usage; | ||
|
|
||
| if (getrusage(RUSAGE_THREAD, &usage) == -1) { |
There was a problem hiding this comment.
Could you add a comment to each of these mysterious functions and explain what they do? It might be tricky for someone else who comes across this code in a few years.
| fields[1] = MICROS_PER_SEC * usage.system_time.seconds + | ||
| usage.system_time.microseconds; | ||
| #elif _WIN32 | ||
| HANDLE thread_ = GetCurrentThread(); |
There was a problem hiding this comment.
Can this throw? I'm not familiar with the API
| @@ -650,6 +715,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, | |||
| SetMethod(isolate, target, "availableMemory", GetAvailableMemory); | |||
| SetMethod(isolate, target, "rss", Rss); | |||
| SetMethod(isolate, target, "cpuUsage", CPUUsage); | |||
| SetMethod(isolate, target, "threadCpuUsage", ThreadCPUUsage); | |||
There was a problem hiding this comment.
Could you also update the typings folder with the correct type? That way, we’ll have even better intellisense support!
| // Replace the native function with the JS version that calls the native | ||
| // function. |
There was a problem hiding this comment.
Why don't we just implement the whole thing in C++? I’m a bit confused about this particular comment.
| if (prevValue) { | ||
| if (!previousValueIsValid(prevValue.user)) { | ||
| validateObject(prevValue, 'prevValue'); | ||
|
|
||
| validateNumber(prevValue.user, 'prevValue.user'); | ||
| throw new ERR_INVALID_ARG_VALUE.RangeError('prevValue.user', | ||
| prevValue.user); | ||
| } | ||
|
|
||
| if (!previousValueIsValid(prevValue.system)) { | ||
| validateNumber(prevValue.system, 'prevValue.system'); | ||
| throw new ERR_INVALID_ARG_VALUE.RangeError('prevValue.system', | ||
| prevValue.system); | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems a lot of this functionality can be removed if we move the implementation to cpp?
This PR add the
threadCpuUsagemethod toprocess.The method works exactly like
cpuUsagebut it returns thread specific metrics.This is already implemented (by me :)) in user-land in
https://www.npmjs.com/package/thread-cpu-usage.The PR is currently a draft as I'm gonna use the CI to see which platform don't support this. I'll add documentation updates once this is solved.