Revert of High resolution timer fix for Windows (https://codereview.chromium.org/395913006/)
Reason for revert:
This patch seems to make following browser_tests flakey
PPAPINaClNewlibTest.Graphics2D_FlushOffscreenUpdate
NetInternalsTest.netInternalsHSTSViewAddOverwrite
NetInternalsTest.netInternalsHSTSViewAddDelete
NetInternalsTest.netInternalsHSTSViewAddTwice
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds/34734
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/32086
http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/builds/47545
Original issue's description:
> This is jamesr@ code I am landing.
>
> On Windows the message pump code tried to manage the systemwide timer resolution to fire delayed tasks with better than 15ms resolution but it was buggy:
>
> 1- A short task that was not followed by any other task will leave the systemwide timer pegged to 1ms
>
> 2- After we decided to crank up the timer we would 'lease' the timer for 1 second, for no good reason.
>
> Both issues are detrimental to battery power.
>
> The source of both problems is that we tried to decide with incomplete information. This patch solves that by having 1 bit for each pending task that requires a high resolution timer and a sum of the number of tasks that require high res timers.
>
> BUG=153139
> TEST=included here, also see the bug for manual testing.
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284625
TBR=jamesr@chromium.org,darin@chromium.org,cpu@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=153139
Review URL: https://codereview.chromium.org/407073004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284664 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 318717747560914c8052a2d072d20d0542c16fb7
diff --git a/base/message_loop/incoming_task_queue.cc b/base/message_loop/incoming_task_queue.cc
index 48c2f42..bcc712b 100644
--- a/base/message_loop/incoming_task_queue.cc
+++ b/base/message_loop/incoming_task_queue.cc
@@ -8,14 +8,12 @@
#include "base/location.h"
#include "base/message_loop/message_loop.h"
#include "base/synchronization/waitable_event.h"
-#include "base/time/time.h"
namespace base {
namespace internal {
IncomingTaskQueue::IncomingTaskQueue(MessageLoop* message_loop)
- : high_res_task_count_(0),
- message_loop_(message_loop),
+ : message_loop_(message_loop),
next_sequence_num_(0) {
}
@@ -27,23 +25,15 @@
AutoLock locked(incoming_queue_lock_);
PendingTask pending_task(
from_here, task, CalculateDelayedRuntime(delay), nestable);
-#if defined(OS_WIN)
- // We consider the task needs a high resolution timer if the delay is
- // more than 0 and less than 32ms. This caps the relative error to
- // less than 50% : a 33ms wait can wake at 48ms since the default
- // resolution on Windows is between 10 and 15ms.
- if (delay > TimeDelta() &&
- delay.InMilliseconds() < (2 * Time::kMinLowResolutionThresholdMs)) {
- ++high_res_task_count_;
- pending_task.is_high_res = true;
- }
-#endif
return PostPendingTask(&pending_task);
}
-bool IncomingTaskQueue::HasHighResolutionTasks() {
- AutoLock lock(incoming_queue_lock_);
- return high_res_task_count_ > 0;
+bool IncomingTaskQueue::IsHighResolutionTimerEnabledForTesting() {
+#if defined(OS_WIN)
+ return !high_resolution_timer_expiration_.is_null();
+#else
+ return true;
+#endif
}
bool IncomingTaskQueue::IsIdleForTesting() {
@@ -51,22 +41,29 @@
return incoming_queue_.empty();
}
-int IncomingTaskQueue::ReloadWorkQueue(TaskQueue* work_queue) {
+void IncomingTaskQueue::ReloadWorkQueue(TaskQueue* work_queue) {
// Make sure no tasks are lost.
DCHECK(work_queue->empty());
// Acquire all we can from the inter-thread queue with one lock acquisition.
AutoLock lock(incoming_queue_lock_);
if (!incoming_queue_.empty())
- incoming_queue_.Swap(work_queue);
+ incoming_queue_.Swap(work_queue); // Constant time
- // Reset the count of high resolution tasks since our queue is now empty.
- int high_res_tasks = high_res_task_count_;
- high_res_task_count_ = 0;
- return high_res_tasks;
+ DCHECK(incoming_queue_.empty());
}
void IncomingTaskQueue::WillDestroyCurrentMessageLoop() {
+#if defined(OS_WIN)
+ // If we left the high-resolution timer activated, deactivate it now.
+ // Doing this is not-critical, it is mainly to make sure we track
+ // the high resolution timer activations properly in our unit tests.
+ if (!high_resolution_timer_expiration_.is_null()) {
+ Time::ActivateHighResolutionTimer(false);
+ high_resolution_timer_expiration_ = TimeTicks();
+ }
+#endif
+
AutoLock lock(incoming_queue_lock_);
message_loop_ = NULL;
}
@@ -78,10 +75,40 @@
TimeTicks IncomingTaskQueue::CalculateDelayedRuntime(TimeDelta delay) {
TimeTicks delayed_run_time;
- if (delay > TimeDelta())
+ if (delay > TimeDelta()) {
delayed_run_time = TimeTicks::Now() + delay;
- else
+
+#if defined(OS_WIN)
+ if (high_resolution_timer_expiration_.is_null()) {
+ // Windows timers are granular to 15.6ms. If we only set high-res
+ // timers for those under 15.6ms, then a 18ms timer ticks at ~32ms,
+ // which as a percentage is pretty inaccurate. So enable high
+ // res timers for any timer which is within 2x of the granularity.
+ // This is a tradeoff between accuracy and power management.
+ bool needs_high_res_timers = delay.InMilliseconds() <
+ (2 * Time::kMinLowResolutionThresholdMs);
+ if (needs_high_res_timers) {
+ if (Time::ActivateHighResolutionTimer(true)) {
+ high_resolution_timer_expiration_ = TimeTicks::Now() +
+ TimeDelta::FromMilliseconds(
+ MessageLoop::kHighResolutionTimerModeLeaseTimeMs);
+ }
+ }
+ }
+#endif
+ } else {
DCHECK_EQ(delay.InMilliseconds(), 0) << "delay should not be negative";
+ }
+
+#if defined(OS_WIN)
+ if (!high_resolution_timer_expiration_.is_null()) {
+ if (TimeTicks::Now() > high_resolution_timer_expiration_) {
+ Time::ActivateHighResolutionTimer(false);
+ high_resolution_timer_expiration_ = TimeTicks();
+ }
+ }
+#endif
+
return delayed_run_time;
}
diff --git a/base/message_loop/incoming_task_queue.h b/base/message_loop/incoming_task_queue.h
index 30f2603..56c5638 100644
--- a/base/message_loop/incoming_task_queue.h
+++ b/base/message_loop/incoming_task_queue.h
@@ -38,17 +38,16 @@
TimeDelta delay,
bool nestable);
- // Returns true if the queue contains tasks that require higher than default
- // timer resolution. Currently only needed for Windows.
- bool HasHighResolutionTasks();
+ // Returns true if the message loop has high resolution timers enabled.
+ // Provided for testing.
+ bool IsHighResolutionTimerEnabledForTesting();
// Returns true if the message loop is "idle". Provided for testing.
bool IsIdleForTesting();
// Loads tasks from the |incoming_queue_| into |*work_queue|. Must be called
- // from the thread that is running the loop. Returns the number of tasks that
- // require high resolution timers.
- int ReloadWorkQueue(TaskQueue* work_queue);
+ // from the thread that is running the loop.
+ void ReloadWorkQueue(TaskQueue* work_queue);
// Disconnects |this| from the parent message loop.
void WillDestroyCurrentMessageLoop();
@@ -66,11 +65,12 @@
// does not retain |pending_task->task| beyond this function call.
bool PostPendingTask(PendingTask* pending_task);
- // Number of tasks that require high resolution timing. This value is kept
- // so that ReloadWorkQueue() completes in constant time.
- int high_res_task_count_;
+#if defined(OS_WIN)
+ TimeTicks high_resolution_timer_expiration_;
+#endif
- // The lock that protects access to the members of this class.
+ // The lock that protects access to |incoming_queue_|, |message_loop_| and
+ // |next_sequence_num_|.
base::Lock incoming_queue_lock_;
// An incoming queue of tasks that are acquired under a mutex for processing
diff --git a/base/message_loop/message_loop.cc b/base/message_loop/message_loop.cc
index 2d05ef1..a7217eb 100644
--- a/base/message_loop/message_loop.cc
+++ b/base/message_loop/message_loop.cc
@@ -127,8 +127,6 @@
MessageLoop::MessageLoop(Type type)
: type_(type),
- pending_high_res_tasks_(0),
- in_high_res_mode_(false),
nestable_tasks_allowed_(true),
#if defined(OS_WIN)
os_modal_loop_(false),
@@ -143,8 +141,6 @@
MessageLoop::MessageLoop(scoped_ptr<MessagePump> pump)
: pump_(pump.Pass()),
type_(TYPE_CUSTOM),
- pending_high_res_tasks_(0),
- in_high_res_mode_(false),
nestable_tasks_allowed_(true),
#if defined(OS_WIN)
os_modal_loop_(false),
@@ -157,11 +153,8 @@
MessageLoop::~MessageLoop() {
DCHECK_EQ(this, current());
+
DCHECK(!run_loop_);
-#if defined(OS_WIN)
- if (in_high_res_mode_)
- Time::ActivateHighResolutionTimer(false);
-#endif
// Clean up any unprocessed tasks, but take care: deleting a task could
// result in the addition of more tasks (e.g., via DeleteSoon). We set a
@@ -376,8 +369,8 @@
return run_loop_ != NULL;
}
-bool MessageLoop::HasHighResolutionTasks() {
- return incoming_task_queue_->HasHighResolutionTasks();
+bool MessageLoop::IsHighResolutionTimerEnabledForTesting() {
+ return incoming_task_queue_->IsHighResolutionTimerEnabledForTesting();
}
bool MessageLoop::IsIdleForTesting() {
@@ -445,11 +438,6 @@
"src_file", pending_task.posted_from.file_name(),
"src_func", pending_task.posted_from.function_name());
- if (pending_task.is_high_res) {
- pending_high_res_tasks_--;
- CHECK(pending_high_res_tasks_ >= 0);
- }
-
DCHECK(nestable_tasks_allowed_);
// Execute the task and assume the worst: It is probably not reentrant.
nestable_tasks_allowed_ = false;
@@ -535,10 +523,8 @@
// |*work_queue| by waiting until the last minute (|*work_queue| is empty) to
// load. That reduces the number of locks-per-task significantly when our
// queues get large.
- if (work_queue_.empty()) {
- pending_high_res_tasks_ +=
- incoming_task_queue_->ReloadWorkQueue(&work_queue_);
- }
+ if (work_queue_.empty())
+ incoming_task_queue_->ReloadWorkQueue(&work_queue_);
}
void MessageLoop::ScheduleWork(bool was_empty) {
@@ -643,14 +629,6 @@
if (run_loop_->quit_when_idle_received_)
pump_->Quit();
- // We will now do a kernel wait for more tasks.
-#if defined(OS_WIN)
- // The Windows scheduler has by default ~15ms resolution. If we have high
- // resolution tasks pending we need to temporarity increase the systemwide
- // timer resolution to 1ms, and if we don't we need to go back to 15ms.
- in_high_res_mode_ = pending_high_res_tasks_ > 0;
- Time::ActivateHighResolutionTimer(in_high_res_mode_);
-#endif
return false;
}
diff --git a/base/message_loop/message_loop.h b/base/message_loop/message_loop.h
index d1e5790..a281435 100644
--- a/base/message_loop/message_loop.h
+++ b/base/message_loop/message_loop.h
@@ -371,6 +371,10 @@
void AddTaskObserver(TaskObserver* task_observer);
void RemoveTaskObserver(TaskObserver* task_observer);
+ // When we go into high resolution timer mode, we will stay in hi-res mode
+ // for at least 1s.
+ static const int kHighResolutionTimerModeLeaseTimeMs = 1000;
+
#if defined(OS_WIN)
void set_os_modal_loop(bool os_modal_loop) {
os_modal_loop_ = os_modal_loop;
@@ -386,7 +390,7 @@
// Returns true if the message loop has high resolution timers enabled.
// Provided for testing.
- bool HasHighResolutionTasks();
+ bool IsHighResolutionTimerEnabledForTesting();
// Returns true if the message loop is "idle". Provided for testing.
bool IsIdleForTesting();
@@ -456,14 +460,6 @@
// this queue is only accessed (push/pop) by our current thread.
TaskQueue work_queue_;
- // How many high resolution tasks are in the pending task queue. This value
- // increases by N every time we call ReloadWorkQueue() and decreases by 1
- // every time we call RunTask() if the task needs a high resolution timer.
- int pending_high_res_tasks_;
- // Tracks if we have requested high resolution timers. Its only use is to
- // turn off the high resolution timer upon loop destruction.
- bool in_high_res_mode_;
-
// Contains delayed tasks, sorted by their 'delayed_run_time' property.
DelayedTaskQueue delayed_work_queue_;
diff --git a/base/message_loop/message_loop_unittest.cc b/base/message_loop/message_loop_unittest.cc
index 866c20b..1b09eb0 100644
--- a/base/message_loop/message_loop_unittest.cc
+++ b/base/message_loop/message_loop_unittest.cc
@@ -730,20 +730,30 @@
const TimeDelta kFastTimer = TimeDelta::FromMilliseconds(5);
const TimeDelta kSlowTimer = TimeDelta::FromMilliseconds(100);
- EXPECT_FALSE(loop.HasHighResolutionTasks());
+ EXPECT_FALSE(loop.IsHighResolutionTimerEnabledForTesting());
+
// Post a fast task to enable the high resolution timers.
loop.PostDelayedTask(FROM_HERE, Bind(&PostNTasksThenQuit, 1),
kFastTimer);
- EXPECT_TRUE(loop.HasHighResolutionTasks());
loop.Run();
- EXPECT_FALSE(loop.HasHighResolutionTasks());
- EXPECT_FALSE(Time::IsHighResolutionTimerInUse());
- // Check that a slow task does not trigger the high resolution logic.
+ EXPECT_TRUE(loop.IsHighResolutionTimerEnabledForTesting());
+
+ // Post a slow task and verify high resolution timers
+ // are still enabled.
loop.PostDelayedTask(FROM_HERE, Bind(&PostNTasksThenQuit, 1),
kSlowTimer);
- EXPECT_FALSE(loop.HasHighResolutionTasks());
loop.Run();
- EXPECT_FALSE(loop.HasHighResolutionTasks());
+ EXPECT_TRUE(loop.IsHighResolutionTimerEnabledForTesting());
+
+ // Wait for a while so that high-resolution mode elapses.
+ PlatformThread::Sleep(TimeDelta::FromMilliseconds(
+ MessageLoop::kHighResolutionTimerModeLeaseTimeMs));
+
+ // Post a slow task to disable the high resolution timers.
+ loop.PostDelayedTask(FROM_HERE, Bind(&PostNTasksThenQuit, 1),
+ kSlowTimer);
+ loop.Run();
+ EXPECT_FALSE(loop.IsHighResolutionTimerEnabledForTesting());
}
#endif // defined(OS_WIN)
diff --git a/base/message_loop/message_pump.h b/base/message_loop/message_pump.h
index a2edb45..4dc501b 100644
--- a/base/message_loop/message_pump.h
+++ b/base/message_loop/message_pump.h
@@ -39,8 +39,7 @@
virtual bool DoDelayedWork(TimeTicks* next_delayed_work_time) = 0;
// Called from within Run just before the message pump goes to sleep.
- // Returns true to indicate that idle work was done. Returning false means
- // the pump will now wait.
+ // Returns true to indicate that idle work was done.
virtual bool DoIdleWork() = 0;
};