Add instrumentation to the Android resource extraction.

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/12e8838a9b9a1fca7fbffe82a6e34f706ac1f342
Cr-Commit-Position: refs/heads/master@{#310772}

[Benoit L, 2014-12-31 15:34:21]

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

[pasko, 2015-01-01 11:25:08]

Please do endSection() more obvious/readable on exception throwing codepaths
(i.e. I propose using more 'finally'), see my comments below)

Since I am slow to respond, please add 'git blame'-relevant person for this
file. If they are not owners, please add an owner as well.

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

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:58:
Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
Please put a TODO(lizeb) to use chrome tracing here once it gets reported for
early startup. This would make it possible to create a performance metric (and a
test) based on the length of the event. I am not sure what we are going to do
with a metric that is supposed to differ significantly in the first run after
install (probably filter out in layers above).

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:89:
Trace.endSection();
This would be nicer:
Trace.endSection(); // checkPakTimeStamp

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:174:
Trace.endSection();
why not in the 'finally' block above?

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:183:
Trace.endSection(); // WalkAssets
these would be less error prone if put in a 'finally' block because it would not
require verifying that the block above does not throw anything beyond
IOException.

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:237: if
(timestamps.length != 1) {
theoretically NPE is possible here, which means Trace.endSection(for
"checkPakTimeStamp") will not be called. Plz surround
checkPakTimestamp(outputDir) on line 67 with try-finally?

[Benoit L, 2015-01-05 13:16:57]

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

[Benoit L, 2015-01-05 13:16:57]

nyquist@chromium.org: 

Hello,

This is a first CL to get profiling data on chrome resource extraction at
startup on Android. Since the native library is not guaranteed to be available
at that time, we use android.os.Trace instead of chrome's TraceEvent.

This is related to the bug
https://code.google.com/p/chromium/issues/detail?id=445718

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

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:58:
Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
On 2015/01/01 11:25:08, pasko wrote:
> Please put a TODO(lizeb) to use chrome tracing here once it gets reported for
> early startup. This would make it possible to create a performance metric (and
a
> test) based on the length of the event. I am not sure what we are going to do
> with a metric that is supposed to differ significantly in the first run after
> install (probably filter out in layers above).

Done.

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:89:
Trace.endSection();
On 2015/01/01 11:25:08, pasko wrote:
> This would be nicer:
> Trace.endSection(); // checkPakTimeStamp

Acknowledged.

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:174:
Trace.endSection();
On 2015/01/01 11:25:08, pasko wrote:
> why not in the 'finally' block above?

Done.

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:183:
Trace.endSection(); // WalkAssets
On 2015/01/01 11:25:08, pasko wrote:
> these would be less error prone if put in a 'finally' block because it would
not
> require verifying that the block above does not throw anything beyond
> IOException.

Done.

https://codereview.chromium.org/805693003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ResourceExtractor.java:237: if
(timestamps.length != 1) {
On 2015/01/01 11:25:08, pasko wrote:
> theoretically NPE is possible here, which means Trace.endSection(for
> "checkPakTimeStamp") will not be called. Plz surround
> checkPakTimestamp(outputDir) on line 67 with try-finally?

Done.

[pasko, 2015-01-07 16:59:19]

thank you, it's very close to what I asked.

Please factor out doInBackgroundImpl().

I am being a bit nitpicky (nits are generally up to author whether to change
them or not).

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

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:59: // This is
currently no doable since the native library is not
nit:

s/no/not/

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:62:
Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
imho it would be more readable if the implementation is moved out like:

Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
try {
  doInBackgroundImpl()
} finally {
  Trace.endSection();
}

This would also make the diff smaller ..

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:176:
Trace.endSection();
nit:
Trace.endSection(); // ExtractResource

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:189:
Trace.endSection();
nit:
Trace.endSection(); // WalkAssets

[Benoit L, 2015-01-07 17:30:57]

Thank you for the review.

All done.

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

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:59: // This is
currently no doable since the native library is not
On 2015/01/07 16:59:19, pasko wrote:
> nit:
> 
> s/no/not/

Thanks for the catch.
Done.

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:62:
Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
On 2015/01/07 16:59:19, pasko wrote:
> imho it would be more readable if the implementation is moved out like:
> 
> Trace.beginSection("ResourceExtractor.ExtractTask.doInBackground");
> try {
>   doInBackgroundImpl()
> } finally {
>   Trace.endSection();
> }
> 
> This would also make the diff smaller ..

Yes, whitespace changes mess git blame output. Agreed, thank you.
Done.

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:176:
Trace.endSection();
On 2015/01/07 16:59:19, pasko wrote:
> nit:
> Trace.endSection(); // ExtractResource

Done.

https://codereview.chromium.org/805693003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:189:
Trace.endSection();
On 2015/01/07 16:59:19, pasko wrote:
> nit:
> Trace.endSection(); // WalkAssets

Done.

[nyquist, 2015-01-07 17:44:33]

lgtm

[pasko, 2015-01-07 17:55:42]

LGTM, thanks!

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

https://codereview.chromium.org/805693003/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ResourceExtractor.java:103: // also add
all .paks that we have for the user's currently
nit: can you plz reflow this comment back to the original form? it was more like
an 80-chars-limit motivated.

[Benoit L, 2015-01-09 15:04:30]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2015-01-09 15:04:56]

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

[commit-bot: I haz the power, 2015-01-09 16:08:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-09 16:09:54]

Patchset 3 (id:??) landed as
https://crrev.com/12e8838a9b9a1fca7fbffe82a6e34f706ac1f342
Cr-Commit-Position: refs/heads/master@{#310772}