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.getProcessMemoryInfo to get the process memory usage #14847

Merged
merged 18 commits into from Nov 28, 2018

Conversation

Projects
None yet
7 participants
@nitsakh
Copy link
Contributor

commented Sep 28, 2018

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

@nitsakh nitsakh requested a review from as a code owner Sep 28, 2018

#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"

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Sep 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Sep 30, 2018

Contributor

Can you please use async / await here?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

Still WIP, but yes. 👍

@zcbenz

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

The crash should have been fixed now.

#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 comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

This sentence should end in because ${reason}

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

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

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

  • Why is the error message in parenthesis?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

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.

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

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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

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

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

Needs a writeup in docs/api/process.md

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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?

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 4, 2018

Contributor

+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.

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 4, 2018

Author Contributor

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.

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

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 4, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 4, 2018

Contributor

+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.

@nitsakh nitsakh force-pushed the memapi branch from 835e71d to fa62ebd Oct 15, 2018

@nitsakh nitsakh requested a review from as a code owner Oct 15, 2018

@nitsakh 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 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 nitsakh force-pushed the memapi branch from be58839 to 22b7e2c Oct 17, 2018

Show resolved Hide resolved atom/common/api/atom_bindings.cc
Show resolved Hide resolved atom/common/api/atom_bindings.cc Outdated
Show resolved Hide resolved atom/common/api/atom_bindings.cc
Show resolved Hide resolved 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);

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 24, 2018

Contributor

Why name this differently on different OSes?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 26, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 26, 2018

Contributor

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.

@nitsakh nitsakh force-pushed the memapi branch from 9487d78 to cad4857 Oct 31, 2018

@nornagon
Copy link
Contributor

left a comment

Just docs questions now I think

* `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.

This comment has been minimized.

Copy link
@nornagon

nornagon Nov 2, 2018

Contributor

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.

Returns `Object`:

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

This comment has been minimized.

Copy link
@nornagon

nornagon Nov 2, 2018

Contributor

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?

@nitsakh nitsakh force-pushed the memapi branch from 5510172 to 1887224 Nov 20, 2018

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

nitsakh and others added some commits Nov 20, 2018

Update docs/api/process.md
Co-Authored-By: nitsakh <nitsakh@icloud.com>
Update docs/api/process.md
Co-Authored-By: nitsakh <nitsakh@icloud.com>
@zcbenz

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

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>

@zcbenz zcbenz self-assigned this Nov 28, 2018

@zcbenz zcbenz force-pushed the memapi branch from bfce76a to ecad1ad Nov 28, 2018

@zcbenz zcbenz force-pushed the memapi branch from ecad1ad to 077caff Nov 28, 2018

@zcbenz

zcbenz approved these changes Nov 28, 2018

@zcbenz zcbenz merged commit 9890d1e into master Nov 28, 2018

26 of 27 checks passed

Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181128.7 succeeded
Details
electron-arm64-testing Build #20181128.7 succeeded
Details
electron-lint Build #20181128.8 succeeded
Details
electron-mas-testing Build #20181128.8 succeeded
Details
electron-osx-testing Build #20181128.9 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 28, 2018

Release Notes Persisted

Add getMemoryFootprint API

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

/trop run backport

@trop

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

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

@trop

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

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

@bpasero

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

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

nitsakh added a commit that referenced this pull request Jan 29, 2019

feat: Implement process.getProcessMemoryInfo to get the process memor…
…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

nitsakh added a commit that referenced this pull request Jan 29, 2019

feat: Implement process.getProcessMemoryInfo to get the process memor…
…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

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@alexeykuzmin great to hear

MarshallOfSound added a commit that referenced this pull request Jan 30, 2019

feat: Implement process.getProcessMemoryInfo to get the process memor…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.