android: Record early toplevel Looper events in tracing.

android: Record early toplevel Looper events in tracing.

Toplevel looper events are recorded on Android, but only once the native
library has been loaded. This adds the support for early java tracing,
in order to trace early startup events.

For instance, this allows to discover that for a cold start on Nexus 5X:
- a ~31ms task executes between ChromeLauncherActivity and
  ChromeTabbedActivity
- Framework UI layout tasks take ~180ms on the UI thread before browser
  initialization can start.

BUG=722813
Review-Url: https://codereview.chromium.org/2886763002
Cr-Commit-Position: refs/heads/master@{#473537}
Committed: https://chromium.googlesource.com/chromium/src/+/699d393fc495fa1e835d7d72a4f308d55ffc80b2

[Benoit L, 2017-05-16 13:25:49]

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

[commit-bot: I haz the power, 2017-05-16 13:26:08]

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, 2017-05-16 14:19:44]

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

[commit-bot: I haz the power, 2017-05-16 14:19:45]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-05-16 14:29:27]

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

[Benoit L, 2017-05-16 14:29:28]

Hi agrieve@,

This is the follow-up to yesterday's patch, adding early tracing support.

PTAL

https://codereview.chromium.org/2886763002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(left):

https://codereview.chromium.org/2886763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:227:
public void onStop() {
No longer needed as we have tracing of the toplevel ActivityThread tasks.

[agrieve, 2017-05-16 15:52:11]

mostly just a question. lgtm

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/TraceEvent.java (right):

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/TraceEvent.java:44: if (sEnabled ||
earlyTracingActive) {
should you cache sEnabled in a local as well since it's marked volatile?

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/TraceEvent.java:46: if (sEnabled)
nativeBeginToplevel(target);
It's not obvious that we should ever do both of these things. maybe set 
boolean earlyTracingActive = !sEnabled && EarlyTraceEvent.isActive();

Or maybe it's fine. Would be great to add a comment to where sEnabled is defined
to explain when it gets set to true.

[Benoit L, 2017-05-16 16:21:58]

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

[commit-bot: I haz the power, 2017-05-16 16:22:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benoit L, 2017-05-16 16:23:21]

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

[Benoit L, 2017-05-16 16:23:22]

Thanks for the comments!
All done.

+mariakhomenko: ChromeLauncherActivity, or everything, as you like. PTAL,
thanks.

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/TraceEvent.java (right):

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/TraceEvent.java:44: if (sEnabled ||
earlyTracingActive) {
On 2017/05/16 15:52:11, agrieve wrote:
> should you cache sEnabled in a local as well since it's marked volatile?

Thanks, added a comment.

This is fine, as even if the value is out of date, EarlyTraceEvent will filter
out the event.

https://codereview.chromium.org/2886763002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/TraceEvent.java:46: if (sEnabled)
nativeBeginToplevel(target);
On 2017/05/16 15:52:11, agrieve wrote:
> It's not obvious that we should ever do both of these things. maybe set 
> boolean earlyTracingActive = !sEnabled && EarlyTraceEvent.isActive();
> 
> Or maybe it's fine. Would be great to add a comment to where sEnabled is
defined
> to explain when it gets set to true.

In this case, due to the state transitions, this is fine. Changed the code as
suggested to make that explicit.

It was fine, since this is on the UI thread, and early tracing disabling /
tracing enabling are done at the same time on the UI thread, so only one of
these calls will do anything.

Also added ThreadUtils.assertOnUiThread() in EarlyTraceEvent to make clear on
which thread the enabled / disabled transitions are taking place.

[commit-bot: I haz the power, 2017-05-16 18:06:54]

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

[commit-bot: I haz the power, 2017-05-16 18:06:56]

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

[Benoit L, 2017-05-17 08:35:02]

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

[commit-bot: I haz the power, 2017-05-17 08:35:41]

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, 2017-05-17 09:33:08]

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

[commit-bot: I haz the power, 2017-05-17 09:33:09]

Dry run: This issue passed the CQ dry run.

[Maria, 2017-05-18 22:10:42]

lgtm

Your CL description is making me go "ouch" :/

[Benoit L, 2017-05-22 08:15:03]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-05-22 08:15:04]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2886763002/#ps40001
(title: ".")

[commit-bot: I haz the power, 2017-05-22 08:15:27]

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, 2017-05-22 09:57:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495440903850900,
"parent_rev": "d26294e6d19f9cf7b19e62af4431ed56583d2751", "commit_rev":
"699d393fc495fa1e835d7d72a4f308d55ffc80b2"}

[commit-bot: I haz the power, 2017-05-22 09:58:08]

Description was changed from

==========
android: Record early toplevel Looper events in tracing.

Toplevel looper events are recorded on Android, but only once the native
library has been loaded. This adds the support for early java tracing,
in order to trace early startup events.

For instance, this allows to discover that for a cold start on Nexus 5X:
- a ~31ms task executes between ChromeLauncherActivity and
  ChromeTabbedActivity
- Framework UI layout tasks take ~180ms on the UI thread before browser
  initialization can start.

BUG=722813
==========

to

==========
android: Record early toplevel Looper events in tracing.

Toplevel looper events are recorded on Android, but only once the native
library has been loaded. This adds the support for early java tracing,
in order to trace early startup events.

For instance, this allows to discover that for a cold start on Nexus 5X:
- a ~31ms task executes between ChromeLauncherActivity and
  ChromeTabbedActivity
- Framework UI layout tasks take ~180ms on the UI thread before browser
  initialization can start.

BUG=722813

Review-Url: https://codereview.chromium.org/2886763002
Cr-Commit-Position: refs/heads/master@{#473537}
Committed:
https://chromium.googlesource.com/chromium/src/+/699d393fc495fa1e835d7d72a4f3...
==========

[commit-bot: I haz the power, 2017-05-22 09:58:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/699d393fc495fa1e835d7d72a4f3...