Linux: Stop using /proc/pid/smaps to calculate memory usage. It's too slow.

Linux: Stop using /proc/pid/smaps to calculate memory usage. It's too slow.

BUG=62854
TEST=See bug.

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

[willchan no longer on Chromium, 2012-03-02 00:05:38]

Sorry, NACK'ing reviews for a bit. erg@, can you cover for me? I can
rubberstamp.

[Lei Zhang, 2012-03-02 00:07:40]

On 2012/03/02 00:05:38, willchan wrote:
> Sorry, NACK'ing reviews for a bit. erg@, can you cover for me? I can
> rubberstamp.

No rush, though please hold off on this for a bit. I need to make sure the
TaskManager is using this data correctly. There may be some inconsistencies...

[Elliot Glaysher, 2012-03-02 00:15:38]

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:337: // Private and Shared working set sizes are
obtained from /proc/<pid>/smaps.
If you're pulling out the whole smaps implementation, you want to cut most of
this comment.

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:369: base::StringToInt(statm_vec[2], &statm_shared);
What's the accuracy implications for going to statm instead of parsing the smaps
file?

[Elliot Glaysher, 2012-03-02 00:15:38]

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:337: // Private and Shared working set sizes are
obtained from /proc/<pid>/smaps.
If you're pulling out the whole smaps implementation, you want to cut most of
this comment.

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:369: base::StringToInt(statm_vec[2], &statm_shared);
What's the accuracy implications for going to statm instead of parsing the smaps
file?

[Elliot Glaysher, 2012-03-02 00:16:24]

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:38: return
FilePath(kProcDir).Append(base::IntToString(pid));
You're replacing both IntToString() and Int64ToString() calls with this
function. What's the sizeof(pid_t)?

[Lei Zhang, 2012-03-02 01:39:02]

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
File base/process_util_linux.cc (right):

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:38: return
FilePath(kProcDir).Append(base::IntToString(pid));
On 2012/03/02 00:16:24, Elliot Glaysher wrote:
> You're replacing both IntToString() and Int64ToString() calls with this
> function. What's the sizeof(pid_t)?

I think pids are generally 32-bit on Linux, but there might be an option
somewhere to make it 64-bit for systems with many processes. We should just
Int64ToString() here.

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:337: // Private and Shared working set sizes are
obtained from /proc/<pid>/smaps.
On 2012/03/02 00:15:38, Elliot Glaysher wrote:
> If you're pulling out the whole smaps implementation, you want to cut most of
> this comment.

Indeed, thanks for catching that.

https://chromiumcodereview.appspot.com/9568046/diff/5001/base/process_util_li...
base/process_util_linux.cc:369: base::StringToInt(statm_vec[2], &statm_shared);
On 2012/03/02 00:15:38, Elliot Glaysher wrote:
> What's the accuracy implications for going to statm instead of parsing the
smaps
> file?

kgr measured this and put his findings on the bug. In practice, "top" uses these
values too. Otherwise top would be chewing through CPU as we are doing now.
(Note, top may be reading the same values from status rather than statm, but
statm is much easier to parse.)

[Lei Zhang, 2012-03-02 01:39:48]

Oh ya, I think this is correct, but the way these values are used in the task
manager is a bit wonky. Though that's another issue.

[Elliot Glaysher, 2012-03-02 17:27:37]

lgtm

[willchan no longer on Chromium, 2012-03-02 17:38:23]

LGTM rubberstamp, thanks Elliot.

On Fri, Mar 2, 2012 at 9:27 AM, <erg@chromium.org> wrote:

> lgtm
>
>
>
>
https://chromiumcodereview.**appspot.com/9568046/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-03-02 17:43:29]

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

[commit-bot: I haz the power, 2012-03-02 20:55:03]

Change committed as 124727