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: add memory to app.getAppMetrics() #18831

Merged
merged 1 commit into from Jul 23, 2019

Conversation

@miniak
Copy link
Contributor

miniak commented Jun 16, 2019

Description of Change

Bring back memory info per process returned by app.getAppMetrics(). Follow-up to #18718.
It does not depend on Chromium internals, it’s calling the OS API directly.

Checklist

Release Notes

Notes: Added memory to app.getAppMetrics().

@miniak miniak requested review from zcbenz, deepak1556, nornagon and ckerr Jun 16, 2019
@miniak miniak marked this pull request as ready for review Jun 16, 2019
Copy link
Member

MarshallOfSound left a comment

Just gonna have to ask the hard question here, what dependencies does this API have on Chromium internals / content APIs. We've been hurt twice now by memory APIs being removed internally in Chromium, if this one could potentially have the same issue we should seriously consider not adding it.

@miniak miniak self-assigned this Jun 17, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jun 17, 2019

Just gonna have to ask the hard question here, what dependencies does this API have on Chromium internals / content APIs. We've been hurt twice now by memory APIs being removed internally in Chromium, if this one could potentially have the same issue we should seriously consider not adding it.

@MarshallOfSound Added more details to the description. We've been affected by the removal of memory info due to the Chromium changes as our telemetry depends on it heavily.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Jun 17, 2019

Compilation on Windows fails:

../../electron/atom/browser/api/process_metric.cc(73,34): error: out-of-line definition of 'K32GetProcessMemoryInfo' does not match any declaration in 'atom::ProcessMetric'; did you mean 'GetProcessMemoryInfo'?
17375ProcessMemoryInfo ProcessMetric::GetProcessMemoryInfo() const {
17376                                 ^~~~~~~~~~~~~~~~~~~~
17377                                 GetProcessMemoryInfo
17378
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 17, 2019
spec-main/api-app-spec.ts Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/memory-info branch from c35f3df to 557f2c2 Jun 20, 2019
Copy link
Member

ckerr left a comment

It would be preferable to support this on Linux, too.

docs/api/structures/process-metric.md Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/memory-info branch 2 times, most recently from f938938 to 1fea3a7 Jul 9, 2019
@miniak miniak requested a review from ckerr Jul 9, 2019
@miniak miniak force-pushed the miniak/memory-info branch 2 times, most recently from 4e2f51c to c19dd24 Jul 9, 2019
@miniak miniak force-pushed the miniak/memory-info branch from c19dd24 to 3b934d5 Jul 9, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jul 17, 2019

@ckerr can you please check the Linux version?

@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jul 17, 2019

@zcbenz can you please check again?

@zcbenz
zcbenz approved these changes Jul 17, 2019
@miniak miniak requested a review from codebytere Jul 23, 2019
@ckerr
ckerr approved these changes Jul 23, 2019
Copy link
Member

ckerr left a comment

The scaffolding, tests, doc changes, and Linux code all LGTM. Tested on Ubuntu 17.04 👍

The Mac and Windows changes look sane too but I haven't tested them personally. If someone else wants to also give this a pass, that would be great.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Jul 23, 2019

(approval given that all of the issues i would have raised have all been addressed)

@codebytere codebytere merged commit 103b386 into master Jul 23, 2019
12 of 13 checks passed
12 of 13 checks passed
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190709.36 succeeded
Details
electron-arm64-testing Build #20190709.38 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 23, 2019

Release Notes Persisted

Added memory to app.getAppMetrics().

@codebytere codebytere deleted the miniak/memory-info branch Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.