Add a field platform_private_footprint_ to ProcessMemoryTotals.

Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
Review-Url: https://codereview.chromium.org/2838803003
Cr-Commit-Position: refs/heads/master@{#467177}
Committed: https://chromium.googlesource.com/chromium/src/+/c874a28819750c3da98cbc8abc8ace690bfa9e1e

[erikchen, 2017-04-25 02:43:36]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 02:43:50]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-25 02:44:00]

erikchen@chromium.org changed reviewers:
+ hjd@chromium.org, primiano@chromium.org

[erikchen, 2017-04-25 02:44:01]

primiano: Please review.
hjd: I've added you to a TODO to fill in the field on Linux.

[commit-bot: I haz the power, 2017-04-25 02:53:05]

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

[commit-bot: I haz the power, 2017-04-25 02:53:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hjd, 2017-04-25 11:18:01]

On 2017/04/25 02:44:01, erikchen wrote:
> primiano: Please review.
> hjd: I've added you to a TODO to fill in the field on Linux.

Noted, I will have a stab at it.

[Primiano Tucci (use gerrit), 2017-04-25 12:43:35]

LGTM with minor comments

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

https://codereview.chromium.org/2838803003/diff/1/base/process/process_metric...
base/process/process_metrics.h:173: size_t GetInternalAndCompressed() const;
Not an owner here but I guess an "unit" in the name somehow would be nice, like,
dunno
GetInternalAndCompressedBytes()
GetInternalAndCompressedMemory()

(or maybe could be just an extra optional output argument of GetMemoryBytes
below?)

https://codereview.chromium.org/2838803003/diff/1/base/process/process_metric...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2838803003/diff/1/base/process/process_metric...
base/process/process_metrics_mac.cc:340: size_t GetInternalAndCompressed() const
{
+ProcessMetrics::

https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_totals.h:35: uint64_t
private_footprint_precursor() const {
I'd:
just call this private_footprint_bytes() and Add a todo instead (down below with
the larger comment is fine) to explain the "precursor" thing.
Renaming refactorings are always a cost, let's avoid an extra CL to our future
selves :)

[Wez, 2017-04-25 17:47:19]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2017-04-25 17:47:19]

https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_totals.h:76: uint64_t
private_footprint_precursor_;
As discussed on Slack: "precursor" isn't quite the right term for this, IMO - it
is some platform-specific value representing the private footprint, from which
we will calculate our metric, so I'd suggest a name like:

platform_private_footprint_
platform_private_footprint_metric_

or even just:

platform_private_bytes_

However, it seems strange to have this described as some sort of "raw" metric
specific to the OS, given that it is in fact *not* a raw metric, but something
calculated on an OS-specific basis from the actual raw platform-specific
metrics.

Does the logic that depends on this end up also having to be platform-specific,
since this itself is platform-specific?  If so then why not just store the raw
platform-specific metrics directly, so that there is just one set of logic?

[erikchen, 2017-04-25 17:49:00]

On 2017/04/25 17:47:19, Wez wrote:
>
https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
> File base/trace_event/process_memory_totals.h (right):
> 
>
https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
> base/trace_event/process_memory_totals.h:76: uint64_t
> private_footprint_precursor_;
> As discussed on Slack: "precursor" isn't quite the right term for this, IMO -
it
> is some platform-specific value representing the private footprint, from which
> we will calculate our metric, so I'd suggest a name like:
> 
> platform_private_footprint_
> platform_private_footprint_metric_
> 
> or even just:
> 
> platform_private_bytes_
> 
> However, it seems strange to have this described as some sort of "raw" metric
> specific to the OS, given that it is in fact *not* a raw metric, but something
> calculated on an OS-specific basis from the actual raw platform-specific
> metrics.
> 
> Does the logic that depends on this end up also having to be
platform-specific,
> since this itself is platform-specific?  If so then why not just store the raw
> platform-specific metrics directly, so that there is just one set of logic?

Fadi is going to be the one doing the mojom plumbing, so it mostly depends on
what they want. Mostly we'd end up with a much-larger than necessary struct.
Maybe that's okay.

[fmeawad, 2017-04-25 17:54:45]

On 2017/04/25 17:49:00, erikchen wrote:
> On 2017/04/25 17:47:19, Wez wrote:
> >
>
https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
> > File base/trace_event/process_memory_totals.h (right):
> > 
> >
>
https://codereview.chromium.org/2838803003/diff/1/base/trace_event/process_me...
> > base/trace_event/process_memory_totals.h:76: uint64_t
> > private_footprint_precursor_;
> > As discussed on Slack: "precursor" isn't quite the right term for this, IMO
-
> it
> > is some platform-specific value representing the private footprint, from
which
> > we will calculate our metric, so I'd suggest a name like:
> > 
> > platform_private_footprint_
> > platform_private_footprint_metric_
> > 
> > or even just:
> > 
> > platform_private_bytes_
> > 
> > However, it seems strange to have this described as some sort of "raw"
metric
> > specific to the OS, given that it is in fact *not* a raw metric, but
something
> > calculated on an OS-specific basis from the actual raw platform-specific
> > metrics.
> > 
> > Does the logic that depends on this end up also having to be
> platform-specific,
> > since this itself is platform-specific?  If so then why not just store the
raw
> > platform-specific metrics directly, so that there is just one set of logic?
> 
> Fadi is going to be the one doing the mojom plumbing, so it mostly depends on
> what they want. Mostly we'd end up with a much-larger than necessary struct.
> Maybe that's okay.

