Rationalize and fix chromium android linker histogram recording.

Rationalize and fix chromium android linker histogram recording.

Replace histogram recording for devices where the chromium android linker
is enabled with the following:

  ChromiumAndroidLinker.BrowserStates
    Whether relro sharing was attempted for the browser process, and if
    attempted, whether it succeeded. (0=normal load, 1=low memory load
    with shared relro, 2=low memory load used backoff so no shared relro)

  ChromiumAndroidLinker.RendererStates
    Whether relro sharing was attempted for a renderer process, and if
    attempted, whether it succeeded. (0=load with shared relro, 1=load
    used backoff so no shared relro, 2=shared relro not attempted)

If the chromium android linker is not enabled, none of the above
histograms is recorded.

BUG=419010
Committed: https://crrev.com/7ae383a91f3906a9b2459ae92d141d341b1d0c73
Cr-Commit-Position: refs/heads/master@{#299291}

[simonb (inactive), 2014-09-30 16:15:55]

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

[simonb (inactive), 2014-10-01 11:21:45]

simonb@chromium.org changed reviewers:
+ andrewhayden@chromium.org

[rmcilroy, 2014-10-01 12:14:43]

Looks good to me overall.  I'm not sure if RenderViewImpl::Initialize is the
best place to do the histogram recording, but hopefully whoever reviews that
part for OWNERs will know better.  A couple of comments.

https://codereview.chromium.org/611393002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/611393002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:789:
sLoadAtFixedAddressFailed = true;
Does this only happen when there is an errorMessage?  Also, do we use
sLoadAtFixedAddressFailed any more.  It looks like it is only used by
loadAtFixedAddressFailed() in this class now, which you no longer call from
LibraryLoader.java.  Maybe you can just remove this now that you are tracking it
explicitly in LibraryLoader now?

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:27: void
RegisterChromiumAndroidLinkerRendererHistogram(
nit - please move down below LibraryLoaded()

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:38: g_has_renderer_histogram
= false;
Maybe call the boolean g_renderer_histogram_registered_but_not_recorded since
you are also using it to ensure you don't send the UMA more than once.

https://codereview.chromium.org/611393002/diff/1/content/app/android/child_pr...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/611393002/diff/1/content/app/android/child_pr...
content/app/android/child_process_service.cc:162: jboolean use_linker,
nit - used_chromium_linker (and in the Java file)

https://codereview.chromium.org/611393002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/611393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:350:
* @param useLinker True if the android chromium linker loaded this process.
usedChromiumLinker

[simonb (inactive), 2014-10-02 15:15:30]

Thanks for the review.  I've cleaned up and rewritten the change somewhat to
produce clearer and more meaningful statistics.

PTAL when convenient.  Thx.

https://codereview.chromium.org/611393002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/611393002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:789:
sLoadAtFixedAddressFailed = true;
On 2014/10/01 12:14:42, rmcilroy wrote:
> Does this only happen when there is an errorMessage?

Yes.  However, it can be deleted, because of what you note below.


> Also, do we use
> sLoadAtFixedAddressFailed any more.  It looks like it is only used by
> loadAtFixedAddressFailed() in this class now, which you no longer call from
> LibraryLoader.java.  Maybe you can just remove this now that you are tracking
it
> explicitly in LibraryLoader now?

Good observation, thanks.  Removed sLoadAtFixedAddressFailed and
loadAtFixedAddressFailed().

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:27: void
RegisterChromiumAndroidLinkerRendererHistogram(
On 2014/10/01 12:14:42, rmcilroy wrote:
> nit - please move down below LibraryLoaded()

Acknowledged.

https://codereview.chromium.org/611393002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:38: g_has_renderer_histogram
= false;
On 2014/10/01 12:14:42, rmcilroy wrote:
> Maybe call the boolean g_renderer_histogram_registered_but_not_recorded since
> you are also using it to ensure you don't send the UMA more than once.

Acknowledged.  The separate boolean disappeared in a rewrite.

https://codereview.chromium.org/611393002/diff/1/content/app/android/child_pr...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/611393002/diff/1/content/app/android/child_pr...
content/app/android/child_process_service.cc:162: jboolean use_linker,
On 2014/10/01 12:14:42, rmcilroy wrote:
> nit - used_chromium_linker (and in the Java file)

Changes to this file reverted entirely as part of a rewrite.

https://codereview.chromium.org/611393002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/611393002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:350:
* @param useLinker True if the android chromium linker loaded this process.
On 2014/10/01 12:14:42, rmcilroy wrote:
> usedChromiumLinker

Acknowledged.  Removed entirely as part of rewrite.

[rmcilroy, 2014-10-02 15:49:30]

Looks cleaner to me. lgtm, thanks.

[Andrew Hayden (chromium.org), 2014-10-02 15:55:30]

LGTM!

[simonb (inactive), 2014-10-02 17:07:53]

simonb@chromium.org changed reviewers:
+ isherman@chromium.org, jamesr@chromium.org

[simonb (inactive), 2014-10-02 17:07:54]

+jamesr for content/renderer portion
+isherman for tools/metrics portion
Thanks.

[jamesr, 2014-10-02 20:26:48]

https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:829:
base::android::RecordChromiumAndroidLinkerRendererHistogram();
why is this call here? this function is called many times over the life of a
render process

[Ilya Sherman, 2014-10-02 20:53:58]

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
File base/android/library_loader/library_loader_hooks.cc (left):

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
base/android/library_loader/library_loader_hooks.cc:48: is_low_memory_device);
Please mark the corresponding histograms as <obsolete>

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
base/android/library_loader/library_loader_hooks.cc:73: }
Why do you need the pending histogram value, rather than just recording this
histogram when the value is set?

[simonb (inactive), 2014-10-03 11:51:42]

Thanks. Update in patch set 4.

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
File base/android/library_loader/library_loader_hooks.cc (left):

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
base/android/library_loader/library_loader_hooks.cc:48: is_low_memory_device);
On 2014/10/02 20:53:57, Ilya Sherman wrote:
> Please mark the corresponding histograms as <obsolete>

Done.

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
base/android/library_loader/library_loader_hooks.cc:73: }
On 2014/10/02 20:53:57, Ilya Sherman wrote:
> Why do you need the pending histogram value, rather than just recording this
> histogram when the value is set?

The value appears when loading libchrome.so, and this is too early for
histograms to work.  The solution is to save it here as 'pending', and push it
to histograms later and once library code is running.

https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:829:
base::android::RecordChromiumAndroidLinkerRendererHistogram();
On 2014/10/02 20:26:48, jamesr wrote:
> why is this call here? this function is called many times over the life of a
> render process

It needs to run after stats/histogram are started, otherwise the histogram
record is simply dropped.  If you can suggest a better location I'm happy to
move it.  The only requirement is that histograms and IPC should be available at
that point.

[Ilya Sherman, 2014-10-06 21:38:18]

Histograms LGTM, thanks.

[jamesr, 2014-10-06 21:43:17]

On 2014/10/03 11:51:42, simonb wrote:
> Thanks. Update in patch set 4.
> 
>
https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
> File base/android/library_loader/library_loader_hooks.cc (left):
> 
>
https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
> base/android/library_loader/library_loader_hooks.cc:48: is_low_memory_device);
> On 2014/10/02 20:53:57, Ilya Sherman wrote:
> > Please mark the corresponding histograms as <obsolete>
> 
> Done.
> 
>
https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
> File base/android/library_loader/library_loader_hooks.cc (right):
> 
>
https://codereview.chromium.org/611393002/diff/20001/base/android/library_loa...
> base/android/library_loader/library_loader_hooks.cc:73: }
> On 2014/10/02 20:53:57, Ilya Sherman wrote:
> > Why do you need the pending histogram value, rather than just recording this
> > histogram when the value is set?
> 
> The value appears when loading libchrome.so, and this is too early for
> histograms to work.  The solution is to save it here as 'pending', and push it
> to histograms later and once library code is running.
> 
>
https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
> content/renderer/render_view_impl.cc:829:
> base::android::RecordChromiumAndroidLinkerRendererHistogram();
> On 2014/10/02 20:26:48, jamesr wrote:
> > why is this call here? this function is called many times over the life of a
> > render process
> 
> It needs to run after stats/histogram are started, otherwise the histogram
> record is simply dropped.  If you can suggest a better location I'm happy to
> move it.  The only requirement is that histograms and IPC should be available
at
> that point.

This function has nothing to do with initializing histograms or IPC, it has to
do with constructing new RenderView instances (which happens many times over the
life of the process).  I suggest you figure out where histograms are initialized
in the render process and put this instrumentation there.

[simonb (inactive), 2014-10-08 12:07:08]

On 2014/10/06 21:43:17, jamesr wrote:
> ...
>
https://codereview.chromium.org/611393002/diff/20001/content/renderer/render_...
> > content/renderer/render_view_impl.cc:829:
> > base::android::RecordChromiumAndroidLinkerRendererHistogram();
> > On 2014/10/02 20:26:48, jamesr wrote:
> > > why is this call here? this function is called many times over the life of
a
> > > render process
> > 
> > It needs to run after stats/histogram are started, otherwise the histogram
> > record is simply dropped.  If you can suggest a better location I'm happy to
> > move it.  The only requirement is that histograms and IPC should be
available
> at
> > that point.
> 
> This function has nothing to do with initializing histograms or IPC, it has to
> do with constructing new RenderView instances (which happens many times over
the
> life of the process).  I suggest you figure out where histograms are
initialized
> in the render process and put this instrumentation there.

Moved to RendererMain, patch set 5.  Please take another look.  Thanks.

[simonb (inactive), 2014-10-10 08:04:29]

On 2014/10/08 12:07:08, simonb wrote:
> ...
> 
> Moved to RendererMain, patch set 5.  Please take another look.  Thanks.

James, is this okay now?  Thanks.

[jamesr, 2014-10-10 23:03:35]

Thanks, lgtm.

[simonb (inactive), 2014-10-13 10:17:28]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-10-13 10:18:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/611393002/80001

[commit-bot: I haz the power, 2014-10-13 10:26:07]

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

[commit-bot: I haz the power, 2014-10-13 10:26:09]

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

[simonb (inactive), 2014-10-13 10:32:44]

simonb@chromium.org changed reviewers:
+ benm@chromium.org

[simonb (inactive), 2014-10-13 10:32:45]

+benm, for review of:
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java

[benm (inactive), 2014-10-13 10:46:58]

On 2014/10/13 10:32:45, simonb wrote:
> +benm, for review of:
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java

content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
LGTM

[simonb (inactive), 2014-10-13 10:58:00]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-10-13 10:58:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/611393002/80001

[commit-bot: I haz the power, 2014-10-13 13:22:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-13 13:23:38]

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