feat: Implement process.getProcessMemoryInfo to get the process memory usage #14847

nitsakh · 2018-09-28T00:14:18Z

Description of Change

This implements the memory footprint API.
Since version 67, chromium has moved to using memory footprint internally as the cross platform memory indicator. They got rid of other various cross platform APIs that mean different things on different platforms #13447.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines

Release Notes

Notes: Add getMemoryFootprint API

alexeykuzmin · 2018-09-30T22:46:09Z

atom/common/api/atom_bindings.cc

+#include "services/resource_coordinator/public/cpp/memory_instrumentation/global_memory_dump.h"
+#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
+// Must be the last in the includes list.
+#include "atom/common/node_includes.h"


Please make sure that clang-format won't "fix" the order of includes here.
Usually leaving an empty line before such an include helps.

I thought the comment helps, and prevents sorting. But will verify, thanks.

alexeykuzmin · 2018-09-30T22:58:45Z

spec/api-process-spec.js

-  // })
+  describe('process.getMemoryFootprint()', () => {
+    it('resolves promise successfully with non zero value', (done) => {
+      process.getMemoryFootprint().then((memoryFootprint) => {


Can you please use async / await here?

Still WIP, but yes. 👍

zcbenz · 2018-10-04T01:58:00Z

The crash should have been fixed now.

ckerr · 2018-10-04T14:44:25Z

atom/common/api/atom_bindings.cc

+#include "services/resource_coordinator/public/cpp/memory_instrumentation/global_memory_dump.h"
+#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
+
+// Must be the last in the includes list.


This sentence should end in because ${reason}

ckerr · 2018-10-04T14:51:04Z

atom/common/api/atom_bindings.cc

+    }
+    if (!resolved) {
+      promise->RejectWithErrorMessage(
+          R"(Failed to find current process memory details in memory dump)");


Why is the R" prefix needed -- there aren't any escaped characters in the string?

Why is the error message in parenthesis?

This was a long sentence, so I think the linter suggested I do this.
The parentheses are part of the syntax for raw string literals, and will not be in the error message; but I could be wrong on this one.

ckerr · 2018-10-04T14:52:37Z

atom/common/api/atom_bindings.cc

+      promise->RejectWithErrorMessage(
+          R"(Failed to find current process memory details in memory dump)");
+    }
+  } else {


Would be slightly cleaner to avoid unnecessary nesting by getting the error condition out of the way first:

if (!success) { promise->RejectWithErrorMessage("..."); return; } bool resolved = false; for (...

to avoid unnecessary nesting

ckerr · 2018-10-04T15:00:38Z

spec/api-process-spec.js

+      let memoryFootprint = await process.getMemoryFootprint()
+      expect(memoryFootprint).to.be.a('number').greaterThan(0)
+    })
+  })


Needs a writeup in docs/api/process.md

Yeah, mainly waiting for reviews on the API itself before writing the documentation.

ckerr · 2018-10-04T15:36:26Z

atom/common/api/atom_bindings.cc

+    for (const memory_instrumentation::GlobalMemoryDump::ProcessDump& dump :
+         global_dump->process_dumps()) {
+      if (base::GetCurrentProcId() == dump.pid()) {
+        promise->Resolve(dump.os_dump().private_footprint_kb);


it looks like os_dump()'s return has both private_footprint_kb and shared_footprint_kb. That's is pretty similar to the MemoryInfo API we were using before, which had privateBytes and sharedBytes.

Would it make sense to keep using MemoryInfo as a return value and store both values in it?

+1, it would be great to expose more than just a single number here. It looks like the os_dump also has resident_set_kb, which would be nice to expose. It looks like you can also request specific allocator dumps from the memory instrumentation code (and starting in M69, the list of dumps to request is becoming non-optional: https://chromium.googlesource.com/chromium/src/+/69.0.3497.106/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h#78). It might be cool to expose those too? Though i'm not entirely sure what sort of metrics you can request dumps of.

Could this be used as a new backend for app.getAppMetrics(), which was in 3.0 but removed in master due to the backend API disappearing?

And, does it make sense to do this?

That's a good idea. 👍
Do we want to keep the same api also, getProcessMemoryInfo? It will have to return a promise though.
What about working set size and peak working set size? The original API returned their values on all platforms, but those terms have meaning only on windows.
Please ignore, didn't see the above two messages before responding.

Yes, I definitely want to add resident_set_kb and shared_footprint_kb to the same API. So. we can probably leave the same getProcessMemoryInfo API and return an updated MemoryInfo struct containing { resident_set_kb, shared_footprint_kb, private_footprint_kb}.
resident_set_kb is mostly a better replacement for the existing working set size attributes.
Regarding requesting specific allocator dumps, I'm not sure if they should be part of this process API. They can be part of the app.getAppMetrics api though.

nornagon · 2018-10-01T19:29:14Z

atom/common/api/atom_bindings.cc

+  scoped_refptr<util::Promise> promise = new util::Promise(isolate);
+  memory_instrumentation::MemoryInstrumentation::GetInstance()
+      ->RequestGlobalDump(std::vector<std::string>(),
+                          base::AdaptCallbackForRepeating(base::BindOnce(


base::AdaptCallbackForRepeating is deprecated and not advised. Can we use BindRepeating here (or justify the use of AdaptCallbackForRepeating?

nornagon · 2018-10-04T18:23:29Z

atom/common/api/atom_bindings.cc

+    for (const memory_instrumentation::GlobalMemoryDump::ProcessDump& dump :
+         global_dump->process_dumps()) {
+      if (base::GetCurrentProcId() == dump.pid()) {
+        promise->Resolve(dump.os_dump().private_footprint_kb);


+1, it would be great to expose more than just a single number here. It looks like the os_dump also has resident_set_kb, which would be nice to expose. It looks like you can also request specific allocator dumps from the memory instrumentation code (and starting in M69, the list of dumps to request is becoming non-optional: https://chromium.googlesource.com/chromium/src/+/69.0.3497.106/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h#78). It might be cool to expose those too? Though i'm not entirely sure what sort of metrics you can request dumps of.

atom/common/api/atom_bindings.cc

nornagon · 2018-10-24T22:26:29Z

atom/common/api/atom_bindings.cc

+#if defined(OS_LINUX)
+      dict.Set("residentSetBytes", osdump.resident_set_kb);
+#elif defined(OS_WIN)
+      dict.Set("workingSetSize", osdump.resident_set_kb);


Why name this differently on different OSes?

I decided to do that because resident set size is called working set size on windows.
Also, internally it calls the similarly named windows API which does that.

I think the concepts are similar enough to share the name "residentSetBytes" (though it looks like it would actually be residentSetKilobytes from the variable name resident_set_kb?). Chromium calls them the same thing.

Refs:

https://docs.microsoft.com/en-us/windows/desktop/psapi/process-memory-usage-information

The working set is the amount of memory physically mapped to the process context at a given time.

https://docs.microsoft.com/en-us/windows/desktop/procthread/process-working-set

The working set of a program is a collection of those pages in its virtual address space that have been recently referenced. It includes both shared and private data.

https://stackoverflow.com/a/21049737

RSS is the Resident Set Size and is used to show how much memory is allocated to that process and is in RAM. It does not include memory that is swapped out. It does include memory from shared libraries as long as the pages from those libraries are actually in memory.

nornagon

Just docs questions now I think

nornagon · 2018-11-02T23:23:44Z

docs/api/process.md

+* `private` Integer - The amount of memory not shared by other processes, such as
+  JS heap or HTML content.
+* `shared` Integer - The amount of memory shared between processes, typically
+  memory consumed by the Electron code itself.


Would be good to add (redundant) information on each of these property descriptions saying they're in KB, the note below is easy to miss.

nornagon · 2018-11-02T23:32:38Z

docs/api/process.md

+Returns `Object`:
+
+* `residentSet` Integer _Linux_ and _Windows_ - The amount of memory 
+currently pinned to actual physical RAM.


I expect we'll get questions about why this metric isn't available on mac. Do you have any references you could add here about what a comparable statistic might be on macOS, or why this number isn't available on mac?

docs/api/process.md

…usage

Co-Authored-By: nitsakh <nitsakh@icloud.com>

zcbenz · 2018-11-28T07:28:29Z

Electron crashed when running the tests:

ok 542 process module process.getIOCounters() returns an io counters object
[875:1120/205259.788721:FATAL:atom_browser_main_parts.cc(264)] Check failed: self_. 
#0 0x559406c3739c base::debug::StackTrace::StackTrace()
#1 0x559406b71c17 logging::LogMessage::~LogMessage()
#2 0x559406a4db55 atom::AtomBrowserMainParts::Get()
#3 0x559406a524e9 atom::Browser::Get()
#4 0x559406a9cd40 atom::AtomBindings::GetProcessMemoryInfo()
#5 0x559406a21032 _ZN4mate8internal7InvokerINS0_13IndicesHolderIJLm0EEEEJPKN4atom3api11WebContentsEEE18DispatchToCallbackIN2v85LocalINSB_5ValueEEEEEvN4base17RepeatingCallbackIFT_S8_EEE
#6 0x559406a9e021 mate::internal::Dispatcher<>::DispatchToCallback()
#7 0x559405a5b0bc v8::internal::FunctionCallbackArguments::Call()
#8 0x5594059fe35f v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#9 0x5594059fc5b8 v8::internal::Builtin_Impl_HandleApiCall()
#10 0x5594059fc011 v8::internal::Builtin_HandleApiCall()
#11 0x559406758c95 <unknown>

release-clerk · 2018-11-28T08:57:56Z

Release Notes Persisted

Add getMemoryFootprint API

alexeykuzmin · 2019-01-09T18:45:14Z

/trop run backport

trop · 2019-01-09T18:45:18Z

The backport process for this PR has been manually initiated, here we go! :D

trop · 2019-01-09T18:45:40Z

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

bpasero · 2019-01-28T15:39:50Z

@alexeykuzmin is this not being considered for a backport to 4.0.x?

…y usage (#14847) * feat: Implement process.getMemoryFootprint to get the process memory usage * Add spec * fix: must enter node env in callback * Update function call * Update spec * Update API data * update spec * Update include * update test for shared bytes * Update atom/common/api/atom_bindings.cc Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update atom/common/api/atom_bindings.cc Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update API * Update the callback isolate * Update to work after app ready * Update docs * Update docs/api/process.md Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update docs/api/process.md Co-Authored-By: nitsakh <nitsakh@icloud.com> * Fix crash

alexeykuzmin · 2019-01-30T14:51:25Z

is this not being considered for a backport to 4.0.x?

@bpasero
I removed the label only because the change couldn't be backported cleanly, if that was the reason of your question.

We are going to backport the change though. See #16591.

bpasero · 2019-01-30T14:56:08Z

@alexeykuzmin great to hear

…y usage (#14847) (#16591) * feat: Implement process.getMemoryFootprint to get the process memory usage * Add spec * fix: must enter node env in callback * Update function call * Update spec * Update API data * update spec * Update include * update test for shared bytes * Update atom/common/api/atom_bindings.cc Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update atom/common/api/atom_bindings.cc Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update API * Update the callback isolate * Update to work after app ready * Update docs * Update docs/api/process.md Co-Authored-By: nitsakh <nitsakh@icloud.com> * Update docs/api/process.md Co-Authored-By: nitsakh <nitsakh@icloud.com> * Fix crash

nitsakh requested a review from a team September 28, 2018 00:14

alexeykuzmin reviewed Sep 30, 2018

View reviewed changes

ckerr reviewed Oct 4, 2018

View reviewed changes

nornagon requested changes Oct 4, 2018

View reviewed changes

nitsakh force-pushed the memapi branch from 835e71d to fa62ebd Compare October 15, 2018 06:42

nitsakh requested a review from a team October 15, 2018 06:42

nitsakh changed the title ~~WIP: feat: Implement process.getMemoryFootprint to get the process memory usage~~ feat: Implement process.getMemoryFootprint to get the process memory usage Oct 15, 2018

nitsakh changed the title ~~feat: Implement process.getMemoryFootprint to get the process memory usage~~ feat: Implement process.getProcessMemoryInfo to get the process memory usage Oct 15, 2018

nitsakh force-pushed the memapi branch from be58839 to 22b7e2c Compare October 17, 2018 21:43

nornagon requested changes Oct 24, 2018

View reviewed changes

nitsakh force-pushed the memapi branch from 9487d78 to cad4857 Compare October 31, 2018 22:06

nornagon approved these changes Nov 2, 2018

View reviewed changes

nitsakh force-pushed the memapi branch from 5510172 to 1887224 Compare November 20, 2018 20:17

nornagon approved these changes Nov 20, 2018

View reviewed changes

docs/api/process.md Outdated Show resolved Hide resolved

docs/api/process.md Outdated Show resolved Hide resolved

nitsakh and others added 14 commits November 28, 2018 16:26

feat: Implement process.getMemoryFootprint to get the process memory …

87c2312

…usage

Add spec

f095e04

fix: must enter node env in callback

a285a63

Update function call

532a8f8

Update spec

5bf5524

Update API data

38d1d0d

update spec

e0dea4a

Update include

96f5caf

update test for shared bytes

488b19d

Update atom/common/api/atom_bindings.cc

f6c3b74

Co-Authored-By: nitsakh <nitsakh@icloud.com>

Update atom/common/api/atom_bindings.cc

51143f6

Co-Authored-By: nitsakh <nitsakh@icloud.com>

Update API

62f8e06

Update the callback isolate

7f78bf4

Update to work after app ready

153b983

nitsakh and others added 3 commits November 28, 2018 16:26

Update docs

d2c6c09

Update docs/api/process.md

1a063b7

Co-Authored-By: nitsakh <nitsakh@icloud.com>

Update docs/api/process.md

0c85253

Co-Authored-By: nitsakh <nitsakh@icloud.com>

zcbenz self-assigned this Nov 28, 2018

zcbenz force-pushed the memapi branch from bfce76a to ecad1ad Compare November 28, 2018 07:53

Fix crash

077caff

zcbenz force-pushed the memapi branch from ecad1ad to 077caff Compare November 28, 2018 07:58

zcbenz approved these changes Nov 28, 2018

View reviewed changes

zcbenz merged commit 9890d1e into master Nov 28, 2018

zcbenz deleted the memapi branch November 28, 2018 08:57

MarshallOfSound mentioned this pull request Dec 22, 2018

app.getAppMetrics() and ProcessMetric - memory no longer available #16179

Closed

bpasero mentioned this pull request Jan 8, 2019

Replace getProcessMemoryInfo API in chromium 67 #13447

Closed

alexeykuzmin added the target/4-0-x label Jan 9, 2019

alexeykuzmin removed the target/4-0-x label Jan 9, 2019

nitsakh mentioned this pull request Jan 29, 2019

feat: Implement process.getProcessMemoryInfo to get the process memory info #16591

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement process.getProcessMemoryInfo to get the process memory usage #14847

feat: Implement process.getProcessMemoryInfo to get the process memory usage #14847

nitsakh commented Sep 28, 2018 •

edited

alexeykuzmin Sep 30, 2018

nitsakh Oct 4, 2018

alexeykuzmin Sep 30, 2018

nitsakh Oct 4, 2018

zcbenz commented Oct 4, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

nitsakh Oct 4, 2018 •

edited

ckerr Oct 4, 2018

ckerr Oct 4, 2018

nitsakh Oct 4, 2018

ckerr Oct 4, 2018

nornagon Oct 4, 2018

ckerr Oct 4, 2018 •

edited

nitsakh Oct 4, 2018 •

edited

nitsakh Oct 4, 2018

nornagon Oct 1, 2018

nornagon Oct 4, 2018

nornagon Oct 24, 2018

nitsakh Oct 26, 2018

nornagon Oct 26, 2018

nornagon left a comment

nornagon Nov 2, 2018

nornagon Nov 2, 2018

zcbenz commented Nov 28, 2018

release-clerk bot commented Nov 28, 2018

alexeykuzmin commented Jan 9, 2019

trop bot commented Jan 9, 2019

trop bot commented Jan 9, 2019

bpasero commented Jan 28, 2019

alexeykuzmin commented Jan 30, 2019

bpasero commented Jan 30, 2019

feat: Implement process.getProcessMemoryInfo to get the process memory usage #14847

feat: Implement process.getProcessMemoryInfo to get the process memory usage #14847

Conversation

nitsakh commented Sep 28, 2018 • edited

Description of Change

Checklist

Release Notes

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

zcbenz commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitsakh Oct 4, 2018 • edited

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

ckerr Oct 4, 2018 • edited

Choose a reason for hiding this comment

nitsakh Oct 4, 2018 • edited

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

nornagon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcbenz commented Nov 28, 2018

release-clerk bot commented Nov 28, 2018

alexeykuzmin commented Jan 9, 2019

trop bot commented Jan 9, 2019

trop bot commented Jan 9, 2019

bpasero commented Jan 28, 2019

alexeykuzmin commented Jan 30, 2019

bpasero commented Jan 30, 2019

nitsakh commented Sep 28, 2018 •

edited

nitsakh Oct 4, 2018 •

edited

ckerr Oct 4, 2018 •

edited

nitsakh Oct 4, 2018 •

edited