Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18181011:
Make a fairer combined message loop (Closed)
|
Created:
7 years, 5 months ago by Kristian Monsen Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake a fairer combined message loop
This will make us insert a new Message into the java message loop each time a new task is added. When those
tasks are handled we will handle one native task.
This is not the full truth as the Android DoRunLoopOnce does not actually just run the loop once, but that is for a later CL to clear up.
BUG=250924
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211303
Patch Set 1 #Patch Set 2 : Removed unused delayed timer message #
Total comments: 18
Patch Set 3 : Updated to ask the pump if it needs to be notified for each work item added #
Total comments: 2
Patch Set 4 : Added OVERRIDE #Patch Set 5 : caching the result from MessagePump::NeedsScheduleWorkPerTask #
Total comments: 6
Patch Set 6 : Keep the AlwaysNotifyPump local in message_loop.cc #Patch Set 7 : Fixes with delayed messages, Bo found some cases were the current cases did not work #
Total comments: 2
Patch Set 8 : Added comment to AlwaysNotifyPump #Patch Set 9 : Improving comment #
Messages
Total messages: 28 (0 generated)
I have performance tested this for startup on Android here: https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AmpHCKuLgIDwdHR0STJ... If the message_loop.cc change has performance impacts outside of Android we can use an #ifdef ANDROID. Hopefully the trybots will let us discover that. For now I am interested in general comments, but I think this will make performance better longer term as it is fairer and easier to understand what is happening.
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); nativeDoRunLoopOnce will do one normal, one delayed work. So we essentially favor the native events. https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... base/message_loop/message_loop.cc:627: if (!was_empty && !IsType(TYPE_UI)) Do you need this to get ScheduleWork() called? Should it inside #if ANDROID?
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 20:57:13, klobag.chromium wrote: > nativeDoRunLoopOnce will do one normal, one delayed work. So we essentially > favor the native events. 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? https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... base/message_loop/message_loop.cc:627: if (!was_empty && !IsType(TYPE_UI)) On 2013/06/28 20:57:13, klobag.chromium wrote: > Do you need this to get ScheduleWork() called? Should it inside #if ANDROID? Yes, this is to avoid early out and always get ScheduleWork called on the UI message loop on Android. If it has a performance impact on other platforms we can add an #ifdef ANDROID, but want to avoid that if possible.
As I discuss in my inline comment, do we actually want a fairer combined message loop? To keep the UI interactive we surely need touch and draw events to take priority over other events. https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: sendEmptyMessage(TIMER_MESSAGE); 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.
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 21:04:41, Kristian Monsen wrote: > On 2013/06/28 20:57:13, klobag.chromium wrote: > > nativeDoRunLoopOnce will do one normal, one delayed work. So we essentially > > favor the native events. > > 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? I am afraid that will delay PostDelayedTask(). Can we add back DELAYED_TIMER_MESSAGE?
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: sendEmptyMessage(TIMER_MESSAGE); 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: > 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. 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. > > 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. 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. > > In my work on splitting up content::Start() I am depending on the existing > behaviour allow in input events between C++ tasks. 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?
I was going to respond to Anthony's comment as well, but Bo's comment is very good. I just want to clarify that we put the native events at the back of the queue, so Java events is not being blocked by native events. (Except where mentioned by Grace). https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 21:55:23, klobag.chromium wrote: > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we essentially > > > favor the native events. > > > > 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? > > I am afraid that will delay PostDelayedTask(). > > Can we add back DELAYED_TIMER_MESSAGE? Do you want a separate native method handling the delayed messages in that case?
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 22:48:57, Kristian Monsen wrote: > On 2013/06/28 21:55:23, klobag.chromium wrote: > > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we > essentially > > > > favor the native events. > > > > > > 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? > > > > I am afraid that will delay PostDelayedTask(). > > > > Can we add back DELAYED_TIMER_MESSAGE? > > Do you want a separate native method handling the delayed messages in that case? Yes. So it is almost 1:1. Normal calls normal, delay calls delay. https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: sendEmptyMessage(TIMER_MESSAGE); 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: > 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: > > 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. > > 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. > > > > > 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. > > 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. > > > > > In my work on splitting up content::Start() I am depending on the existing > > behaviour allow in input events between C++ tasks. > > 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?
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 23:18:12, klobag.chromium wrote: > On 2013/06/28 22:48:57, Kristian Monsen wrote: > > On 2013/06/28 21:55:23, klobag.chromium wrote: > > > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we > > essentially > > > > > favor the native events. > > > > > > > > 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? > > > > > > I am afraid that will delay PostDelayedTask(). > > > > > > Can we add back DELAYED_TIMER_MESSAGE? > > > > Do you want a separate native method handling the delayed messages in that > case? > > Yes. So it is almost 1:1. Normal calls normal, delay calls delay. I will implement this and test in the weekend.
On 2013/06/28 22:32:06, boliu wrote: > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: > sendEmptyMessage(TIMER_MESSAGE); > 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: > > 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. > > 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. The sorts of events I am thinking about are events that are handled purely in the browser (probably purely on the Java side). This includes, for example, responding to touch or keypress events on the toolbar/omnibox by changing focus or displaying the key pressed. Previously these were never delayed by more than the time to process two C++ tasks (plus any Java tasks in the queue), with this change they will be delayed until all previously queued Java and C++ tasks have been completed. > > Native tasks on UI will never block and should be short in general, so hopefully > the delay should not be significant. This is only true if we keep the length of the native task queue short, since with this code the complete queue of native tasks will block new input events. > > 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. > > > > > 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. > > 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. > > > > > In my work on splitting up content::Start() I am depending on the existing > > behaviour allow in input events between C++ tasks. > > 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? Actually no, I am removing this bias in my experimental code. My problem is that there about a second's worth of C++ startup code that has to be run on the UI thread to initialize C++ side. To keep the omnibox responsive while this is running I need to break this up into a number (preferably at least 10) of separate tasks and run these in such a way that UI events can happen between these tasks. At present I can do these by simply adding all these tasks to the queue in one go, but this change will mean that I have to keep my own queue of tasks and submit them to the task queue one at a time.
On 2013/06/28 23:51:40, aberent wrote: > On 2013/06/28 22:32:06, boliu wrote: > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > File base/android/java/src/org/chromium/base/SystemMessageHandler.java > (right): > > > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: > > sendEmptyMessage(TIMER_MESSAGE); > > 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: > > > 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. > > > > 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. > > The sorts of events I am thinking about are events that are handled purely in > the browser (probably purely on the Java side). This includes, for example, > responding to touch or keypress events on the toolbar/omnibox by changing focus > or displaying the key pressed. Previously these were never delayed by more than > the time to process two C++ tasks (plus any Java tasks in the queue), with this > change they will be delayed until all previously queued Java and C++ tasks have > been completed. > > > > Native tasks on UI will never block and should be short in general, so > hopefully > > the delay should not be significant. > This is only true if we keep the length of the native task queue short, since > with this code the complete queue of native tasks will block new input events. > > > > 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. > > > > > > > > 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. > > > > 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. > > > > > > > > In my work on splitting up content::Start() I am depending on the existing > > > behaviour allow in input events between C++ tasks. > > > > 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? > > Actually no, I am removing this bias in my experimental code. My problem is that > there about a second's worth of C++ startup code that has to be run on the UI > thread to initialize C++ side. To keep the omnibox responsive while this is > running I need to break this up into a number (preferably at least 10) of > separate tasks and run these in such a way that UI events can happen between > these tasks. At present I can do these by simply adding all these tasks to the > queue in one go, but this change will mean that I have to keep my own queue of > tasks and submit them to the task queue one at a time. I don't think you need to make your own queue, would it work to just post the next task at the end of each of your ~10 tasks?
On 2013/06/28 23:51:40, aberent wrote: > On 2013/06/28 22:32:06, boliu wrote: > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > File base/android/java/src/org/chromium/base/SystemMessageHandler.java > (right): > > > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: > > sendEmptyMessage(TIMER_MESSAGE); > > 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: > > > 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. > > > > 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. > > The sorts of events I am thinking about are events that are handled purely in > the browser (probably purely on the Java side). This includes, for example, > responding to touch or keypress events on the toolbar/omnibox by changing focus > or displaying the key pressed. Previously these were never delayed by more than > the time to process two C++ tasks (plus any Java tasks in the queue), with this > change they will be delayed until all previously queued Java and C++ tasks have > been completed. This is true, and is something to look out for. I think in general the solution is to not spam the UI message loop. Startup is probably a corner case. I think we need to solve this by understanding why it happens, and see which messages take a long time on the UI message loop. This is difficult, but I would like to help you in working on solving this if it becomes a problem. > > > > Native tasks on UI will never block and should be short in general, so > hopefully > > the delay should not be significant. > This is only true if we keep the length of the native task queue short, since > with this code the complete queue of native tasks will block new input events. > > > > 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. > > > > > > > > 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. > > > > 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. > > > > > > > > In my work on splitting up content::Start() I am depending on the existing > > > behaviour allow in input events between C++ tasks. > > > > 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? > > Actually no, I am removing this bias in my experimental code. My problem is that > there about a second's worth of C++ startup code that has to be run on the UI > thread to initialize C++ side. To keep the omnibox responsive while this is > running I need to break this up into a number (preferably at least 10) of > separate tasks and run these in such a way that UI events can happen between > these tasks. At present I can do these by simply adding all these tasks to the > queue in one go, but this change will mean that I have to keep my own queue of > tasks and submit them to the task queue one at a time.
On 2013/06/29 00:20:19, Kristian Monsen wrote: > On 2013/06/28 23:51:40, aberent wrote: > > On 2013/06/28 22:32:06, boliu wrote: > > > > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > > File base/android/java/src/org/chromium/base/SystemMessageHandler.java > > (right): > > > > > > > > > https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... > > > base/android/java/src/org/chromium/base/SystemMessageHandler.java:32: > > > sendEmptyMessage(TIMER_MESSAGE); > > > 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: > > > > 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. > > > > > > 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. > > > > The sorts of events I am thinking about are events that are handled purely in > > the browser (probably purely on the Java side). This includes, for example, > > responding to touch or keypress events on the toolbar/omnibox by changing > focus > > or displaying the key pressed. Previously these were never delayed by more > than > > the time to process two C++ tasks (plus any Java tasks in the queue), with > this > > change they will be delayed until all previously queued Java and C++ tasks > have > > been completed. > > > > > > Native tasks on UI will never block and should be short in general, so > > hopefully > > > the delay should not be significant. > > This is only true if we keep the length of the native task queue short, since > > with this code the complete queue of native tasks will block new input events. > > > > > > 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. > > > > > > > > > > > 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. > > > > > > 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. > > > > > > > > > > > In my work on splitting up content::Start() I am depending on the existing > > > > behaviour allow in input events between C++ tasks. > > > > > > 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? > > > > Actually no, I am removing this bias in my experimental code. My problem is > that > > there about a second's worth of C++ startup code that has to be run on the UI > > thread to initialize C++ side. To keep the omnibox responsive while this is > > running I need to break this up into a number (preferably at least 10) of > > separate tasks and run these in such a way that UI events can happen between > > these tasks. At present I can do these by simply adding all these tasks to the > > queue in one go, but this change will mean that I have to keep my own queue of > > tasks and submit them to the task queue one at a time. > > I don't think you need to make your own queue, would it work to just post the > next task at the end of each of your ~10 tasks? I was thinking of the logic rather than the implementation; somehow I have to keep track of which tasks have run and which needs to run next. This is what I meant by having my own queue of tasks. Whether I do this explicitly using a lsit, or implicitly by each task knowing which should run next I regard as an implementation detail. I think the code will be cleaner if I do use an explicit list, but I am still working on this, so my view might change (or be changed by reviewers).
The yield model should not assume the current UI messageLoop behavior is permanent. Yield should always append a new task in the current task to give other events a chance to execute.
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); What if nativeDoRunLoopOnce() returns true? https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... base/message_loop/message_loop.cc:627: if (!was_empty && !IsType(TYPE_UI)) Might be cleaner to delegate this to the pump: if (!was_empty && !pump->NeedsScheduleWorkPerTask()) ... also, It would be easy for someone to regress this and break android. Can we think of anyway to add a test for this?
Updated and looked at the feedback. I did not update with code to do delayed messaged separate from regular messages for reason indicated inline. If you feel strongly about this Grace I will update as I am not strongly against it. I think it would be good to get this landed soon as it is early in the release cycle and we have to to evaluate if is working as intended and maybe revert if needed before the next release. https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/07/01 19:28:54, joth wrote: > What if nativeDoRunLoopOnce() returns true? 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. https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/06/28 23:43:05, Kristian Monsen wrote: > On 2013/06/28 23:18:12, klobag.chromium wrote: > > On 2013/06/28 22:48:57, Kristian Monsen wrote: > > > On 2013/06/28 21:55:23, klobag.chromium wrote: > > > > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > > > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we > > > essentially > > > > > > favor the native events. > > > > > > > > > > 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? > > > > > > > > I am afraid that will delay PostDelayedTask(). > > > > > > > > Can we add back DELAYED_TIMER_MESSAGE? > > > > > > Do you want a separate native method handling the delayed messages in that > > case? > > > > Yes. So it is almost 1:1. Normal calls normal, delay calls delay. > > I will implement this and test in the weekend. 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. https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/2001/base/message_loop/message_... base/message_loop/message_loop.cc:627: if (!was_empty && !IsType(TYPE_UI)) On 2013/07/01 19:28:54, joth wrote: > > Might be cleaner to delegate this to the pump: > if (!was_empty && !pump->NeedsScheduleWorkPerTask()) ... > > > also, It would be easy for someone to regress this and break android. Can we > think of anyway to add a test for this? Delegated to the pump.
lgtm https://codereview.chromium.org/18181011/diff/23001/base/message_loop/message... File base/message_loop/message_pump_android.h (right): https://codereview.chromium.org/18181011/diff/23001/base/message_loop/message... base/message_loop/message_pump_android.h:31: virtual bool NeedsScheduleWorkPerTask(); OVERRIDE
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/07/02 21:32:27, Kristian Monsen wrote: > On 2013/06/28 23:43:05, Kristian Monsen wrote: > > On 2013/06/28 23:18:12, klobag.chromium wrote: > > > On 2013/06/28 22:48:57, Kristian Monsen wrote: > > > > On 2013/06/28 21:55:23, klobag.chromium wrote: > > > > > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > > > > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > > > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we > > > > essentially > > > > > > > favor the native events. > > > > > > > > > > > > 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? > > > > > > > > > > I am afraid that will delay PostDelayedTask(). > > > > > > > > > > Can we add back DELAYED_TIMER_MESSAGE? > > > > > > > > Do you want a separate native method handling the delayed messages in that > > > case? > > > > > > Yes. So it is almost 1:1. Normal calls normal, delay calls delay. > > > > I will implement this and test in the weekend. > > 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. 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?
https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right): https://codereview.chromium.org/18181011/diff/2001/base/android/java/src/org/... base/android/java/src/org/chromium/base/SystemMessageHandler.java:26: nativeDoRunLoopOnce(mMessagePumpDelegateNative); On 2013/07/02 22:53:02, klobag.chromium wrote: > On 2013/07/02 21:32:27, Kristian Monsen wrote: > > On 2013/06/28 23:43:05, Kristian Monsen wrote: > > > On 2013/06/28 23:18:12, klobag.chromium wrote: > > > > On 2013/06/28 22:48:57, Kristian Monsen wrote: > > > > > On 2013/06/28 21:55:23, klobag.chromium wrote: > > > > > > On 2013/06/28 21:04:41, Kristian Monsen wrote: > > > > > > > On 2013/06/28 20:57:13, klobag.chromium wrote: > > > > > > > > nativeDoRunLoopOnce will do one normal, one delayed work. So we > > > > > essentially > > > > > > > > favor the native events. > > > > > > > > > > > > > > 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? > > > > > > > > > > > > I am afraid that will delay PostDelayedTask(). > > > > > > > > > > > > Can we add back DELAYED_TIMER_MESSAGE? > > > > > > > > > > Do you want a separate native method handling the delayed messages in > that > > > > case? > > > > > > > > Yes. So it is almost 1:1. Normal calls normal, delay calls delay. > > > > > > I will implement this and test in the weekend. > > > > 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. > > 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? 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. https://codereview.chromium.org/18181011/diff/23001/base/message_loop/message... File base/message_loop/message_pump_android.h (right): https://codereview.chromium.org/18181011/diff/23001/base/message_loop/message... base/message_loop/message_pump_android.h:31: virtual bool NeedsScheduleWorkPerTask(); On 2013/07/02 21:43:17, joth wrote: > OVERRIDE Done.
lgtm Let us experiment with it in the trunk to see how it goes.
drive-by: don't know enough to under whether the notification logic is sane, but I've added a few small style nits. https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.cc:190: always_notify_pump_ = pump_->NeedsScheduleWorkPerTask(); Always_notify_pump_ seems to effectively be a platform constant. It almost seems cleaner to just keep the #if define(OS_ANDROID) in this file and avoid introducing the new variable + method. https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.cc:630: // a task is added to the incoming queue nit: Please end sentences with a period. https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.h:532: nit: spurious newline
awong: thank you for the drive-by review, I simplified a bit based on the comments. https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.cc:190: always_notify_pump_ = pump_->NeedsScheduleWorkPerTask(); On 2013/07/03 19:09:22, awong wrote: > Always_notify_pump_ seems to effectively be a platform constant. It almost seems > cleaner to just keep the #if define(OS_ANDROID) in this file and avoid > introducing the new variable + method. I had a CL doing this locally, was not sure which one was best. I kind of liked what you are describing, but one of the reviewer thought this was better. One nice side effect of keeping it local is that reverting will be cleaner ... https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.cc:630: // a task is added to the incoming queue On 2013/07/03 19:09:22, awong wrote: > nit: Please end sentences with a period. Done. https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/18181011/diff/32001/base/message_loop/message... base/message_loop/message_loop.h:532: On 2013/07/03 19:09:22, awong wrote: > nit: spurious newline Removed the variable, no changes in this file anymore.
lgtm https://codereview.chromium.org/18181011/diff/39001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/39001/base/message_loop/message... base/message_loop/message_loop.cc:100: bool AlwaysNotifyPump(MessageLoop::Type type) { Can you add a comment for this function? Like what should be notified and when?
https://codereview.chromium.org/18181011/diff/39001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/18181011/diff/39001/base/message_loop/message... base/message_loop/message_loop.cc:100: bool AlwaysNotifyPump(MessageLoop::Type type) { On 2013/07/10 18:27:03, brettw wrote: > Can you add a comment for this function? Like what should be notified and when? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/18181011/52001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/18181011/52001
Message was sent while issue was closed.
Change committed as 211303 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
