Make a fairer combined message loop

base/android/java/src/org/chromium/base/SystemMessageHandler.java

Index: base/android/java/src/org/chromium/base/SystemMessageHandler.java
diff --git a/base/android/java/src/org/chromium/base/SystemMessageHandler.java b/base/android/java/src/org/chromium/base/SystemMessageHandler.java
index f7bb19f19e4e322c8dec3f281263368a2ece3983..6dca2a84f65aa67b44984627777909109e328902 100644
--- a/base/android/java/src/org/chromium/base/SystemMessageHandler.java
+++ b/base/android/java/src/org/chromium/base/SystemMessageHandler.java
@@ -13,75 +13,35 @@ import java.util.concurrent.atomic.AtomicBoolean;
 class SystemMessageHandler extends Handler {
 
     private static final int TIMER_MESSAGE = 1;
-    private static final int DELAYED_TIMER_MESSAGE = 2;
 
     // Native class pointer set by the constructor of the SharedClient native class.
     private int mMessagePumpDelegateNative = 0;
 
-    // Used to ensure we have at most one TIMER_MESSAGE pending at once.
-    private AtomicBoolean mTimerFired = new AtomicBoolean(true);
-
-    // Used to insert TIMER_MESSAGE on the front of the system message queue during startup only.
-    // This is a wee hack, to give a priority boost to native tasks during startup as they tend to
-    // be on the critical path. (After startup, handling the UI with minimum latency is more
-    // important).
-    private boolean mStartupComplete = false;
-    private final long mStartupCompleteTime = System.currentTimeMillis() + 2000;
-    private final boolean startupComplete() {
-        if (!mStartupComplete && System.currentTimeMillis() > mStartupCompleteTime) {
-            mStartupComplete = true;
-        }
-        return mStartupComplete;
-    }
-
     private SystemMessageHandler(int messagePumpDelegateNative) {
         mMessagePumpDelegateNative = messagePumpDelegateNative;
     }
 
     @Override
     public void handleMessage(Message msg) {
-        if (msg.what == TIMER_MESSAGE) {
-            mTimerFired.set(true);
-        }
-        while (nativeDoRunLoopOnce(mMessagePumpDelegateNative)) {
-            if (startupComplete()) {
-                setTimer();
-                break;
-            }
-        }
+        nativeDoRunLoopOnce(mMessagePumpDelegateNative);

[klobag.chromium, 2013/06/28 20:57:13]

nativeDoRunLoopOnce will do one normal, one delayed work. So we essentially
favor the native events.

[Kristian Monsen, 2013/06/28 21:04:41]

Yes, I am aware of this. But not sure how to handle it. I think the simplest
would be to only do one normal work, and perform one delayed work if there is no
normal work. What do you think?

[klobag.chromium, 2013/06/28 21:55:23]

I am afraid that will delay PostDelayedTask().

Can we add back DELAYED_TIMER_MESSAGE?

[Kristian Monsen, 2013/06/28 22:48:57]

Do you want a separate native method handling the delayed messages in that case?

[klobag.chromium, 2013/06/28 23:18:12]

Yes. So it is almost 1:1. Normal calls normal, delay calls delay.

[Kristian Monsen, 2013/06/28 23:43:05]

I will implement this and test in the weekend.

[joth, 2013/07/01 19:28:54]

What if nativeDoRunLoopOnce() returns true? 

[Kristian Monsen, 2013/07/02 21:32:27]

That is not important with the current implementation.There will always be
enough messages in the java message loop to handle all messages in the native
queue.

[Kristian Monsen, 2013/07/02 21:32:27]

I tested this, and I don't think it is optimal for a few reasons:
- It doesn't really solve the problem (all delayed messages starts as regular
messages, but handling them means just pushing them to a different queue, which
means we could handle two messages in that case)
- The code will be more complicated and will diverge more from the other
platforms
- It had some (2-3%) impact on startup performance.

Not that there is two things to optimize for on startup:
(a) Finish render startup page
(b) First time we have a responsive java ui
Those two goals can conflict, and optimizing for one might make the other worse.

I think this change will slightly make (b), java responsiveness, better as we
remove the old startup hack.

[klobag.chromium, 2013/07/02 22:53:02]

Took another look, now wondering whether this change may starve the UI events.

e.g. Android framework sends the input events based on vsync tick. This may
leave a window for our native code to fill the events in the event queue. With
this change, these events will be executed before vsync tick can happen in our
UI thread, which may cause unresponsive UI.

This may be easier to see in the startup.

Should we consider to limit the number of pending native TIMER_MESSAGE?

[Kristian Monsen, 2013/07/02 23:05:29]

That is sort of the problem with sharing the message loop. I think no matter
what we do there will be problems.

I have to admit I don't have the full picture here, but I think we should not
limit native events since we can't limit how many java events there are. For
WebView we could be stuck behind a slow java event from the app, on clank we
have more control over that, but I would be surprised if it never happens.

It could also be that clank and the webview needs different solutions, but I
hope not.

I think the ideal case would be to land this and than fix the times when there
are slow native events or too many of them, but that may not be practical.

     }
 
+    @SuppressWarnings("unused")
     @CalledByNative
     private void setTimer() {
-        if (!mTimerFired.getAndSet(false)) {
-            // mTimerFired was already false.
-            return;
-        }
-        if (startupComplete()) {
-            sendEmptyMessage(TIMER_MESSAGE);
-        } else {
-            sendMessageAtFrontOfQueue(obtainMessage(TIMER_MESSAGE));
-        }
+        sendEmptyMessage(TIMER_MESSAGE);

[aberent, 2013/06/28 21:52:28]

The current (mTimerFired) code only allows one normal and one delayed C++ task
to be queued at a time. After each pair has been run all the queued Java tasks,
including all input and draw events, will be processed before any further C++
tasks are processed.

Taking this out means that Java tasks/messages, including Android system
messages, and C++ tasks have the same priority. The problem with this is that
means that input (and draw) events will be put on the back of the event queue,
behind C++ tasks, so the UI will become less responsive.

In my work on splitting up content::Start() I am depending on the existing
behaviour allow in input events between C++ tasks.

[boliu, 2013/06/28 22:32:06]

The motivation for this change is that in android webview, where we do not
control what the app does with the UI loop. The pathological case would be the
app keeps the ui task loop full with a bunch of java tasks, then native tasks
will be starved and performance will be horrible.

On 2013/06/28 21:52:28, aberent wrote:
Input latency is from when event is generated to when we respond to the event by
say, updating the screen with new content. It is true that this will slightly
delay sending the event, but it will help with responding to the event since
these are done with native tasks, especially in the pathological case above.

Native tasks on UI will never block and should be short in general, so hopefully
the delay should not be significant.

Also in JB+, input events are already batched and throttled at 60 times per
second (ie between draws), so I don't think it will matter much anyway.
We should definitely measure this, but I don't think it will be true. Also
tweaking task scheduling is not the right solution to reduce latency. Instead we
should keep the UI thread responsive by never running long tasks or blocking on
IO.
I thought in the current code, we bias towards native tasks in the first 2
seconds of start up. I guess you are referring to after those 2 seconds?

[klobag.chromium, 2013/06/28 23:18:12]

I am not surprised that it is bad in the webview as UI is the compositor thread
which accepts a LOT of messages.

The new way treats Java events and native events equally. The order in the queue
is when they are added to the queue. So it is mostly fairer except native can
consume two tasks per run. This is discussed above.

On 2013/06/28 22:32:06, boliu wrote:

     }
 
-    // If millis <=0, it'll send a TIMER_MESSAGE instead of
-    // a DELAYED_TIMER_MESSAGE.
     @SuppressWarnings("unused")
     @CalledByNative
     private void setDelayedTimer(long millis) {
-        if (millis <= 0) {
-            setTimer();
-        } else {
-            removeMessages(DELAYED_TIMER_MESSAGE);
-            sendEmptyMessageDelayed(DELAYED_TIMER_MESSAGE, millis);
-        }
+        sendEmptyMessageDelayed(TIMER_MESSAGE, millis);
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
     private void removeTimer() {
         removeMessages(TIMER_MESSAGE);
-        removeMessages(DELAYED_TIMER_MESSAGE);
     }
 
     @CalledByNative