From 29a20657bb92335c4e671cc1ee4c5e36ee1ffa1b Mon Sep 17 00:00:00 2001 From: ananta Date: Wed, 10 Jun 2015 16:08:46 -0700 Subject: [PATCH] Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message. Relanding this patch with fixes for content_unittests failures. We need to wait for the UI worker thread to start. Currently the MessagePumpForUI class peeks Windows messages when we receive the kMsgHaveWork message in our main message loop and in nested modal loops. This is because the posted message starves the message loop of input and other lower priority messages. While this is ok for the main message loop our peeking and dispatching messages in nested loops is wrong and violates the silent contract where in the nested loop should be the one peeking and dispatching messages. To fix this the approach we are taking is to create a worker thread which uses a waitable timer of 3 ms which posts the kMsgHaveWork message to the main loop when the timer fires. As a result we can safely delete all the code in the MessagePumpForUI::ScheduleWork function and simplify the ProcessPumpReplacementMessage function. The MessagePumpForUI::ScheduleWork function now checks the delay for the task at the head of the queue If it is 0 then we post the message right away as it is a regular task. Added functions (GetNewlyAddedTaskDelay) to the MessagePump::Delegate class and the IncomingTaskQueue for this. The other change here is to change the GPU process to use the IO message pump by default and use the UI pump only if we are using opengl. Reason being this patch causes a delay in processing tasks due to the worker thread which causes tests like webgl_conformance to fail. We will continue working on addressing this over the coming days. BUG=492837 TBR=cpu, scottmg Review URL: https://codereview.chromium.org/1173193002 Cr-Commit-Position: refs/heads/master@{#333831} --- base/message_loop/incoming_task_queue.cc | 5 + base/message_loop/incoming_task_queue.h | 3 + base/message_loop/message_loop.cc | 4 + base/message_loop/message_loop.h | 1 + base/message_loop/message_pump.h | 3 + base/message_loop/message_pump_win.cc | 139 ++++++++++++++--------- base/message_loop/message_pump_win.h | 31 +++++ content/gpu/gpu_main.cc | 15 ++- 8 files changed, 145 insertions(+), 56 deletions(-) diff --git a/base/message_loop/incoming_task_queue.cc b/base/message_loop/incoming_task_queue.cc index 5e9a4613da1cad..642222efc8f0e5 100644 --- a/base/message_loop/incoming_task_queue.cc +++ b/base/message_loop/incoming_task_queue.cc @@ -119,6 +119,11 @@ void IncomingTaskQueue::StartScheduling() { ScheduleWork(); } +TimeTicks IncomingTaskQueue::GetNewlyAddedTaskDelay() { + return !incoming_queue_.empty() ? incoming_queue_.front().delayed_run_time : + TimeTicks(); +} + IncomingTaskQueue::~IncomingTaskQueue() { // Verify that WillDestroyCurrentMessageLoop() has been called. DCHECK(!message_loop_); diff --git a/base/message_loop/incoming_task_queue.h b/base/message_loop/incoming_task_queue.h index 7dd1e82312914c..544f3e907ee223 100644 --- a/base/message_loop/incoming_task_queue.h +++ b/base/message_loop/incoming_task_queue.h @@ -57,6 +57,9 @@ class BASE_EXPORT IncomingTaskQueue // scheduling work. void StartScheduling(); + // Returns the delay for the most recently added task. + TimeTicks GetNewlyAddedTaskDelay(); + private: friend class RefCountedThreadSafe; virtual ~IncomingTaskQueue(); diff --git a/base/message_loop/message_loop.cc b/base/message_loop/message_loop.cc index 4222c774dd58b2..b3c895cb58f44f 100644 --- a/base/message_loop/message_loop.cc +++ b/base/message_loop/message_loop.cc @@ -640,6 +640,10 @@ bool MessageLoop::DoIdleWork() { return false; } +TimeTicks MessageLoop::GetNewlyAddedTaskDelay() { + return incoming_task_queue_->GetNewlyAddedTaskDelay(); +} + void MessageLoop::DeleteSoonInternal(const tracked_objects::Location& from_here, void(*deleter)(const void*), const void* object) { diff --git a/base/message_loop/message_loop.h b/base/message_loop/message_loop.h index f2f89d0574dc05..2d67fe51807708 100644 --- a/base/message_loop/message_loop.h +++ b/base/message_loop/message_loop.h @@ -475,6 +475,7 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate { bool DoWork() override; bool DoDelayedWork(TimeTicks* next_delayed_work_time) override; bool DoIdleWork() override; + TimeTicks GetNewlyAddedTaskDelay() override; const Type type_; diff --git a/base/message_loop/message_pump.h b/base/message_loop/message_pump.h index a2edb458a96b53..77a49cee49416c 100644 --- a/base/message_loop/message_pump.h +++ b/base/message_loop/message_pump.h @@ -42,6 +42,9 @@ class BASE_EXPORT MessagePump : public NonThreadSafe { // Returns true to indicate that idle work was done. Returning false means // the pump will now wait. virtual bool DoIdleWork() = 0; + + // Returns the delay for the newly added task. + virtual TimeTicks GetNewlyAddedTaskDelay() = 0; }; MessagePump(); diff --git a/base/message_loop/message_pump_win.cc b/base/message_loop/message_pump_win.cc index cdbf0c260a9a49..a84e04e93a5ef9 100644 --- a/base/message_loop/message_pump_win.cc +++ b/base/message_loop/message_pump_win.cc @@ -12,6 +12,7 @@ #include "base/process/memory.h" #include "base/profiler/scoped_tracker.h" #include "base/strings/stringprintf.h" +#include "base/threading/thread.h" #include "base/trace_event/trace_event.h" #include "base/win/wrapped_window_proc.h" @@ -34,6 +35,10 @@ static const wchar_t kWndClassFormat[] = L"Chrome_MessagePumpWindow_%p"; // task (a series of such messages creates a continuous task pump). static const int kMsgHaveWork = WM_USER + 1; +// The default delay for the waitable timer used to wake up the UI worker +// thread. +static const int64 kDefaultUIWorkerThreadWakeupTimerMs = 3; + //----------------------------------------------------------------------------- // MessagePumpWin public: @@ -90,35 +95,39 @@ int MessagePumpWin::GetCurrentDelay() const { MessagePumpForUI::MessagePumpForUI() : atom_(0) { InitMessageWnd(); + + ui_worker_thread_timer_.Set(::CreateWaitableTimer(NULL, FALSE, NULL)); + ui_worker_thread_.reset(new base::Thread("UI Pump Worker thread")); + ui_worker_thread_->Start(); + ui_worker_thread_->WaitUntilThreadStarted(); + ui_worker_thread_->task_runner()->PostTask( + FROM_HERE, + base::Bind(&MessagePumpForUI::DoWorkerThreadRunLoop, + base::Unretained(this))); } MessagePumpForUI::~MessagePumpForUI() { DestroyWindow(message_hwnd_); UnregisterClass(MAKEINTATOM(atom_), GetModuleFromAddress(&WndProcThunk)); + + ::QueueUserAPC( + reinterpret_cast(&MessagePumpForUI::ShutdownWorkerThread), + ui_worker_thread_->thread_handle().platform_handle(), NULL); + ui_worker_thread_->Stop(); } void MessagePumpForUI::ScheduleWork() { - if (InterlockedExchange(&have_work_, 1)) - return; // Someone else continued the pumping. - - // Make sure the MessagePump does some work for us. - BOOL ret = PostMessage(message_hwnd_, kMsgHaveWork, - reinterpret_cast(this), 0); - if (ret) - return; // There was room in the Window Message queue. - - // We have failed to insert a have-work message, so there is a chance that we - // will starve tasks/timers while sitting in a nested message loop. Nested - // loops only look at Windows Message queues, and don't look at *our* task - // queues, etc., so we might not get a time slice in such. :-( - // We could abort here, but the fear is that this failure mode is plausibly - // common (queue is full, of about 2000 messages), so we'll do a near-graceful - // recovery. Nested loops are pretty transient (we think), so this will - // probably be recoverable. - InterlockedExchange(&have_work_, 0); // Clarify that we didn't really insert. - UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", MESSAGE_POST_ERROR, - MESSAGE_LOOP_PROBLEM_MAX); + // If we have a regular posted task at the head of queue then we need to + // process it quickly. + if (state_ && state_->delegate->GetNewlyAddedTaskDelay().is_null()) { + // Make sure the MessagePump does some work for us. + PostWorkMessage(); + return; + } + // Set a one shot timer to fire after 3 milliseconds. The actual resolution + // of the timer is dependent on timeBeginPeriod being called. + SetWakeupTimer(kDefaultUIWorkerThreadWakeupTimerMs); } void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) { @@ -409,45 +418,65 @@ bool MessagePumpForUI::ProcessMessageHelper(const MSG& msg) { } bool MessagePumpForUI::ProcessPumpReplacementMessage() { - // When we encounter a kMsgHaveWork message, this method is called to peek - // and process a replacement message, such as a WM_PAINT or WM_TIMER. The - // goal is to make the kMsgHaveWork as non-intrusive as possible, even though - // a continuous stream of such messages are posted. This method carefully - // peeks a message while there is no chance for a kMsgHaveWork to be pending, - // then resets the have_work_ flag (allowing a replacement kMsgHaveWork to - // possibly be posted), and finally dispatches that peeked replacement. Note - // that the re-post of kMsgHaveWork may be asynchronous to this thread!! - - bool have_message = false; - MSG msg; - // We should not process all window messages if we are in the context of an - // OS modal loop, i.e. in the context of a windows API call like MessageBox. - // This is to ensure that these messages are peeked out by the OS modal loop. - if (MessageLoop::current()->os_modal_loop()) { - // We only peek out WM_PAINT and WM_TIMER here for reasons mentioned above. - have_message = PeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) || - PeekMessage(&msg, NULL, WM_TIMER, WM_TIMER, PM_REMOVE); - } else { - have_message = PeekMessage(&msg, NULL, 0, 0, PM_REMOVE) != FALSE; - } + // Since we discarded a kMsgHaveWork message, we must update the flag. + InterlockedExchange(&have_work_, 0); + return true; +} - DCHECK(!have_message || kMsgHaveWork != msg.message || - msg.hwnd != message_hwnd_); +void MessagePumpForUI::DoWorkerThreadRunLoop() { + DCHECK(ui_worker_thread_timer_.Get()); + while (TRUE) { + DWORD ret = WaitForSingleObjectEx( + ui_worker_thread_timer_.Get(), INFINITE, TRUE); + // The only APC this thread could receive is the Shutdown APC. + if (ret == WAIT_IO_COMPLETION) + return; - // Since we discarded a kMsgHaveWork message, we must update the flag. - int old_have_work = InterlockedExchange(&have_work_, 0); - DCHECK(old_have_work); + // Make sure the MessagePump does some work for us. + PostWorkMessage(); - // We don't need a special time slice if we didn't have_message to process. - if (!have_message) - return false; + // Set a one shot timer to process pending delayed tasks if any in the + // queue. The actual resolution of the timer is dependent on the + // timeBeginPeriod API being called. + SetWakeupTimer(kDefaultUIWorkerThreadWakeupTimerMs); + } +} + +// static +void CALLBACK MessagePumpForUI::ShutdownWorkerThread(ULONG_PTR param) { + // This function is empty because we only use the fact that an APC was posted + // to the worker thread to shut it down. + return; +} + +void MessagePumpForUI::PostWorkMessage() { + BOOL posted = PostMessage(message_hwnd_, kMsgHaveWork, + reinterpret_cast(this), + 0); + if (!posted) { + // We have failed to insert a have-work message, so there is a chance + // that we will starve tasks/timers while sitting in a nested message + // loop. Nested loops only look at Windows Message queues, and don't + // look at *our* task queues, etc., so we might not get a time slice in + // such. :-( + // We could abort here, but the fear is that this failure mode is + // plausibly common (queue is full, of about 2000 messages), so we'll + // do a near-graceful recovery. Nested loops are pretty transient + // (we think), so this will probably be recoverable. + UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", + MESSAGE_POST_ERROR, + MESSAGE_LOOP_PROBLEM_MAX); + } +} - // Guarantee we'll get another time slice in the case where we go into native - // windows code. This ScheduleWork() may hurt performance a tiny bit when - // tasks appear very infrequently, but when the event queue is busy, the - // kMsgHaveWork events get (percentage wise) rarer and rarer. - ScheduleWork(); - return ProcessMessageHelper(msg); +void MessagePumpForUI::SetWakeupTimer(int64 delay_ms) { + // Set the timer for the delay passed in. The actual resolution of the + // timer is dependent on whether timeBeginPeriod was called. + LARGE_INTEGER due_time = {0}; + due_time.QuadPart = -delay_ms * 10000; + BOOL timer_set = ::SetWaitableTimer(ui_worker_thread_timer_.Get(), + &due_time, 0, NULL, NULL, FALSE); + CHECK(timer_set); } //----------------------------------------------------------------------------- diff --git a/base/message_loop/message_pump_win.h b/base/message_loop/message_pump_win.h index 00a1e77e2798cf..042efdf9d9f729 100644 --- a/base/message_loop/message_pump_win.h +++ b/base/message_loop/message_pump_win.h @@ -11,6 +11,7 @@ #include "base/base_export.h" #include "base/basictypes.h" +#include "base/memory/scoped_ptr.h" #include "base/message_loop/message_pump.h" #include "base/message_loop/message_pump_dispatcher.h" #include "base/observer_list.h" @@ -19,6 +20,8 @@ namespace base { +class Thread; + // MessagePumpWin serves as the base for specialized versions of the MessagePump // for Windows. It provides basic functionality like handling of observers and // controlling the lifetime of the message pump. @@ -135,11 +138,39 @@ class BASE_EXPORT MessagePumpForUI : public MessagePumpWin { bool ProcessMessageHelper(const MSG& msg); bool ProcessPumpReplacementMessage(); + // We spawn a worker thread to periodically post (3 ms) the kMsgHaveWork + // message to the UI message pump. This is to ensure that the main thread + // gets to process tasks and delayed tasks when there is no activity in the + // Windows message pump or when there is a nested modal loop (sizing/moving/ + // drag drop/message boxes) etc. + void DoWorkerThreadRunLoop(); + + // This function is posted as part of a user mode APC to shutdown the worker + // thread when the main message pump is shutting down. + static void CALLBACK ShutdownWorkerThread(ULONG_PTR param); + + // Helper function for posting the kMsgHaveWork message to wake up the pump + // for processing tasks. + void PostWorkMessage(); + + // Helper function to set the waitable timer used to wake up the UI worker + // thread for processing delayed tasks. + // |delay_ms| : The delay in milliseconds. + void SetWakeupTimer(int64 delay_ms); + // Atom representing the registered window class. ATOM atom_; // A hidden message-only window. HWND message_hwnd_; + + // This thread is used to periodically wake up the main thread to process + // tasks. + scoped_ptr ui_worker_thread_; + + // The UI worker thread waits on this timer indefinitely. When the main + // thread has tasks ready to be processed it sets the timer. + base::win::ScopedHandle ui_worker_thread_timer_; }; //----------------------------------------------------------------------------- diff --git a/content/gpu/gpu_main.cc b/content/gpu/gpu_main.cc index c6a59fc9c19c74..71e493bc7b82a5 100644 --- a/content/gpu/gpu_main.cc +++ b/content/gpu/gpu_main.cc @@ -160,9 +160,22 @@ int GpuMain(const MainFunctionParams& parameters) { bool dead_on_arrival = false; #if defined(OS_WIN) + base::MessageLoop::Type loop_type = base::MessageLoop::TYPE_IO; // Use a UI message loop because ANGLE and the desktop GL platform can // create child windows to render to. - base::MessageLoop main_message_loop(base::MessageLoop::TYPE_UI); + // TODO(ananta) : Recent changes to the UI message pump class on Windows + // will cause delays in tasks getting processed in the GPU process which + // will show up on the bots in webgl conformance tests, perf tests etc. + // It will be great if we can work around the issues with desktop GL and + // avoid creating a child window in the GPU process which requires a UI + // message pump. + if ((command_line.HasSwitch(switches::kUseGL) && + command_line.GetSwitchValueASCII(switches::kUseGL) == "desktop") || + (command_line.HasSwitch(switches::kUseANGLE) && + command_line.GetSwitchValueASCII(switches::kUseANGLE) == "gl")) { + loop_type = base::MessageLoop::TYPE_UI; + } + base::MessageLoop main_message_loop(loop_type); #elif defined(OS_LINUX) && defined(USE_X11) // We need a UI loop so that we can grab the Expose events. See GLSurfaceGLX // and https://crbug.com/326995.