[Android] Experimental sync barrier detection for tracing

[Android] Experimental sync barrier detection for tracing

When an Android View is invalidated outside of frame dispatch
(animation/input/draw), a sync barrier may be inserted into the shared
UI thread message loop. This effectively blocks dispatch of all Chrome
tasks posted to the browser UI thread for an entire frame (or more if
the View is continually invalidated).

There are currently no easy or even automated ways to avoid this
untimely invalidation, and judicious coding can only take us so far. As
an intermediate assist in debugging the issue, use reflection and a
crude form of MessageQueue inspection to trace known cases where the
MessageQueue is blocked by a sync barrier.

Note that this detection is not perfect, neither is it exact, but it's a
solid proxy for informing traces about such pipeline stalls.

BUG=407133
Committed: https://crrev.com/85bceaa6e05ee1bf42ac220ddcbb50ca268c1ea4
Cr-Commit-Position: refs/heads/master@{#296249}

[jdduke (slow), 2014-09-22 21:09:21]

jdduke@chromium.org changed reviewers:
+ epenner@chromium.org

[jdduke (slow), 2014-09-22 21:09:21]

epenner@: Do you think this would be useful? If so, could you take a look (and
I'll add appropriate owners)?

I've already found it useful in debugging and blaming a few cases where the
pipeline was stalling around page load and scrolling (see
https://codereview.chromium.org/583673002/ and 
https://chrome-internal-review.googlesource.com/#/c/176791/).

[epennerAtGoogle, 2014-09-22 21:23:59]

On 2014/09/22 21:09:21, jdduke wrote:
> epenner@: Do you think this would be useful? If so, could you take a look (and
> I'll add appropriate owners)?
> 
> I've already found it useful in debugging and blaming a few cases where the
> pipeline was stalling around page load and scrolling (see
> https://codereview.chromium.org/583673002/ and 
> https://chrome-internal-review.googlesource.com/#/c/176791/).

This looks cool!  I'm looking over it now, but only real concern is whether
there is any perf impact. We could add "TracingEnabled" checks if that's the
case.

[epennerAtGoogle, 2014-09-22 21:26:48]

epenner@google.com changed reviewers:
+ epenner@google.com

[epennerAtGoogle, 2014-09-22 21:26:49]

LGTM

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

https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:77: private
void updateWhetherQueueHasBlockingSyncBarrier() {
If there is any cost to analyzing a message, we could check if tracing is
enabled and early out.

[epennerAtGoogle, 2014-09-22 21:28:24]

Btw, although we could add a trace in Android, this is available on prior
releases, and also doesn't require adding systraces (which wouldn't be obvious
to enable). So I really like this approach.

[jdduke (slow), 2014-09-22 21:32:57]

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

https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:77: private
void updateWhetherQueueHasBlockingSyncBarrier() {
On 2014/09/22 21:26:48, epennerAtGoogle wrote:
> If there is any cost to analyzing a message, we could check if tracing is
> enabled and early out.

Ohh, I like that, didn't realize that was exposed from TraceEvent, I'll make the
change.

[jdduke (slow), 2014-09-22 22:12:14]

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

[jdduke (slow), 2014-09-22 22:12:14]

nyquist@: PTAL for owner review. Sorry for the churn here, we're hoping to find
a more permanent solution here than mucking about with reflection...

[nyquist, 2014-09-23 00:08:39]

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

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:86: final
Message queueHead = (Message)getField(mMessageQueue, mMessageQueueMessageField);
These final qualifiers are unnecessary I believe.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:87: final
boolean queueHasSyncbarrier = isSyncBarrierMessage(queueHead);
queueHasSyncBarrier

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:104:
mMessageQueue = (MessageQueue)(getQueueMethod.invoke(getLooper()));
Just (MessageQueue) getQueueMethod.invoke(getLooper()) ?

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:109:
mMessageTargetField = new Message().getClass().getDeclaredField("target");
Could this just be Message.class.getDeclaredField("target")?

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:125: } catch
(RuntimeException e) {
This is masking IllegalArgumentException thrown by invoke. Add both separately?
It's OK as is as well since it's just a RuntimeException, but I wasn't sure it
was intentional.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:140: private
void setQueueHasSyncBarrier(boolean queueHasSyncbarrier) {
queueHasSyncBarrier

[jdduke (slow), 2014-09-23 16:20:46]

Thanks for review!

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

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:86: final
Message queueHead = (Message)getField(mMessageQueue, mMessageQueueMessageField);
On 2014/09/23 00:08:39, nyquist wrote:
> These final qualifiers are unnecessary I believe.

Done.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:87: final
boolean queueHasSyncbarrier = isSyncBarrierMessage(queueHead);
On 2014/09/23 00:08:38, nyquist wrote:
> queueHasSyncBarrier

Done.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:104:
mMessageQueue = (MessageQueue)(getQueueMethod.invoke(getLooper()));
On 2014/09/23 00:08:38, nyquist wrote:
> Just (MessageQueue) getQueueMethod.invoke(getLooper()) ?

Done.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:109:
mMessageTargetField = new Message().getClass().getDeclaredField("target");
On 2014/09/23 00:08:38, nyquist wrote:
> Could this just be Message.class.getDeclaredField("target")?

Done.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:125: } catch
(RuntimeException e) {
On 2014/09/23 00:08:39, nyquist wrote:
> This is masking IllegalArgumentException thrown by invoke. Add both
separately?
> It's OK as is as well since it's just a RuntimeException, but I wasn't sure it
> was intentional.

Not intentional, good catch.

https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:140: private
void setQueueHasSyncBarrier(boolean queueHasSyncbarrier) {
On 2014/09/23 00:08:39, nyquist wrote:
> queueHasSyncBarrier

Done.

[nyquist, 2014-09-23 18:55:52]

Thanks for the extra cleanup (inlining calls and using Looper.class, etc.).
lgtm!

Good luck with your experiment!

[jdduke (slow), 2014-09-23 18:57:16]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:31:34]

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

[commit-bot: I haz the power, 2014-09-23 21:18:32]

Committed patchset #6 (id:100001) as a53bb32cf7f79a9f1dffa9b54ad31e915681eefc

[commit-bot: I haz the power, 2014-09-23 21:19:07]

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

[jdduke (slow), 2014-11-13 16:54:09]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/726623002/ by jdduke@chromium.org.

The reason for reverting is: Barrier detection is no longer necessary, see
crrev.com/512333002..