Speed up Android resource extraction in the common case (nothing to do)

Speed up Android resource extraction in the common case (nothing to do)

In the vast majority of cases (no update and no locale change), there is
actually nothing to extract at startup. This commit fixes the logic that
allows for a quick exit in this case. Instrumentation shows that the
cost of the useless checks that are currently done is very large (400+ms
on hammerhead with Lollipop for a cold start).

Add instrumentation to the Android resource extraction.

The instrumentation uses android.os.Trace instead of the more common
org.chromium.base.TraceEvent as the resource extraction is typically done before
the native library initialization, so TraceEvent calls are dropped (per
TraceEvent documentation).

BUG=445718
Committed: https://crrev.com/648a2e7ab2989441a5df20b19d61cc2be9e166fe
Cr-Commit-Position: refs/heads/master@{#311312}

[Benoit L, 2014-12-31 16:59:38]

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

[pasko, 2015-01-01 11:57:33]

I like the idea of the fix!

Let's first land the CL that this depends on
(https://codereview.chromium.org/805693003/), with a metric and results visible
on chromeperf.appspot.com, then work out the details how to avoid obvious
regressions later (with new file types introduced).

I suspect this CL might be useful as-is for stable/beta channels. If local
benchmarking proves so, it makes a lot of sense to commit this CL with minimal
changes early and cherry-pick to appropriate branches. But we would need a very
good justification (on startup or even PLT benchmarks). Do you have numbers? :)

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:82:
HashSet<String> allFiles = new HashSet<String>(Arrays.asList(sMandatoryPaks));
why introducing allFiles? was 'filenames' not good?

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:89: File
directory = isAppDataFile ? getAppDataDir() : outputDir;
oh huh, so these files were always not found because they should reside in
AppDataDir? Awesome catch!

How can we make sure other file types do *not* crawl in later and make this
slower again? We will look at the performance metric, sure, but having a
mechanism that would fail early for devs would be nice. Can we play with
assertions against the number of files in outputDir?

[Benoit L, 2015-01-07 18:21:49]

New version of the CL:
- Remove "filenames" that is not used anywhere else
- Rebase on top of https://codereview.chromium.org/805693003/

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:82:
HashSet<String> allFiles = new HashSet<String>(Arrays.asList(sMandatoryPaks));
On 2015/01/01 11:57:33, pasko wrote:
> why introducing allFiles? was 'filenames' not good?

Done.

https://codereview.chromium.org/808053005/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:89: File
directory = isAppDataFile ? getAppDataDir() : outputDir;
On 2015/01/01 11:57:33, pasko wrote:
> oh huh, so these files were always not found because they should reside in
> AppDataDir? Awesome catch!
> 
> How can we make sure other file types do *not* crawl in later and make this
> slower again? We will look at the performance metric, sure, but having a
> mechanism that would fail early for devs would be nice. Can we play with
> assertions against the number of files in outputDir?

No, this line was never reached because filenames was shorter than
sMandatoryPaks. And filenames didn't contain any file in getAppDataDir().

[pasko, 2015-01-08 16:47:15]

I am mostly happy, small suggestions only. Had trouble trying to understand the
original logic and the history of it. I am still puzzled.

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:37: private
static final String PAK_FILENAMES = "Pak filenames";
PAK_FILENAMES is now not used, but prefs are preserved when apps upgrade.

I think it would be proper to rename it to PAK_FILENAMES_LEGACY_NOREUSE, since
somebody might decide to use this pref name in future, there would exist some
upgrade paths that might break because of it. Ha ha, I like purely hypothetical
problems.

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:81: boolean
isAppDataFile = file.equals(ICU_DATA_FILENAME)
this small piece of is repeated twice, how about private static boolean
IsAppDataFile(...)?

[Benoit L, 2015-01-08 17:24:42]

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:37: private
static final String PAK_FILENAMES = "Pak filenames";
On 2015/01/08 16:47:15, pasko wrote:
> PAK_FILENAMES is now not used, but prefs are preserved when apps upgrade.
> 
> I think it would be proper to rename it to PAK_FILENAMES_LEGACY_NOREUSE, since
> somebody might decide to use this pref name in future, there would exist some
> upgrade paths that might break because of it. Ha ha, I like purely
hypothetical
> problems.

Done.

https://codereview.chromium.org/808053005/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:81: boolean
isAppDataFile = file.equals(ICU_DATA_FILENAME)
On 2015/01/08 16:47:15, pasko wrote:
> this small piece of is repeated twice, how about private static boolean
> IsAppDataFile(...)?

Done.

[pasko, 2015-01-08 17:44:30]

lgtm, thank you

[Benoit L, 2015-01-08 17:50:39]

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

[Benoit L, 2015-01-08 17:50:40]

Hello,

This is the follow-up CL to (the already LGTM'd CL)
https://codereview.chromium.org/805693003 .

This one enables the fast path in resource extraction (nothing to extract) that
was present before but never taken.

The tracking bug (https://code.google.com/p/chromium/issues/detail?id=445718 )
contains context and data about the improvement (tl;dr: 200+ms on a cold start
for N5, going from 1200 to <1000).

Thank you.

[nyquist, 2015-01-08 18:24:19]

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:50: return
file.equals(ICU_DATA_FILENAME)
Nit: How about flipping the equals statement to have
CONSTANT.equals(possiblyNullObject)? It's not something that would ever fail in
this file, but it's nice to be consistent with that across the codebase.

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:87: for (String
file : sMandatoryPaks) {
I don't think the diff is what you think it is.

[Benoit L, 2015-01-09 17:10:47]

Thank you for the review.

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:50: return
file.equals(ICU_DATA_FILENAME)
On 2015/01/08 18:24:19, nyquist wrote:
> Nit: How about flipping the equals statement to have
> CONSTANT.equals(possiblyNullObject)? It's not something that would ever fail
in
> this file, but it's nice to be consistent with that across the codebase.

Done.

https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:87: for (String
file : sMandatoryPaks) {
On 2015/01/08 18:24:19, nyquist wrote:
> I don't think the diff is what you think it is.

Could you clarify ? I'm not sure that I'm understanding you here.

[Benoit L, 2015-01-13 15:17:45]

Friendly ping :-)

On 2015/01/09 17:10:47, lizeb wrote:
> Thank you for the review.
> 
>
https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):
> 
>
https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/ResourceExtractor.java:50: return
> file.equals(ICU_DATA_FILENAME)
> On 2015/01/08 18:24:19, nyquist wrote:
> > Nit: How about flipping the equals statement to have
> > CONSTANT.equals(possiblyNullObject)? It's not something that would ever fail
> in
> > this file, but it's nice to be consistent with that across the codebase.
> 
> Done.
> 
>
https://codereview.chromium.org/808053005/diff/120001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/ResourceExtractor.java:87: for (String
> file : sMandatoryPaks) {
> On 2015/01/08 18:24:19, nyquist wrote:
> > I don't think the diff is what you think it is.
> 
> Could you clarify ? I'm not sure that I'm understanding you here.

[nyquist, 2015-01-13 17:23:42]

thanks for the ping!

lgtm

[Benoit L, 2015-01-13 18:42:36]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2015-01-13 18:43:44]

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

[commit-bot: I haz the power, 2015-01-13 19:46:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-01-13 19:47:59]

Patchset 10 (id:??) landed as
https://crrev.com/648a2e7ab2989441a5df20b19d61cc2be9e166fe
Cr-Commit-Position: refs/heads/master@{#311312}