Add direct access to rss without iterating pages by Aschen · Pull Request #34291 · nodejs/node · GitHub

Accessing the rss value through process.memoryUsage().rss can be expensive because this method will also generate memory usage statistics by iterating on each page.

The overhead can be quite huge when a lot of memory has been allocated, consider this snippet:

rss-access.js

const ar = []

for (let i = parseInt(process.argv[2], 10); i--; ) {
  ar.push({
    very: 'This is a lot of datazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz',
    big: {
      object: 'I m blue tadoudidadouda I m blue tadoudidadouda I m blue tadoudidadouda'
    }
  })
}

let start = process.hrtime();

process.memoryUsage().rss

let hrend = process.hrtime(start);

console.log(hrend[1] / 1000000, 'memoryUsage()');

start = process.hrtime();

process.rss()

hrend = process.hrtime(start);

console.log(hrend[1] / 1000000, 'rss()')

$ ./out/Release/node test.js 1000
0.361226 memoryUsage()
0.05305 rss()

$ ./out/Release/node test.js 10000000
1.208851 memoryUsage()
0.075418 rss()

This PR aim to offer an API to access directly the rss value in a (almost) constant time.

// Don't
process.memoryUsage().rss;

// Do
process.memoryUsage.rss();

EDIT: 2020-07-13

Following @bnoordhuis proposal I added a rssOnly option to process.memoryUsage instead of adding the new process.rss method:

process.memoryUsage({ rssOnly: true });
{
  rss: 36270080,
  heapTotal: 0,
  heapUsed: 0,
  external: 0,
  arrayBuffers: 0
}

EDIT: 2020-08-11

Following @jasnell and @bnoordhuis advices the rss value is now accessible through process.memoryUsage.rss

See #33384

Checklist

