android: Record the memory size of the GSA process.

android: Record the memory size of the GSA process.

Chrome connects to a bound service exposed by GSA, elevating its
importance to the system (see the linked bug). This requirement is going
away, and this CL is assessing the amount of memory wasted / saved this
way.

On a Nexus 5X, the cost of gathering this data has been
measured (through Trace events) to be ~60ms of wall clock time (on a
low-priority background thread), with ~2ms of CPU time on a little
core. The data is gathered at most once per startup.

BUG=614388
Committed: https://crrev.com/0d37cc58e86c456e635fc75681768303b13ced8e
Cr-Commit-Position: refs/heads/master@{#430288}

[Benoit L, 2016-11-04 14:17:36]

lizeb@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2016-11-04 16:47:56]

lgtm w/ some nits

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java
(right):

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:73:
if (msg.what == RESPONSE_UPDATE_SSB) {
as a small nit, I would change this to:

if (msg.what != RESPONSE_UPDATE_SSB) {
    super.handleMessage(msg);
    return;
}

at this point and then reduce the indent of the major block below.

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:135:
if (componentName == null) return -1;
I would probably have a constant like INVALID_PSS = -1 and use that for slight
better clarify.

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:151:
Debug.MemoryInfo infos[] = activityManager.getProcessMemoryInfo(pids);
you could inline pids as "new int[] {pid}" if you want to avoid the variable

[Benoit L, 2016-11-04 17:20:58]

lizeb@chromium.org changed reviewers:
+ asvitkine@chromium.org, yfriedman@chromium.org

[Benoit L, 2016-11-04 17:20:58]

Thanks!

+yfriedman for base/andtoid
+asvitkine for histograms.xml

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java
(right):

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:73:
if (msg.what == RESPONSE_UPDATE_SSB) {
On 2016/11/04 16:47:56, Ted C wrote:
> as a small nit, I would change this to:
> 
> if (msg.what != RESPONSE_UPDATE_SSB) {
>     super.handleMessage(msg);
>     return;
> }
> 
> at this point and then reduce the indent of the major block below.

Done.

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:135:
if (componentName == null) return -1;
On 2016/11/04 16:47:56, Ted C wrote:
> I would probably have a constant like INVALID_PSS = -1 and use that for slight
> better clarify.

Done.

https://codereview.chromium.org/2477513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java:151:
Debug.MemoryInfo infos[] = activityManager.getProcessMemoryInfo(pids);
On 2016/11/04 16:47:56, Ted C wrote:
> you could inline pids as "new int[] {pid}" if you want to avoid the variable

Done.

[Benoit L, 2016-11-04 17:21:10]

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

[commit-bot: I haz the power, 2016-11-04 17:21:29]

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

[Alexei Svitkine (slow), 2016-11-04 17:25:03]

lgtm % optional suggestion

https://codereview.chromium.org/2477513004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2477513004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:56350: +<histogram
name="Search.GsaProcessMemoryPss" units="KB">
I feel like it may be better to nest this under Android.* rather than Search.* -
but I'll defer to you to choose.

[commit-bot: I haz the power, 2016-11-04 19:15:35]

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

[commit-bot: I haz the power, 2016-11-04 19:15:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Benoit L, 2016-11-07 10:41:37]

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

[commit-bot: I haz the power, 2016-11-07 10:41:49]

Dry run: 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, 2016-11-07 12:04:49]

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

[commit-bot: I haz the power, 2016-11-07 12:04:49]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-11-07 13:43:10]

lizeb@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Benoit L, 2016-11-07 13:43:11]

+rmclilroy for 
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (Yaron is
OOO)

PTAL, thanks.

https://codereview.chromium.org/2477513004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2477513004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:56350: +<histogram
name="Search.GsaProcessMemoryPss" units="KB">
On 2016/11/04 17:25:03, Alexei Svitkine (very slow) wrote:
> I feel like it may be better to nest this under Android.* rather than Search.*
-
> but I'll defer to you to choose.

This is very closely related to Search.GsaBroadcastsAccountChanges, so I prefer
to keep the naming this way, for more consistency.

[rmcilroy, 2016-11-07 14:59:34]

base/ LGTM with two nits.

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java
(right):

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:209: * Good
for sizes up to 500e6 bytes.
nit - could you put this in MB like histogram_macros.h - "Good for sizes up to
about 500MB).

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:216: }
nit - could we move this below recordCustomTimesHistogram (to keep time
histograms grouped like in histogram_macros.h)

[Benoit L, 2016-11-07 15:15:25]

Thanks!

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java
(right):

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:209: * Good
for sizes up to 500e6 bytes.
On 2016/11/07 14:59:34, rmcilroy wrote:
> nit - could you put this in MB like histogram_macros.h - "Good for sizes up to
> about 500MB).

Done.

https://codereview.chromium.org/2477513004/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:216: }
On 2016/11/07 14:59:33, rmcilroy wrote:
> nit - could we move this below recordCustomTimesHistogram (to keep time
> histograms grouped like in histogram_macros.h)

Done.

[Benoit L, 2016-11-07 15:15:31]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-11-07 15:15:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, asvitkine@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2477513004/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-07 15:15:50]

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, 2016-11-07 16:27:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-07 16:42:49]

Description was changed from

==========
android: Record the memory size of the GSA process.

Chrome connects to a bound service exposed by GSA, elevating its
importance to the system (see the linked bug). This requirement is going
away, and this CL is assessing the amount of memory wasted / saved this
way.

On a Nexus 5X, the cost of gathering this data has been
measured (through Trace events) to be ~60ms of wall clock time (on a
low-priority background thread), with ~2ms of CPU time on a little
core. The data is gathered at most once per startup.

BUG=614388
==========

to

==========
android: Record the memory size of the GSA process.

Chrome connects to a bound service exposed by GSA, elevating its
importance to the system (see the linked bug). This requirement is going
away, and this CL is assessing the amount of memory wasted / saved this
way.

On a Nexus 5X, the cost of gathering this data has been
measured (through Trace events) to be ~60ms of wall clock time (on a
low-priority background thread), with ~2ms of CPU time on a little
core. The data is gathered at most once per startup.

BUG=614388

Committed: https://crrev.com/0d37cc58e86c456e635fc75681768303b13ced8e
Cr-Commit-Position: refs/heads/master@{#430288}
==========

[commit-bot: I haz the power, 2016-11-07 16:42:51]

Patchset 7 (id:??) landed as
https://crrev.com/0d37cc58e86c456e635fc75681768303b13ced8e
Cr-Commit-Position: refs/heads/master@{#430288}