Clean up some Mac memory code.

Clean up some Mac memory code.

Removes some forward declarations that were needed for <=10.7 and
updates a comment about how the code calculates memory on Mac that
was previously a TODO.

BUG=none

Review-Url: https://codereview.chromium.org/2782083003
Cr-Commit-Position: refs/heads/master@{#461939}
Committed: https://chromium.googlesource.com/chromium/src/+/d84dc8d364680c7b6d60c37747d4362d0c049ed5

[Alexei Svitkine (slow), 2017-03-29 20:12:25]

asvitkine@chromium.org changed reviewers:
+ erikchen@chromium.org, thakis@chromium.org

[Alexei Svitkine (slow), 2017-03-29 20:12:26]

thakis: PTAL
erikchen: FYI since you're working on Mac memory nowadays

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metrics.h
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metric...
base/process/process_metrics.h:67: // priv:           Resident size (RSS)
including shared memory.
Note: based on what the GetCommittedAndWorkingSetKBytes() code.

[erikchen, 2017-03-29 20:25:14]

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metrics.h
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metric...
base/process/process_metrics.h:67: // priv:           Resident size (RSS)
including shared memory.
On 2017/03/29 20:12:26, Alexei Svitkine (slow) wrote:
> Note: based on what the GetCommittedAndWorkingSetKBytes() code.

GetCommittedAndWorkingSetKBytes returns pmap->stats.resident_count
[see osfmk/i386/pmap.h and osfmk/kern/task.c].

This provides an underestimate of RSS [by any reasonable definition]. See
Appendix C under
https://docs.google.com/document/d/1zcfWSsgwTY8WYYlxd0G8QTR6_b2j38uMfjjXBOuMZ...,
look at section titled  "macOS: Use task_info(TASK_VM_INFO, …)".

I've intentionally avoided touching this comment because I want to overhaul this
entire file once my design doc has been fully reviewed.

[Nico, 2017-03-29 20:31:23]

cc file lgtm

[Alexei Svitkine (slow), 2017-03-29 21:02:39]

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metrics.h
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metric...
base/process/process_metrics.h:67: // priv:           Resident size (RSS)
including shared memory.
On 2017/03/29 20:25:14, erikchen wrote:
> On 2017/03/29 20:12:26, Alexei Svitkine (slow) wrote:
> > Note: based on what the GetCommittedAndWorkingSetKBytes() code.
> 
> GetCommittedAndWorkingSetKBytes returns pmap->stats.resident_count
> [see osfmk/i386/pmap.h and osfmk/kern/task.c].
> 
> This provides an underestimate of RSS [by any reasonable definition]. See
> Appendix C under
>
https://docs.google.com/document/d/1zcfWSsgwTY8WYYlxd0G8QTR6_b2j38uMfjjXBOuMZ...,
> look at section titled  "macOS: Use task_info(TASK_VM_INFO, …)".
> 
> I've intentionally avoided touching this comment because I want to overhaul
this
> entire file once my design doc has been fully reviewed.

I see - should I just update the TODO to you and reference a crbug then?

Reason I wanted to provide more info is because in the meantime, this is being
used for Memory.Total2 and other metrics and having more info about what that's
counting rather than less is better in the meantime. I could expand my change
here and add a warning about it - similar (but shorter) to what you have in the
doc. WDYT?

[erikchen, 2017-03-29 21:10:39]

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metrics.h
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metric...
base/process/process_metrics.h:67: // priv:           Resident size (RSS)
including shared memory.
On 2017/03/29 21:02:39, Alexei Svitkine (slow) wrote:
> On 2017/03/29 20:25:14, erikchen wrote:
> > On 2017/03/29 20:12:26, Alexei Svitkine (slow) wrote:
> > > Note: based on what the GetCommittedAndWorkingSetKBytes() code.
> > 
> > GetCommittedAndWorkingSetKBytes returns pmap->stats.resident_count
> > [see osfmk/i386/pmap.h and osfmk/kern/task.c].
> > 
> > This provides an underestimate of RSS [by any reasonable definition]. See
> > Appendix C under
> >
>
https://docs.google.com/document/d/1zcfWSsgwTY8WYYlxd0G8QTR6_b2j38uMfjjXBOuMZ...,
> > look at section titled  "macOS: Use task_info(TASK_VM_INFO, …)".
> > 
> > I've intentionally avoided touching this comment because I want to overhaul
> this
> > entire file once my design doc has been fully reviewed.
> 
> I see - should I just update the TODO to you and reference a crbug then?
> 
> Reason I wanted to provide more info is because in the meantime, this is being
> used for Memory.Total2 and other metrics and having more info about what
that's
> counting rather than less is better in the meantime. I could expand my change
> here and add a warning about it - similar (but shorter) to what you have in
the
> doc. WDYT?

You're welcome to add a warning and a more accurate description.

[Alexei Svitkine (slow), 2017-03-29 21:16:54]

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metrics.h
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2782083003/diff/1/base/process/process_metric...
base/process/process_metrics.h:67: // priv:           Resident size (RSS)
including shared memory.
On 2017/03/29 21:10:38, erikchen wrote:
> On 2017/03/29 21:02:39, Alexei Svitkine (slow) wrote:
> > On 2017/03/29 20:25:14, erikchen wrote:
> > > On 2017/03/29 20:12:26, Alexei Svitkine (slow) wrote:
> > > > Note: based on what the GetCommittedAndWorkingSetKBytes() code.
> > > 
> > > GetCommittedAndWorkingSetKBytes returns pmap->stats.resident_count
> > > [see osfmk/i386/pmap.h and osfmk/kern/task.c].
> > > 
> > > This provides an underestimate of RSS [by any reasonable definition]. See
> > > Appendix C under
> > >
> >
>
https://docs.google.com/document/d/1zcfWSsgwTY8WYYlxd0G8QTR6_b2j38uMfjjXBOuMZ...,
> > > look at section titled  "macOS: Use task_info(TASK_VM_INFO, …)".
> > > 
> > > I've intentionally avoided touching this comment because I want to
overhaul
> > this
> > > entire file once my design doc has been fully reviewed.
> > 
> > I see - should I just update the TODO to you and reference a crbug then?
> > 
> > Reason I wanted to provide more info is because in the meantime, this is
being
> > used for Memory.Total2 and other metrics and having more info about what
> that's
> > counting rather than less is better in the meantime. I could expand my
change
> > here and add a warning about it - similar (but shorter) to what you have in
> the
> > doc. WDYT?
> 
> You're welcome to add a warning and a more accurate description.

Done. PTAL.

[erikchen, 2017-04-04 22:28:44]

lgtm

[Alexei Svitkine (slow), 2017-04-04 22:29:04]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-04-04 22:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2782083003/#ps20001
(title: "Update comment.")

[commit-bot: I haz the power, 2017-04-04 22:29:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 23:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 23:47:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexei Svitkine (slow), 2017-04-04 23:51:43]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-04-04 23:52:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 02:12:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491349903133060,
"parent_rev": "cdd55622ffec06c7513b366777fe53566aabde4f", "commit_rev":
"d84dc8d364680c7b6d60c37747d4362d0c049ed5"}

[commit-bot: I haz the power, 2017-04-05 02:13:19]

Description was changed from

==========
Clean up some Mac memory code.

Removes some forward declarations that were needed for <=10.7 and
updates a comment about how the code calculates memory on Mac that
was previously a TODO.

BUG=none
==========

to

==========
Clean up some Mac memory code.

Removes some forward declarations that were needed for <=10.7 and
updates a comment about how the code calculates memory on Mac that
was previously a TODO.

BUG=none

Review-Url: https://codereview.chromium.org/2782083003
Cr-Commit-Position: refs/heads/master@{#461939}
Committed:
https://chromium.googlesource.com/chromium/src/+/d84dc8d364680c7b6d60c37747d4...
==========

[commit-bot: I haz the power, 2017-04-05 02:13:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d84dc8d364680c7b6d60c37747d4...