We plan to make the struct much bigger, so there is no worry here. Also, we want
to send
as much raw data back to the service. That being said, We have not thought about
platform
specific logic in the service, but that should not be an issue (as long as the
service
clients are always on the same platform).

[Wez, 2017-04-25 18:05:58]

OK; I would suggest either that really-raw values are passed there, with
platform-specific logic in the receiving service, or that this field be
populated with the platform-agnostic value we've derived from the detailed
info - it sounds like the former is the preferred approach, if we want the
service to have access to as much raw data as possible?

On 25 April 2017 at 10:54, <fmeawad@chromium.org> wrote:

> On 2017/04/25 17:49:00, erikchen wrote:
> > On 2017/04/25 17:47:19, Wez wrote:
> > >
> >
> https://codereview.chromium.org/2838803003/diff/1/base/
> trace_event/process_memory_totals.h
> > > File base/trace_event/process_memory_totals.h (right):
> > >
> > >
> >
> https://codereview.chromium.org/2838803003/diff/1/base/
> trace_event/process_memory_totals.h#newcode76
> > > base/trace_event/process_memory_totals.h:76: uint64_t
> > > private_footprint_precursor_;
> > > As discussed on Slack: "precursor" isn't quite the right term for
> this, IMO
> -
> > it
> > > is some platform-specific value representing the private footprint,
> from
> which
> > > we will calculate our metric, so I'd suggest a name like:
> > >
> > > platform_private_footprint_
> > > platform_private_footprint_metric_
> > >
> > > or even just:
> > >
> > > platform_private_bytes_
> > >
> > > However, it seems strange to have this described as some sort of "raw"
> metric
> > > specific to the OS, given that it is in fact *not* a raw metric, but
> something
> > > calculated on an OS-specific basis from the actual raw
> platform-specific
> > > metrics.
> > >
> > > Does the logic that depends on this end up also having to be
> > platform-specific,
> > > since this itself is platform-specific? If so then why not just store
> the
> raw
> > > platform-specific metrics directly, so that there is just one set of
> logic?
> >
> > Fadi is going to be the one doing the mojom plumbing, so it mostly
> depends on
> > what they want. Mostly we'd end up with a much-larger than necessary
> struct.
> > Maybe that's okay.
>
> We plan to make the struct much bigger, so there is no worry here. Also,
> we want
> to send
> as much raw data back to the service. That being said, We have not thought
> about
> platform
> specific logic in the service, but that should not be an issue (as long as
> the
> service
> clients are always on the same platform).
>
> https://codereview.chromium.org/2838803003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2017-04-25 18:20:00]

On 2017/04/25 18:05:58, Wez wrote:
> OK; I would suggest either that really-raw values are passed there, with
> platform-specific logic in the receiving service, 

> or that this field be populated with the platform-agnostic value we've derived
from the detailed info
This would be ideal, but my understanding is that is just not technically
possible. My rule of thumb in this case is:
1. Making things technically possible wins over ideal things :)
2. When bending the idealistic rules for 1. to the simplest thing that achieves
the goal and keeps the code readable.

> - it sounds like the former is the preferred approach, if we want the
> service to have access to as much raw data as possible?
> 
> On 25 April 2017 at 10:54, <mailto:fmeawad@chromium.org> wrote:
> 
> > On 2017/04/25 17:49:00, erikchen wrote:
> > > On 2017/04/25 17:47:19, Wez wrote:
> > > >
> > >
> > https://codereview.chromium.org/2838803003/diff/1/base/
> > trace_event/process_memory_totals.h
> > > > File base/trace_event/process_memory_totals.h (right):
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2838803003/diff/1/base/
> > trace_event/process_memory_totals.h#newcode76
> > > > base/trace_event/process_memory_totals.h:76: uint64_t
> > > > private_footprint_precursor_;
> > > > As discussed on Slack: "precursor" isn't quite the right term for
> > this, IMO
> > -
> > > it
> > > > is some platform-specific value representing the private footprint,
> > from
> > which
> > > > we will calculate our metric, so I'd suggest a name like:
> > > >
> > > > platform_private_footprint_
> > > > platform_private_footprint_metric_
> > > >
> > > > or even just:
> > > >
> > > > platform_private_bytes_
> > > >
> > > > However, it seems strange to have this described as some sort of "raw"
> > metric
> > > > specific to the OS, given that it is in fact *not* a raw metric, but
> > something
> > > > calculated on an OS-specific basis from the actual raw
> > platform-specific
> > > > metrics.
> > > >
> > > > Does the logic that depends on this end up also having to be
> > > platform-specific,
> > > > since this itself is platform-specific? If so then why not just store
> > the
> > raw
> > > > platform-specific metrics directly, so that there is just one set of
> > logic?
> > >
> > > Fadi is going to be the one doing the mojom plumbing, so it mostly
> > depends on
> > > what they want. Mostly we'd end up with a much-larger than necessary
> > struct.
> > > Maybe that's okay.
> >
> > We plan to make the struct much bigger, so there is no worry here. Also,
> > we want
> > to send
> > as much raw data back to the service. That being said, We have not thought
> > about
> > platform
> > specific logic in the service, but that should not be an issue (as long as
> > the
> > service
> > clients are always on the same platform).
> >
> > https://codereview.chromium.org/2838803003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2017-04-25 19:34:33]

