[Android] Mark posted UI thread tasks as asynchronous

[Android] Mark posted UI thread tasks as asynchronous

Chromium shares a message loop with Android on the browser UI thread.
This can cause problems when the associated Looper has a sync barrier,
preventing posted Chromium tasks from being dispatched until the
barrier is removed. Make this sharing more fair by marking all Chromium
Message tasks as asynchronous, avoiding stalls when there is a sync
barrier.

Note: This change is for gathering data about the perf impact and we'll
revert it before cutting a release branch.

BUG=407149, 380781, 407133
Committed: https://crrev.com/feabeebb3ac5810f896ac8303a77ee695acaf9d4
Cr-Commit-Position: refs/heads/master@{#292551}

[jdduke (slow), 2014-08-28 17:57:31]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, epenner@chromium.org, sievers@chromium.org

[jdduke (slow), 2014-08-28 17:57:31]

There may be some side effects here, but they better be pretty nasty for us to
not go ahead with this.  It definitely fixes all the nasty stalls we've noticed
when the UI is invalidated out-of-concert with the compositor.

[Sami, 2014-08-28 18:01:50]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2014-08-28 18:01:50]

Are async messages processed out of order w.r.t. sync messages? I'm wondering if
this could introduce some subtle ordering bugs.

I'm also wondering if we could emulate this with postAtFrontOfQueue(), which is
another hairy API but at least it's public.

[no sievers, 2014-08-28 18:14:05]

On 2014/08/28 18:01:50, Sami wrote:
> Are async messages processed out of order w.r.t. sync messages? I'm wondering
if
> this could introduce some subtle ordering bugs.
> 
> I'm also wondering if we could emulate this with postAtFrontOfQueue(), which
is
> another hairy API but at least it's public.

postAtFrontOfQueue() would probably end up starving the view system in some
cases.
But it's possible that our msg pump could be reworked entirely to make it work
somehow.
We also noticed that there is a bit of overhead with calling into Java and
inserting a task there for every native postTask() (see AlwaysNotifyPump()
hack).

You could say that what we really want is to have our native msg queue be pumped
as part of the outer system loop somehow (and somewhat regularly with low
latency) . We currently achieve this by queuing a whole lot of tasks, but maybe
there is another way.

[no sievers, 2014-08-28 18:16:11]

On 2014/08/28 18:14:05, sievers wrote:
> On 2014/08/28 18:01:50, Sami wrote:
> > Are async messages processed out of order w.r.t. sync messages? I'm
wondering
> if
> > this could introduce some subtle ordering bugs.
> > 


I don't think anyone should make assumptions about ordering between tasks posted
from Java to the looper and tasks posted natively to the Chrome queue. So as
long as we don't break ordering within Chrome tasks, it is probably ok.

[jdduke (slow), 2014-08-28 22:07:10]

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

[jdduke (slow), 2014-08-28 22:07:11]

nyquist: PTAL for owner review.  We'd like to land this for a few days and gauge
the performance/stability impact, after which we'll likely revert it. If we
gather sufficient evidence for this change, we'll use it to motivate Android to
make public the corresponding asynchronous API.

[nyquist, 2014-08-28 23:27:27]

rubberstamp lgtm

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

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:42: } catch
(Exception e) {
Why is this not catching RuntimeException?

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:84: } catch
(Exception e) {
Why is this not catching IllegalAccessException, IllegalArgumentException,
InvocationTargetException? If you really need to catch RuntimeException, you can
specify that separately.

[jdduke (slow), 2014-08-28 23:37:03]

Thanks for review.

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

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:42: } catch
(Exception e) {
On 2014/08/28 23:27:27, nyquist wrote:
> Why is this not catching RuntimeException?

Is there anything to gain from catching that in addition to Exception?
ClassNotFound and NoSuchMethod are illustrative, but what does Runtime add to
the plain old Exception?

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:84: } catch
(Exception e) {
On 2014/08/28 23:27:27, nyquist wrote:
> Why is this not catching IllegalAccessException, IllegalArgumentException,
> InvocationTargetException? If you really need to catch RuntimeException, you
can
> specify that separately.

In practice, I'm not sure that we care, and I don't think we'd change the
reaction. If you think that would be helpful to differentiate, I'd be happy to
add them.

[jdduke (slow), 2014-08-28 23:54:41]

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

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:42: } catch
(Exception e) {
On 2014/08/28 23:37:03, jdduke wrote:
> On 2014/08/28 23:27:27, nyquist wrote:
> > Why is this not catching RuntimeException?
> 
> Is there anything to gain from catching that in addition to Exception?
> ClassNotFound and NoSuchMethod are illustrative, but what does Runtime add to
> the plain old Exception?

N/m, I have seen the error of my ways, done!

https://codereview.chromium.org/512333002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:84: } catch
(Exception e) {
On 2014/08/28 23:37:03, jdduke wrote:
> On 2014/08/28 23:27:27, nyquist wrote:
> > Why is this not catching IllegalAccessException, IllegalArgumentException,
> > InvocationTargetException? If you really need to catch RuntimeException, you
> can
> > specify that separately.
> 
> In practice, I'm not sure that we care, and I don't think we'd change the
> reaction. If you think that would be helpful to differentiate, I'd be happy to
> add them.

Ditto above.

[jdduke (slow), 2014-08-28 23:55:13]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-28 23:56:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/512333002/60001

[commit-bot: I haz the power, 2014-08-29 01:17:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-29 02:14:13]

Committed patchset #4 (id:60001) as 6a100f5a90bf045a4508c9d5df0af6cca0aa2084

[commit-bot: I haz the power, 2014-09-10 03:05:25]

Patchset 4 (id:??) landed as
https://crrev.com/feabeebb3ac5810f896ac8303a77ee695acaf9d4
Cr-Commit-Position: refs/heads/master@{#292551}

[jdduke (slow), 2014-09-15 18:28:31]

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

The reason for reverting is: This patch was purely experimental, and the
experiment has expired..