[/] make -j4 test (UNIX), or vcbuild test (Windows) passes (Unix is ok but unfortunately I don't have a windows computer)
tests and/or benchmarks are included
documentation is changed or added (I will add the documentation once we agree on the API)
commit message follows commit guidelines

For your (and everyone else's) consideration, perhaps:

the os module is a more appropriate place for a new method?
instead of a new method, make process.memoryUsage() take an optional { rssOnly: true } options object?

This is not reliable because the RSS can change between the two calls.

Suggested change

      
              // Get the double array pointer from the Float64Array argument.
          
              CHECK(args[0]->IsFloat64Array());
          
              Local<Float64Array> array = args[0].As<Float64Array>();
          
              CHECK_EQ(array->Length(), 1);
          
              Local<ArrayBuffer> ab = array->Buffer();
          
              double* fields = static_cast<double*>(ab->GetBackingStore()->Data());
          
              fields[0] = rss;
          
              args.GetReturnValue().Set(static_cast<double>(rss));

Float64Array is overkill if you're returning a single floating-point value. You can expose the method directly, without going through a JS method first.

@bnoordhuis

IMHO the os module is not the right place for that because as far as I understand it's information about the whole OS and not the currently running process.

But I like your idea of a { rssOnly: true } option object 👍

Note: I'm not happy with the process.rss API, if someone have a better suggestion I'm in 👍

It would be possible to use process.memoryUsage.rss();. I am not certain if this might be confusing for some people though.

It would be possible to use process.memoryUsage.rss();. I am not certain if this might be confusing for some people though.

I thought about this one but I couldn't figure an easy way to document it since it will involve a property on a function which is a function

@bnoordhuis I updated the PR with the rssOnly option so you may want to review the code again

Suggested change

      
            const result = process.memoryUsage({ rssOnly: true });
          
            process.memoryUsage();
          
            const result = process.memoryUsage({ rssOnly: true });

Then re-run the test. :-)

(You need to clear the other fields when rssOnly=true or they'll contain stale values.)

Leftover from the previous iteration of the PR? There's no process.rss() anymore.

@Aschen :

It would be possible to use process.memoryUsage.rss();. I am not certain if this might be confusing for some people though.

I thought about this one but I couldn't figure an easy way to document it since it will involve a property on a function which is a function

Take a look at the documentation for process.hrtime.bigint() for an example of this pattern. I think I'd prefer the process.memoryUsage.rss() approach also.

@bnoordhuis Are you also ok with the process.memoryUsage.rss() approach?

@Aschen Sure, there's precedent, e.g., fs.realpath.native().

I got a strange error on the file node_snapshop.cc in the compilation even if I didn't modify this file 🤔

https://github.com/nodejs/node/pull/34291/checks?check_run_id=1371318709

LD_LIBRARY_PATH=/home/runner/work/node/node/out/Release/lib.host:/home/runner/work/node/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /home/runner/work/node/node/out/Release/obj/gen; "/home/runner/work/node/node/out/Release/node_mksnapshot" "/home/runner/work/node/node/out/Release/obj/gen/node_snapshot.cc"
Unknown external reference 0x55a462331140.
<unresolved>
Illegal instruction (core dumped)
node.target.mk:26: recipe for target '/home/runner/work/node/node/out/Release/obj/gen/node_snapshot.cc' failed
rm 2ad06b2c9baade6b27c5c82cdcc7af11c369378d.intermediate 9ac559b3661ef3e3ae7143f36ccc6691e578b31c.intermediate 02a486ff0aa87df511c38b125409357fdc559df2.intermediate 5e4cf980d3dc346b9b9ba01305d21285c5aecbf4.intermediate
make[2]: *** [/home/runner/work/node/node/out/Release/obj/gen/node_snapshot.cc] Error 132
Makefile:104: recipe for target 'node' failed
make[1]: *** [node] Error 2
Makefile:530: recipe for target 'build-ci' failed
make: *** [build-ci] Error 2
Error: Process completed with exit code 2.

I find can be slow quite vague - in special as a caller can't do anything to avoid this to test if it will be slow.
Are we talking about µs, ms, or seconds here?

It depends on the program size in memory. Access the RSS take a fixed time but compute heap statistics is quite costly since you have to iterate over every memory page.
The difference can be in seconds. (see the benchmark in the PR description)

The sample in PR description prints milli seconds not seconds so not that bad.
I tried it local on my machine and even for 4GB heaps it was below 0.2ms. But for sure the rss only API is much faster.

At the beginning I have done this PR because I found very strange results while benchmarking promises (#33384).
So even if the difference is not that huge it can be misleading in some cases

I got a strange error on the file node_snapshop.cc in the compilation even if I didn't modify this file

I think you have to add registry->Register(Rss) in RegisterProcessMethodsExternalReferences (node_process_methods.cc).

@nodejs/process

PR-URL: #36768 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #36769 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36766 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Accessing the rss value through memoryUsage() can be expensive because this method will also generate memory usage statistics by iterating on each page. This commit intend to offer a more direct access to rss value. Refs: #33384 PR-URL: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #34291 PR-URL: #36781 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36768 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #36769 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36766 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412) * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603) * crypto: * implement basic secure heap support (James M Snell) [#36779](#36779) * fixup bug in keygen error handling (James M Snell) [#36779](#36779) * introduce X509Certificate API (James M Snell) [#36804](#36804) * implement randomuuid (James M Snell) [#36729](#36729) * doc: * update release key for Danielle Adams (Danielle Adams) [#36793](#36793) * add dnlup to collaborators (Daniele Belardi) [#36849](#36849) * add panva to collaborators (Filip Skokan) [#36802](#36802) * add yashLadha to collaborator (Yash Ladha) [#36666](#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291) * v8: * fix native constructors (ExE Boss) [#36549](#36549)

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) (#29412) * support AbortSignal in fork (Benjamin Gruenbaum) (#36603) * crypto: * implement basic secure heap support (James M Snell) (#36779) * fixup bug in keygen error handling (James M Snell) (#36779) * introduce X509Certificate API (James M Snell) (#36804) * implement randomuuid (James M Snell) (#36729) * doc: * update release key for Danielle Adams (Danielle Adams) (#36793) * add dnlup to collaborators (Daniele Belardi) (#36849) * add panva to collaborators (Filip Skokan) (#36802) * add yashLadha to collaborator (Yash Ladha) (#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) (#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) (#34291) * v8: * fix native constructors (ExE Boss) (#36549)

PR-URL: nodejs#36839 Refs: nodejs#34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36839 Refs: #34291 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: James M Snell <jasnell@gmail.com>