1255843 - Develop a tool to measure resource usage of each subprocess

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This should be useful for the multiprocess effort. This is also our Outreachy Project for the e10s team. Here's the description:

"With multi-process Firefox going out the door in the very near future, we're looking at scaling up and tuning the number of content processes that Firefox starts and uses.

Memory usage is something we want to keep an eye on while we do this, so this project is about building a Content Process Management tool that can track real-time memory usage across each process. We might increase the number of uses of the management tool over time, but we'll start with memory management."

Comment 1

[Eric Rahm [:erahm]]

What kind of tool are you looking for here? An external process? Something built into Fx as an add-on? A heads-up display like framerate? What data do you want?

Real-time about:memory isn't feasible but gathering RSS and USS is reasonable at maybe 1-10Hz (USS takes a bit to calculate, a reasonable RSS on mac can be expensive too) from within Fx, externally you could probably just go as fast as the system lets you.

Externally, parts of atsy [1] may be of use, there's also some sort of b2g realtime memory monitoring thing written in node.js (the name, maintainer, and location elude me) that might be useful.

I'd be happy to meet with the Outreachy folks when things get going if you'd like.

[1] https://github.com/EricRahm/atsy

Comment 2

[Mike Conley (:mconley) (:⚙️)]

When mrbkap and I talked about this, I think we were thinking about something built-in to the browser, a la the Chromium "Task Manager". I'm not certain we'd expose it by default - that's probably something to explore as we work on this.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Eric Rahm [:erahm] from comment #1)
> Real-time about:memory isn't feasible but gathering RSS and USS is
> reasonable at maybe 1-10Hz (USS takes a bit to calculate, a reasonable RSS
> on mac can be expensive too) from within Fx, externally you could probably
> just go as fast as the system lets you.

That's almost certainly sufficient.

> I'd be happy to meet with the Outreachy folks when things get going if you'd
> like.
> 

Thanks!

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP UI wireframe

This is the UI wireframe that Rutuja, an Outreachy candidate, has put together for me. I'll be giving feedback in this bug.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey Rutuja, I think this is a good start. I think we can probably simplify it a bit, so here are some suggestions:

1) Get rid of the Network and CPU columns of that table. At least for now, we only care about memory usage.
2) Since we're not initially going to be creating an individual process per individual tab, I don't think the Page column is exactly right. What we might want to do, at least initially, is have a column listing how many tabs are being rendered inside that process.
3) I think the PID should probably be given priority and put in the left-most column of that table
4) We should probably add columns for resident memory, and unique memory usage (this is the RSS and USS stuff that erahm brought up). There might be other more fine-grained readings that we might want, but those two are good to start with.
5) Data Used is out of scope - can be removed.
6) The timeline over the day or week is also probably out of scope. At least for now, I suspect we only care about memory readings for the current session.
7) It also might be worth linking to about:memory for each content process, which could then cause a memory report to be generated.
8) Napping processes and setting priority is also probably out of scope to start with.

So I think there's a lot of simplification that can be done here.

Also, we might want to consider putting this information in a tab (that runs in the parent process) rather than a window. There's a concerted effort to remove windows from Firefox, and I'd rather we didn't go against that if we can.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8729767 [details]
WIP UI wireframe

The "review now granted" I just gave is because I have some review feedback that I think can be fed into a next iteration. Thanks for your work Rutuja, and let me know if you have any questions!

Comment 7

[Rutuja]

Attachment: Revised UI wireframe, with suggested changes and a couple of more things I thought we could include. Please do review this.

Revised UI wireframe,  with suggested changes and a couple of more things I thought we could include. Please do review this.

Comment 8

[Rutuja]

Attachment: Another revised version of the wireframe.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Eric Rahm [:erahm] from comment #1)
> What kind of tool are you looking for here? An external process? Something
> built into Fx as an add-on? A heads-up display like framerate? What data do
> you want?

My original thinking was that this would be a content that that runs in the parent process. Every few seconds, it'd get a simple report on memory usage for each content process (not nearly as detailed as about:memory - something that can be done without disrupting the user's browsing every few seconds).

Rutuja is our Outreachy intern working on this project. Do you think that there are parts of the nsIMemoryReporter framework she could use for this work? Or would something else need to be built in parallel to it?

Comment 11

[Nicholas Nethercote [inactive]]

