/electron

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 info #16591

Merged
merged 1 commit into from Jan 30, 2019
+123 −11

Conversation

Reviewers

@felixrieseberg felixrieseberg

@alexeykuzmin alexeykuzmin

@miniak miniak

@MarshallOfSound MarshallOfSound

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@nitsakh @felixrieseberg @alexeykuzmin @miniak @MarshallOfSound
@nitsakh
Copy link
Contributor

nitsakh commented Jan 29, 2019 •
edited

Backport of #14847.

Description of Change

Checklist

Release Notes

Notes: Added getProcessMemoryInfo API
@nitsakh
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
Loading status checks…
4353531

@nitsakh nitsakh requested review from electron/docs as code owners Jan 29, 2019

@miniak

miniak requested changes Jan 29, 2019
View changes

Copy link
Contributor

miniak left a comment

Please include this fix #16593
@felixrieseberg

felixrieseberg reviewed Jan 30, 2019
View changes

atom/common/api/atom_bindings.cc

if (mate::Locker::IsBrowserProcess() && !Browser::Get()->is_ready()) {
promise->RejectWithErrorMessage(
"Memory Info is available only after app ready");

This comment has been minimized.

Sign in to view
Copy link
@felixrieseberg

felixrieseberg Jan 30, 2019

Member
Suggested change Beta
Give us feedback
"Memory Info is available only after app ready");
"Memory information is only available after the app's 'ready' event (See: electronjs.org/docs/api/app)");
@felixrieseberg

felixrieseberg reviewed Jan 30, 2019
View changes

docs/api/process.md
memory consumed by the Electron code itself in Kilobytes.

Returns an object giving memory usage statistics about the current process. Note
that all statistics are reported in Kilobytes.

This comment has been minimized.

Sign in to view
Copy link
@felixrieseberg

felixrieseberg Jan 30, 2019

Member
Suggested change Beta
Give us feedback
that all statistics are reported in Kilobytes.
that all statistics are reported in kilobytes.

(Typically spelled using lowercase)
@felixrieseberg

felixrieseberg reviewed Jan 30, 2019
View changes

docs/api/process.md

Returns an object giving memory usage statistics about the current process. Note
that all statistics are reported in Kilobytes.
This api should be called after app ready.

This comment has been minimized.

Sign in to view
Copy link
@felixrieseberg

felixrieseberg Jan 30, 2019

Member
Suggested change Beta
Give us feedback
This api should be called after app ready.
This API should not be called before the `app` has emitted the `ready` event.
@felixrieseberg

felixrieseberg reviewed Jan 30, 2019
View changes

docs/api/process.md
that all statistics are reported in Kilobytes.
This api should be called after app ready.

Chromium does not provide `residentSet` value for macOS. This is because macOS

This comment has been minimized.

Sign in to view
Copy link
@felixrieseberg

felixrieseberg Jan 30, 2019

Member
Suggested change Beta
Give us feedback
Chromium does not provide `residentSet` value for macOS. This is because macOS
Chromium does not provide a `residentSet` value for macOS. This is because macOS
@felixrieseberg

felixrieseberg reviewed Jan 30, 2019
View changes

docs/api/process.md

Chromium does not provide `residentSet` value for macOS. This is because macOS
performs in-memory compression of pages that haven't been recently used. As a
result the resident set size value is not what one would expect. `private` memory

This comment has been minimized.

Sign in to view
Copy link
@felixrieseberg

felixrieseberg Jan 30, 2019

Member
Suggested change Beta
Give us feedback
result the resident set size value is not what one would expect. `private` memory
result, the resident set size value is not what one would expect. `private` memory
@felixrieseberg

This comment has been minimized.

Sign in to view
Copy link
Member

felixrieseberg commented Jan 30, 2019

Sorry for the swarm of suggestions, feel free to disregard them as you please. Thank you for the overall effort!
@nitsakh

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nitsakh commented Jan 30, 2019

😬 This is a backport of #14847 . Suggestions look good, but I'll probably address those in a separate PR.

@alexeykuzmin alexeykuzmin referenced this pull request Jan 30, 2019

Merged

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

0 of 5 tasks complete
@alexeykuzmin

alexeykuzmin approved these changes Jan 30, 2019
View changes

@alexeykuzmin

This comment has been minimized.

Sign in to view
Copy link
Contributor

alexeykuzmin commented Jan 30, 2019

@miniak I agree with Nitish, let's merge those changes separately.
@alexeykuzmin

This comment has been minimized.

Sign in to view
Copy link
Contributor

alexeykuzmin commented Jan 30, 2019

@felixrieseberg Thank you for the suggestions, they're awesome.
But I agree with Nitish, let's make them in the master first, so we don't accidentally forget doing that after merging them here.
@miniak

miniak approved these changes Jan 30, 2019
View changes

@MarshallOfSound

MarshallOfSound approved these changes Jan 30, 2019
View changes

@MarshallOfSound MarshallOfSound merged commit ada60a9 into 4-0-x Jan 30, 2019
20 checks passed

20 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
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
release-notes Release notes found
@release-clerk

This comment has been minimized.

Sign in to view
Copy link

release-clerk bot commented Jan 30, 2019

Release Notes Persisted

Added getProcessMemoryInfo API

@MarshallOfSound MarshallOfSound deleted the memapi-4 branch Jan 30, 2019

@alexeykuzmin alexeykuzmin referenced this pull request Jan 31, 2019

Open

docs: process.getProcessMemoryInfo() returns a Promise #16593

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