Thanks for all the feedback.

I've decided that it's best for all logic to live in the Memory-Infra service,
so I'm going raw data through ProcessMemoryTotals, instead of munging into a
platform-dependent "precursor".

[erikchen, 2017-04-25 19:34:41]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 19:35:20]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-25 19:40:30]

Description was changed from

==========
Add a field private_footprint_precursor_ to ProcessMemoryTotals.

The field represents a platform specific number that will be used by the
Memory-Infra service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
==========

to

==========
Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
==========

[Primiano Tucci (use gerrit), 2017-04-25 19:52:56]

Base/Trace_event still lgtm

[erikchen, 2017-04-25 20:15:24]

Description was changed from

==========
Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
==========

to

==========
Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
==========

[erikchen, 2017-04-25 20:23:43]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2017-04-25 20:24:13]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2017-04-25 20:24:14]

mark: Please review.

[commit-bot: I haz the power, 2017-04-25 20:24:33]

Dry run: CQ is trying da patch.

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

[Mark Mentovai, 2017-04-25 20:29:44]

LGTM

https://codereview.chromium.org/2838803003/diff/60001/base/process/process_me...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2838803003/diff/60001/base/process/process_me...
base/process/process_metrics.h:173: uint64_t phys_footprint = 0;
Do these measure bytes or pages? Say so somewhere, or name the fields
whatever_bytes.

https://codereview.chromium.org/2838803003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/2838803003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_totals.h:39: uint64_t phys_footprint_bytes = 0;
Like you did here. :)

https://codereview.chromium.org/2838803003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_totals.h:47: uint64_t rss_anon_bytes = 0;
Should we only define the proper OS-specific fields on each OS?

[erikchen, 2017-04-25 20:49:46]

https://codereview.chromium.org/2838803003/diff/60001/base/process/process_me...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2838803003/diff/60001/base/process/process_me...
base/process/process_metrics.h:173: uint64_t phys_footprint = 0;
On 2017/04/25 20:29:44, Mark Mentovai wrote:
> Do these measure bytes or pages? Say so somewhere, or name the fields
> whatever_bytes.

Added a comment.

https://codereview.chromium.org/2838803003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/2838803003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_totals.h:47: uint64_t rss_anon_bytes = 0;
On 2017/04/25 20:29:44, Mark Mentovai wrote:
> Should we only define the proper OS-specific fields on each OS?

I considered if-def-souping this, but these parameters are going to be passed
over IPC, and I think it's cleaner to just have them all defined.

[erikchen, 2017-04-25 20:49:50]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-25 20:49:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2838803003/#ps80001
(title: "Comments from mark.")

[commit-bot: I haz the power, 2017-04-25 20:50:22]

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-25 20:58:47]

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

[commit-bot: I haz the power, 2017-04-25 20:58:48]

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

[erikchen, 2017-04-25 22:08:50]

erikchen@chromium.org changed reviewers:
+ sky@chromium.org

[erikchen, 2017-04-25 22:08:51]

sky: Please review chrome/

[sky, 2017-04-25 22:56:14]

LGTM

[sky, 2017-04-25 22:56:23]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-04-25 22:56:57]

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-26 00:21:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493160983422330,
"parent_rev": "554dfdf5e23b14a6feb4da9bbd9ec8b7fe1853ae", "commit_rev":
"c874a28819750c3da98cbc8abc8ace690bfa9e1e"}

[commit-bot: I haz the power, 2017-04-26 00:22:53]

Description was changed from

==========
Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021
==========

to

==========
Add a field platform_private_footprint_ to ProcessMemoryTotals.

The field holds platform-specific data that will be used by the Memory-Infra
service to compute the private memory footprint.

This CL populates the field on macOS, and leaves TODOs to populate the field for
other OSes.

BUG=707021

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

[commit-bot: I haz the power, 2017-04-26 00:22:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c874a28819750c3da98cbc8abc8a...