Skip to content
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

feat: implement process.cpuUsage (Deno.cpuUsage) #27217

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

CyanChanges
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2024

CLA assistant check
All committers have signed the CLA.

@CyanChanges CyanChanges marked this pull request as ready for review December 3, 2024 16:09
@Hajime-san
Copy link
Contributor

Does it need to check --allow-sys permission?

@CyanChanges
Copy link
Author

CyanChanges commented Dec 4, 2024

Does it need to check --allow-sys permission?

memoryUsage doesn't check --allow-sys, so i guess no?
Also process.cpuUsage() process.memoryUsage() are both for current process.

@CyanChanges CyanChanges marked this pull request as ready for review December 4, 2024 05:34
@CyanChanges
Copy link
Author

Do I need to change something after adding an op? After add the op_runtime_cpu_usage, some tests fails

@CyanChanges CyanChanges marked this pull request as draft December 5, 2024 16:42
@irbull
Copy link
Contributor

irbull commented Dec 5, 2024

The operations are listed in 2 different arrays, and they must have the same order:

See
https://github.com/CyanChanges/deno/blob/main/runtime/ops/os/mod.rs#L45
https://github.com/CyanChanges/deno/blob/main/runtime/ops/os/mod.rs#L15

Look at the last 2 elements of both lists.

runtime/ops/os/mod.rs Outdated Show resolved Hide resolved
@CyanChanges CyanChanges marked this pull request as ready for review December 6, 2024 04:20
runtime/ops/os/mod.rs Outdated Show resolved Hide resolved
@CyanChanges CyanChanges marked this pull request as draft December 9, 2024 15:06
@CyanChanges CyanChanges marked this pull request as ready for review December 9, 2024 15:06
@CyanChanges CyanChanges marked this pull request as draft December 13, 2024 07:22
@CyanChanges CyanChanges marked this pull request as ready for review December 13, 2024 07:23
Signed-off-by: Cyan <[email protected]>
@CyanChanges CyanChanges marked this pull request as draft December 13, 2024 07:32
Signed-off-by: Cyan <[email protected]>
Signed-off-by: Cyan <[email protected]>
@CyanChanges CyanChanges marked this pull request as ready for review December 13, 2024 07:40
Signed-off-by: Cyan <[email protected]>
Signed-off-by: Cyan <[email protected]>
@CyanChanges CyanChanges requested a review from irbull December 13, 2024 08:37
@CyanChanges
Copy link
Author

@irbull can you please review again?

@irbull
Copy link
Contributor

irbull commented Dec 15, 2024

@irbull can you please review again?

I'm not on the core team so I can't actually review this, but I'll look at this again tomorrow and leave my thoughts, and I can ask on Discord for someone on the team to take a final look.

@bartlomieju bartlomieju requested review from littledivy and removed request for irbull December 15, 2024 07:22
user: 0,
system: 0,
};
return Deno.cpuUsage();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but maybe define this the same way that memoryUsage is defined. That is, create a function and then just Process.prototype.memoryUsage = memoryUsage;.

I was going to suggest moving this next to memoryUsage too, but it seems that the prototypes are mostly organized in alphabetical order (not all), so maybe leave it here.

external: usize,
#[op2]
#[serde]
fn op_runtime_cpu_usage() -> (usize, usize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the other ops return a Struct, not just a tuple. While a tuple may be faster (I'm not sure how much performance matters here) consistency might be better.

s.external_memory(),
);

(rss, heap_total, heap_used, external)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see you changed the memory_usage one too. Consistency FTW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants