Add UMA for testing whether the Chromium library was page aligned in the APK file.

Add UMA for testing whether the Chromium library was page aligned in the APK file.

An extra value "Not aligned" was added to the ChromiumAndroidLinker.LibraryLoadFromApkStatus histogram.

BUG=

Committed: https://crrev.com/2a112d50b0a28f82a519e194230b7de91ee21023
Cr-Commit-Position: refs/heads/master@{#301813}

[petrcermak, 2014-10-27 18:19:13]

petrcermak@chromium.org changed reviewers:
+ alexclarke@chromium.org, anton@chromium.org, isherman@chromium.org,
picksi@chromium.org, rmcilroy@chromium.org, simonb@chromium.org,
skyostil@google.com

[petrcermak, 2014-10-27 18:19:14]

Hi, please review this CL.

isherman: tools/metrics/histograms
rmcilroy: base/android
everyone else: FYI

Thanks,
Petr

[petrcermak, 2014-10-27 18:20:25]

On 2014/10/27 18:19:14, petrcermak wrote:
> Hi, please review this CL.
> 
> isherman: tools/metrics/histograms
> rmcilroy: base/android
> everyone else: FYI
> 
> Thanks,
> Petr

rmcilroy: base/android, third_party/android_crazy_linker

[Ilya Sherman, 2014-10-28 00:21:54]

https://codereview.chromium.org/684453003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/684453003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:2710: +<histogram
name="ChromiumAndroidLinker.LibraryAlignedInApk" enum="Boolean">
Could you please define a BooleanAligned enumeration, and use that?

[Anton, 2014-10-28 09:21:00]

lgtm

[petrcermak, 2014-10-28 10:16:02]

https://codereview.chromium.org/684453003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/684453003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:2710: +<histogram
name="ChromiumAndroidLinker.LibraryAlignedInApk" enum="Boolean">
On 2014/10/28 00:21:54, Ilya Sherman wrote:
> Could you please define a BooleanAligned enumeration, and use that?

Done.

[rmcilroy, 2014-10-28 10:28:45]

As discussed offline, lets do the check early (where it would be done for the
fallback) and augment the current histogram with a new status (e.g.,
LIBRARY_NOT_ALIGNED) rather than adding a new UMA metric.

[petrcermak, 2014-10-28 11:35:58]

I merged the new histogram (ChromiumAndroidLinker.LibraryAlignedInApk) into
ChromiumAndroidLinker.LibraryLoadFromApkStatus by adding an extra value "Not
aligned". PTAL.

Thanks,
Petr

[picksi1, 2014-10-28 11:37:09]

some nits.

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:318:
nativeRecordChromiumAndroidLinkerLibraryAlignmentInApk(
This reads like you're checking if the linker is in a zip file (?) if this is
your function should it be called isAPKZipped() ... or have I misunderstood its
use?

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:351:
Log.w(TAG, "NativeLibraryes.LIBRARIES should contain exactly 2 libraries");
Is there a const or enum etc. we can use to avoid using a magic number 2 here?

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:435: }
Instead of returning -1 can we return a constant (e.g.) UNLOADABLE and also
check for this at the call site(s). You should probably also assert  that the
'correct' result (i.e. offset) doesn't equal UNLOADABLE, in case someone fiddles
with this code later on.

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:442: if
((offset & (kPageSize - 1)) != 0) {
nit: This only works because kPageSize is a power of 2, is it worth adding a
build time assert (if such a thing exists in this code base) to confirm this is
the case, e.g. ASSERT( (kPageSize & (kPageSize-1)) == 0 )?

[petrcermak, 2014-10-28 12:16:48]

Thanks for your comments.

Petr

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:318:
nativeRecordChromiumAndroidLinkerLibraryAlignmentInApk(
On 2014/10/28 11:37:08, picksi1 wrote:
> This reads like you're checking if the linker is in a zip file (?) if this is
> your function should it be called isAPKZipped() ... or have I misunderstood
its
> use?

I don't like the name either. It returns true iff the Chromium library should be
loaded directly from the APK. I didn't change the name because the whole class
will need a complete refactoring anyway.

https://codereview.chromium.org/684453003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:351:
Log.w(TAG, "NativeLibraryes.LIBRARIES should contain exactly 2 libraries");
On 2014/10/28 11:37:08, picksi1 wrote:
> Is there a const or enum etc. we can use to avoid using a magic number 2 here?

I removed this code. The point was that there should be only the Chromium linker
library (which is *not* loaded directly from the APK) and Chromium library
(which *is* loaded directly from the APK).

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:435: }
On 2014/10/28 11:37:09, picksi1 wrote:
> Instead of returning -1 can we return a constant (e.g.) UNLOADABLE and also
> check for this at the call site(s). You should probably also assert  that the
> 'correct' result (i.e. offset) doesn't equal UNLOADABLE, in case someone
fiddles
> with this code later on.

Done.

https://codereview.chromium.org/684453003/diff/20001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:442: if
((offset & (kPageSize - 1)) != 0) {
On 2014/10/28 11:37:08, picksi1 wrote:
> nit: This only works because kPageSize is a power of 2, is it worth adding a
> build time assert (if such a thing exists in this code base) to confirm this
is
> the case, e.g. ASSERT( (kPageSize & (kPageSize-1)) == 0 )?

Done (using C++11 static_assert).

[picksi1, 2014-10-28 12:38:33]

I admire your dedication to replacing -1s with CRAZY_OFFSET_FAILED! I think you
missed on though... :(

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right):

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:380: return
CRAZY_STATUS_FAILURE;
Can you use CRAZY_OFFSET_FAILED here too? Or is it out of scope?

[rmcilroy, 2014-10-28 13:07:15]

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:58: //
file.
if Linker.isInZipFile.

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:176:
if (Linker.DEBUG) Log.i(TAG, "ignoring self-linker load");
Don't use Linker.DEBUG here - have a seperate one for LibraryLoader if you need
it.

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:821: public
static boolean isLinkerLibrary(String library) {
nit - isChromiumLinkerLibrary

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:417:
const size_t kPageSize = 4096;
nit - please put these back at the top of the file, and have
LibraryList::FindAlignedLibraryInZipFile above 
LibraryList::LoadLibraryInZipFile

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:419:
"kPageSize must be a power of 2");
Please move this to FindAlignedLibraryInZipFile so it is near the code which has
this requirement.

[petrcermak, 2014-10-28 13:48:16]

Thanks for your comments. PTAL.

Petr

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:58: //
file.
On 2014/10/28 13:07:15, rmcilroy wrote:
> if Linker.isInZipFile. 

Done.

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:176:
if (Linker.DEBUG) Log.i(TAG, "ignoring self-linker load");
On 2014/10/28 13:07:15, rmcilroy wrote:
> Don't use Linker.DEBUG here - have a seperate one for LibraryLoader if you
need
> it.

Done (added a separate one).

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/684453003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:821: public
static boolean isLinkerLibrary(String library) {
On 2014/10/28 13:07:15, rmcilroy wrote:
> nit - isChromiumLinkerLibrary

Done.

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right):

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:380: return
CRAZY_STATUS_FAILURE;
On 2014/10/28 12:38:33, picksi1 wrote:
> Can you use CRAZY_OFFSET_FAILED here too? Or is it out of scope?

Done (needed to move CRAZY_OFFSET_FAILED to crazy_linker_util.h).

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:417:
const size_t kPageSize = 4096;
On 2014/10/28 13:07:15, rmcilroy wrote:
> nit - please put these back at the top of the file, and have
> LibraryList::FindAlignedLibraryInZipFile above 
> LibraryList::LoadLibraryInZipFile

Done (I put the constants in the anonymous namespace at the top).

https://codereview.chromium.org/684453003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:419:
"kPageSize must be a power of 2");
On 2014/10/28 13:07:15, rmcilroy wrote:
> Please move this to FindAlignedLibraryInZipFile so it is near the code which
has
> this requirement.

Done.

[picksi1, 2014-10-28 14:07:51]

lgtm :)

[rmcilroy, 2014-10-28 15:55:13]

lgtm with one last comment, thanks!

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:414: if
(error) {
Probably better just to pass error in all cases and drop these checks.  It is
probably a good idea to Log this error in any case just in case the error is not
alignment.

[simonb (inactive), 2014-10-28 16:48:43]

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27: const
size_t kPageSize = 4096;
Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page size not
necessarily the same as the platforms?)

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:414: if
(error) {
On 2014/10/28 15:55:13, rmcilroy wrote:
> Probably better just to pass error in all cases and drop these checks.  It is
> probably a good idea to Log this error in any case just in case the error is
not
> alignment.

+1.  error ought never be null.

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:427:
"kPageSize must be a power of 2");
Can be eliminated if using PAGE_SIZE?

[Anton, 2014-10-28 16:56:27]

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27: const
size_t kPageSize = 4096;
On 2014/10/28 16:48:43, simonb wrote:
> Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page size
not
> necessarily the same as the platforms?)

I prefer to leave it like this. It really has to be 4096 as that is the
alignment we bake into the file.
If we ever want a different value on some platform we will have to bake in the
lowest common multiple on all platforms which share the APK.

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

On 2014/10/28 16:56:27, Anton wrote:
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
> (right):
> 
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27:
const
> size_t kPageSize = 4096;
> On 2014/10/28 16:48:43, simonb wrote:
> > Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page size
> not
> > necessarily the same as the platforms?)
> 
> I prefer to leave it like this. It really has to be 4096 as that is the
> alignment we bake into the file.
> If we ever want a different value on some platform we will have to bake in the
> lowest common multiple on all platforms which share the APK.

In which case, how about a) renaming it to kZipAlignmentPageSize or similar, and
b) checking that it is some multiple of PAGE_SIZE?

[Anton, 2014-10-28 17:26:32]

On 2014/10/28 17:07:09, simonb wrote:
> On 2014/10/28 16:56:27, Anton wrote:
> >
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> > File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> > third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27:
> const
> > size_t kPageSize = 4096;
> > On 2014/10/28 16:48:43, simonb wrote:
> > > Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page
size
> > not
> > > necessarily the same as the platforms?)
> > 
> > I prefer to leave it like this. It really has to be 4096 as that is the
> > alignment we bake into the file.
> > If we ever want a different value on some platform we will have to bake in
the
> > lowest common multiple on all platforms which share the APK.
> 
> In which case, how about a) renaming it to kZipAlignmentPageSize or similar,
and
> b) checking that it is some multiple of PAGE_SIZE?

Sounds reasonable to me. Petr?

[petrcermak, 2014-10-28 19:26:37]

Patchset #6 (id:100001) has been deleted

[petrcermak, 2014-10-28 19:28:31]

Thanks for your comments.

Petr

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
(right):

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27: const
size_t kPageSize = 4096;
On 2014/10/28 16:56:26, Anton wrote:
> On 2014/10/28 16:48:43, simonb wrote:
> > Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page size
> not
> > necessarily the same as the platforms?)
> 
> I prefer to leave it like this. It really has to be 4096 as that is the
> alignment we bake into the file.
> If we ever want a different value on some platform we will have to bake in the
> lowest common multiple on all platforms which share the APK.

I renamed it to kZipAlignmentPageSize and added an extra static_assert as simonb
sugested.

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:414: if
(error) {
On 2014/10/28 16:48:43, simonb wrote:
> On 2014/10/28 15:55:13, rmcilroy wrote:
> > Probably better just to pass error in all cases and drop these checks.  It
is
> > probably a good idea to Log this error in any case just in case the error is
> not
> > alignment.
> 
> +1.  error ought never be null.

Done (I added a local "error" variable to
crazy_linker_check_library_aligned_in_zip_file in crazy_linker_api.cpp).

https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:427:
"kPageSize must be a power of 2");
On 2014/10/28 16:48:43, simonb wrote:
> Can be eliminated if using PAGE_SIZE?

It is very unlikely that any architecture will ever use a page size that is not
a power of 2. However, I don't see any harm in including this check (especially
since it's static).

[petrcermak, 2014-10-28 19:29:14]

On 2014/10/28 17:26:32, Anton wrote:
> On 2014/10/28 17:07:09, simonb wrote:
> > On 2014/10/28 16:56:27, Anton wrote:
> > >
> >
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> > > File
third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/684453003/diff/80001/third_party/android_craz...
> > > third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp:27:
> > const
> > > size_t kPageSize = 4096;
> > > On 2014/10/28 16:48:43, simonb wrote:
> > > > Why not just use PAGE_SIZE, as elsewhere?  (Or is zip's idea of a page
> size
> > > not
> > > > necessarily the same as the platforms?)
> > > 
> > > I prefer to leave it like this. It really has to be 4096 as that is the
> > > alignment we bake into the file.
> > > If we ever want a different value on some platform we will have to bake in
> the
> > > lowest common multiple on all platforms which share the APK.
> > 
> > In which case, how about a) renaming it to kZipAlignmentPageSize or similar,
> and
> > b) checking that it is some multiple of PAGE_SIZE?
> 
> Sounds reasonable to me. Petr?

I agree and modified the code accordingly.

[Ilya Sherman, 2014-10-28 20:56:40]

histograms lgtm

[petrcermak, 2014-10-29 10:09:06]

The CQ bit was checked by petrcermak@chromium.org

[commit-bot: I haz the power, 2014-10-29 10:09:41]

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

[commit-bot: I haz the power, 2014-10-29 11:07:05]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-10-29 11:07:44]

Patchset 6 (id:??) landed as
https://crrev.com/2a112d50b0a28f82a519e194230b7de91ee21023
Cr-Commit-Position: refs/heads/master@{#301813}