Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086123002:
MessagePumpDefault can spin for up to 1 ms on Windows (Closed)
DescriptionMessagePumpDefault can spin for up to 1 ms on Windows
There are two power efficiency issues that this change is trying
to address:
1) On Windows if we call event_.TimedWait(delay) with a sub-ms
delay then the wait function will return promptly.
This means we end up busy-waiting down to zero.
Rounding these times up to 1 ms lets Chrome go idle.
ETW profiles show that the spinning makes the message
loop inefficient. In some cases up to 50% of all inclusive
MessagePumpDefault::Run cycles are spent inside the timed
wait.
2) On Windows versions 7 and above wait functions (Sleep,
WaitForSingleObject, WaitForMultipleObjects, etc) time out
up to 1 ms earlier. That results in iterating through the
loop with no new work available (the event isn't
set) and too early for delayed tasks to run.
This change adds an inner loop around just event_.TimedWait(delay)
that makes it go straight to wait for the remaining part of
the delay should it wake up too early.
This change will trigger a regression in "percentage smooth"
metric of smoothness.top_25_smooth benchmark. But giving an
increasing focus on power consumption (when running on
batteries) it might be worth fixing this now and deal with
the smoothness benchmark regression later (see below).
"Percentage smooth" uses deltas between Renderer / Browser swap
timestamps as an estimation for frame times and counts the
percentage of frames under 17 ms. This makes the metric overly
sensitive to even slight delays in scheduling of swaps which
are mostly avoided in the current implementation due to busy-
spinning the loop in the last millisecond of the wait. Swap times
can actually move forward & backward inside a frame and still meet
V-sync which the existing implementation can't tell without
the actual frame statistics.
I've filed crbug.com/614154 and crbug.com/614147 to track a more
accurate implementation of "Percentage smooth" on Windows.
BUG=487724
Committed: https://crrev.com/56ee6be040caa993583170bd5edc472e7b969c9e
Cr-Commit-Position: refs/heads/master@{#402027}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added const to min_delay #
Total comments: 17
Patch Set 3 : Addressed CR feedback #
Total comments: 3
Patch Set 4 : Added a comment #Messages
Total messages: 28 (6 generated)
Description was changed from ========== Applied MessagePumpDefault wait fix BUG= ========== to ========== MessagePumpDefault can spin for up to 1 ms on Windows There are two power efficiency issues that this change is trying to address: 1) On Windows if we call event_.TimedWait(delay) with a sub-ms delay then the wait function will return promptly. This means we end up busy-waiting down to zero. Rounding these times up to 1 ms lets Chrome go idle. ETW profiles show that the spinning makes the message loop inefficient. In some cases up to 50% of all inclusive MessagePumpDefault::Run cycles are spent inside the timed wait. 2) On Windows versions 7 and above wait functions (Sleep, WaitForSingleObject, WaitForMultipleObjects, etc) time out up to 1 ms earlier. That results in iterating through the loop with no new work available (the event isn't set) and too early for delayed tasks to run. This change adds an inner loop around just event_.TimedWait(delay) that makes it go straight to wait for the remaining part of the delay should it wake up too early. This change will trigger a regression in "percentage smooth" metric of smoothness.top_25_smooth benchmark. But giving an increasing focus on power consumption (when running on batteries) it might be worth fixing this now and deal with the smoothness benchmark regression later (see below). "Percentage smooth" uses deltas between Renderer / Browser swap timestamps as an estimation for frame times and counts the percentage of frames under 17 ms. This makes the metric overly sensitive to even slight delays in scheduling of swaps which are mostly avoided in the current implementation due to busy- spinning the loop in the last millisecond of the wait. Swap times can actually move forward & backward inside a frame and still meet V-sync which the existing implementation can't tell without the actual frame statistics. I've filed crbug.com/614154 and crbug.com/614147 to track a more accurate implementation of "Percentage smooth" on Windows. BUG=487724 ==========
stanisc@chromium.org changed reviewers: + brucedawson@chromium.org, danakj@chromium.org, vmiura@chromium.org
PTAL!
The overhead varies but I'm pretty sure I've seen ETW traces where about half of the CPU consumption in some Chrome processes is from spinning in this loop. I look forward to this fix. lgtm with one nit. https://codereview.chromium.org/2086123002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_default.cc:62: TimeDelta min_delay = TimeDelta::FromMilliseconds(1); This should be const to make the intent clearer.
https://codereview.chromium.org/2086123002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_default.cc:62: TimeDelta min_delay = TimeDelta::FromMilliseconds(1); On 2016/06/21 21:35:11, brucedawson wrote: > This should be const to make the intent clearer. Done.
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) I am wondering about your choice to put this change here in an #ifdef of in waitable_event_win.cc. Should this not apply to other uses of the method? I see TimedWait is allowed to return early if it returns false, so maybe not that part? But the min_delay seems more general. Any thoughts? https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:62: const TimeDelta min_delay = TimeDelta::FromMilliseconds(1); nit: You could write constexpr instead for even betterness. https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:71: // go just go back to wait. "to just go" https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:73: delay = delayed_work_time_ - TimeTicks::Now(); IIRC Now() is quite expensive on some windows systems, are those all deprecated? Or are there cases this could be worse?
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) On 2016/06/22 22:26:55, danakj wrote: > I am wondering about your choice to put this change here in an #ifdef of in > waitable_event_win.cc. Should this not apply to other uses of the method? > > I see TimedWait is allowed to return early if it returns false, so maybe not > that part? But the min_delay seems more general. > > Any thoughts? Yeah, would it make sense to use max_time.InMillisecondsRoundedUp() in WaitableEvent::TimedWait()?
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) On 2016/06/22 22:26:55, danakj wrote: > I am wondering about your choice to put this change here in an #ifdef of in > waitable_event_win.cc. Should this not apply to other uses of the method? > > I see TimedWait is allowed to return early if it returns false, so maybe not > that part? But the min_delay seems more general. > > Any thoughts? I considered that but wanted to minimize the impact. I agree the fix inside TimedWait would be cleaner. I'll try that and run through the lab to see if there are any issues. https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:73: delay = delayed_work_time_ - TimeTicks::Now(); On 2016/06/22 22:26:55, danakj wrote: > IIRC Now() is quite expensive on some windows systems, are those all deprecated? > Or are there cases this could be worse? Good question! If we don't do this inner loop with Now(), we'd go through the outer loop which would still call Now() inside MessageLoop::DoDelayedWork (assuming there is at least one future delayed task). In addition to that the outer loop would go into MessageLoop::DoWork and attempt to reload work queue (which involves acquiring a lock) and do a bit of other work. So there is more work overall. If we fix the spinning issue above the difference might be small enough to not care. Also if we fix the minimum delay inside waitable_timer_win.cc as you suggested above then perhaps no changes would be needed in this file at all.
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) That would require evaluating all uses of TimedWait() to make sure that rounding up is appropriate. This is the only place where we have seen the problematic spinning so this is the minimum change to fix the spinning. InMillisecondsRoundedUp() does sound like an interesting alternate way of implementing this. https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:73: delay = delayed_work_time_ - TimeTicks::Now(); I don't think Now() is ever particularly expensive? It's sometimes inaccurate. RolloverProtectedNow just calls the tick function, which is cheap. QPCNow has a variable cost depending on whether it uses rdtsc or the timer chip (I believe) but I don't know that it is ever expensive enough to not make this check appropriate. The times when I've seen it on a profile have all been because we were spinning in a loop calling it.
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) On 2016/06/22 23:41:51, brucedawson wrote: > That would require evaluating all uses of TimedWait() to make sure that rounding > up is appropriate. This is the only place where we have seen the problematic > spinning so this is the minimum change to fix the spinning. > > InMillisecondsRoundedUp() does sound like an interesting alternate way of > implementing this. Hm, I guess sometimes people would want to not wait for 1ms when their delay is less. Can this comment be more explicit that this may make us wait too long, but we're doing this to ensure chrome goes idle at least once when there is some delay present? https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:69: // Windows. It doesn't make sense to run the outer loop in that case What part of this is windows-specific, since TimedWait() can return false on any platform? Do other platforms need to make a loop thru everything when it was false/early return? https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:73: delay = delayed_work_time_ - TimeTicks::Now(); On 2016/06/22 23:41:51, brucedawson wrote: > I don't think Now() is ever particularly expensive? It's sometimes inaccurate. > > RolloverProtectedNow just calls the tick function, which is cheap. > > QPCNow has a variable cost depending on whether it uses rdtsc or the timer chip > (I believe) but I don't know that it is ever expensive enough to not make this > check appropriate. > > The times when I've seen it on a profile have all been because we were spinning > in a loop calling it. Interesting, I recall in compositor land we went to some work to try avoiding calling Now() as much, due to perceived costs. I see we would circle around and call Now() anyways so this is strictly better tho, cool.
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:69: // Windows. It doesn't make sense to run the outer loop in that case The Windows wait/sleep functions all take millisecond times in integers, thus forcing the delay to be rounded down, and it is that rounding down that this change is working around. I don't think other platforms have this unfortunate behavior. I would love to have nanosleep on Windows, but alas...
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:69: // Windows. It doesn't make sense to run the outer loop in that case On 2016/06/23 00:01:12, brucedawson wrote: > The Windows wait/sleep functions all take millisecond times in integers, thus > forcing the delay to be rounded down, and it is that rounding down that this > change is working around. > > I don't think other platforms have this unfortunate behavior. > > I would love to have nanosleep on Windows, but alas... That makes the min_delay windows specific. But what if the delay was > 1 millisecond, but TimedWait returned false? That can happen anywhere, I think.
https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:59: #if defined(OS_WIN) On 2016/06/22 23:52:59, danakj wrote: > On 2016/06/22 23:41:51, brucedawson wrote: > > That would require evaluating all uses of TimedWait() to make sure that > rounding > > up is appropriate. This is the only place where we have seen the problematic > > spinning so this is the minimum change to fix the spinning. > > > > InMillisecondsRoundedUp() does sound like an interesting alternate way of > > implementing this. > > Hm, I guess sometimes people would want to not wait for 1ms when their delay is > less. Can this comment be more explicit that this may make us wait too long, but > we're doing this to ensure chrome goes idle at least once when there is some > delay present? The behavior of timed wait depends on platform. On Windows 7 the wait times out earlier almost always, but on Windows 8 or Windows 10 it very often takes an extra millisecond to time out. If we round up unconditionally then we might end up waiting up to 2 extra milliseconds. I think the better approach is to round down, and then if the wait times out early do another short wait. The only exception would be a sub-millisecond wait where we want to always round up. This kind of logic is easy to implement in the message pump - that's what my change essentially does.
I've addressed a couple of issues and improved the comment around the |min_delay|. Separately I've also tried an alternative fix inside TimedWait implementation -- see https://codereview.chromium.org/2091613002/. That seems to not cause any obvious issues. But I think it is still more risky than a localized change in MessagePumpDefault. https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:62: const TimeDelta min_delay = TimeDelta::FromMilliseconds(1); On 2016/06/22 22:26:55, danakj wrote: > nit: You could write constexpr instead for even betterness. Done. https://codereview.chromium.org/2086123002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_default.cc:71: // go just go back to wait. On 2016/06/22 22:26:55, danakj wrote: > "to just go" Done.
https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... base/message_loop/message_pump_default.cc:66: do { Thanks. One last question I had, that I think might have got lost: I can see why the min_delay is OS_WIN specific. But why is this do{}while loop only on windows? TimedWait can return early (with false) on any platform it seems.
https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... base/message_loop/message_pump_default.cc:66: do { On 2016/06/23 22:08:29, danakj wrote: > Thanks. One last question I had, that I think might have got lost: > > I can see why the min_delay is OS_WIN specific. But why is this do{}while loop > only on windows? TimedWait can return early (with false) on any platform it > seems. I looked at waitable_event_posix.cc and it has for(;;) loop that seems to ensure current_time >= end_time condition for the time out. So it shouldn't exit the wait early. The posix implementation doesn't have the problem with the millisecond granularity and sub-millisecond wait. That is entirely Windows specific issue. So yes, that might be another reason to move this entire change, including the nested loop, inside waitable_event_win.cc. Agree?
https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... File base/message_loop/message_pump_default.cc (right): https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... base/message_loop/message_pump_default.cc:66: do { On 2016/06/24 01:32:30, stanisc wrote: > On 2016/06/23 22:08:29, danakj wrote: > > Thanks. One last question I had, that I think might have got lost: > > > > I can see why the min_delay is OS_WIN specific. But why is this do{}while loop > > only on windows? TimedWait can return early (with false) on any platform it > > seems. > > I looked at waitable_event_posix.cc and it has for(;;) loop that seems to ensure > current_time >= end_time condition for the time out. So it shouldn't exit the > wait early. The posix implementation doesn't have the problem with the > millisecond granularity and sub-millisecond wait. That is entirely Windows > specific issue. > > So yes, that might be another reason to move this entire change, including the > nested loop, inside waitable_event_win.cc. Agree? So, TimedWait() could stop returning bool at that point. I did see a return false is possible in the posix implementation tho. But it's if the sw.fired() was false but current time >= end time. Which seems like it should be true.. am I reading that right? https://cs.chromium.org/chromium/src/base/synchronization/waitable_event_posi... Would you be up for staging this in a few patches? There are only 39 calls to TimedWait I think, outside of tests, but we should be careful. We could land this as a first step (and DCHECK that delay <= TimeDelta() in non-windows? Then we could make sure the posix implementation always returns true. Then we could move this into the windows implementation and make it return true. Then we can make the return type void. That should give us some good feedback along each step. WDYT?
On 2016/06/24 19:53:44, danakj wrote: > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > File base/message_loop/message_pump_default.cc (right): > > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > base/message_loop/message_pump_default.cc:66: do { > On 2016/06/24 01:32:30, stanisc wrote: > > On 2016/06/23 22:08:29, danakj wrote: > > > Thanks. One last question I had, that I think might have got lost: > > > > > > I can see why the min_delay is OS_WIN specific. But why is this do{}while > loop > > > only on windows? TimedWait can return early (with false) on any platform it > > > seems. > > > > I looked at waitable_event_posix.cc and it has for(;;) loop that seems to > ensure > > current_time >= end_time condition for the time out. So it shouldn't exit the > > wait early. The posix implementation doesn't have the problem with the > > millisecond granularity and sub-millisecond wait. That is entirely Windows > > specific issue. > > > > So yes, that might be another reason to move this entire change, including the > > nested loop, inside waitable_event_win.cc. Agree? > > So, TimedWait() could stop returning bool at that point. I did see a return > false is possible in the posix implementation tho. But it's if the sw.fired() > was false but current time >= end time. Which seems like it should be true.. am > I reading that right? > https://cs.chromium.org/chromium/src/base/synchronization/waitable_event_posi... > > Would you be up for staging this in a few patches? There are only 39 calls to > TimedWait I think, outside of tests, but we should be careful. > > We could land this as a first step (and DCHECK that delay <= TimeDelta() in > non-windows? > Then we could make sure the posix implementation always returns true. > Then we could move this into the windows implementation and make it return true. > Then we can make the return type void. > > That should give us some good feedback along each step. WDYT? The return value indicates whether even was signaled or not while waiting. True means signaled and false - timed out. I don't think that could be changed. If I had to stage this change, the first change I'd make is to ensure that TimedWait actually waits for sub-millisecond intervals as opposed to returning promptly. The next change could ensure that it doesn't time out sooner than max_time if the event isn't signaled - that would make it consistent with the posix implementation.
On 2016/06/24 20:54:54, stanisc wrote: > On 2016/06/24 19:53:44, danakj wrote: > > > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > > File base/message_loop/message_pump_default.cc (right): > > > > > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > > base/message_loop/message_pump_default.cc:66: do { > > On 2016/06/24 01:32:30, stanisc wrote: > > > On 2016/06/23 22:08:29, danakj wrote: > > > > Thanks. One last question I had, that I think might have got lost: > > > > > > > > I can see why the min_delay is OS_WIN specific. But why is this do{}while > > loop > > > > only on windows? TimedWait can return early (with false) on any platform > it > > > > seems. > > > > > > I looked at waitable_event_posix.cc and it has for(;;) loop that seems to > > ensure > > > current_time >= end_time condition for the time out. So it shouldn't exit > the > > > wait early. The posix implementation doesn't have the problem with the > > > millisecond granularity and sub-millisecond wait. That is entirely Windows > > > specific issue. > > > > > > So yes, that might be another reason to move this entire change, including > the > > > nested loop, inside waitable_event_win.cc. Agree? > > > > So, TimedWait() could stop returning bool at that point. I did see a return > > false is possible in the posix implementation tho. But it's if the sw.fired() > > was false but current time >= end time. Which seems like it should be true.. > am > > I reading that right? > > > https://cs.chromium.org/chromium/src/base/synchronization/waitable_event_posi... > > > > Would you be up for staging this in a few patches? There are only 39 calls to > > TimedWait I think, outside of tests, but we should be careful. > > > > We could land this as a first step (and DCHECK that delay <= TimeDelta() in > > non-windows? > > Then we could make sure the posix implementation always returns true. > > Then we could move this into the windows implementation and make it return > true. > > Then we can make the return type void. > > > > That should give us some good feedback along each step. WDYT? > > The return value indicates whether even was signaled or not while waiting. True > means signaled and false - timed out. > I don't think that could be changed. Ah right. I read "If this method returns false, then it does not necessarily mean that max_time was exceeded" wrongly. It doesn't imply that true means max_time was exceeded. I guess that part of the comment could go away then. :) > If I had to stage this change, the first change I'd make is to ensure that > TimedWait actually waits for sub-millisecond intervals as opposed to returning > promptly. > The next change could ensure that it doesn't time out sooner than max_time if > the event isn't signaled - that would make it consistent with the posix > implementation. I think that'd be nice if we can do it. So LGTM for this change as a first step.
Thanks! Just to be clear, the approval is for the current implementation, right? I can add a TODO comment and/or a separate bug for this logic to move inside waitable_event_win.cc. On Fri, Jun 24, 2016 at 2:03 PM, <danakj@chromium.org> wrote: > On 2016/06/24 20:54:54, stanisc wrote: > > On 2016/06/24 19:53:44, danakj wrote: > > > > > > > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > > > File base/message_loop/message_pump_default.cc (right): > > > > > > > > > > https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... > > > base/message_loop/message_pump_default.cc:66: do { > > > On 2016/06/24 01:32:30, stanisc wrote: > > > > On 2016/06/23 22:08:29, danakj wrote: > > > > > Thanks. One last question I had, that I think might have got lost: > > > > > > > > > > I can see why the min_delay is OS_WIN specific. But why is this > do{}while > > > loop > > > > > only on windows? TimedWait can return early (with false) on any > platform > > it > > > > > seems. > > > > > > > > I looked at waitable_event_posix.cc and it has for(;;) loop that > seems to > > > ensure > > > > current_time >= end_time condition for the time out. So it shouldn't > exit > > the > > > > wait early. The posix implementation doesn't have the problem with > the > > > > millisecond granularity and sub-millisecond wait. That is entirely > Windows > > > > specific issue. > > > > > > > > So yes, that might be another reason to move this entire change, > including > > the > > > > nested loop, inside waitable_event_win.cc. Agree? > > > > > > So, TimedWait() could stop returning bool at that point. I did see a > return > > > false is possible in the posix implementation tho. But it's if the > sw.fired() > > > was false but current time >= end time. Which seems like it should be > true.. > > am > > > I reading that right? > > > > > > > https://cs.chromium.org/chromium/src/base/synchronization/waitable_event_posi... > > > > > > Would you be up for staging this in a few patches? There are only 39 > calls > to > > > TimedWait I think, outside of tests, but we should be careful. > > > > > > We could land this as a first step (and DCHECK that delay <= > TimeDelta() in > > > non-windows? > > > Then we could make sure the posix implementation always returns true. > > > Then we could move this into the windows implementation and make it > return > > true. > > > Then we can make the return type void. > > > > > > That should give us some good feedback along each step. WDYT? > > > > The return value indicates whether even was signaled or not while > waiting. > True > > means signaled and false - timed out. > > I don't think that could be changed. > > Ah right. I read "If this method returns false, then it does not > necessarily > mean that max_time was exceeded" wrongly. It doesn't imply that true means > max_time was exceeded. I guess that part of the comment could go away > then. :) > > > > If I had to stage this change, the first change I'd make is to ensure > that > > TimedWait actually waits for sub-millisecond intervals as opposed to > returning > > promptly. > > The next change could ensure that it doesn't time out sooner than > max_time if > > the event isn't signaled - that would make it consistent with the posix > > implementation. > > I think that'd be nice if we can do it. So LGTM for this change as a first > step. > > https://codereview.chromium.org/2086123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jun 24, 2016 at 2:17 PM, Stanislav Chiknavaryan < stanisc@chromium.org> wrote: > Thanks! Just to be clear, the approval is for the current implementation, > right? > I can add a TODO comment and/or a separate bug for this logic to move > inside waitable_event_win.cc. > Yeah. Sounds good with a TODO and a bug. > > On Fri, Jun 24, 2016 at 2:03 PM, <danakj@chromium.org> wrote: > >> On 2016/06/24 20:54:54, stanisc wrote: >> > On 2016/06/24 19:53:44, danakj wrote: >> > > >> > >> >> https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... >> > > File base/message_loop/message_pump_default.cc (right): >> > > >> > > >> > >> >> https://codereview.chromium.org/2086123002/diff/40001/base/message_loop/messa... >> > > base/message_loop/message_pump_default.cc:66: do { >> > > On 2016/06/24 01:32:30, stanisc wrote: >> > > > On 2016/06/23 22:08:29, danakj wrote: >> > > > > Thanks. One last question I had, that I think might have got lost: >> > > > > >> > > > > I can see why the min_delay is OS_WIN specific. But why is this >> do{}while >> > > loop >> > > > > only on windows? TimedWait can return early (with false) on any >> platform >> > it >> > > > > seems. >> > > > >> > > > I looked at waitable_event_posix.cc and it has for(;;) loop that >> seems to >> > > ensure >> > > > current_time >= end_time condition for the time out. So it >> shouldn't exit >> > the >> > > > wait early. The posix implementation doesn't have the problem with >> the >> > > > millisecond granularity and sub-millisecond wait. That is entirely >> Windows >> > > > specific issue. >> > > > >> > > > So yes, that might be another reason to move this entire change, >> including >> > the >> > > > nested loop, inside waitable_event_win.cc. Agree? >> > > >> > > So, TimedWait() could stop returning bool at that point. I did see a >> return >> > > false is possible in the posix implementation tho. But it's if the >> sw.fired() >> > > was false but current time >= end time. Which seems like it should be >> true.. >> > am >> > > I reading that right? >> > > >> > >> >> https://cs.chromium.org/chromium/src/base/synchronization/waitable_event_posi... >> > > >> > > Would you be up for staging this in a few patches? There are only 39 >> calls >> to >> > > TimedWait I think, outside of tests, but we should be careful. >> > > >> > > We could land this as a first step (and DCHECK that delay <= >> TimeDelta() in >> > > non-windows? >> > > Then we could make sure the posix implementation always returns true. >> > > Then we could move this into the windows implementation and make it >> return >> > true. >> > > Then we can make the return type void. >> > > >> > > That should give us some good feedback along each step. WDYT? >> > >> > The return value indicates whether even was signaled or not while >> waiting. >> True >> > means signaled and false - timed out. >> > I don't think that could be changed. >> >> Ah right. I read "If this method returns false, then it does not >> necessarily >> mean that max_time was exceeded" wrongly. It doesn't imply that true means >> max_time was exceeded. I guess that part of the comment could go away >> then. :) >> >> >> > If I had to stage this change, the first change I'd make is to ensure >> that >> > TimedWait actually waits for sub-millisecond intervals as opposed to >> returning >> > promptly. >> > The next change could ensure that it doesn't time out sooner than >> max_time if >> > the event isn't signaled - that would make it consistent with the posix >> > implementation. >> >> I think that'd be nice if we can do it. So LGTM for this change as a >> first step. >> >> https://codereview.chromium.org/2086123002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2086123002/#ps60001 (title: "Added a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MessagePumpDefault can spin for up to 1 ms on Windows There are two power efficiency issues that this change is trying to address: 1) On Windows if we call event_.TimedWait(delay) with a sub-ms delay then the wait function will return promptly. This means we end up busy-waiting down to zero. Rounding these times up to 1 ms lets Chrome go idle. ETW profiles show that the spinning makes the message loop inefficient. In some cases up to 50% of all inclusive MessagePumpDefault::Run cycles are spent inside the timed wait. 2) On Windows versions 7 and above wait functions (Sleep, WaitForSingleObject, WaitForMultipleObjects, etc) time out up to 1 ms earlier. That results in iterating through the loop with no new work available (the event isn't set) and too early for delayed tasks to run. This change adds an inner loop around just event_.TimedWait(delay) that makes it go straight to wait for the remaining part of the delay should it wake up too early. This change will trigger a regression in "percentage smooth" metric of smoothness.top_25_smooth benchmark. But giving an increasing focus on power consumption (when running on batteries) it might be worth fixing this now and deal with the smoothness benchmark regression later (see below). "Percentage smooth" uses deltas between Renderer / Browser swap timestamps as an estimation for frame times and counts the percentage of frames under 17 ms. This makes the metric overly sensitive to even slight delays in scheduling of swaps which are mostly avoided in the current implementation due to busy- spinning the loop in the last millisecond of the wait. Swap times can actually move forward & backward inside a frame and still meet V-sync which the existing implementation can't tell without the actual frame statistics. I've filed crbug.com/614154 and crbug.com/614147 to track a more accurate implementation of "Percentage smooth" on Windows. BUG=487724 ========== to ========== MessagePumpDefault can spin for up to 1 ms on Windows There are two power efficiency issues that this change is trying to address: 1) On Windows if we call event_.TimedWait(delay) with a sub-ms delay then the wait function will return promptly. This means we end up busy-waiting down to zero. Rounding these times up to 1 ms lets Chrome go idle. ETW profiles show that the spinning makes the message loop inefficient. In some cases up to 50% of all inclusive MessagePumpDefault::Run cycles are spent inside the timed wait. 2) On Windows versions 7 and above wait functions (Sleep, WaitForSingleObject, WaitForMultipleObjects, etc) time out up to 1 ms earlier. That results in iterating through the loop with no new work available (the event isn't set) and too early for delayed tasks to run. This change adds an inner loop around just event_.TimedWait(delay) that makes it go straight to wait for the remaining part of the delay should it wake up too early. This change will trigger a regression in "percentage smooth" metric of smoothness.top_25_smooth benchmark. But giving an increasing focus on power consumption (when running on batteries) it might be worth fixing this now and deal with the smoothness benchmark regression later (see below). "Percentage smooth" uses deltas between Renderer / Browser swap timestamps as an estimation for frame times and counts the percentage of frames under 17 ms. This makes the metric overly sensitive to even slight delays in scheduling of swaps which are mostly avoided in the current implementation due to busy- spinning the loop in the last millisecond of the wait. Swap times can actually move forward & backward inside a frame and still meet V-sync which the existing implementation can't tell without the actual frame statistics. I've filed crbug.com/614154 and crbug.com/614147 to track a more accurate implementation of "Percentage smooth" on Windows. BUG=487724 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MessagePumpDefault can spin for up to 1 ms on Windows There are two power efficiency issues that this change is trying to address: 1) On Windows if we call event_.TimedWait(delay) with a sub-ms delay then the wait function will return promptly. This means we end up busy-waiting down to zero. Rounding these times up to 1 ms lets Chrome go idle. ETW profiles show that the spinning makes the message loop inefficient. In some cases up to 50% of all inclusive MessagePumpDefault::Run cycles are spent inside the timed wait. 2) On Windows versions 7 and above wait functions (Sleep, WaitForSingleObject, WaitForMultipleObjects, etc) time out up to 1 ms earlier. That results in iterating through the loop with no new work available (the event isn't set) and too early for delayed tasks to run. This change adds an inner loop around just event_.TimedWait(delay) that makes it go straight to wait for the remaining part of the delay should it wake up too early. This change will trigger a regression in "percentage smooth" metric of smoothness.top_25_smooth benchmark. But giving an increasing focus on power consumption (when running on batteries) it might be worth fixing this now and deal with the smoothness benchmark regression later (see below). "Percentage smooth" uses deltas between Renderer / Browser swap timestamps as an estimation for frame times and counts the percentage of frames under 17 ms. This makes the metric overly sensitive to even slight delays in scheduling of swaps which are mostly avoided in the current implementation due to busy- spinning the loop in the last millisecond of the wait. Swap times can actually move forward & backward inside a frame and still meet V-sync which the existing implementation can't tell without the actual frame statistics. I've filed crbug.com/614154 and crbug.com/614147 to track a more accurate implementation of "Percentage smooth" on Windows. BUG=487724 ========== to ========== MessagePumpDefault can spin for up to 1 ms on Windows There are two power efficiency issues that this change is trying to address: 1) On Windows if we call event_.TimedWait(delay) with a sub-ms delay then the wait function will return promptly. This means we end up busy-waiting down to zero. Rounding these times up to 1 ms lets Chrome go idle. ETW profiles show that the spinning makes the message loop inefficient. In some cases up to 50% of all inclusive MessagePumpDefault::Run cycles are spent inside the timed wait. 2) On Windows versions 7 and above wait functions (Sleep, WaitForSingleObject, WaitForMultipleObjects, etc) time out up to 1 ms earlier. That results in iterating through the loop with no new work available (the event isn't set) and too early for delayed tasks to run. This change adds an inner loop around just event_.TimedWait(delay) that makes it go straight to wait for the remaining part of the delay should it wake up too early. This change will trigger a regression in "percentage smooth" metric of smoothness.top_25_smooth benchmark. But giving an increasing focus on power consumption (when running on batteries) it might be worth fixing this now and deal with the smoothness benchmark regression later (see below). "Percentage smooth" uses deltas between Renderer / Browser swap timestamps as an estimation for frame times and counts the percentage of frames under 17 ms. This makes the metric overly sensitive to even slight delays in scheduling of swaps which are mostly avoided in the current implementation due to busy- spinning the loop in the last millisecond of the wait. Swap times can actually move forward & backward inside a frame and still meet V-sync which the existing implementation can't tell without the actual frame statistics. I've filed crbug.com/614154 and crbug.com/614147 to track a more accurate implementation of "Percentage smooth" on Windows. BUG=487724 Committed: https://crrev.com/56ee6be040caa993583170bd5edc472e7b969c9e Cr-Commit-Position: refs/heads/master@{#402027} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/56ee6be040caa993583170bd5edc472e7b969c9e Cr-Commit-Position: refs/heads/master@{#402027} |
