Add Deno.memoryInfo() #6417

marcopacini · 2020-06-21T22:17:32Z

Add Deno.freemem() that returns the amount of free memory as a number. Implemented using sys-info::mem_info.
Related to https://github.com/denoland/deno/issues/3802

CLAassistant · 2020-06-21T22:17:38Z

All committers have signed the CLA.

ry · 2020-06-22T12:09:40Z

cli/ops/os.rs

+  state.check_unstable("Deno.freemem");
+  state.check_env()?;
+  match sys_info::mem_info() {
+    Ok(info) => Ok(JsonOp::Sync(json!(info.free))),


Maybe this op should be called mem_info instead and return the whole sys_info::MemInfo to JavaScript?

It make sense. I made it return only free memory because in issue https://github.com/denoland/deno/issues/3802 it is related to Node.js os.freemem() function.

That could be added later in std's node compat.

Sure, now that memoryInfo() returns all memory info the freemen() function can be something like this:

function freemen(): number { return memoryInfo().free; }

If you think it could be fine I can add it.

ry · 2020-06-22T12:10:41Z

cli/tests/unit/os_test.ts

+  assertNotEquals(Deno.freemem(), "");
+});
+
+unitTest({ perms: { env: false } }, function freememPerm(): void {


I'm not sure about the env permission...

Mee to. I just opted for the same permission used by other os functions.

ry

Nice job at implementing this - it looks correct. Thanks for the work!

I wonder if the allow-env permission is what we want here.

Also I'm curious about why you want this op? What's your use-case?

marcopacini · 2020-06-22T13:04:36Z

No use case 😅 but let me explain. I wrote a Prometheus client for Deno (ts-prometheus) but when I started to add default metrics (process stats) I got that they aren't accessible from Deno cli. But I got confused and started to implement a freemem function for os rather then a memoryUsage for process 🤦. When I realised it was too late… I already did it so I open the PR.

OT: I'm going to work on #5732 but I don't think there is an easy way to do it in Rust for every platform.

ry · 2020-06-22T13:13:45Z

I'm going to work on #5732 but I don't think there is an easy way to do it in Rust for every platform.

That would be a welcomed effort. I think https://docs.rs/sys-info/0.7.0/sys_info/struct.MemInfo.html has everything you need?

You can just modify this PR to expose Deno.memoryInfo() ... but we still have to figure out the permissions.

ry · 2020-06-22T15:19:07Z

@caspervonb Number has 53 bits of integer precision - that's about 9000 terabytes. That should be enough, I think...

marcopacini · 2020-06-22T18:18:25Z

Waiting a decision about permissions I modified Deno.memoryInfo() returning now a MemoryInfo object.

caspervonb · 2020-06-22T18:57:44Z

@caspervonb Number has 53 bits of integer precision - that's about 9000 terabytes. That should be enough, I think...

Errr.. yeah.... it's a double, was thinking 32bit for some reason 🤔

cli/tests/unit/os_test.ts

caspervonb

LGTM; some very minor nits on naming, sorry did not catch that earlier. We should at-least try to avoid snake_case in public APIs and the test case could be a little bit stronger.

caspervonb · 2020-06-24T10:18:41Z

cli/js/lib.deno.unstable.d.ts

+  export interface MemoryInfo {
+    total: number;
+    free: number;
+    avail: number;


available: number;

Ops, I forgot it 😊

caspervonb · 2020-06-24T10:19:38Z

cli/js/lib.deno.unstable.d.ts

+    avail: number;
+    buffers: number;
+    cached: number;
+    swap_total: number;


swapTotal : number;

You are right, I am going to change it. Probably it would be better if the linter could raise an error.

I think it would if the test used it, but seems like we don't lint type member names 🤔

caspervonb · 2020-06-24T10:20:16Z

cli/js/lib.deno.unstable.d.ts

+    buffers: number;
+    cached: number;
+    swap_total: number;
+    swap_free: number;


swapFree: number;

caspervonb · 2020-06-24T10:24:38Z

cli/tests/unit/os_test.ts

-unitTest({ perms: { env: true } }, function freemem(): void {
-  assertNotEquals(Deno.freemem(), "");
+unitTest({ perms: { env: true } }, function memoryInfo(): void {
+  assertNotEquals(Deno.memoryInfo(), {});


const info = Deno.memoryInfo(); assert(info.total); assert(info.free); assert(info.avail); // ... and so on to ensure that the fields are being serialized ; // I believe the linter will object to `swap_total` and `swap_free` having underscores.

I honestly can't figure out whether it is right check every field. Because memory_info function return an empty object when an error occurs. Otherwise it return a MemoryInfo object and I expect its fields are properly set. Or they can be undefined? I expect Rust code doesn't set fields to undefined but I'm honestly not sure.

Asserting n >= 0 should be fine.

In the exceptional case that it returns an error in Rust; an exception should be thrown in the JavaScript runtime as-well (e.g return an op-error).

marcopacini · 2020-07-06T14:18:53Z

By trying to implement process.memoryUsage() I noticed a potential conflict in getting memory info related to process and os. I think it could be better to have something like Deno.os.memoryUsage() and Deno.process.memoryUsage() or rather Deno.memoryInfo() could return an object like this:

{
    "os": {
        ...
    },
    "process": {
        ...
    }
}

What do you think?

bartlomieju · 2020-08-24T16:43:31Z

@marcopacini sorry for slow review. Your suggestion with Deno.memoryInfo() returning an object sounds good to me. Please rebase your branch

bartlomieju · 2020-09-09T09:30:45Z

Superseded by #7350

Add os.freemem()

5167148

marcopacini added 2 commits June 22, 2020 00:33

Fix format errors

4adf4e8

Fix unit tests

0f52c81

ry reviewed Jun 22, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Refactor return type

4906488

caspervonb reviewed Jun 24, 2020

View reviewed changes

cli/tests/unit/os_test.ts Outdated Show resolved Hide resolved

Update unit tests

d4b01db

caspervonb reviewed Jun 24, 2020

View reviewed changes

bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS labels Jun 24, 2020

bartlomieju added this to the 1.2.0 milestone Jun 24, 2020

marcopacini and others added 3 commits June 24, 2020 17:05

Remove snake case

6fb7cfb

Rename field

d7633b3

Merge branch 'master' into feature/os-freemem

793ef8a

marcopacini changed the title ~~Add os.freemem()~~ Add Deno.memoryInfo() Jul 6, 2020

bartlomieju modified the milestones: 1.2.0, 1.3.0 Jul 14, 2020

bartlomieju removed this from the 1.3.0 milestone Aug 11, 2020

humancalico mentioned this pull request Sep 5, 2020

feat(cli) add Deno.systemMemoryInfo() #7350

Merged

bartlomieju closed this Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deno.memoryInfo() #6417

Add Deno.memoryInfo() #6417

marcopacini commented Jun 21, 2020

CLAassistant commented Jun 21, 2020 •

edited

Loading

ry Jun 22, 2020

marcopacini Jun 22, 2020

hayd Jun 23, 2020

marcopacini Jun 23, 2020

ry Jun 22, 2020

marcopacini Jun 22, 2020

ry left a comment •

edited

Loading

marcopacini commented Jun 22, 2020 •

edited

Loading

ry commented Jun 22, 2020

This comment has been minimized.

ry commented Jun 22, 2020 •

edited

Loading

marcopacini commented Jun 22, 2020

caspervonb commented Jun 22, 2020

caspervonb left a comment •

edited

Loading

caspervonb Jun 24, 2020

marcopacini Jun 24, 2020

caspervonb Jun 24, 2020

marcopacini Jun 24, 2020

caspervonb Jun 24, 2020

caspervonb Jun 24, 2020

caspervonb Jun 24, 2020

marcopacini Jun 24, 2020

caspervonb Jun 25, 2020 •

edited

Loading

marcopacini commented Jul 6, 2020

bartlomieju commented Aug 24, 2020

bartlomieju commented Sep 9, 2020

Add Deno.memoryInfo() #6417

Add Deno.memoryInfo() #6417

Conversation

marcopacini commented Jun 21, 2020

CLAassistant commented Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

marcopacini commented Jun 22, 2020 • edited Loading

ry commented Jun 22, 2020

This comment has been minimized.

ry commented Jun 22, 2020 • edited Loading

marcopacini commented Jun 22, 2020

caspervonb commented Jun 22, 2020

caspervonb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspervonb Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

marcopacini commented Jul 6, 2020

bartlomieju commented Aug 24, 2020

bartlomieju commented Sep 9, 2020

CLAassistant commented Jun 21, 2020 •

edited

Loading

ry left a comment •

edited

Loading

marcopacini commented Jun 22, 2020 •

edited

Loading

ry commented Jun 22, 2020 •

edited

Loading

caspervonb left a comment •

edited

Loading

caspervonb Jun 25, 2020 •

edited

Loading