Linux: about:memory

Linux: about:memory

(based on http://code.google.com/p/chromium/issues/detail?id=16251)

Add about:memory support to Linux. Rather than try and copy the
Windows output, we use a couple of metrics which make more sense on
Linux: USS and PSS.

[Evan Martin, 2009-08-28 22:32:09]

http://codereview.chromium.org/177024/diff/2001/2002
File base/process_util.h (right):

http://codereview.chromium.org/177024/diff/2001/2002#newcode288
Line 288: // Working Set (resident) memory usage broken down by
Mention this one is Windows-specific?

http://codereview.chromium.org/177024/diff/2001/2003
File base/process_util_linux.cc (right):

http://codereview.chromium.org/177024/diff/2001/2003#newcode298
Line 298: ws_usage->shared = 0;
I have no ability to vouch for the correctness of this, so I will only note that
L296 has weird tabbing.

http://codereview.chromium.org/177024/diff/2001/2006
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/177024/diff/2001/2006#newcode203
Line 203: // Other types ignored.
Should this default be NOTREACHED() with the list of other types explicitly
handled as cases where they are explicitly ignored?  I'm worried about
overlooking new process types.

http://codereview.chromium.org/177024/diff/2001/2008
File chrome/browser/memory_details_linux.cc (right):

http://codereview.chromium.org/177024/diff/2001/2008#newcode46
Line 46: const char process_name[17];
17, heh.

http://codereview.chromium.org/177024/diff/2001/2008#newcode47
Line 47: BrowserType browser; }
this would be clearer if the } was at the beginning of L48 I think.

http://codereview.chromium.org/177024/diff/2001/2008#newcode50
Line 50: { "firefox-3.5", FIREFOX },
firefox-3.0?

% ls /usr/bin/firefox-*
/usr/bin/firefox-2  /usr/bin/firefox-3.0  /usr/bin/firefox-3.5

http://codereview.chromium.org/177024/diff/2001/2008#newcode112
Line 112: if (sscanf(buf, "%u (%a[^)]) %*c %u", &pid, &process_name, &ppid) !=
3)
wow, didn't realize sscanf could do regexes.

http://codereview.chromium.org/177024/diff/2001/2008#newcode173
Line 173: for (std::vector<Process>::const_iterator
weird wrapping

http://codereview.chromium.org/177024/diff/2001/2008#newcode187
Line 187: std::vector<ProcessMemoryInformation> child_info) {
const &

http://codereview.chromium.org/177024/diff/2001/2008#newcode198
Line 198: for (std::vector<Process>::const_iterator
weird line wrapping

http://codereview.chromium.org/177024/diff/2001/2008#newcode219
Line 219: browsers_found.insert(current->pid);
You could just use this test on each process to decide whether it's a root,
rather than attempting to find parents of processes like you currently do.

http://codereview.chromium.org/177024/diff/2001/2008#newcode238
Line 238: current_browser.process_name = L"chrome";
is this gonna need to be "translated" for chromium?

http://codereview.chromium.org/177024/diff/2001/2008#newcode241
Line 241: for (std::set<pid_t>::const_iterator
Could you add a comment above this block describing what it's doing?  It looks
like it's processing each process root and all its children.

http://codereview.chromium.org/177024/diff/2001/2009
File chrome/browser/memory_details_win.cc (right):

http://codereview.chromium.org/177024/diff/2001/2009#newcode3
Line 3: // found in the LICENSE file.
rubber-stamping this file, assume it's the same as before

http://codereview.chromium.org/177024/diff/2001/2015
File chrome/chrome.gyp (left):

http://codereview.chromium.org/177024/diff/2001/2015#oldcode2591
Line 2591: 'browser/memory_details.cc',
do you still need this exclude on mac?

[agl, 2009-08-28 22:55:38]

http://codereview.chromium.org/177024/diff/2001/2002
File base/process_util.h (right):

http://codereview.chromium.org/177024/diff/2001/2002#newcode288
Line 288: // Working Set (resident) memory usage broken down by
On 2009/08/28 22:32:09, Evan Martin wrote:
> Mention this one is Windows-specific?

Done.

http://codereview.chromium.org/177024/diff/2001/2003
File base/process_util_linux.cc (right):

http://codereview.chromium.org/177024/diff/2001/2003#newcode298
Line 298: ws_usage->shared = 0;
On 2009/08/28 22:32:09, Evan Martin wrote:
> I have no ability to vouch for the correctness of this, so I will only note
that
> L296 has weird tabbing.

Done.

http://codereview.chromium.org/177024/diff/2001/2006
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/177024/diff/2001/2006#newcode203
Line 203: // Other types ignored.
On 2009/08/28 22:32:09, Evan Martin wrote:
> Should this default be NOTREACHED() with the list of other types explicitly
> handled as cases where they are explicitly ignored?  I'm worried about
> overlooking new process types.

It shouldn't be UNREACHED unless we want to add new UMA types. I don't really
understand UMA and I don't know if one can just add new strings like that.

http://codereview.chromium.org/177024/diff/2001/2008
File chrome/browser/memory_details_linux.cc (right):

http://codereview.chromium.org/177024/diff/2001/2008#newcode47
Line 47: BrowserType browser; }
On 2009/08/28 22:32:09, Evan Martin wrote:
> this would be clearer if the } was at the beginning of L48 I think.

Done.

http://codereview.chromium.org/177024/diff/2001/2008#newcode173
Line 173: for (std::vector<Process>::const_iterator
On 2009/08/28 22:32:09, Evan Martin wrote:
> weird wrapping

agl standard wrapping.

http://codereview.chromium.org/177024/diff/2001/2008#newcode187
Line 187: std::vector<ProcessMemoryInformation> child_info) {
On 2009/08/28 22:32:09, Evan Martin wrote:
> const &

Actually no, it's a callback

http://codereview.chromium.org/177024/diff/2001/2008#newcode219
Line 219: browsers_found.insert(current->pid);
On 2009/08/28 22:32:09, Evan Martin wrote:
> You could just use this test on each process to decide whether it's a root,
> rather than attempting to find parents of processes like you currently do.

Done.

http://codereview.chromium.org/177024/diff/2001/2008#newcode238
Line 238: current_browser.process_name = L"chrome";
On 2009/08/28 22:32:09, Evan Martin wrote:
> is this gonna need to be "translated" for chromium?

Done.

http://codereview.chromium.org/177024/diff/2001/2015
File chrome/chrome.gyp (left):

http://codereview.chromium.org/177024/diff/2001/2015#oldcode2591
Line 2591: 'browser/memory_details.cc',
On 2009/08/28 22:32:09, Evan Martin wrote:
> do you still need this exclude on mac?

Yes I think so. We don't have the needed code for Mac.