Move process.memoryUsage() to os.memoryUsage() (fix #988) #1079

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

I messed up with git, so that's why I opened this new pull request (closing the old one now). Tried fixing the things that ry commented on:

"Please do process.memoryUsage() (leave it in the test) and please stub out the other platform's GetProcessMemory() functions and put assert(0 && "implement me") int them."

If I misunderstood anything, please say so.

mscdex commented May 21, 2011

IMHO this is better left on process and not os. memoryUsage() is the usage for the current node process, whereas os is for the entire system. There are already memory usage functions for os: http://nodejs.org/docs/v0.4.8/api/os.html#os.totalmem

This patch includes an extension to memoryUsage() so that it can be given a pid (as requested by Ryan). You can then use it to get memory usage for any process, so I think it belongs in os (as requested by Ryan).

mscdex commented May 21, 2011

Hmmm.... it still seems like it would be confusing though, at least to me. Users might think memoryUsage() gives the difference between totalmem() and freemem() or something and not expect it to return information about the current process when no arguments are passed.

I think that's primarily a documentation problem :)

mscdex commented May 21, 2011

Maybe, I guess I just wouldn't expect anything in the os module to give me local information (by default) :)

Which is exactly what os.memoryUsage() does. Gives you local information by default ;)

mscdex commented May 21, 2011

That's what I mean, that's what I wouldn't expect.

h0tw1r3 commented May 27, 2011

I tend to agree with mscdex, it would be confusing, but my reasoning is that the current OS object does not fit the intended purpose (it's not like "os" in python). Second is the inconsistency of returned data, heap for self, no heap for others. Gathering data on two different things would seem to indicate they should be called from two different methods, otherwise it feels like "magic" to me.

I do think being able to inspect other processes would be convenient for a number of reasons outside of memory usage.

May I suggest addressing this as a more general solution by providing an object for inspecting and signaling external processes.

Example:

try {
  var proc = os.processOpen(pid);
  console.log([proc.memory, proc.cmdline, proc.cwd, proc.environment]);
  proc.stdin.write("i will kill you\n");
  proc.signal(proc.SIG_KILL);
} catch(e) {
  // access denied, not found, fd closed, etc.
}

japj commented Jul 10, 2011

@ry any thoughts on this?

this should be moved to libuv since it deals with cross platform work

bnoordhuis closed this Jul 28, 2012

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