Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(35)

Issue 575103002: [Android] Experimental sync barrier detection for tracing (Closed)

Created:
4 years, 6 months ago by jdduke (slow)
Modified:
4 years, 5 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, no sievers, aelias_OOO_until_Jul13
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 #

Patch Set 2 : No logging #

Patch Set 3 : Rebase #

Total comments: 2

Patch Set 4 : Early out #

Patch Set 5 : Updates #

Total comments: 12

Patch Set 6 : Code review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -1 line) Patch
M base/android/java/src/org/chromium/base/SystemMessageHandler.java View 1 2 3 4 5 4 chunks +106 lines, -1 line 0 comments Download

Messages

Total messages: 17 (4 generated)
jdduke (slow)
epenner@: Do you think this would be useful? If so, could you take a look ...
4 years, 5 months ago (2014-09-22 21:09:21 UTC) #2
epennerAtGoogle
On 2014/09/22 21:09:21, jdduke wrote: > epenner@: Do you think this would be useful? If ...
4 years, 5 months ago (2014-09-22 21:23:59 UTC) #3
epennerAtGoogle
LGTM https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/org/chromium/base/SystemMessageHandler.java File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/org/chromium/base/SystemMessageHandler.java#newcode77 base/android/java/src/org/chromium/base/SystemMessageHandler.java:77: private void updateWhetherQueueHasBlockingSyncBarrier() { If there is any ...
4 years, 5 months ago (2014-09-22 21:26:49 UTC) #5
epennerAtGoogle
Btw, although we could add a trace in Android, this is available on prior releases, ...
4 years, 5 months ago (2014-09-22 21:28:24 UTC) #6
jdduke (slow)
https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/org/chromium/base/SystemMessageHandler.java File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/575103002/diff/40001/base/android/java/src/org/chromium/base/SystemMessageHandler.java#newcode77 base/android/java/src/org/chromium/base/SystemMessageHandler.java:77: private void updateWhetherQueueHasBlockingSyncBarrier() { On 2014/09/22 21:26:48, epennerAtGoogle wrote: ...
4 years, 5 months ago (2014-09-22 21:32:57 UTC) #7
jdduke (slow)
nyquist@: PTAL for owner review. Sorry for the churn here, we're hoping to find a ...
4 years, 5 months ago (2014-09-22 22:12:14 UTC) #9
nyquist
https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/org/chromium/base/SystemMessageHandler.java File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/org/chromium/base/SystemMessageHandler.java#newcode86 base/android/java/src/org/chromium/base/SystemMessageHandler.java:86: final Message queueHead = (Message)getField(mMessageQueue, mMessageQueueMessageField); These final qualifiers ...
4 years, 5 months ago (2014-09-23 00:08:39 UTC) #10
jdduke (slow)
Thanks for review! https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/org/chromium/base/SystemMessageHandler.java File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/575103002/diff/80001/base/android/java/src/org/chromium/base/SystemMessageHandler.java#newcode86 base/android/java/src/org/chromium/base/SystemMessageHandler.java:86: final Message queueHead = (Message)getField(mMessageQueue, mMessageQueueMessageField); ...
4 years, 5 months ago (2014-09-23 16:20:46 UTC) #11
nyquist
Thanks for the extra cleanup (inlining calls and using Looper.class, etc.). lgtm! Good luck with ...
4 years, 5 months ago (2014-09-23 18:55:52 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/575103002/100001
4 years, 5 months ago (2014-09-23 20:31:34 UTC) #14
commit-bot: I haz the power
Committed patchset #6 (id:100001) as a53bb32cf7f79a9f1dffa9b54ad31e915681eefc
4 years, 5 months ago (2014-09-23 21:18:32 UTC) #15
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/85bceaa6e05ee1bf42ac220ddcbb50ca268c1ea4 Cr-Commit-Position: refs/heads/master@{#296249}
4 years, 5 months ago (2014-09-23 21:19:07 UTC) #16
jdduke (slow)
4 years, 4 months ago (2014-11-13 16:54:09 UTC) #17
Message was sent while issue was closed.
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..

Powered by Google App Engine
This is Rietveld 408576698