Overhaul base/process_util_linux.cc.

Overhaul base/process_util_linux.cc.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125913

[Lei Zhang, 2012-03-02 22:52:40]

erg: I was reading through process_util_linux.cc and saw a few things that made
me think "WTF?" and I ended up rewriting a large chunk of the code.

willchan: Feel free to review if you have bandwidth, otherwise just OWNERS
review.

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc
File base/process_util_linux.cc (left):

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:51: base::SplitString(mem_stats, ' ', proc_stats);
This did not take into account the fact that process names can have spaces in
them.

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:285: const size_t kVmPeak = 21;
This is wrong according to man 5 proc.

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:313: const size_t kVmHwm = 23;
This is clearly wrong, because GetPeakWorkingSetSize() and GetWorkingSetSize()
returns the exact same thing.

[Elliot Glaysher, 2012-03-02 23:23:44]

I think I understand this, so lgtm.

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:152: ProcStatsFields field_num) {
Is this used?

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:184: // e.g. /proc/self/ will return 0, whereas
/proc/1234 will return 124.
Did you mean 1234?

[willchan no longer on Chromium, 2012-03-02 23:25:42]

rubberstamping erg's lgtm

On Fri, Mar 2, 2012 at 3:23 PM, <erg@chromium.org> wrote:

> I think I understand this, so lgtm.
>
>
> https://chromiumcodereview.**appspot.com/9584024/diff/4/**
>
base/process_util_linux.cc<https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc>
> File base/process_util_linux.cc (right):
>
> https://chromiumcodereview.**appspot.com/9584024/diff/4/**
>
base/process_util_linux.cc#**newcode152<https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc#newcode152>
> base/process_util_linux.cc:**152: ProcStatsFields field_num) {
> Is this used?
>
> https://chromiumcodereview.**appspot.com/9584024/diff/4/**
>
base/process_util_linux.cc#**newcode184<https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc#newcode184>
> base/process_util_linux.cc:**184: // e.g. /proc/self/ will return 0,
> whereas /proc/1234 will return 124.
> Did you mean 1234?
>
>
https://chromiumcodereview.**appspot.com/9584024/<https://chromiumcodereview....
>

[Lei Zhang, 2012-03-02 23:48:40]

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux.cc
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:152: ProcStatsFields field_num) {
On 2012/03/02 23:23:44, Elliot Glaysher wrote:
> Is this used?

Nope! Delete.

https://chromiumcodereview.appspot.com/9584024/diff/4/base/process_util_linux...
base/process_util_linux.cc:184: // e.g. /proc/self/ will return 0, whereas
/proc/1234 will return 124.
On 2012/03/02 23:23:44, Elliot Glaysher wrote:
> Did you mean 1234?

Ya.

[commit-bot: I haz the power, 2012-03-02 23:49:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9584024/3004

[commit-bot: I haz the power, 2012-03-03 01:34:53]

Try job failure for 9584024-3004 (retry) (retry) on linux_rel for steps
"net_unittests, unit_tests, content_unittests, browser_tests, ui_tests".
It's a second try, previously, steps "net_unittests, unit_tests,
content_unittests, browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Lei Zhang, 2012-03-03 01:46:27]

On 2012/03/03 01:34:53, I haz the power (commit-bot) wrote:
> Try job failure for 9584024-3004 (retry) (retry) on linux_rel for steps
> "net_unittests, unit_tests, content_unittests, browser_tests, ui_tests".
> It's a second try, previously, steps "net_unittests, unit_tests,
> content_unittests, browser_tests, ui_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Whoops, I had a typo in the code, but the DCHECK caught it.

[commit-bot: I haz the power, 2012-03-03 01:46:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9584024/9001

[commit-bot: I haz the power, 2012-03-03 08:34:25]

Change committed as 124839

[Lei Zhang, 2012-03-08 01:00:37]

On 2012/03/03 08:34:25, I haz the power (commit-bot) wrote:
> Change committed as 124839

Turns out I was wrong about pid_t. It's currently a 32-bit int on both 32 and
64-bit Linux. This CL got reverted, so we get to try again.

[commit-bot: I haz the power, 2012-03-09 20:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9584024/14001

[commit-bot: I haz the power, 2012-03-09 21:48:35]

Change committed as 125913

[Jeffrey Yasskin, 2012-09-27 23:33:07]

http://codereview.chromium.org/9584024/diff/14001/base/process_util_linux.cc
File base/process_util_linux.cc (right):

http://codereview.chromium.org/9584024/diff/14001/base/process_util_linux.cc#...
base/process_util_linux.cc:114: NOTREACHED();
This NOTREACHED gets hit when a field in proc is too big for an int. My
`unit_tests` run just crashed when proc_stats[VM_VSIZE]=="3073200128", which
isn't a crazy value for it to have.

[Lei Zhang, 2012-09-27 23:43:16]

On 2012/09/27 23:33:07, Jeffrey Yasskin wrote:
> http://codereview.chromium.org/9584024/diff/14001/base/process_util_linux.cc
> File base/process_util_linux.cc (right):
> 
>
http://codereview.chromium.org/9584024/diff/14001/base/process_util_linux.cc#...
> base/process_util_linux.cc:114: NOTREACHED();
> This NOTREACHED gets hit when a field in proc is too big for an int. My
> `unit_tests` run just crashed when proc_stats[VM_VSIZE]=="3073200128", which
> isn't a crazy value for it to have.

I'd be happy to fix it tomorrow if you file a bug today.