> My original thinking was that this would be a content that that runs in the
> parent process. Every few seconds, it'd get a simple report on memory usage
> for each content process (not nearly as detailed as about:memory - something
> that can be done without disrupting the user's browsing every few seconds).

You should talk to Panos about this. A couple of years ago I implemented support for lighterweight per-tab memory measurements, and he worked on a UI in devtools for exposing the info. I don't know what happened to that UI.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

I had built a prototype UI in the form of an add-on that I subsequently turned into a patch in bug 975675, which we eventually decided not to ship. What actually landed in Firefox devtools is a much more feature-complete module for taking precise memory measurements and displaying them in the devtools memory tab. Nick (Fitzgerald) can describe how all of that works.

That module however still uses nsIMemoryReporter in some cases, which might be roughly what you need:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/performance/memory.js#353
https://dxr.mozilla.org/mozilla-central/source/devtools/server/performance/memory.js#385

Comment 13

[Rutuja]

(In reply to Panos Astithas [:past] from comment #12)
> I had built a prototype UI in the form of an add-on that I subsequently
> turned into a patch in bug 975675, which we eventually decided not to ship.
> What actually landed in Firefox devtools is a much more feature-complete
> module for taking precise memory measurements and displaying them in the
> devtools memory tab. Nick (Fitzgerald) can describe how all of that works.
> 
> That module however still uses nsIMemoryReporter in some cases, which might
> be roughly what you need:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/performance/
> memory.js#353
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/performance/
> memory.js#385

Thanks! :)

Comment 14

[Mike Conley (:mconley) (:⚙️)]

In terms of UI placement, I talked to Yoric, and about:performance sounds like an appropriate place to put the monitoring UI, whatever it ends up looking like.

In the meantime, I've been poking around, talking to various people (mainly erahm and Yoric) about how best to get the memory readings from subprocesses. Originally, it sounded like adding a PerformanceStats Probe was a good bet, along with these nsIMemoryReporterManager attributes: https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/xpcom/base/nsIMemoryReporter.idl#376-381

There's a flaw with that plan, however - messaging each subprocess every few seconds to get memory usage information will wake that child up, and might prevent optimizations like paging away the memory from background tabs to disk.

A way around this would be to make it possible for the parent to ask the underlying OS for memory statistics on all subprocesses. This way, no messaging is required to the children, and they can stay asleep (if they're asleep).

Hey njn, I assume that it should be possible for the parent process to ask the OS for memory usage of the subprocesses, but I also assume that this facility is not built anywhere into nsMemoryReporterManager and friends (at least, I can't find it)...

Would it be advisable to add such a capability?

Comment 15

[Nicholas Nethercote [inactive]]

> There's a flaw with that plan, however - messaging each subprocess every few
> seconds to get memory usage information will wake that child up, and might
> prevent optimizations like paging away the memory from background tabs to
> disk.

Yes. When Panos and I implemented the earlier prototype e10s wasn't really a thing, so we didn't have this problem.

> Hey njn, I assume that it should be possible for the parent process to ask
> the OS for memory usage of the subprocesses, but I also assume that this
> facility is not built anywhere into nsMemoryReporterManager and friends (at
> least, I can't find it)...
> 
> Would it be advisable to add such a capability?

Correct. There are memory reporters for OS things (e.g. "resident", "vsize") but each process has to measure its own values.

As for whether OS measurements for each child process would be useful: I'm skeptical. Coarse memory measurements aren't all that useful. As soon as one of them gets high enough to be interesting you immediately want to get a more detailed breakdown. 

Putting it another way: do you want to measure memory usage, or do you want to measure memory usage in a way that is actionable when it gets high? Because the latter is a lot harder :(

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Nicholas Nethercote [:njn] from comment #15)
> 
> As for whether OS measurements for each child process would be useful: I'm
> skeptical. Coarse memory measurements aren't all that useful. As soon as one
> of them gets high enough to be interesting you immediately want to get a
> more detailed breakdown. 

Ah, I think I've been a bit confused about the audience of this particular feature - though it's now become more clear for me.

The audience of this feature is power users who are likely to be interested in knowing how much memory each content process is consuming. So this tool isn't necessarily targeted at Gecko hackers who are interested in paring down memory usage per content process (about:memory is probably more useful for that). This is more of a high-level display of memory usage for each content process.

> Putting it another way: do you want to measure memory usage, or do you want
> to measure memory usage in a way that is actionable when it gets high?
> Because the latter is a lot harder :(

The former, for now. I think we just want a way for users to know how much memory each content process is consuming without forcing them to open Task Manager / Activity Manager / top / whathaveyou.

Comment 17

[Nicholas Nethercote [inactive]]

Fair enough -- so I guess the hoped-for use case is something like this:

- Firefox is sluggish.

- User opens this tool and sees tab X is using a ton of memory.

- User closes tab X.

- Firefox speeds up again.

?

Probably the best measurement to use is "resident-unique", which is available via nsIMemoryReporterManager.residentUnique and is now implemented on Windows, Mac and Linux. (Not sure about Android, but that may not be relevant.) You'll have to wire up the IPC yourself, though.

Comment 18

[rsurve]

Attachment: aboutperformance.js

Comment 19

[rsurve]

Attachment: aboutperformance.xhtml

Comment 20

[rsurve]

Attachment: nsMemoryReporterManager.cpp

Comment 21

[rsurve]

Attachment: nsMemoryReporterManager.h

Comment 22

[rsurve]

Attachment: nsMemoryReporterManager.idl

Comment 23

[rsurve]

I've attached the entire files of all the codes I've modified by far to this bug. I shall soon attach a diff file as well..

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to rsurve from comment #23)
> I've attached the entire files of all the codes I've modified by far to this
> bug. I shall soon attach a diff file as well..

Thanks Rutuja. The individual files are less necessary than the diff file - that's the most important part. I'm going to "obsolete" the individual files so that they don't clutter up the attachments pane.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: Linux Patch

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8778979 [details] [diff] [review]
Linux Patch

Review of attachment 8778979 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Rutuja, thanks for the patch! Glad to see things taking shape.

I have a number of suggestions below. A lot of them are style issues - I suggest checking out the code style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style but also especially to look at the surrounding code to get a sense of the styles and conventions within the files you're modifying.

Let me know if you have any questions about the stuff I've brought up!

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +115,5 @@
>    }
>  };
>  
> +/**
> + * Returns a Promise that's resolved after the next turn of the event loop.

Looks like this is a fragment of a patch for bug 911216 in here - https://bugzilla.mozilla.org/attachment.cgi?id=8756116&action=diff.

Can you show me how you're generating your diff?

@@ +604,5 @@
>            eltImpact.textContent = ` is currently considerably slowing down ${BRAND_NAME}.`;
>          }
>  
>          cachedElements.eltFPS.textContent = `Impact on framerate: ${delta.diff.jank.longestDuration + 1}/${delta.diff.jank.durations.length}${jankSuffix}.`;
> +        cachedElements.eltCPU.textContent = `CPU usage: ${Math.ceil(delta.diff.jank.totalCPUTime/delta.diff.deltaT/10)}%.`;

And this looks to be from bug 1244491.

@@ +945,5 @@
> +
> +
> +//New Global object, is display mode necessary?
> +var SubprocessMonitor = {
> +  init: function() {

Please add a docstring to this and every other function in this object describing what the function does, what arguments it takes (if any) and what it returns (if anything).

@@ +946,5 @@
> +
> +//New Global object, is display mode necessary?
> +var SubprocessMonitor = {
> +  init: function() {
> +    dump("Calling the new SubProcessMonitor init function in aboutPerformance.js.\n");

Please remove debugging dump statement.

@@ +947,5 @@
> +//New Global object, is display mode necessary?
> +var SubprocessMonitor = {
> +  init: function() {
> +    dump("Calling the new SubProcessMonitor init function in aboutPerformance.js.\n");
> +    //calling get reports from init

I don't think this comment is adding much - probably safe to remove.

@@ +948,5 @@
> +var SubprocessMonitor = {
> +  init: function() {
> +    dump("Calling the new SubProcessMonitor init function in aboutPerformance.js.\n");
> +    //calling get reports from init
> +   // SubprocessMonitor.updateTable();

Please remove the dead code here and on line 956.

@@ +949,5 @@
> +  init: function() {
> +    dump("Calling the new SubProcessMonitor init function in aboutPerformance.js.\n");
> +    //calling get reports from init
> +   // SubprocessMonitor.updateTable();
> +    setInterval(() => SubprocessMonitor.updateTable(), 5000);

Let's put a property "UPDATE_INTERVAL_MS" on this SubprocessMonitor object, and refer to that instead so we can avoid the magic number.

We should also probably save the return value of setInterval so that we can clearInterval if need be (for example, I think we should probably use the page visibility API[1] so that we disable the interval updates if the tab stops being visible, and to re-enable it once it becomes visible again).

[1]: https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API

@@ +956,5 @@
> +   // SubprocessMonitor.getReports();
> +  },
> +
> +  getReports: function() {
> +    dump("Calling the new SubProcessMonitor getReports function in aboutPerformance.js.\n");

Please remove this dump statement.

@@ +959,5 @@
> +  getReports: function() {
> +    dump("Calling the new SubProcessMonitor getReports function in aboutPerformance.js.\n");
> +    
> +    let result = {};
> +    var Cc = Components.classes;

Components.classes and Components.interfaces are already aliased to Cc and Ci here: http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/toolkit/components/aboutperformance/content/aboutPerformance.js#9

So no need to do it again.

@@ +963,5 @@
> +    var Cc = Components.classes;
> +    var Ci = Components.interfaces;
> +// Get access to the nsIMemoryReporterManager from JS...
> +    let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                  .getService(Ci.nsIMemoryReporterManager);

Let's line this up so that the . for .getService is positioned below the [ of ["@mozilla.org

@@ +966,5 @@
> +    let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                  .getService(Ci.nsIMemoryReporterManager);
> +
> +
> +  //call with path as empty string, kind as NON_HEAP (2), units as bytes (0), value as the amount we'll

I don't think this comment is helpful - it's just describing what the callback is accepting, which is evident by just reading it. If possible, comments should explain _why_, not _what_.

@@ +969,5 @@
> +
> +  //call with path as empty string, kind as NON_HEAP (2), units as bytes (0), value as the amount we'll
> +  //be populating our table with, description returned will be either RSS or USS, closure is always nullptr
> +  //7 arguments to call back that get accommodated in the nsICallback pointer by XPCOM
> +    memManager.getReportsForSubprocesses(function(pid, path = "" , kind = 2 , units = 0 , value, description, closure = nullptr) {

You should use the constants instead of hard-coding the numbers here.

Example: Ci.nsIMemoryReporterManager.KIND_NONHEAP and Ci.nsIMemoryReporterManager.UNITS_BYTES.

Note that the comment from 970-972 is wrong - KIND_NONHEAP is 0, UNITS_BYTES is also 0. Neither are 2.

@@ +970,5 @@
> +  //call with path as empty string, kind as NON_HEAP (2), units as bytes (0), value as the amount we'll
> +  //be populating our table with, description returned will be either RSS or USS, closure is always nullptr
> +  //7 arguments to call back that get accommodated in the nsICallback pointer by XPCOM
> +    memManager.getReportsForSubprocesses(function(pid, path = "" , kind = 2 , units = 0 , value, description, closure = nullptr) {
> +   // memManager.getReportsForSubprocesses(function(pid, path = "" , kind = KIND_NONHEAP , units = UNITS_BYTES , value, description, closure = nullptr) {

Please remove this dead code as well.

@@ +972,5 @@
> +  //7 arguments to call back that get accommodated in the nsICallback pointer by XPCOM
> +    memManager.getReportsForSubprocesses(function(pid, path = "" , kind = 2 , units = 0 , value, description, closure = nullptr) {
> +   // memManager.getReportsForSubprocesses(function(pid, path = "" , kind = KIND_NONHEAP , units = UNITS_BYTES , value, description, closure = nullptr) {
> +
> +      if (!(pid in result)) {

Let's add a quick comment to explain why we're doing this conversion; because we're going to eventually re-design the getReportsForSubprocesses API to return a JS object that resembles what we're assembling here.

@@ +976,5 @@
> +      if (!(pid in result)) {
> +        result[pid] = {};
> +      }
> +      result[pid][description] = value;
> +     // dump(`For pid: ${pid}, the ${description} value is  ${value}\n`);

Please remove all dump statements from here down.

@@ +984,5 @@
> +    });
> +    dump("Length of result is " + JSON.stringify(result ,null, "\t").length);
> +    dump("\n Result object for getReports: " + JSON.stringify(result ,null, "\t") + "\n");
> +    return result;
> +    

Please remove trailing whitespace.

@@ +988,5 @@
> +    
> +  },
> +
> +
> +  updateTable:function() {

Space after :

@@ +989,5 @@
> +  },
> +
> +
> +  updateTable:function() {
> +    var result = SubprocessMonitor.getReports();

Use let, not var.

@@ +990,5 @@
> +
> +
> +  updateTable:function() {
> +    var result = SubprocessMonitor.getReports();
> +    var oElem = document.getElementById("subprocess-reports");

let, not var. Also, oElem isn't the most descriptive variable. Maybe name this resultTable?

@@ +996,5 @@
> +    let rno = 1;
> +    let cno = 1;
> +
> + 
> +      for(var pid in result) {

Just a note that we're using 2 space indentation in this file. Be sure to indent new blocks 2 spaces and no more. For example, line 1000 down seems to be mis-aligned.

@@ +998,5 @@
> +
> + 
> +      for(var pid in result) {
> +        var pidfound = 0;
> +        for (var i = 1, row; row = oElem.rows[i]; i++) {

There's an easier way to query for elements with a particular ID. You can do this:

let row = resultTable.querySelector(`#${pid}`);
if (!row) {
  row = document.createElement("tr");
  row.id = pid;
  // Insert a cell for USS...
  row.insertCell();
  // Insert another cell for RSS...
  row.insertCell();
  // And finally append the row to the table
  resultTable.appendChild(row);
}

// Now we update the values in the row. We're hardcoding these for now, but we might want to make this more adaptable in the future.
row.cells[0].textContent = result[pid].uss;
row.cells[1].textContent = result[pid].uss;

@@ +1057,5 @@
>    Services.obs.addObserver(testUpdate, TEST_DRIVER_TOPIC, false);
>    window.addEventListener("unload", () => Services.obs.removeObserver(testUpdate, TEST_DRIVER_TOPIC));
>  
>    yield Control.update();
> +  yield wait(BUFFER_SAMPLING_RATE_MS * 1.1);

This is another fragment from an unrelated patch.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +10,5 @@
> +    <link rel="icon" type="image/png" id="favicon"
> +          href="chrome://branding/content/icon32.png"/>
> +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css"
> +          type="text/css"/>
> +    <link rel="stylesheet" href="chrome://global/skin/aboutSupport.css"

Out of curiosity, what do you need aboutSupport.css for?

@@ +23,5 @@
>        @import url("chrome://global/skin/in-content/common.css");
> +
> +
> +
> +     html {

The indentation is busted here. Try to maintain the same indentation style as the rest of the CSS in this file.

@@ +32,5 @@
> +  body {
> +   margin: 40px 48px;
> +  }
> +
> +  .page-subtitle {

There are some rules in here that I'm not sure apply. For example, I can't find a single item in this markup that uses the page.subtitle class.

Can you remove the rules that don't apply please?

@@ +199,5 @@
> +#subprocess-reports {
> +  font: 100%  message-box;
> +  background-color: var(--aboutSupport-table-background);
> +  color: var(--in-content-text-color);
> +  font: message-box;

This is overriding the rule on line 200. Try to make sure you don't have redundant or overridden rules.

@@ +255,5 @@
> +}
> +
> +
> +
> +#subprocess-reports .prefs-table {

This is another case where I'm seeing a class that I don't see being used. prefs-table. I don't think that's anywhere in the markup.

@@ +319,4 @@
>      </style>
>    </head>
>    <body onload="go()">
> +    <table cellspacing="40" id="subprocess-reports">

cellspacing is an outmoded attribute - we should use CSS to style and space the cells if we need to.

@@ +321,5 @@
>    <body onload="go()">
> +    <table cellspacing="40" id="subprocess-reports">
> +      <thead>Performance of Subprocesses</thead>
> +
> +    <tr class = "parent">

No spaces on either side of =. Applies to the rest of the nodes as well.

Also, this needs to be indented so that it's aligned with <thead> above (since this is at the same depth in the DOM tree as the thead).

Make sure to indent the other nodes in this block properly. Feel free to ping me for guidance.

@@ +322,5 @@
> +    <table cellspacing="40" id="subprocess-reports">
> +      <thead>Performance of Subprocesses</thead>
> +
> +    <tr class = "parent">
> +    <th> Process ID </th>   

Trailing whitespace. No spaces on either side of the Process ID header.

@@ +324,5 @@
> +
> +    <tr class = "parent">
> +    <th> Process ID </th>   
> +    <th class = "rparent">Resident Set Size<span class="r">RSS measures the pages resident in the main
> +      memory for the process</span> </th>

Try not to break strings over two lines like this.

::: xpcom/base/nsIMemoryReporter.idl
@@ +188,5 @@
>     */
>    const int32_t KIND_NONHEAP = 0;
>    const int32_t KIND_HEAP    = 1;
>    const int32_t KIND_OTHER   = 2;
> +  const int32_t SELF_PID     = -1;

This isn't part of the KIND group, so I don't think it should be put here.

In fact, I'm not sure this needs to be in the IDL at all, since I don't think it's used outside of nsMemoryReporterManager.cpp.

We should probably define this as a constants inside of nsMemoryReporterManager.cpp

Example: http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/extensions/cookie/nsCookiePermission.cpp#40

@@ +214,5 @@
>     */
>    void init();
>  
> +  /* getReportsForSubprocesses functionality */
> +   void getReportsForSubprocesses(in nsIMemoryReporterCallback aCallback);

This needs to have one less space before it to align properly.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +28,5 @@
>  #include "mozilla/UniquePtrExtensions.h"
>  #include "mozilla/dom/PMemoryReportRequestParent.h" // for dom::MemoryReport
>  #include "mozilla/dom/ContentParent.h"
>  #include "mozilla/ipc/FileDescriptorUtils.h"
> +#include <iostream>

Please remove iostream, cstring and string includes (I'm guessing these were added for printf debugging)

@@ +44,5 @@
>  using namespace mozilla;
>  
>  #if defined(MOZ_MEMORY)
>  #  define HAVE_JEMALLOC_STATS 1
> +const int32_t SELF_PID     = -1;

This is the right idea, adding the const here - but I think it needs to be done outside of the MOZ_MEMORY ifdef. Perhaps under using namespace mozilla;

@@ +61,4 @@
>  {
>    // There are more than two fields, but we're only interested in the first
>    // two.
> +  nsCString pathvar;

This doesn't appear to be used anymore.

@@ +61,5 @@
>  {
>    // There are more than two fields, but we're only interested in the first
>    // two.
> +  nsCString pathvar;
> +  

Trailing whitespace.

@@ +66,5 @@
>    static const int MAX_FIELD = 2;
>    size_t fields[MAX_FIELD];
>    MOZ_ASSERT(aField < MAX_FIELD, "bad field number");
> +  nsAutoCString statmPath("/proc/");
> + if (pid == SELF_PID) {

Busted indentation from here down - make sure to use the same spacing and indent depth as the above lines, and increase as you go further into code blocks. Example:

This is at depth 0 {
  And this is two spaces indent
  Another line at two spaces indent {
    And because I'm in a new block, I go another two spaces in, etc.
  }
}

@@ +75,5 @@
> + statmPath.Append("/statm");
> +
> +FILE* f = fopen(statmPath.BeginReading(), "r");
> +/*
> +  if(pid == SELF_PID){

Please remove this dead code.

@@ +110,5 @@
>    // statm's "shared" value actually counts pages backed by files, which has
>    // little to do with whether the pages are actually shared. /proc/self/smaps
>    // on the other hand appears to give us the correct information.
> +   nsAutoCString smapsPath("/proc/");
> +  if (pid == SELF_PID) {

Same indentation note as above.

@@ +119,5 @@
> +  smapsPath.Append("/smaps");
> +
> +  FILE* f = fopen(smapsPath.BeginReading(), "r");
> +
> + // FILE* f = fopen("/proc/self/smaps", "r");

Please remove this dead code.

@@ +180,5 @@
>  
>  static nsresult
>  ResidentFastDistinguishedAmount(int64_t* aN)
>  {
> +  return ResidentDistinguishedAmount(aN,SELF_PID);

Space after ,

@@ +2329,5 @@
>  {
> +  return HelperResident(aAmount, SELF_PID);
> +}
> +
> +NS_IMETHODIMP

This method doesn't implement a function from an IDL, so this can just have an nsresult return type instead of using NS_IMETHODIMP.

@@ +2340,5 @@
>    *aAmount = 0;
>    return NS_ERROR_NOT_AVAILABLE;
>  #endif
> +
> +

Please remove these extra newlines.

@@ +2398,5 @@
>  {
> +  return HelperUniqueResident(aAmount, SELF_PID);
> +}
> +
> +

Please remove these extra newlines.

@@ +2416,2 @@
>  }
> +NS_IMETHODIMP

Newline before the definition of a new method.

@@ +2461,5 @@
>  
>  /*static*/ int64_t
>  nsMemoryReporterManager::ResidentUnique()
>  {
> +  #ifdef HAVE_RESIDENT_UNIQUE_REPORTER

I don't think this is valid - ifdef's need to always start at column 0 (so it ignores indentation).

So this whole block needs to be un-indented.

@@ +2469,5 @@
> +    return amount;
> +  #else
> +    return 0;
> +  #endif
> +

Please remove this newline.

@@ +2474,3 @@
>  }
>  
> +

Please remove this newline.

::: xpcom/base/nsMemoryReporterManager.h
@@ +191,5 @@
>    void DispatchReporter(nsIMemoryReporter* aReporter, bool aIsAsync,
>                          nsIHandleReportCallback* aHandleReport,
>                          nsISupports* aHandleReportData,
>                          bool aAnonymize);
> +// NS_IMETHODIMP HelperResident(int64_t* aAmount, int32_t pid);

Please remove this line.

@@ +192,5 @@
>                          nsIHandleReportCallback* aHandleReport,
>                          nsISupports* aHandleReportData,
>                          bool aAnonymize);
> +// NS_IMETHODIMP HelperResident(int64_t* aAmount, int32_t pid);
> +nsresult HelperResident(int64_t* aAmount, int32_t pid);

Please indent these properly (two spaces).

@@ +199,3 @@
>  
>    static void TimeoutCallback(nsITimer* aTimer, void* aData);
> +

Please remove this newline.

Comment 27

[rsurve]

Attachment: Lin_Win_Patch

This is the patch for both Linux and Windows combined

Comment 28

[rsurve]

The patch attached (Lin+Win) has a relative diff file (wrt previous locally changed code), I shall also attach the absolute diff file. This one encapsulates all changes made after the first review, though..

Comment 29

[rsurve]

Attachment: Absolute patch (Linux + Windows)

Review this patch and ignore the previous one please..

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8780655 [details] [diff] [review]
Lin_Win_Patch

Review of attachment 8780655 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Rutuja, this is another good step forward. See my notes below.

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +942,5 @@
>    _displayMode: MODE_GLOBAL,
>  };
>  
> +var SubprocessMonitor = {
> +  //Init calls the updatetable function after a fixed interval, which updates the RSS/USS values

More general style stuff:

1) All trailing whitespace (any of the red blocks in the diff) need to be removed
2) Where possible / practical, please try to limit each line to 80 characters or less.
3) Comments before functions should follow the styling in the rest of the file. Here's an example: http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/toolkit/components/aboutperformance/content/aboutPerformance.js#553
4) This file has 2 space indentation. In more than one place in your patch, I'm seeing bad alignment - for example, on line 948, the setInterval is only indented a single space instead of 2. I know this might seem unimportant, but the consistency is very important.

Probably also a good idea to run eslint on your code before putting up your next diff.

Step 1) In mozilla-central, do ./mach eslint --setup
Step 2) Do ./mach eslint toolkit/components/aboutperformance/content/aboutPerformance.js

If it finds any problems in any of the lines you changed, you should correct them. Note that eslint is only for JavaScript changes.

@@ +944,5 @@
>  
> +var SubprocessMonitor = {
> +  //Init calls the updatetable function after a fixed interval, which updates the RSS/USS values
> +   init: function() {
> +    setInterval(() => SubprocessMonitor.updateTable(), UPDATE_INTERVAL_MS);

In my previous review, I mentioned using the page visibility API to stop the interval from running in the background. Did you try that?

@@ +953,5 @@
> +     let result = {};
> +    // Get access to the nsIMemoryReporterManager from JS.
> +    //The result object stores the RSS and USS for every pid
> +     let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                         .getService(Ci.nsIMemoryReporterManager);

Please align the . so that it's below the [.

@@ +954,5 @@
> +    // Get access to the nsIMemoryReporterManager from JS.
> +    //The result object stores the RSS and USS for every pid
> +     let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                         .getService(Ci.nsIMemoryReporterManager);
> +    memManager.getReportsForSubprocesses(function(pid, path = "" , kind = Ci.nsIMemoryReporterManager.KIND_NONHEAP , units = Ci.nsIMemoryReporterManager.UNITS_BYTES , value, description, closure = nullptr) {

No need to put spaces before the commas. After, yes, but not before.

@@ +965,5 @@
> +   },
> + 
> +  //This function adds a row for every new pid and populates and regularly updates the table with RSS/USS measurements
> +  updateTable: function() {
> +    let result = SubprocessMonitor.getReports();

You can just use this.getReports();

@@ +970,5 @@
> +    let resultTable = document.getElementById("subprocess-reports");
> +     let flag = 0;
> +     let rno = 1;
> +     let cno = 1;
> +     //Suggested method (not working) 

Okay, if this method is not working, I need you to explore _why_ it's not working. Try using the JS debugger and stepping through to see where it's going wrong.

@@ +972,5 @@
> +     let rno = 1;
> +     let cno = 1;
> +     //Suggested method (not working) 
> +    /*
> +    for(var pid in result) {

Use let, not var.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +172,5 @@
>    <body onload="go()">
> +    <table id="subprocess-reports">
> +       <thead>Performance of Subprocesses</thead>
> +      <tr class="parent">
> +        <th>Process ID</th>   

Instead of using these special "r" and "u" spans with hover effects, let's use the title attribute on the th elements instead.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/title

Then you can remove the special classes above too.

::: xpcom/base/nsIMemoryReporter.idl
@@ +212,5 @@
>     * Initialize.
>     */
>    void init();
>  
> +   /* getReportsForSubprocesses functionality */

This comment doesn't add much. Can you describe what exactly the function does?

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +155,1 @@
> + ResidentDistinguishedAmount(int64_t* aN, int32_t pid)

Please remove the extra space at the start of this line.

@@ +604,4 @@
>  {
>    PROCESS_MEMORY_COUNTERS pmc;
>    pmc.cb = sizeof(PROCESS_MEMORY_COUNTERS);
> +  HANDLE proc = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

We should be doing a check for SELF_PID here, no? And then use GetCurrentProcess() if pid == SELF_PID?

@@ +629,5 @@
>    PSAPI_WORKING_SET_INFORMATION tmp;
>    DWORD tmpSize = sizeof(tmp);
>    memset(&tmp, 0, tmpSize);
>  
> +  HANDLE proc = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

Same as above - we need to handle the SELF_PID case.

@@ -2326,5 @@
>  
>  NS_IMETHODIMP
>  nsMemoryReporterManager::GetResident(int64_t* aAmount)
>  {
>    return HelperResident(aAmount, SELF_PID);

The SELF_PID on the left indicates to me that this patch is based upon previous work that you've done. The next time you post a patch, can you post all changes in the same diff?

@@ +2385,5 @@
>    return NS_ERROR_NOT_AVAILABLE;
>  #endif
>  }
> +
> + NS_IMETHODIMP

No space at the start of each line. Take a look at the other functions and methods around in this file for hints on how to style things.

@@ +2386,5 @@
>  #endif
>  }
> +
> + NS_IMETHODIMP
> + nsMemoryReporterManager::GetReportsForSubprocesses( nsIMemoryReporterCallback* aCallback)

No space before nsIMemoryReporterCallback

@@ +2393,5 @@
> +  int32_t pid;
> +  nsresult rv;
> +  nsresult rsv;
> +  ContentParent::GetAll(childWeakRefs);
> +  if (!childWeakRefs.IsEmpty()) {

If childWeakRefs.IsEmpty(), then the Length() should be 0, and then we'll just skip the loop. We can probably get rid of this check.

@@ +2395,5 @@
> +  nsresult rsv;
> +  ContentParent::GetAll(childWeakRefs);
> +  if (!childWeakRefs.IsEmpty()) {
> +    for (size_t i = 0; i < childWeakRefs.Length(); ++i) {
> +      pid = childWeakRefs[i]->Pid();

Be really careful with indentation. Note how line 2399 is indented 2 spaces (correctly) inside a for loop block. But line 2400 until 2405 are indented 3 spaces.

@@ +2398,5 @@
> +    for (size_t i = 0; i < childWeakRefs.Length(); ++i) {
> +      pid = childWeakRefs[i]->Pid();
> +       nsAutoCString process;
> +       process.AppendInt(pid); 
> +       int64_t value = 0;

value isn't too descriptive - can you make it clearer what this will hold?

@@ +2399,5 @@
> +      pid = childWeakRefs[i]->Pid();
> +       nsAutoCString process;
> +       process.AppendInt(pid); 
> +       int64_t value = 0;
> +       int64_t valueuss = 0;

Probably valueUSS here.

@@ +2400,5 @@
> +       nsAutoCString process;
> +       process.AppendInt(pid); 
> +       int64_t value = 0;
> +       int64_t valueuss = 0;
> +       rv = ResidentDistinguishedAmount(&value, pid);

After each of these calls where we get an rv, we should do:

if (NS_WARN_IF(NS_FAILED(rv)) {
  return rv;
}

And then you can re-use the rv instead of creating rsv.

@@ +2403,5 @@
> +       int64_t valueuss = 0;
> +       rv = ResidentDistinguishedAmount(&value, pid);
> +       rv = aCallback->Callback(
> +      process, EmptyCString(), 0, 0, value,
> +      NS_LITERAL_CSTRING("RSS") , nullptr);

No space before ,

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8780786 [details] [diff] [review]
Absolute patch (Linux + Windows)

Review of attachment 8780786 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for folding together the patches! That's much clearer. See my comments in my last review - the rest should still apply.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +38,5 @@
>  #include <unistd.h>
>  #endif
>  
>  using namespace mozilla;
> +const int32_t SELF_PID     = -1;

No need for the big gap before the =.

Comment 32

[rsurve]

Thank you very much for the detailed review, I'll surely make the changes you have suggested. In the meanwhile, I'll complete the setup for Mac OS X. I'll be done with the setup and will have made the suggested changes by Sunday/Monday (latest).

Comment 33

[Rutuja]

Attachment: Windows and Linux (Patch after changes according to review)

Comment 34

[Rutuja]

I've attached the patch after making changes according to the second review. The comments may still look long and clumsy in a couple of places, I'll soon space them out and edit them.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781995 [details] [diff] [review]
Windows and Linux (Patch after changes according to review)

Hey njn,

Rutuja isn't 100% done yet (she's in the process of porting the nsMemoryReporterManager changes to OS X), but we were hoping you could give what she's got a glance to make sure it's going to be acceptable (or to course correct if the approach isn't going to work out).

It's more or less how we described it in London - basically, we're adding helpers to get at the USS / RSS readings for particular pids, instead of defaulting to the current process pid.

The interface that we're using to return the values is something we're hoping to modify in the future (it calls nsIMemoryReporterCallback multiple times per subprocess - once per measurement, and (ab)uses the process and description arguments to pass the pid string and the measurement type). I'm hoping we can modify it in the future to return a JS Object that is in the structure of:

{
  <pid>: {
    "uss": <value>,
    "rss": <value>
  }
}

But I think that's out of scope for this patch, and might be good fodder for a follow-up.

Anyhow, we wanted to know if you had any initial concerns with the approach here.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8781995 [details] [diff] [review]
Windows and Linux (Patch after changes according to review)

Review of attachment 8781995 [details] [diff] [review]:
-----------------------------------------------------------------

I've got feedback about the front-end. I've not reviewed nsMemoryReporterManager.cpp/.h, since I'm hoping njn can weigh in on whether or not we're doing the right thing there.

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +941,5 @@
>    // The display mode. One of `MODE_GLOBAL` or `MODE_RECENT`.
>    _displayMode: MODE_GLOBAL,
>  };
>  
> +var SubprocessMonitor = {

Can you please document the responsibility of SubprocessMonitor?

@@ +946,5 @@
> +  _interval : null,
> +
> +  handleVisibilityChange: function() {
> +    if (!document.hidden) {
> +     SubprocessMonitor._interval = setInterval(() => SubprocessMonitor.updateTable(), UPDATE_INTERVAL_MS);

See below - I think because you're using an arrow function for setInterval, you can use "this" properly in here.

Also, you're using only 1-space indentation here. Please be sure to consistently use 2-space indentation in this file.

@@ +954,5 @@
> +    }
> +  },
> +
> +  //Init calls the updatetable function after a fixed interval, which updates the RSS/USS values
> +  init: function() {

Let's move the init function to the top of the function list, since this function is the main entry point.

@@ +955,5 @@
> +  },
> +
> +  //Init calls the updatetable function after a fixed interval, which updates the RSS/USS values
> +  init: function() {
> +     if (!document.hidden) {

I think we can just call handleVisibilityChange here, no?

@@ +956,5 @@
> +
> +  //Init calls the updatetable function after a fixed interval, which updates the RSS/USS values
> +  init: function() {
> +     if (!document.hidden) {
> +        SubprocessMonitor._interval = setInterval(() => SubprocessMonitor.updateTable(), UPDATE_INTERVAL_MS);

Since you're using an arrow function here (which is good!), I think that means that you can use "this" properly inside handleVisibilityChange.

Also, just a note that you're using 3-space indentation here, instead of the requested 2-space indentation.

@@ +958,5 @@
> +  init: function() {
> +     if (!document.hidden) {
> +        SubprocessMonitor._interval = setInterval(() => SubprocessMonitor.updateTable(), UPDATE_INTERVAL_MS);
> +     }
> +     document.addEventListener("visibilitychange", SubprocessMonitor.handleVisibilityChange, true);

No need to have the third "true" argument - doesn't need to be in the capturing phase.

@@ +961,5 @@
> +     }
> +     document.addEventListener("visibilitychange", SubprocessMonitor.handleVisibilityChange, true);
> +  },
> +
> +  //getReports returns the RSS/USS value (description) for a pid using "result", a JS object.

Can you please use this format for comments:

/**
 * getReports returns the RSS/USS values (etc)
 *
 * @returns (JS Object)
 *          An Object in the following format:
 *
 *          {
 *            <pid>: {
 *              "rss": <int>,
 *              "uss": <int>
 *            },
 *            ...<more pids>
 *          }
 *
 *  (etc)
 */
and then show an example?

Please format the rest of your documentation for each function in this way.

@@ +964,5 @@
> +
> +  //getReports returns the RSS/USS value (description) for a pid using "result", a JS object.
> +  getReports: function() {
> +    let result = {};
> +    // Get access to the nsIMemoryReporterManager from JS.

I don't think comment 968 and 969 add much - though it'd be good if we could explain what we're doing (taking the results from the callbacks and turning them into an Object).

@@ +968,5 @@
> +    // Get access to the nsIMemoryReporterManager from JS.
> +    //The result object stores the RSS and USS for every pid
> +    let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                       .getService(Ci.nsIMemoryReporterManager);
> +    memManager.getReportsForSubprocesses(function(pid, path = "", kind=Ci.nsIMemoryReporterManager.KIND_NONHEAP, units=Ci.nsIMemoryReporterManager.UNITS_BYTES, value, description, closure=nullptr) {

This is a super long line. Can you find a way of making it max 80 characters? This might involve getting rid of the default arguments, which I don't think we need in the first place.

@@ +969,5 @@
> +    //The result object stores the RSS and USS for every pid
> +    let memManager = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                       .getService(Ci.nsIMemoryReporterManager);
> +    memManager.getReportsForSubprocesses(function(pid, path = "", kind=Ci.nsIMemoryReporterManager.KIND_NONHEAP, units=Ci.nsIMemoryReporterManager.UNITS_BYTES, value, description, closure=nullptr) {
> +    if (!(pid in result)) {

Broken indentation here from 973-976. Notice the first character of }); on line 977 is lined up with the "r" in result on line 976. Be sure to use 2-space indentation.

@@ +981,5 @@
> +  //This function adds a row for every new pid and populates and regularly updates the table with RSS/USS measurements
> +  updateTable: function() {
> +    let result = this.getReports();
> +    let resultTable = document.getElementById("subprocess-reports");
> +    let cno = 1;

cno is no longer used and can be removed.

@@ +989,5 @@
> +      let row = resultTable.querySelector(sel);
> +      if (!row) {
> +        row = document.createElement("tr");
> +        row.id = "pid-" + pid;
> +        //Insert cell for pid

Space after //

@@ +1001,5 @@
> +      }
> +      // Now we update the values in the row. We're hardcoding these for now, but we might want to make this more adaptable in the future.
> +      row.cells[0].textContent = pid;
> +      for(let description in result[pid]){
> +        row.cells[cno].textContent = result[pid][description];

Objects don't come in order. Instead of a for-loop, we should probably manually do this:

row.cells[1].textContent = result[pid].rss;
row.cells[2].textContent = result[pid].uss;

@@ +1006,5 @@
> +        cno++;
> +      }
> +      cno=1;
> +    }
> +  }, 

Trailing whitespace.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +161,5 @@
>    <body onload="go()">
> +    <table id="subprocess-reports">
> +      <thead>Performance of Subprocesses</thead>
> +      <tr class="parent">
> +        <th>Process ID</th>  

Trailing whitespace.

::: xpcom/base/nsIMemoryReporter.idl
@@ +213,5 @@
>     */
>    void init();
>  
> +  /* getReportsForSubprocesses functionality.... */
> +  void getReportsForSubprocesses(in nsIMemoryReporterCallback aCallback); 

Trailing whitespace.

Comment 37

[rsurve]

Attachment: Patch after frontend review

Comment 38

[rsurve]

Attachment: Patch after frontend review (final)

Comment 39

[rsurve]

Attachment: Win+Linux_Patch

Comment 40

[rsurve]

Attachment: Win+Linux patch after Frorntend review

I've attached the patch after modifying it according to the third review (frontend)

Comment 41

[rsurve]

Attachment: Patch (Linux+Windows) after frontend (third) review

Comment 42

[Eric Rahm [:erahm]]

Comment on attachment 8782444 [details] [diff] [review]
Patch (Linux+Windows) after frontend (third) review

Review of attachment 8782444 [details] [diff] [review]:
-----------------------------------------------------------------

I've added some notes for the nsIMemoryReporterManager side. In general I think this would be best just implemented in JS, this would
  a) avoid IDL changes
  b) work on OSX (which no longer lets you query the info we need for USS from other processes)

Roughly I was thinking:

> // In parent:
> for each child:
>   results[child.pid] = (run in child): return { nsIMemoryReporterManager.resident, nsIMemoryReporterManager.residentUnique }
> 
> results[parent_pid] = { nsIMemoryReporterManager.resident, nsIMemoryReporterManager.residentUnique }

Obviously that's hand-wavey pseudo code and it would be a bit more involved. If we still want to add the function to nsIMemoryReporterManager we can do something similar to |getReports|.

I know there was a bit of concern about waking up the child processes, but considering you have to open up about:performance we'll already be actively measuring when potentially messaging the child processes (so they're probably already being poked).

On the other hand if you're just looking for a proof-of-concept this route is probably okay.

::: xpcom/base/nsIMemoryReporter.idl
@@ +213,5 @@
>     */
>    void init();
>  
> +  /* getReportsForSubprocesses functionality.... */
> +  void getReportsForSubprocesses(in nsIMemoryReporterCallback aCallback);

If we go this route it needs a clearer name. Something like |getUssAndRssReports|, but feel free to bikeshed that. It also needs more thorough documentation and location-wise would be be better to declare down below with the other |get*| functions.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +51,5 @@
>  #include <string.h>
>  #include <stdlib.h>
>  
>  static nsresult
> +GetProcSelfStatmField(int aField, int64_t* aN, int32_t pid)

As this isn't just for self anymore we should rename the function, |GetProcStatmField| is probably fine.

@@ +66,5 @@
> +    statmPath.AppendInt(pid);
> +  }
> +  statmPath.Append("/statm");
> +
> +  FILE* f = fopen(statmPath.BeginReading(), "r");

I think we want to use |PromiseFlatCString(statmPath).get()| here.

@@ +79,5 @@
>    return NS_ERROR_FAILURE;
>  }
>  
>  static nsresult
> +GetProcSelfSmapsPrivate(int64_t* aN, int32_t pid)

As this isn't just for self anymore we should rename the function, |GetProcSmapsPrivate| is probably fine.

@@ +94,5 @@
> +    smapsPath.AppendInt(pid);
> +  }
> +  smapsPath.Append("/smaps");
> +
> +  FILE* f = fopen(smapsPath.BeginReading(), "r");

I think we want to use |PromiseFlatCString(smapsPath).get()| here.

@@ +606,5 @@
>    pmc.cb = sizeof(PROCESS_MEMORY_COUNTERS);
> +  HANDLE proc;
> +  if (pid == SELF_PID){
> +    proc = GetCurrentProcess();
> +  }else{

nit: spaces between braces

@@ +607,5 @@
> +  HANDLE proc;
> +  if (pid == SELF_PID){
> +    proc = GetCurrentProcess();
> +  }else{
> +    proc = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

We probably need to check this for failure. I'm not a 100% sure this will work with sandboxing enabled.

@@ +637,5 @@
>  
> +  HANDLE proc;
> +  if (pid == SELF_PID){
> +    proc = GetCurrentProcess();
> +  }else{

nit: spaces between braces

@@ +638,5 @@
> +  HANDLE proc;
> +  if (pid == SELF_PID){
> +    proc = GetCurrentProcess();
> +  }else{
> +   proc = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

We probably need to check this for failure. I'm not a 100% sure this will work with sandboxing enabled.

@@ +2326,2 @@
>  #ifdef HAVE_VSIZE_AND_RESIDENT_REPORTERS
> +  return ResidentDistinguishedAmount(aAmount, pid);

This will most likely break all other platforms (mac, bsd, etc) that declare HAVE_VSIZE_AND_RESIDENT_REPORTERS but don't have the new version of the function that handles |pid|.

@@ +2391,2 @@
>  #ifdef HAVE_RESIDENT_UNIQUE_REPORTER
> +  return ResidentUniqueDistinguishedAmount(aAmount, pid);

This will most likely break all other platforms (mac, bsd, etc) that declare HAVE_VSIZE_AND_RESIDENT_REPORTERS but don't have the new version of the function that handles |pid|.

@@ +2396,5 @@
>  #endif
>  }
>  
> +NS_IMETHODIMP
> +nsMemoryReporterManager::GetReportsForSubprocesses(nsIMemoryReporterCallback* aCallback)

Overall design thought: Similar to |GetReports{Extended}| this should probably just return details for *all* processes (unless we're not interested in the parent process, but that seems odd to me).

@@ +2400,5 @@
> +nsMemoryReporterManager::GetReportsForSubprocesses(nsIMemoryReporterCallback* aCallback)
> +{
> +  nsTArray<ContentParent*> childWeakRefs;
> +  int32_t pid;
> +  nsresult rv;

nit: Just declare |pid| and |rv| where they're used.

@@ +2405,5 @@
> +  ContentParent::GetAll(childWeakRefs);
> +  for (size_t i = 0; i < childWeakRefs.Length(); ++i) {
> +    pid = childWeakRefs[i]->Pid();
> +    nsAutoCString process;
> +    process.AppendInt(pid); 

nit: trailing space

|nsPrintfCString| might be a bit cleaner here, ie: |nsPrintfCString process("%d", pid);|

@@ +2407,5 @@
> +    pid = childWeakRefs[i]->Pid();
> +    nsAutoCString process;
> +    process.AppendInt(pid); 
> +    int64_t valueRSS = 0;
> +    int64_t valueUSS = 0;

nit: declare |valueUSS| where you use it.

@@ +2408,5 @@
> +    nsAutoCString process;
> +    process.AppendInt(pid); 
> +    int64_t valueRSS = 0;
> +    int64_t valueUSS = 0;
> +    rv = ResidentDistinguishedAmount(&valueRSS, pid);

Check |rv| for failure.

@@ +2411,5 @@
> +    int64_t valueUSS = 0;
> +    rv = ResidentDistinguishedAmount(&valueRSS, pid);
> +    rv = aCallback->Callback(
> +    process, EmptyCString(), 0, 0, valueRSS,
> +    NS_LITERAL_CSTRING("RSS"), nullptr);

nit: indent the wrapped function args

@@ +2415,5 @@
> +    NS_LITERAL_CSTRING("RSS"), nullptr);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +    rv = ResidentUniqueDistinguishedAmount(&valueUSS, pid);

Check |rv| for failure.

@@ +2422,5 @@
> +      NS_LITERAL_CSTRING("USS") , nullptr);
> +    if (NS_WARN_IF(NS_FAILED(rv))){
> +      return rv;
> +    }
> +  } 

nit: trailing space

@@ +2431,5 @@
>  nsMemoryReporterManager::ResidentUnique()
>  {
>  #ifdef HAVE_RESIDENT_UNIQUE_REPORTER
>    int64_t amount = 0;
> +  nsresult rv = ResidentUniqueDistinguishedAmount(&amount, SELF_PID);

Again, this will probably break other platforms.

::: xpcom/base/nsMemoryReporterManager.h
@@ +193,5 @@
>                          nsISupports* aHandleReportData,
>                          bool aAnonymize);
>  
> +  nsresult HelperResident(int64_t* aAmount, int32_t pid);
> +  nsresult HelperUniqueResident(int64_t* aAmount, int32_t pid);

The helpers can probably just be static and local to the cpp file.

Comment 43

[Eric Rahm [:erahm]]

To be more specific on my thoughts on the design:

 - For the "do it all in JS" case I was thinking something like what telemetry does [1].
 - For the "add an IDL interface" case I think what we want is an interface that's forward looking, so it would be designed to be async ready for if/when we add OS X support. It could look like |getReports| [2], ie it would take a callback for the values, and a callback for when measurements have completed. This could simply wrap what is currently implemented, but would be easy to extend later.

[1] https://hg.mozilla.org/releases/mozilla-beta/rev/2a04abb3101e
[2] https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/xpcom/base/nsIMemoryReporter.idl#256-276

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8782444 [details] [diff] [review]
Patch (Linux+Windows) after frontend (third) review

Review of attachment 8782444 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking quite good, Rutuja. I think the front-end is almost in shape! Good job! See my notes below.

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +943,5 @@
>  };
>  
> +/**
> + * This functionality gets memory related information of sub-processes and
> + * updates the performance table regularly.

"performance table" isn't exactly right. Maybe "Subprocess Performance" is more accurate.

@@ +944,5 @@
>  
> +/**
> + * This functionality gets memory related information of sub-processes and
> + * updates the performance table regularly.
> + * If the page goes hidden, it also handles visibility change by not

"If the page becomes hidden"

@@ +951,5 @@
> +var SubprocessMonitor = {
> +  _interval : null,
> +
> +  /**
> +   * Init calls 'handleVisibilityChange' if the page is not hidden and adds an eventlistener

This is all kind of implementation detail. What init really does is act as the main entrypoint of the SubprocessMonitor component, and start the process of polling for subprocess memory usage.

@@ +955,5 @@
> +   * Init calls 'handleVisibilityChange' if the page is not hidden and adds an eventlistener
> +   * for handling any  visibility changes.
> +   */
> +  init: function() {
> +     if (!document.hidden) {

Because handleVisibilityChange already checks if document.hidden, we can call it without first doing this check.

@@ +963,5 @@
> +  },
> +
> +  /**
> +   * This function updates the table after an interval if the page is visible
> +   * and clears the interval otherwise.

Good comment!

@@ +975,5 @@
> +     }
> +  },
> +
> +  /**
> +   * getReports returns the RSS/USS values for the content-processes

Period at the end of a sentence. Also, it looks like you broke the line too early - you can probably put some of the next line on this one before hitting the 80 char limit.

@@ +1011,5 @@
> +   */
> +  updateTable: function() {
> +    let result = this.getReports();
> +    let resultTable = document.getElementById("subprocess-reports");
> +    

Trailing whitespace.

@@ +1012,5 @@
> +  updateTable: function() {
> +    let result = this.getReports();
> +    let resultTable = document.getElementById("subprocess-reports");
> +    
> +    for(let pid in result) {

Space before (

@@ +1028,5 @@
> +        // And finally append the row to the table
> +        resultTable.appendChild(row);
> +      }
> +      /**
> +       * Now we update the values in the row, which is hardcoded for now, 

For non-header blocks, I think we tend to use // comment style. Also, trailing whitespace.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

(In reply to Eric Rahm [:erahm] from comment #42)
> 
> > // In parent:
> > for each child:
> >   results[child.pid] = (run in child): return { nsIMemoryReporterManager.resident, nsIMemoryReporterManager.residentUnique }
> > 
> > results[parent_pid] = { nsIMemoryReporterManager.resident, nsIMemoryReporterManager.residentUnique }
> 
> Obviously that's hand-wavey pseudo code and it would be a bit more involved.
> If we still want to add the function to nsIMemoryReporterManager we can do
> something similar to |getReports|.
> 
> I know there was a bit of concern about waking up the child processes, but
> considering you have to open up about:performance we'll already be actively
> measuring when potentially messaging the child processes (so they're
> probably already being poked).
> 

This was our original idea, however you're right in that I recall that Yoric was concerned about waking up content processes, and preventing them from being paged out (which could affect measurements).

> we'll already be actively
> measuring when potentially messaging the child processes (so they're
> probably already being poked).

I don't think I fully understand this bit - can you elaborate?

Comment 46

[Eric Rahm [:erahm]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #45)
> > measuring when potentially messaging the child processes (so they're
> > probably already being poked).
> 
> I don't think I fully understand this bit - can you elaborate?

Since we're actively choosing to measure (this measurement isn't in the background), we'll most likely cause the browser to interact with child processes one way or another (I'd also bet that they're almost never idle, telemetry I'm looking at you kid), so the concern of waking up content processes and paging memory in and out seems like somewhat of a non-issue. 

Additionally that memory probably isn't going to be paged out unless the system is under memory pressure, so if we're *not* under memory pressure it will actually look like we're using more memory than if we are. It's probably better to be consistent.

Comment 47

[Nicholas Nethercote [inactive]]

Comment on attachment 8782444 [details] [diff] [review]
Patch (Linux+Windows) after frontend (third) review

Review of attachment 8782444 [details] [diff] [review]:
-----------------------------------------------------------------

f+ because this feels like it's heading in the right direction, though it still requires some work. I agree with pretty much everything erahm said, too. Thanks.

(BTW, this is a challenging task you've taken on -- you're having to learn about a whole bunch of different parts of the system, and how they interact. Good job with what you've done so far! :)

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +38,5 @@
>  #include <unistd.h>
>  #endif
>  
>  using namespace mozilla;
> +const int32_t SELF_PID = -1;

Add |static|.

@@ +51,5 @@
>  #include <string.h>
>  #include <stdlib.h>
>  
>  static nsresult
> +GetProcSelfStatmField(int aField, int64_t* aN, int32_t pid)

Change |pid| to |aPid|, please.

And it might be nice to use a default parameter, i.e. |int32_t aPid = SELF_PID|.

@@ +60,5 @@
>    size_t fields[MAX_FIELD];
>    MOZ_ASSERT(aField < MAX_FIELD, "bad field number");
> +  nsAutoCString statmPath("/proc/");
> +  if (pid == SELF_PID) {
> +    statmPath.Append("self");

You can use AppendLiteral() here.

@@ +61,5 @@
>    MOZ_ASSERT(aField < MAX_FIELD, "bad field number");
> +  nsAutoCString statmPath("/proc/");
> +  if (pid == SELF_PID) {
> +    statmPath.Append("self");
> +  } else{

Space before the '{'.

@@ +64,5 @@
> +    statmPath.Append("self");
> +  } else{
> +    statmPath.AppendInt(pid);
> +  }
> +  statmPath.Append("/statm");

And here.

@@ +79,5 @@
>    return NS_ERROR_FAILURE;
>  }
>  
>  static nsresult
> +GetProcSelfSmapsPrivate(int64_t* aN, int32_t pid)

My comments from GetProcSelfSmapsPrivate() apply here too.

@@ +147,5 @@
>  #define HAVE_VSIZE_AND_RESIDENT_REPORTERS 1
>  static nsresult
>  VsizeDistinguishedAmount(int64_t* aN)
>  {
> +  return GetProcSelfStatmField(0, aN, SELF_PID);

If you add the default parameter, as suggested above, you can remove the SELF_PID here.

@@ +599,5 @@
>    return NS_OK;
>  }
>  
>  static nsresult
> +ResidentDistinguishedAmount(int64_t* aN, int32_t pid)

|aPid| instead of |pid|.

And, again, a default parameter would be nice.

@@ +2414,5 @@
> +    process, EmptyCString(), 0, 0, valueRSS,
> +    NS_LITERAL_CSTRING("RSS"), nullptr);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }

You can just use NS_ENSURE_SUCCESS(rv, rv) to replace these three lines.

@@ +2421,5 @@
> +      process, EmptyCString(), 0, 0, valueUSS,
> +      NS_LITERAL_CSTRING("USS") , nullptr);
> +    if (NS_WARN_IF(NS_FAILED(rv))){
> +      return rv;
> +    }

Again, NS_ENSURE_SUCCESS(rv, rv) is shorter.

::: xpcom/base/nsMemoryReporterManager.h
@@ +193,5 @@
>                          nsISupports* aHandleReportData,
>                          bool aAnonymize);
>  
> +  nsresult HelperResident(int64_t* aAmount, int32_t pid);
> +  nsresult HelperUniqueResident(int64_t* aAmount, int32_t pid);

It's not clear why you split these functions out. They both are only called once each, as far as I can tell.

Comment 48

[rsurve]

(In reply to Nicholas Nethercote [:njn] from comment #47)
> Comment on attachment 8782444 [details] [diff] [review]
> Patch (Linux+Windows) after frontend (third) review
> 
> Review of attachment 8782444 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: xpcom/base/nsMemoryReporterManager.h
> @@ +193,5 @@
> >                          nsISupports* aHandleReportData,
> >                          bool aAnonymize);
> >  
> > +  nsresult HelperResident(int64_t* aAmount, int32_t pid);
> > +  nsresult HelperUniqueResident(int64_t* aAmount, int32_t pid);
> 
> It's not clear why you split these functions out. They both are only called
> once each, as far as I can tell.
Hey!
Thank you for the feedback! I'll certainly make the changes suggested by you.
We introduced these helper functions because we won't be permitted to modify the definitions of the "getter" functions GetResident and GetResidentUnique to include an extra argument for the pid.

Comment 49

[rsurve]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #44)
> Comment on attachment 8782444 [details] [diff] [review]
> Patch (Linux+Windows) after frontend (third) review
> 
> Review of attachment 8782444 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks! I'll soon update the frontend patch with these changes. I'm also done with the build for OS X now.

Comment 50

[rsurve]

(In reply to Eric Rahm [:erahm] from comment #43)
> To be more specific on my thoughts on the design:
> 
>  - For the "do it all in JS" case I was thinking something like what
> telemetry does [1].
>  - For the "add an IDL interface" case I think what we want is an interface
> that's forward looking, so it would be designed to be async ready for
> if/when we add OS X support. It could look like |getReports| [2], ie it
> would take a callback for the values, and a callback for when measurements
> have completed. This could simply wrap what is currently implemented, but
> would be easy to extend later.
> 
> [1] https://hg.mozilla.org/releases/mozilla-beta/rev/2a04abb3101e
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 97a52326b06a07930216ebefa5af333271578904/xpcom/base/nsIMemoryReporter.
> idl#256-276

Thank you for the feedback! :) I'm trying to examine the "all JS" approach in detail now and shall get back in case I have questions.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Hey Rutuja,

I've been talking to erahm today, and I think we've got a plan forward.

To sum - the current course for the backend isn't going to work out, particularly for OS X. The reason for this is because of something called "SIP" (https://en.wikipedia.org/wiki/System_Integrity_Protection) that OS X El Capitan is introducing that will prevent the parent process from requesting memory information from content processes. This puts a big hole in the plan.

Instead of having a special workaround for OS X, I think we should re-think the back-end a little bit. Here's what I propose:

1) Introduce a new module at toolkit/modules, called Memory.jsm. For now, Memory.jsm will contain a single Memory JS Object that offers a single function, called "summary". That method will return a DOM Promise that will resolve with the JS structure we've been talking about ({ <pid>: {"uss": <measurement>, "rss": <measurement>} }) which your front-end will be able to understand.

2) When "summary" is called, have Memory.jsm make sure every content process has a special process script[1] loaded in them that will respond with the USS and RSS values. They'll also return their PIDs, since (and thanks for erahm for pointing this out), a process can determine its own PID from JS using this:

let pid = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processID;

3) Have your front-end call Memory.summary(), and hold onto the Promise. When the Promise resolves, update the table, and then queue a timer to request the next summary after some delay (so we'll be using setTimeout instead of setInterval).

Memory.jsm has the advantage of a simpler interface (which WebExtensions can make use of). It might also make sense down the line of putting all of the subprocess functionality of nsMemoryReporterManager into Memory.jsm, and have about:memory talk to Memory.jsm instead of talking directly with nsMemoryReporterManager, but that's out of scope.

I know this sounds like a lot, but I think this is going to simplify things a great deal. The other advantage is that it's no longer platform specific implementation - all of this should work cross-platform.

Rutuja - I'll coordinate with you in email to lay out more granular steps.

[1]: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Message_Manager/Process_scripts

Comment 52

[Rutuja]

Attachment: Patch with new Backend approach

Comment 53

[Rutuja]

Attachment: Patch with modified Backend

Comment 54

[Rutuja]

Attachment: Screenshot of the tool

Comment 55

[Rutuja]

I have attached the modified patch according to the new backend approach and the screenshot of the tool as well.
Currently, the parent row gets added in the end. I'll try adding it as the first row (I may have to use regex for this as the parent's summary gets appended only in the finish method of Memory.jsm, because of which it gets appended last to the table).
The patch hasn't gotten appended with the Memory.jsm module (Absolute patch generation is a bit troublesome, and may be because it is a new module altogether). I'm sorry, but I'm attaching the entire code here.. 
Here's what this module is like:

Path: toolkit/modules/Memory.jsm

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+this.EXPORTED_SYMBOLS = ["Memory"];
+
+const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
+
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/Timer.jsm");
+
+this.Memory = {
+  summary() {
+    if (!this._pendingPromise) {
+      this._pendingPromise = new Promise((resolve) => {
+        this._pendingResolve = resolve;
+        this._summaries = {};
+        Services.ppmm.broadcastAsyncMessage("Memory:GetSummary");
+        Services.ppmm.addMessageListener("Memory:Summary", this);
+        this._pendingTimeout = setTimeout(() => { this.finish(); }, 2000); 
+        let pid = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processID;
+      });
+    }
+    return this._pendingPromise;
+  },
+
+  receiveMessage(msg) {
+    if (msg.name != "Memory:Summary" || !this._pendingResolve) {
+      return;
+    }
+    this._summaries[msg.data.pid] = msg.data.summary;
+    // Now we check if we are done for all content processes
+    if (Object.keys(this._summaries).length >= Services.ppmm.childCount - 1) {
+      this.finish();
+    } 
+  },
+
+  finish() {
+    // Code to gather the USS and RSS values for the parent process. 
+    let memMgr = Cc["@mozilla.org/memory-reporter-manager;1"]
+                       .getService(Ci.nsIMemoryReporterManager);
+    let pid = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processID;
+    let rss = memMgr.resident;
+    let uss = memMgr.residentUnique;
+    let parent = "Parent_" + pid;
+    this._summaries[parent] = {
+      "uss": uss,
+      "rss": rss
+    };
+
+    this._pendingResolve(this._summaries);
+    this._pendingResolve = null;
+    this._summaries = null;
+    this._pendingPromise = null;
+    clearTimeout(this._pendingTimeout); 
+  } 
+};

Comment 56

[Nicholas Nethercote [inactive]]

A couple of comments just from looking at the screen shot.

- "Performance of Subprocesses" -- I think "Memory usage of Subprocesses" would be a better title.

- The measurements have no units. They should.

- It's probably better to show the measurements in megabytes rather than bytes. Standard Mozilla style is to (unfortunately) use "MB" for base-1024 megabytes (as opposed to "MiB"). I.e. divide by 1,048,576 rather than 1,000,000.

- Separators make large numbers easier to read, e.g. 1,234,567. Which separator you use is locale-dependent, and I'm not sure how that's specified. This must be a solved problem in other parts of the codebase, though.

Comment 57

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Nicholas Nethercote [:njn] from comment #56)

> - Separators make large numbers easier to read, e.g. 1,234,567. Which
> separator you use is locale-dependent, and I'm not sure how that's
> specified. This must be a solved problem in other parts of the codebase,
> though.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat :)

Comment 58

[Zibi Braniecki [:zbraniecki][:gandalf]]

In bug 1261154 they are adding unit format localization entities for KiB/MiB etc. You may want to share it with them.

In that case you'll have sth like:

unit.properties:

unitformat-kib-short = {0} KiB
unitformat-mib-short = {0} MiB

unit.js:

entity.replace('{0}', value.toLocaleString(navigator.language, {
  useGrouping: true
}));

Comment 59

[rsurve]

(In reply to Nicholas Nethercote [:njn] from comment #56)
> A couple of comments just from looking at the screen shot.
> 
> - "Performance of Subprocesses" -- I think "Memory usage of Subprocesses"
> would be a better title.
> 
> - The measurements have no units. They should.
Thank you for the feedback! :)
I'll make the changes you've suggested and also try making the measurements easier to read.

Comment 60

[rsurve]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #58)
> In bug 1261154 they are adding unit format localization entities for KiB/MiB
> etc. You may want to share it with them.
> 
> In that case you'll have sth like:
> 
> unit.properties:
> 
> unitformat-kib-short = {0} KiB
> unitformat-mib-short = {0} MiB
> 
> unit.js:
> 
> entity.replace('{0}', value.toLocaleString(navigator.language, {
>   useGrouping: true
> }));

Hey!
Thanks for the feedback! I didn't understand much from the formatAbbreviatedBytes link, but from the wiki, I could gather this:
Suppose I have a sample measurement, say number, I can make it locale-specific and hence more readable by:
new Intl.NumberFormat().format(number) (for US English specific locales)
new Intl.NumberFormat('XYZ').format(number) (for XYZ specific locales).
Does the code-snippet specified by you do the MiB conversion for any number?
Should I keep the units in Mb or  in MiB? Could you elaborate a bit on the snippet (wrt what entity.replace does and what's unitformat-mib-short = {0} MiB). 
Thanks a lot! :)

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785157 [details] [diff] [review]
Patch with modified Backend

Review of attachment 8785157 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking like it's moving in the right direction, Rutuja. Good work.

The Memory.jsm file appears to be missing / empty. Can you update the patch with that file?

I have some suggestions below.

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +948,5 @@
> + * If the page goes hidden, it also handles visibility change by not
> + * querying the content processes unnecessarily.
> + */
> +var SubprocessMonitor = {
> +  _interval : null,

I don't think we're going to be using setInterval anymore (instead, we're going to be using a setTimeout once the Memory.summary Promise resolves).

So I think we can probably rename this to _timeout.

@@ +952,5 @@
> +  _interval : null,
> +
> +  /**
> +   * Init calls 'handleVisibilityChange' if the page is not hidden and adds an eventlistener
> +   * for handling any  visibility changes.

Nit - extra space before "visibility"

@@ +967,5 @@
> +   * and clears the interval otherwise.
> +   */
> +  handleVisibilityChange: function() {
> +    if (!document.hidden) {
> +      this._interval = setInterval(() => this.updateTable(), UPDATE_INTERVAL_MS);

We're going to need to restructure this a little bit, since we need to wait for Memory.summary to resolve before we setTimeout for the next one.

Here's my suggestion:

Have updateTable, upon resolving and updating the table, call a function defined here on the SubprocessMonitor Object. That function should use setTimeout to have updateTable be called again after UPDATE_INTERVAL_MS. Maybe call this function queueUpdate. It should also stash the return value of setTimeout as _timout on the SubprocessMonitor Object.

updateTable should then check to see if the document is hidden, and return without doing anything if it is. If it's visible, it calls Memory.summary(), and updates the table once the Promise resolves, and then calls queueUpdate again.

The handleVisibilityChange event handler should be changed to use clearTimeout if the document has become hidden. If the document has become visible, then queueTimeout should be called.

What do you think of that?

@@ +979,5 @@
> +   * This function adds a row to the subprocess-performance table for every new pid
> +   * and populates and regularly updates it with RSS/USS measurements.
> +   */
> +  updateTable: function() {
> +    Cu.import("resource://gre/modules/Memory.jsm");

Now that this is starting to work, we should move this import higher up in aboutPerformance.js - probably around here: http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/toolkit/components/aboutperformance/content/aboutPerformance.js#16

@@ +980,5 @@
> +   * and populates and regularly updates it with RSS/USS measurements.
> +   */
> +  updateTable: function() {
> +    Cu.import("resource://gre/modules/Memory.jsm");
> +    let resultTable = document.getElementById("subprocess-reports");

No need to define this out here - might as well put it inside the then() function.

@@ +982,5 @@
> +  updateTable: function() {
> +    Cu.import("resource://gre/modules/Memory.jsm");
> +    let resultTable = document.getElementById("subprocess-reports");
> +    Memory.summary().then((summaries) => {
> +      dump("Summaries: " + JSON.stringify(summaries, null, '\t') + "\n");

Leftover logging to remove.

@@ +983,5 @@
> +    Cu.import("resource://gre/modules/Memory.jsm");
> +    let resultTable = document.getElementById("subprocess-reports");
> +    Memory.summary().then((summaries) => {
> +      dump("Summaries: " + JSON.stringify(summaries, null, '\t') + "\n");
> +      for(let pid in summaries) {

Space before (. Make sure to run ./mach eslint against your changes here, and to update your patch with any fixes it recommends.

@@ +998,5 @@
> +          row.insertCell();
> +          // And finally append the row to the table
> +          resultTable.appendChild(row);
> +        }
> +        /**

From what I've seen, we tend to use // style for comments that aren't function definitions. That seems to be the pattern in this file anyways, and we should try to stay consistent.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +161,5 @@
>    <body onload="go()">
> +    <table id="subprocess-reports">
> +      <thead>Performance of Subprocesses</thead>
> +      <tr class="parent">
> +        <th>Process ID</th>  

Trailing whitespace.

::: toolkit/content/process-content.js
@@ +45,5 @@
> +    receiveMessage(msg) {
> +      if (msg.name != "Memory:GetSummary") {
> +        return;
> +      }
> +      let pid = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processID;

I think you can use Services.appinfo.processID, since Services.appinfo is a shortcut to the nsIXULRuntime service.

@@ +47,5 @@
> +        return;
> +      }
> +      let pid = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processID;
> +      let memMgr = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                       .getService(Ci.nsIMemoryReporterManager);

Please align the . before getService with the [ after Cc.

@@ +57,5 @@
> +          uss,
> +          rss,
> +        }
> +      });
> +    }, 

Trailing whitespace.

Comment 62

[rsurve]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #61)
> Comment on attachment 8785157 [details] [diff] [review]
> Patch with modified Backend
> 
> Review of attachment 8785157 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/aboutperformance/content/aboutPerformance.js 
> @@ +967,5 @@
> > +   * and clears the interval otherwise.
> > +   */
> > +  handleVisibilityChange: function() {
> > +    if (!document.hidden) {
> > +      this._interval = setInterval(() => this.updateTable(), UPDATE_INTERVAL_MS);
> 
> We're going to need to restructure this a little bit, since we need to wait
> for Memory.summary to resolve before we setTimeout for the next one.
> 
> Here's my suggestion:
> 
> Have updateTable, upon resolving and updating the table, call a function
> defined here on the SubprocessMonitor Object. That function should use
> setTimeout to have updateTable be called again after UPDATE_INTERVAL_MS.
> Maybe call this function queueUpdate. It should also stash the return value
> of setTimeout as _timout on the SubprocessMonitor Object.
> 
> updateTable should then check to see if the document is hidden, and return
> without doing anything if it is. If it's visible, it calls Memory.summary(),
> and updates the table once the Promise resolves, and then calls queueUpdate
> again.
> 
> The handleVisibilityChange event handler should be changed to use
> clearTimeout if the document has become hidden. If the document has become
> visible, then queueTimeout should be called.
> 
> What do you think of that?
> 
Thanks for the review.. :)
Yes sure, I'll try modifying it this way. I shall attach the updated patch soon and let you know in case I have questions. I'll also make the other changes as pointed in the review.

Comment 63

[Rutuja]

Attachment: Patch with latest changes

Comment 64

[Rutuja]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #61)
> Comment on attachment 8785157 [details] [diff] [review]
> Patch with modified Backend
> 
> Review of attachment 8785157 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> The Memory.jsm file appears to be missing / empty. Can you update the patch
> with that file?
Sorry for this, my diff file is not getting appended with the jsm file. I tried to add dummy comments and building and then did a diff to see if any changes to the jsm file get reflected in the diff, they don't.. I have attached a patch with all the changes suggested. Comment 55 has the content of Memory.jsm.

Comment 65

[Rutuja]

Attachment: Latest Memory.jsm file

Comment 66

[Zibi Braniecki [:zbraniecki][:gandalf]]

If you need an answer, it's better to needinfo people :)

(In reply to rsurve from comment #60)
> Thanks for the feedback! I didn't understand much from the
> formatAbbreviatedBytes link, but from the wiki, I could gather this:
> Suppose I have a sample measurement, say number, I can make it
> locale-specific and hence more readable by:
> new Intl.NumberFormat().format(number) (for US English specific locales)
> new Intl.NumberFormat('XYZ').format(number) (for XYZ specific locales).

Yes. Creating a new formatter object makes sense if you're going to format a lot of numbers at once, for example:

const nf = new Intl.NumberFormat(navigator.languages, {
  useGrouping: true
});

for (i = 1; i < 10000; i++) {
  console.log(nf.format(uss[i]);
  console.log(nf.format(rss[i]);
}

If you only plan to format one or two, you can just use:

console.log(uss.toLocaleString(navigator.languages, { useGrouping: true}));
console.log(rss.toLocaleString(navigator.languages, { useGrouping: true}));

> Does the code-snippet specified by you do the MiB conversion for any number?

What's "MiB conversion"?
We don't yet have UnitFormatter, and even once we will have it we will not do any conversion. You have to do the calculation and choose if you need MiB or KiB or TiB.

> Should I keep the units in Mb or  in MiB?

That's a UX question for your reviewer/mentor/UX team.

> Could you elaborate a bit on the snippet (wrt what entity.replace does and what's unitformat-mib-short = {0}
> MiB). 

entity is just a string, so `entity.replace` replaces a substring - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

`unitformat-mib-short = {0} MiB` is a suggestion for how the localization entity should look like.

Since you are adding new strings, you need to place them in localization files (*.properties) and then retrieve using StringBundle service (like any other string retrieved in JS code). Then I suggest that you use `String.prototype.replace` to replace the `{0}` with your formatted number and the result string is what you put into DOM.

Comment 67

[rsurve]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #66)
Thank you! :)

Comment 68

[Rutuja]

Attachment: Screenshot (with units for measurement)

Comment 69

[Rutuja]

Attachment: Latest patch

Please review this patch and ignore the previous one (I've made it obsolete).

Comment 70

[timbugzilla]

Perhaps it would be a good idea to provide the associated tab name(s) when there are multiple content processes?

Comment 71

[Rutuja]

(In reply to timbugzilla from comment #70)
> Perhaps it would be a good idea to provide the associated tab name(s) when
> there are multiple content processes?

Yes.. This is a good idea.

Comment 72

[Zibi Braniecki [:zbraniecki][:gandalf]]

The `DownloadUtils.convertByteUnits` is a horrible way to localize units, but I see it pretty widespread so I guess it's fine to reuse it here and we'll have to redo how the API works all together in a separate bug.

Comment 73

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786183 [details] [diff] [review]
Latest patch

Review of attachment 8786183 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, Rutuja! Just some really minor things now.

I was, however, unable to apply this patch to test. Mercurial (and patch) complain that the diff is malformed. How did you generate it?

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +1005,5 @@
> +             row.insertCell();
> +             // And finally append the row to the table
> +             resultTable.appendChild(row);
> +           }
> +           // Now we update the values in the row, which is hardcoded for now, 

Trailing whitespace

::: toolkit/modules/Memory.jsm
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");
> +
> +this.Memory = {
> +  summary() {

Please add documentation for this function. Specifically, what it returns (a Promise), and what that Promise resolves with (the structure of the result object).

@@ +17,5 @@
> +        this._summaries = {};
> +        Services.ppmm.broadcastAsyncMessage("Memory:GetSummary");
> +        Services.ppmm.addMessageListener("Memory:Summary", this);
> +        this._pendingTimeout = setTimeout(() => { this.finish(); }, 2000);
> +        let pid = Services.appinfo.processID;

This doesn't appear to be used.

@@ +24,5 @@
> +    return this._pendingPromise;
> +  },
> +
> +  receiveMessage(msg) {
> +

Please remove this newline.

@@ +30,5 @@
> +      return;
> +    }
> +    this._summaries[msg.data.pid] = msg.data.summary;
> +    // Now we check if we are done for all content processes.
> +    if (Object.keys(this._summaries).length >= Services.ppmm.childCount - 1) {

We should probably document why we're doing >= childCount - 1.

@@ +32,5 @@
> +    this._summaries[msg.data.pid] = msg.data.summary;
> +    // Now we check if we are done for all content processes.
> +    if (Object.keys(this._summaries).length >= Services.ppmm.childCount - 1) {
> +      this.finish();
> +    } 

Trailing whitespace.

@@ +43,5 @@
> +                   .getService(Ci.nsIMemoryReporterManager);
> +    let pid = Services.appinfo.processID;
> +    let rss = memMgr.resident;
> +    let uss = memMgr.residentUnique;
> +    let parent = "Parent_" + pid;

Not entirely sure it's necessary to include the pid with the parent. The parent pid will never, ever change throughout the lifetime of the browsing session. I think we can return something like "parent" safely.

@@ +46,5 @@
> +    let uss = memMgr.residentUnique;
> +    let parent = "Parent_" + pid;
> +    this._summaries[parent] = {
> +      "uss": uss,
> +      "rss": rss

You can also do:

this._summaries[parent] = { rss, uss };

This is equivalent.

@@ +53,5 @@
> +    this._pendingResolve(this._summaries);
> +    this._pendingResolve = null;
> +    this._summaries = null;
> +    this._pendingPromise = null;
> +    clearTimeout(this._pendingTimeout); 

Trailing whitespace here and below.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786183 [details] [diff] [review]
Latest patch

Review of attachment 8786183 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +159,5 @@
>      </style>
>    </head>
>    <body onload="go()">
> +    <table id="subprocess-reports">
> +      <thead>Memory usage of Subprocesses</thead>

Also, this looks a little out of place with the rest of about:performance.

Would you mind moving this out to a header, like "Performance of Add-ons" down below?

Comment 75

[Rutuja]

Attachment: Patch with changes

Comment 76

[Rutuja]

(In reply to Rutuja from comment #75)
> Created attachment 8786416 [details] [diff] [review]
> Patch with changes

This patch has changes as suggested in the latest review

Comment 77

[Rutuja]

Attachment: Patch : Latest

Comment 78

[Rutuja]

Attachment: Latest Patch

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786428 [details] [diff] [review]
Latest Patch

Review of attachment 8786428 [details] [diff] [review]:
-----------------------------------------------------------------

I just tested this locally, and it works quite nicely. Great job, Rutuja!

While testing it, I noticed a few small issues. Primarily, I think we have a problem with node removal. I have a suggestion for dealing with that below.

Great job on the alignment and documentation!

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +958,5 @@
> +   * for handling any visibility changes.
> +   */
> +  init: function() {
> +     if (!document.hidden) {
> +       SubprocessMonitor.handleVisibilityChange();

Having tried the patch, when we first load about:performance, we probably don't want to wait UPDATE_INTERVAL_MS for the first results to be displayed. So let's call updateTable() here if !document.hidden instead.

@@ +988,5 @@
> +  updateTable: function() {
> +    if (!document.hidden) {
> +      Memory.summary().then((summaries) => {
> +        let resultTable = document.getElementById("subprocess-reports");
> +        for (let pid in summaries) {

I've just realized that there's a slight problem here: if a content process closes while we have about:performance open, the row for the closed content process will never be removed.

So thinking about it, we might want to change this algorithm slightly. This will also make it so that we don't have to put "pid-" as a prefix for the ids of each row.

1) Instead of iterating the summaries first, iterate the the resultTable.children

2) For each child of resultTable.children, get the data-pid attribute (via child.dataset.pid, which we'll add in a later step), and see if there's a summary for that pid in summaries. See https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_data_attributes for documentation on dataset.

If there's a summary for it: update that row with the summary, and remove that pid from summaries, since we're done with it.
If there's no summary for it: this is a row for a pid that no longer exists. Add it to a list of nodes we can recycle.

3) Now iterate the remaining pids in summaries. For each pid, see if there's a node in the recycle-list we can re-use.

If so: update the row, and append it to the table. Make sure to set the row.dataset.pid attribute to the pid too.
If not: create a new row, set the row.dataset.pid to the pid, and update the summary on it

4) For each row in the recycled list that hasn't been re-used, set hidden = true on it, like: child.hidden = true;.

So what we're doing here is DOM node recycling. This makes it so that we don't end up creating more DOM nodes than we need - we hide the ones that no longer are useful, and recycle them once we need more rows. And if we ever run out of recyclable rows, we create new ones.

Thoughts on that?

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +110,5 @@
> +        text-align: start;
> +        border: 1px solid var(--in-content-border-color);
> +        border-spacing: 0px;
> +        float: right;
> +        margin-top: 2em;

Let's get rid of this margin-top so that there's not so much of a gap between the header and the table.

@@ +119,5 @@
> +      }
> +      #subprocess-reports:-moz-dir(rtl) {
> +        float: left;
> +      }
> +      /* basic shared rules */

I think we can probably get rid of this comment.

@@ +137,5 @@
> +      #subprocess-reports th.column {
> +        white-space: nowrap;
> +        width: 0px;
> +      }
> +      /* body */

We can get rid of this comment too.

@@ +145,5 @@
> +        border-color: var(--in-content-table-border-dark-color);
> +        border-spacing: 40px;
> +      }
> +
> +      th[title]:hover:after {

What does this do? There's no content, so the :after pseudoelement is empty. I think we can get rid of this entire block of rules.

@@ +161,5 @@
>    <body onload="go()">
>      <div>
> +      <h2>Memory usage of Subprocesses</h2>
> +      <table id="subprocess-reports">
> +        <tr class="parent">

Is the parent class used? If not, we can remove it.

::: toolkit/modules/Memory.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/Timer.jsm");
> +
> +this.Memory = {
> +  /**
> +   * This function return a Promise along with sending its broadcast.

"return" -> "returns"

"its broadcast" is an implementation detail. I like that you mention that we're returning a Promise. Perhaps something more like:

This function returns a Promise that resolves with an Object that
describes basic memory usage for each content process and the parent
process.

@@ +13,5 @@
> +  /**
> +   * This function return a Promise along with sending its broadcast.
> +   * The promise resolves with a “summaries” Object, which is used to store
> +   * the results from each content process.
> +   * @returns (JS Object)

Thanks for this comment. Let's change this to:

@returns Promise
@resolves JS Object
          An Object in the following format:

Note that the format is wrong - the pid is the key. So this should be:

An Object in the following format:
{
  "parent": {
    uss: <int>,
    rss: <int>,
  },
  <pid>: {
    uss: <int>,
    rss: <int>,
  },
  <pid>...
}

@@ +29,5 @@
> +      this._pendingPromise = new Promise((resolve) => {
> +        this._pendingResolve = resolve;
> +        this._summaries = {};
> +        Services.ppmm.broadcastAsyncMessage("Memory:GetSummary");
> +        Services.ppmm.addMessageListener("Memory:Summary", this);

We should remove this message listener once we've resolved the promise in finish().

Via: Services.ppmm.removeMessageListener("Memory:Summary", this);

@@ +30,5 @@
> +        this._pendingResolve = resolve;
> +        this._summaries = {};
> +        Services.ppmm.broadcastAsyncMessage("Memory:GetSummary");
> +        Services.ppmm.addMessageListener("Memory:Summary", this);
> +        this._pendingTimeout = setTimeout(() => { this.finish(); }, 2000);

This 2000ms hardcoded timeout should be set as a constant somewhere in this file near the top.

@@ +58,5 @@
> +    let memMgr = Cc["@mozilla.org/memory-reporter-manager;1"]
> +                   .getService(Ci.nsIMemoryReporterManager);
> +    let rss = memMgr.resident;
> +    let uss = memMgr.residentUnique;
> +    this._summaries["Parent"] = {uss, rss};

Nit: space around uss, rss, example:

this._summaries["parent"] = { uss, rss };

Comment 80

[rsurve]

This is with respect to the updateTable function:
Could you elaborate on how to query an item from the summaries object and how to delete it? Will something like delete summaries[pid] or summaries.find(pid)work? Can we not directly use a queryselector or something? Also, instead of making a separate list for recycling nodes (rows), can we not have an extra attribute per row, say data-recycle? If it is 1, then we hide this row or re-use it for another pid if needed, setting it 0. If it is 0, this means the corresponding tab is active, so we simply update the values. If not this approach, then how do we segregate the rows into recyclable or currently in use? 
With respect to the xhtml file:
<tr class=parent> is not being used anywhere now, so I'll remove that.
The th[title]:hover:after is for the detailed info. regarding what RSS/USS is that the user gets after hovering over the columns (th) with RSS/USS. The content for this is in the title attribute of the RSS and USS <th>s defined below in the table. So, I think its necessary for styling that extra info.

Comment 81

[Mike Conley (:mconley) (:⚙️)]

(In reply to rsurve from comment #80)
> This is with respect to the updateTable function:
> Could you elaborate on how to query an item from the summaries object and
> how to delete it? Will something like delete summaries[pid] or
> summaries.find(pid)work? Can we not directly use a queryselector or
> something?

Testing whether an item is in the summaries object can go like this:

if (summaries[pid]) {
  // We enter here if there's an entry for
  // summaries[pid].
}

Or if you wanted to save some keystrokes, you could also do:

if (let summary = summaries[pid]) {
  // If summary is not undefined, then we enter here with
  // summary in scope. That way, we don't have to keep doing
  // summaries[pid] every time we want to access a property of
  // the summary.
}

> Also, instead of making a separate list for recycling nodes
> (rows), can we not have an extra attribute per row, say data-recycle? If it
> is 1, then we hide this row or re-use it for another pid if needed, setting
> it 0. If it is 0, this means the corresponding tab is active, so we simply
> update the values. If not this approach, then how do we segregate the rows
> into recyclable or currently in use? 

The problem with marking nodes as recyclable with an attribute is that it means we have to loop through the rows again. Granted, this list of nodes should always be quite small, but a lot of that double-looping is probably unnecessary.

Hiding the nodes using .hidden is how I suggest segregating them. When doing the first loop through, for the nodes that are hidden, toss them into the recyclable node array for use in the 3rd step.

Does that make sense?

> With respect to the xhtml file:
> <tr class=parent> is not being used anywhere now, so I'll remove that.

Great, thanks.

> The th[title]:hover:after is for the detailed info. regarding what RSS/USS
> is that the user gets after hovering over the columns (th) with RSS/USS. The
> content for this is in the title attribute of the RSS and USS <th>s defined
> below in the table. So, I think its necessary for styling that extra info.

The title attribute is using the built-in tooltip functionality of the browser. I suspect that if you remove that rule, you'll still see the same behaviour.

Comment 82

[Mike Conley (:mconley) (:⚙️)]

Sorry, I forgot one last thing - deleting an entry from summaries can be done like this:

delete summaries[pid];

Comment 83

[Rutuja]

Attachment: Final Patch with changes

Comment 84

[Rutuja]

Attachment: Final patch (with latest changes)

Comment 85

[Rutuja]

Attachment: Final Patch (with changes)

Comment 86

[Rutuja]

Attachment: Final Patch

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786883 [details] [diff] [review]
Final Patch

Review of attachment 8786883 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fantastic, Rutuja! I just tested this locally and it works wonderfully. Fantastic work.

There's one minor bug (see my comment regarding updateRow and the dataset.pid), and the rest of my comments are cosmetic / documentation. I suspect we'll be able to land this this week!

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +950,5 @@
> + * If the page goes hidden, it also handles visibility change by not
> + * querying the content processes unnecessarily.
> + */
> +var SubprocessMonitor = {
> +  _timeout : null,

No space before :

@@ +953,5 @@
> +var SubprocessMonitor = {
> +  _timeout : null,
> +
> +  /**
> +   * Init calls 'handleVisibilityChange' if the page is not hidden and adds an eventlistener

"Init calls 'handleVisibilityChange' if the page is not hidden"

is no longer true. Also, it's kind of an implementation detail. We should maybe talk about the effect instead of the method name, for example:

"Init will start the process of updating the table if the page is not hidden, and set up an event listener for handling visibility changes."

@@ +957,5 @@
> +   * Init calls 'handleVisibilityChange' if the page is not hidden and adds an eventlistener
> +   * for handling any visibility changes.
> +   */
> +  init: function() {
> +     if (!document.hidden) {

You've got 3-space indentation from lines 961-964. Please use 2-space.

@@ +969,5 @@
> +   * and clears the interval otherwise.
> +   */
> +  handleVisibilityChange: function() {
> +    if (!document.hidden) {
> +      SubprocessMonitor.queueUpdate();

Busted indentation from lines 973-977

@@ +976,5 @@
> +       this._timeout = null;
> +     }
> +  },
> +
> +  queueUpdate: function() {

Needs documentation

@@ +981,5 @@
> +    this._timeout = setTimeout(() => this.updateTable(), UPDATE_INTERVAL_MS);
> +  },
> +
> +  /**
> +   * This is a helper function for updateTable, which updates a particular row.

Thanks for the documentation! Make sure to document the arguments that are expected as well:

Like:

@param row (<tr> node)
       <Put a description here>
@param summaries (object)
       <Put a description here>
@param pid (string)
       <Put a description here>

@@ +984,5 @@
> +  /**
> +   * This is a helper function for updateTable, which updates a particular row.
> +   */
> +  updateRow: function(row, summaries, pid) {
> +    row.cells[0].textContent = pid;

We need to update the row.dataset.pid here as well, in case we're recycling a row.

@@ +1003,5 @@
> +        let recycle = [];
> +        // We first iterate the table to check if summaries exist for rowPids,
> +        // if yes, update them and delete the pid's summary or else hide the row
> +        // for recycling it.
> +        for (let i = 1, row; row = resultTable.rows[i]; i++){

We should document why we're starting at 1 instead of 0 (to skip the header row).

@@ +1012,5 @@
> +            // but we might want to make this more adaptable in the future.
> +            SubprocessMonitor.updateRow(row, summaries, rowPid);
> +            delete summaries[rowPid];
> +          } else {
> +              row.hidden = true;

Busted indentation here from lines 1016-1018

Maybe add a comment here for what we're doing (taking this unnecessary row, hiding it, and stashing it for potential re-use)

::: toolkit/modules/Memory.jsm
@@ +4,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["Memory"];
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +const TIMEOUT_INTERVAL = 2000;

Please document what TIMEOUT_INTERVAL is for.

@@ +36,5 @@
> +        this._pendingResolve = resolve;
> +        this._summaries = {};
> +        Services.ppmm.broadcastAsyncMessage("Memory:GetSummary");
> +        Services.ppmm.addMessageListener("Memory:Summary", this);
> +        this._pendingTimeout = setTimeout(() => { this.finish(); }, TIMEOUT_INTERVAL); 

Trailing whitespace

@@ +58,5 @@
> +    }
> +  },
> +
> +  finish() {
> +    // Code to gather the USS and RSS values for the parent process. This 

Trailing whitespace.

Comment 88

[Rutuja]

Attachment: PATCH (LATEST)

Comment 89

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8787302 [details] [diff] [review]
PATCH (LATEST)

Review of attachment 8787302 [details] [diff] [review]:
-----------------------------------------------------------------

Just noticed one more minor bug while testing, and some trailing whitespace. We're very close!

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +953,5 @@
> +var SubprocessMonitor = {
> +  _timeout: null,
> +
> +  /**
> +   * Init will start the process of updating the table if the page is not hidden, and set 

Trailing whitespace.

@@ +977,5 @@
> +    }
> +  },
> +
> +  /**
> +   * This function queues a timer to request the next summary using updateTable 

Trailing whitespace.

@@ +986,5 @@
> +  },
> +
> +  /**
> +   * This is a helper function for updateTable, which updates a particular row.
> +   * @param {<tr> node} row The row to be updated.                   

Trailing whitespace.

@@ +1005,5 @@
> +   */
> +  updateTable: function() {
> +    if (!document.hidden) {
> +      Memory.summary().then((summaries) => {
> +        let resultTable = document.getElementById("subprocess-reports");

I forgot, we need to account for the case where summaries is empty (due to Memory.summary timing out). In that case, we should probably just queue another update and bail out. That'd probably be simplest for now.

You can tell if the object is empty by using something like:

if (!(Object.keys(summaries).length)) {
  // The summaries list was empty, which means we timed out getting
  // the memory reports. We'll try again later.
  SubprocessMonitor.queueUpdate();
  return;
}

@@ +1040,5 @@
> +            row.insertCell();
> +            // Insert another cell for RSS.
> +            row.insertCell();
> +          }
> +          // Set the data-pid attribute to pid.

I think this comment doesn't add much - it's pretty clear what's going on.

::: toolkit/modules/Memory.jsm
@@ +4,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["Memory"];
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +// How long we should wait for the promise to resolve. 

Trailing whitespace. "promise" -> "Promise".

Comment 90

[Rutuja]

Attachment: **Patch:(Latest)**

Comment 91

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8787481 [details] [diff] [review]
**Patch:(Latest)**

Review of attachment 8787481 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fantastic to me! Great job, Rutuja - thanks for sticking it out!

There's a strange conflict on the moz.build file when applying the patch, but I've sorted it (it's only a one-line change). I'll land this on inbound tomorrow morning once the tree re-opens. I'll also file some follow-up bugs, because I've got some ideas for future work and improvement (I'd be eager to hear your ideas for future work and improvement as well).

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad925dd2e4e
Add process memory reporting tool to about:performance. r=mconley

Comment 93

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad925dd2e4e9f6943b228f0173d01278a74c2a8
Bug 1255843 - Add process memory reporting tool to about:performance. r=mconley

Comment 94

[Mike Conley (:mconley) (:⚙️)]

I had to back this out because of build failures like:

https://treeherder.mozilla.org/logviewer.html#?job_id=35179227&repo=mozilla-inbound

Something is up with the patch - I suspect Memory.jsm, despite being in the patch, isn't being included for some odd reason. I'll investigate tomorrow.

Comment 95

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e9d711aa977

Comment 96

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e2d63ab8723e932899b1ba1be4403850876133
Bug 1255843 - Add process memory reporting tool to about:performance. r=mconley

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22e2d63ab872

Comment 98

[Nicholas Nethercote [inactive]]

Congratulations on landing your change, Rutuja :)

I just tried it out in Nightly and it looks good! I have one small comment, about the "Display only the latest 10s" and "Refresh automatically" checkboxes. After checking and unchecking them a number of times, I *think* they apply to all the measurements, is that right? But they look like they only apply to the memory usage measurements because of the layout.

Also, the "Display on the the latest 10s" checkbox doesn't really apply to the memory measurements, given that they are instantaneous. (At first I thought maybe I would get a historical record showing prior memory measurements every 10 seconds, or something.)

So, I found those checkboxes a bit confusing, and others might too. Perhaps something for a follow-up bug.

Comment 99

[Blake Kaplan (:mrbkap) (inactive)]

Rutuja, great job here! The new memory info looks great. Thanks for all of your hard work getting this in.

Comment 100

[Mike Conley (:mconley) (:⚙️)]

I just filed a bunch of follow-ups blocking this bug. Rutuja, if you're still interested, let me know if you want to hack on any of these - I'll happily mentor!

Comment 101

[rsurve]

(In reply to Blake Kaplan (:mrbkap) from comment #99)
> Rutuja, great job here! The new memory info looks great. Thanks for all of
> your hard work getting this in.

Thank you! :)

Comment 102

[rsurve]

(In reply to Nicholas Nethercote [:njn] from comment #98) 
> So, I found those checkboxes a bit confusing, and others might too. Perhaps
> something for a follow-up bug.
Yes sure, I'd like to help in fixing these issues with follow-up bugs.

Comment 103

[rsurve]

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #100)
> I just filed a bunch of follow-ups blocking this bug. Rutuja, if you're
> still interested, let me know if you want to hack on any of these - I'll
> happily mentor!

Yes sure, I'd like to try tackling these one by one, starting with something easy first. :)