New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 610005 link

Starred by 2 users
Status: Fixed
Owner:
█ stanisc@chromium.org
not on Chrome anymore
Closed: May 2016
Cc:
brucedaw...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

MessagePumpForUI::WaitForWork might sometimes wait much longer than the intended delay.

Project Member Reported by stanisc@chromium.org, May 6 2016 
I found this issue when investigating MessagePump efficiency on Windows.
The problem is with WaitMessage call below which is made in some cases when MsgWaitForMultipleObjectsEx goes out of wait due to a message that doesn't belong to the thread that runs the message pump.

When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time.
To verify if there is a real issue I've instrumented the code and ran a number of different scenarios. 
It turns out it is pretty common for WaitMessage to block for up 15-16 ms when the intended delay is 1 or 2 ms.
And in one case I've seen it block for 500+ ms when the delay was 35 ms.

void MessagePumpForUI::WaitForWork() {
  // Wait until a message is available, up to the time needed by the timer
  // manager to fire the next set of timers.
  int delay = GetCurrentDelay();
  if (delay < 0)  // Negative value means no timers waiting.
    delay = INFINITE;
  DWORD result;
  result = MsgWaitForMultipleObjectsEx(0, NULL, delay, QS_ALLINPUT,
                                       MWMO_INPUTAVAILABLE);
  if (WAIT_OBJECT_0 == result) {
    // A WM_* message is available.
    // If a parent child relationship exists between windows across threads
    // then their thread inputs are implicitly attached.
    // This causes the MsgWaitForMultipleObjectsEx API to return indicating
    // that messages are ready for processing (Specifically, mouse messages
    // intended for the child window may appear if the child window has
    // capture).
    // The subsequent PeekMessages call may fail to return any messages thus
    // causing us to enter a tight loop at times.
    // The WaitMessage call below is a workaround to give the child window
    // some time to process its input messages.
    MSG msg = {0};
    bool has_pending_sent_message =
        (HIWORD(GetQueueStatus(QS_SENDMESSAGE)) & QS_SENDMESSAGE) != 0;
    if (!has_pending_sent_message &&
        !PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE)) {
      WaitMessage();
    }
    return;
  }
  DCHECK_NE(WAIT_FAILED, result) << GetLastError();
}

Comment 1 by brucedaw...@chromium.org, May 9 2016 

> It turns out it is pretty common for WaitMessage to block for up
> 15-16 ms when the intended delay is 1 or 2 ms.

That is normal and expected. The timeouts for Sleep, WaitForSingleObject, and WaitMessage are only processed when the system timer interrupt fires and by default this happens only every 15-16 ms. So, this is known and expected and as much as possible our code should be resistant to these delays. When we need higher resolution we must raise the timer frequency, but this should not be done routinely as it harms CPU power consumption.

> And in one case I've seen it block for 500+ ms when the delay was 35 ms.

That is unexpected. It can happen if the CPUs are all busy with other tasks but that should be rare. Do you have an ETW trace that shows this?

Comment 2 by stanisc@chromium.org, May 10 2016 

My point was that since WaitMessage doesn't get a timeout argument it might end up waiting an arbitrary amount of time. Its wake-up is determined by a next message rather than a delay to the next delayed task. So it ends up arbitrarily postponing a delayed task beyond its intended delay. A prompt task should be OK because its work message would interrupt WaitMessage.
Project Member

Comment 3 by bugdroid1@chromium.org, May 12 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05e1178680e47f4263f9c7087dad1d8e099a5027

commit 05e1178680e47f4263f9c7087dad1d8e099a5027
Author: stanisc <stanisc@chromium.org>
Date: Thu May 12 22:17:54 2016

Stop using WaitMessage in MessagePumpForUI::WaitForWork

When WaitMessage is called it ignores the current delay
timeout and might end up waiting an arbitrary time. See the
bug for the explanation.

The fix is to loop back and call MsgWaitForMultipleObjectEx
again with the remaining timeout.

BUG= 610005 

Review-Url: https://codereview.chromium.org/1953393002
Cr-Commit-Position: refs/heads/master@{#393375}

[modify] https://crrev.com/05e1178680e47f4263f9c7087dad1d8e099a5027/base/message_loop/message_pump_win.cc
Project Member

Comment 4 by bugdroid1@chromium.org, May 16 2016

Labels: merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11489eff83fe66f994f8d824f5327b15dde0e2e7

commit 11489eff83fe66f994f8d824f5327b15dde0e2e7
Author: Stanislav Chiknavaryan <stanisc@chromium.org>
Date: Mon May 16 20:59:31 2016

Stop using WaitMessage in MessagePumpForUI::WaitForWork

When WaitMessage is called it ignores the current delay
timeout and might end up waiting an arbitrary time. See the
bug for the explanation.

The fix is to loop back and call MsgWaitForMultipleObjectEx
again with the remaining timeout.

BUG= 610005 ,596190

Review-Url: https://codereview.chromium.org/1953393002
Cr-Commit-Position: refs/heads/master@{#393375}
(cherry picked from commit 05e1178680e47f4263f9c7087dad1d8e099a5027)

Review URL: https://codereview.chromium.org/1981953003 .

Cr-Commit-Position: refs/branch-heads/2704@{#565}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/11489eff83fe66f994f8d824f5327b15dde0e2e7/base/message_loop/message_pump_win.cc

Comment 5 by stanisc@chromium.org, May 16 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment