cc: Fix and simplify DelayBasedTimeSource

- This converts some floating point math to integer math
to avoid rounding errors.

- Assigns last_tick_time_ to be the time it was *supposed*
to tick and no longer uses it as a synonym for "now".

- Adds trace events to help debug when the interval or
phase changes.

BUG=256650

Review URL: https://chromiumcodereview.appspot.com/18589002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@210101 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 38 insertions, 64 deletions

diff --git a/cc/scheduler/delay_based_time_source.cc b/cc/scheduler/delay_based_time_source.cc
index 5d2f20b..6038a3b 100644
--- a/cc/scheduler/delay_based_time_source.cc
+++ b/cc/scheduler/delay_based_time_source.cc
@@ -15,10 +15,10 @@ namespace cc {
 
 namespace {
 
-// kDoubleTickThreshold prevents ticks from running within the specified
+// kDoubleTickDivisor prevents ticks from running within the specified
 // fraction of an interval.  This helps account for jitter in the timebase as
 // well as quick timer reactivation.
-static const double kDoubleTickThreshold = 0.25;
+static const int kDoubleTickDivisor = 4;
 
 // kIntervalChangeThreshold is the fraction of the interval that will trigger an
 // immediate interval change.  kPhaseChangeThreshold is the fraction of the
@@ -40,10 +40,10 @@ scoped_refptr<DelayBasedTimeSource> DelayBasedTimeSource::Create(
 DelayBasedTimeSource::DelayBasedTimeSource(
     base::TimeDelta interval, base::SingleThreadTaskRunner* task_runner)
     : client_(NULL),
-      has_tick_target_(false),
+      last_tick_time_(base::TimeTicks() - interval),
       current_parameters_(interval, base::TimeTicks()),
       next_parameters_(interval, base::TimeTicks()),
-      state_(STATE_INACTIVE),
+      active_(false),
       task_runner_(task_runner),
       weak_factory_(this) {}
 
@@ -51,32 +51,19 @@ DelayBasedTimeSource::~DelayBasedTimeSource() {}
 
 void DelayBasedTimeSource::SetActive(bool active) {
   TRACE_EVENT1("cc", "DelayBasedTimeSource::SetActive", "active", active);
-  if (!active) {
-    state_ = STATE_INACTIVE;
-    weak_factory_.InvalidateWeakPtrs();
+  if (active == active_)
     return;
-  }
+  active_ = active;
 
-  if (state_ == STATE_STARTING || state_ == STATE_ACTIVE)
-    return;
-
-  if (!has_tick_target_) {
-    // Becoming active the first time is deferred: we post a 0-delay task.
-    // When it runs, we use that to establish the timebase, become truly
-    // active, and fire the first tick.
-    state_ = STATE_STARTING;
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&DelayBasedTimeSource::OnTimerFired,
-                                      weak_factory_.GetWeakPtr()));
+  if (!active_) {
+    weak_factory_.InvalidateWeakPtrs();
     return;
   }
 
-  state_ = STATE_ACTIVE;
-
   PostNextTickTask(Now());
 }
 
-bool DelayBasedTimeSource::Active() const { return state_ != STATE_INACTIVE; }
+bool DelayBasedTimeSource::Active() const { return active_; }
 
 base::TimeTicks DelayBasedTimeSource::LastTickTime() { return last_tick_time_; }
 
@@ -85,17 +72,11 @@ base::TimeTicks DelayBasedTimeSource::NextTickTime() {
 }
 
 void DelayBasedTimeSource::OnTimerFired() {
-  DCHECK(state_ != STATE_INACTIVE);
-
-  base::TimeTicks now = this->Now();
-  last_tick_time_ = now;
+  DCHECK(active_);
 
-  if (state_ == STATE_STARTING) {
-    SetTimebaseAndInterval(now, current_parameters_.interval);
-    state_ = STATE_ACTIVE;
-  }
+  last_tick_time_ = current_parameters_.tick_target;
 
-  PostNextTickTask(now);
+  PostNextTickTask(Now());
 
   // Fire the tick.
   if (client_)
@@ -110,9 +91,8 @@ void DelayBasedTimeSource::SetTimebaseAndInterval(base::TimeTicks timebase,
                                                   base::TimeDelta interval) {
   next_parameters_.interval = interval;
   next_parameters_.tick_target = timebase;
-  has_tick_target_ = true;
 
-  if (state_ != STATE_ACTIVE) {
+  if (!active_) {
     // If we aren't active, there's no need to reset the timer.
     return;
   }
@@ -123,6 +103,8 @@ void DelayBasedTimeSource::SetTimebaseAndInterval(base::TimeTicks timebase,
       std::abs((interval - current_parameters_.interval).InSecondsF());
   double interval_change = interval_delta / interval.InSecondsF();
   if (interval_change > kIntervalChangeThreshold) {
+    TRACE_EVENT_INSTANT0("cc", "DelayBasedTimeSource::IntervalChanged",
+                         TRACE_EVENT_SCOPE_THREAD);
     SetActive(false);
     SetActive(true);
     return;
@@ -140,6 +122,8 @@ void DelayBasedTimeSource::SetTimebaseAndInterval(base::TimeTicks timebase,
       fmod(target_delta, interval.InSecondsF()) / interval.InSecondsF();
   if (phase_change > kPhaseChangeThreshold &&
       phase_change < (1.0 - kPhaseChangeThreshold)) {
+    TRACE_EVENT_INSTANT0("cc", "DelayBasedTimeSource::PhaseChanged",
+                         TRACE_EVENT_SCOPE_THREAD);
     SetActive(false);
     SetActive(true);
     return;
@@ -205,26 +189,24 @@ base::TimeTicks DelayBasedTimeSource::Now() const {
 //      now=37   tick_target=16.667  new_target=50.000  -->
 //          tick(), PostDelayedTask(floor(50.000-37)) --> PostDelayedTask(13)
 base::TimeTicks DelayBasedTimeSource::NextTickTarget(base::TimeTicks now) {
+  const base::TimeDelta epsilon(base::TimeDelta::FromMicroseconds(1));
   base::TimeDelta new_interval = next_parameters_.interval;
   int intervals_elapsed =
-      static_cast<int>(floor((now - next_parameters_.tick_target).InSecondsF() /
-                             new_interval.InSecondsF()));
-  base::TimeTicks last_effective_tick =
+      (now - next_parameters_.tick_target + new_interval - epsilon) /
+      new_interval;
+  base::TimeTicks new_tick_target =
       next_parameters_.tick_target + new_interval * intervals_elapsed;
-  base::TimeTicks new_tick_target = last_effective_tick + new_interval;
-  DCHECK(now < new_tick_target)
+  DCHECK(now <= new_tick_target)
       << "now = " << now.ToInternalValue()
       << "; new_tick_target = " << new_tick_target.ToInternalValue()
       << "; new_interval = " << new_interval.InMicroseconds()
       << "; tick_target = " << next_parameters_.tick_target.ToInternalValue()
-      << "; intervals_elapsed = " << intervals_elapsed
-      << "; last_effective_tick = " << last_effective_tick.ToInternalValue();
+      << "; intervals_elapsed = " << intervals_elapsed;
 
   // Avoid double ticks when:
   // 1) Turning off the timer and turning it right back on.
   // 2) Jittery data is passed to SetTimebaseAndInterval().
-  if (new_tick_target - last_tick_time_ <=
-      new_interval / static_cast<int>(1.0 / kDoubleTickThreshold))
+  if (new_tick_target - last_tick_time_ <= new_interval / kDoubleTickDivisor)
     new_tick_target += new_interval;
 
   return new_tick_target;
@@ -234,10 +216,9 @@ void DelayBasedTimeSource::PostNextTickTask(base::TimeTicks now) {
   base::TimeTicks new_tick_target = NextTickTarget(now);
 
   // Post another task *before* the tick and update state
-  base::TimeDelta delay = new_tick_target - now;
-  DCHECK(delay.InMillisecondsF() <=
-         next_parameters_.interval.InMillisecondsF() *
-         (1.0 + kDoubleTickThreshold));
+  base::TimeDelta delay;
+  if (now <= new_tick_target)
+    delay = new_tick_target - now;
   task_runner_->PostDelayedTask(FROM_HERE,
                                 base::Bind(&DelayBasedTimeSource::OnTimerFired,
                                            weak_factory_.GetWeakPtr()),
diff --git a/cc/scheduler/delay_based_time_source.h b/cc/scheduler/delay_based_time_source.h
index 1dc4d6f..009b9a3 100644
--- a/cc/scheduler/delay_based_time_source.h
+++ b/cc/scheduler/delay_based_time_source.h
@@ -47,12 +47,6 @@ class CC_EXPORT DelayBasedTimeSource : public TimeSource {
   void PostNextTickTask(base::TimeTicks now);
   void OnTimerFired();
 
-  enum State {
-    STATE_INACTIVE,
-    STATE_STARTING,
-    STATE_ACTIVE,
-  };
-
   struct Parameters {
     Parameters(base::TimeDelta interval, base::TimeTicks tick_target)
         : interval(interval), tick_target(tick_target) {}
@@ -61,7 +55,6 @@ class CC_EXPORT DelayBasedTimeSource : public TimeSource {
   };
 
   TimeSourceClient* client_;
-  bool has_tick_target_;
   base::TimeTicks last_tick_time_;
 
   // current_parameters_ should only be written by PostNextTickTask.
@@ -71,7 +64,7 @@ class CC_EXPORT DelayBasedTimeSource : public TimeSource {
   Parameters current_parameters_;
   Parameters next_parameters_;
 
-  State state_;
+  bool active_;
 
   base::SingleThreadTaskRunner* task_runner_;
   base::WeakPtrFactory<DelayBasedTimeSource> weak_factory_;
diff --git a/cc/scheduler/delay_based_time_source_unittest.cc b/cc/scheduler/delay_based_time_source_unittest.cc
index df84c97..ac54f57a 100644
--- a/cc/scheduler/delay_based_time_source_unittest.cc
+++ b/cc/scheduler/delay_based_time_source_unittest.cc
@@ -88,7 +88,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenExactlyOnRequestedTime) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -109,7 +109,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenSlightlyAfterRequestedTime) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -122,7 +122,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenSlightlyAfterRequestedTime) {
 }
 
 // At 60Hz, when the tick returns at exactly 2*interval after the requested next
-// time, make sure a 16ms next delay is posted.
+// time, make sure a 0ms next delay is posted.
 TEST(DelayBasedTimeSource, NextDelaySaneWhenExactlyTwiceAfterRequestedTime) {
   scoped_refptr<base::TestSimpleTaskRunner> task_runner =
       new base::TestSimpleTaskRunner;
@@ -131,7 +131,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenExactlyTwiceAfterRequestedTime) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -139,7 +139,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenExactlyTwiceAfterRequestedTime) {
   timer->SetNow(timer->Now() + 2 * Interval());
   task_runner->RunPendingTasks();
 
-  EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
+  EXPECT_EQ(0, task_runner->NextPendingTaskDelay().InMilliseconds());
 }
 
 // At 60Hz, when the tick returns at 2*interval and a bit after the requested
@@ -152,7 +152,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenSlightlyAfterTwiceRequestedTime) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -174,7 +174,7 @@ TEST(DelayBasedTimeSource, NextDelaySaneWhenHalfAfterRequestedTime) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -196,7 +196,7 @@ TEST(DelayBasedTimeSource, SaneHandlingOfJitteryTimebase) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -227,7 +227,7 @@ TEST(DelayBasedTimeSource, HandlesSignificantTimebaseChangesImmediately) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());
@@ -271,7 +271,7 @@ TEST(DelayBasedTimeSource, HanldlesSignificantIntervalChangesImmediately) {
       FakeDelayBasedTimeSource::Create(Interval(), task_runner.get());
   timer->SetClient(&client);
   timer->SetActive(true);
-  // Run the first task, as that activates the timer and picks up a timebase.
+  // Run the first tick.
   task_runner->RunPendingTasks();
 
   EXPECT_EQ(16, task_runner->NextPendingTaskDelay().InMilliseconds());