API to get memory of all processes of the app #9214

juturu · 2017-04-17T12:04:41Z

Adding api which will help apps to get memory info for all processes.

MarshallOfSound · 2017-04-17T12:15:54Z

docs/api/app.md

@@ -760,6 +760,23 @@ Disables hardware acceleration for current app.

 This method can only be called before app is ready.

+### `app.getAppMemoryInfo()`
+
+Returns `Object[]`:


This should be a new structure, maybe ProcessMemoryInfo[]

Then the object described below should be moved to a new structure markdown file 👍

poiru · 2017-04-17T20:06:16Z

atom/browser/api/atom_api_app.h

@@ -157,6 +160,17 @@ class App : public AtomBrowserClient::Delegate,
  DISALLOW_COPY_AND_ASSIGN(App);
 };

+class AppIdProcessIterator : public base::ProcessIterator {


Does this need to be in the header or could it be moved into the .cc file?

poiru · 2017-04-17T20:09:09Z

atom/browser/api/atom_api_app.cc

@@ -904,6 +904,38 @@ void App::GetFileIcon(const base::FilePath& path,
  }
 }

+v8::Local<v8::Value> App::GetAppMemoryInfo(v8::Isolate* isolate) {
+  AppIdProcessIterator processIterator;


Nit: Please use snake_case

poiru · 2017-04-17T20:11:17Z

docs/api/app.md

+    memory consumed by the Electron code itself
+
+Returns an object giving memory usage statistics about all the processes associated with 
+the app. Note that all statistics are reported in Kilobytes.


It seems weird to me that sharedBytes represents kilobytes instead of bytes. Is there a reason for that? If not, it might make more sense to just use bytes and perform the division in the app code.

anaisbetts · 2017-04-18T07:49:11Z

This seems super similar to https://github.com/electron/electron/blob/master/docs/api/process.md#processgetsystemmemoryinfo, maybe we can unify these?

alexstrat · 2017-04-18T12:29:47Z

docs/api/app.md

+
+Returns `Object[]`:
+
+* `pid` Integer - The process id for which memory info is collected for


Does this allow to link to a specific web-contents? Can I use webContents.fromId(pid)?

kevinsawicki

Left a few minor comments

kevinsawicki · 2017-04-25T22:00:47Z

atom/browser/api/atom_api_app.cc

+      dict.Set("sharedBytes", static_cast<double>(shared_bytes >> 10));
+    }
+
+    result.Set(std::to_string(pid).c_str(), dict); 


I don't think you need the .c_str() here, result.Set should handle std::string as the first argument.

kevinsawicki · 2017-04-25T22:01:52Z

docs/api/app.md

@@ -760,6 +760,23 @@ Disables hardware acceleration for current app.

 This method can only be called before app is ready.

+### `app.getAppMemoryInfo()`


I think we should add a spec for this.

kevinsawicki · 2017-04-25T22:02:33Z

atom/browser/api/atom_api_app.h

@@ -157,6 +160,17 @@ class App : public AtomBrowserClient::Delegate,
  DISALLOW_COPY_AND_ASSIGN(App);
 };

+class AppIdProcessIterator : public base::ProcessIterator {
+ public:
+  AppIdProcessIterator() : base::ProcessIterator(NULL) {}


Could nullptr be used here instead?

juturu · 2017-04-27T18:43:28Z

@MarshallOfSound @poiru @kevinsawicki made the changes requested and fixed other spec issues. Please take a look.
@alexstrat this returns memory info of all the process corresponding to the app. You can iterate over the array to get the memory info for the pid you are interested in. Does that help?
@paulcbetts yes it is similar for the part of memory retrieve from a process. We can unify it. Any suggestion on where the utility method could go to?

kevinsawicki · 2017-04-27T21:25:38Z

atom/browser/api/atom_api_app.cc

+    processEntry = process_iterator.NextProcessEntry();
+  }
+
+  return mate::ConvertToV8(isolate, result);


I believe this method can just have a return type of std::vector<mate::Dictionary> and then you don't need to do this explicit conversion here.

yep, this works. updated.

juturu · 2017-05-03T23:11:50Z

@zcbenz anything blocking in merging this PR?

poiru · 2017-05-03T23:25:09Z

atom/browser/api/atom_api_app.cc

+      base::ProcessMetrics::CreateProcessMetrics(process.Handle()));
+#endif
+
+    mate::Dictionary pidDict = mate::Dictionary::CreateEmpty(isolate);


If you're bored, you could change processEntry, pidDict, and memoryDict to use snake case. (Sorry. 🙃)

kevinsawicki · 2017-05-04T21:00:40Z

Thanks for this @juturu 👍 🚢

alexstrat · 2017-05-15T15:56:22Z

@juturu app.getAppMemoryInfo actually returns the actual OS process id as pid, and on webContents there is no method (documented or undocumented) to get the actual OS process id. Therefore I don't see a way to make a link between a webContents and the associated process memory metrics coming from getAppMemoryInfo => FUP in #9222

juturu · 2017-05-16T00:47:00Z

@alexstrat please look at #9486 to see if that help with what you want to achieve.

juturu requested review from zcbenz, kevinsawicki and deepak1556 April 17, 2017 12:04

MarshallOfSound reviewed Apr 17, 2017

View reviewed changes

poiru reviewed Apr 17, 2017

View reviewed changes

poiru mentioned this pull request Apr 18, 2017

Add OS pid to web-contents #9222

Merged

2 tasks

alexstrat reviewed Apr 18, 2017

View reviewed changes

zcbenz self-assigned this Apr 25, 2017

kevinsawicki suggested changes Apr 25, 2017

View reviewed changes

juturu force-pushed the app-memoryinfo branch from 53e3d21 to 397f468 Compare April 27, 2017 20:37

juturu and others added 6 commits April 27, 2017 14:06

API to get memory of all processes of the app

bef7d5a

Adding docs, specs and fixing object returned

27aad90

Fixing lint error

d77c131

Fixing mac build

3d12440

fixing mac ut failure

81bd9fa

Removing describe.only

88ad28b

juturu force-pushed the app-memoryinfo branch from 397f468 to 88ad28b Compare April 27, 2017 21:07

kevinsawicki reviewed Apr 27, 2017

View reviewed changes

Return mate::Dictionary instead of v8::value

07b53c0

kevinsawicki approved these changes May 1, 2017

View reviewed changes

poiru reviewed May 3, 2017

View reviewed changes

kevinsawicki and others added 4 commits May 4, 2017 13:27

🎨 Use snake case

e206aae

Link to MemoryInfo structure

edd28c9

Make pid an integer

ab1bcef

Assert memory info for all returned processes

3b3e7cc

kevinsawicki merged commit 5951135 into master May 4, 2017

kevinsawicki deleted the app-memoryinfo branch May 4, 2017 21:00

zeke mentioned this pull request May 5, 2017

Adding CPU and I/O metrics to process module #9373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to get memory of all processes of the app #9214

API to get memory of all processes of the app #9214

juturu commented Apr 17, 2017

MarshallOfSound Apr 17, 2017

poiru Apr 17, 2017

poiru Apr 17, 2017

poiru Apr 17, 2017

anaisbetts commented Apr 18, 2017

alexstrat Apr 18, 2017

kevinsawicki left a comment

kevinsawicki Apr 25, 2017

kevinsawicki Apr 25, 2017

kevinsawicki Apr 25, 2017

juturu commented Apr 27, 2017

kevinsawicki Apr 27, 2017

juturu Apr 30, 2017

juturu commented May 3, 2017

poiru May 3, 2017

kevinsawicki commented May 4, 2017

alexstrat commented May 15, 2017 •

edited

Loading

juturu commented May 16, 2017


		Returns `Object[]`:

		* `pid` Integer - The process id for which memory info is collected for

		@@ -760,6 +760,23 @@ Disables hardware acceleration for current app.

		This method can only be called before app is ready.

		### `app.getAppMemoryInfo()`

API to get memory of all processes of the app #9214

API to get memory of all processes of the app #9214

Conversation

juturu commented Apr 17, 2017

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

anaisbetts commented Apr 18, 2017

Choose a reason for hiding this comment

kevinsawicki left a 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

juturu commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juturu commented May 3, 2017

Choose a reason for hiding this comment

kevinsawicki commented May 4, 2017

alexstrat commented May 15, 2017 • edited Loading

juturu commented May 16, 2017

alexstrat commented May 15, 2017 •

edited

Loading