[ThreadPool] Introduce ThreadPoolClock as the new source of mockable Now()

Timings that should remain real-time even under MOCK_TIME are now
explicitly using subtle::TimeTicksNowIgnoringOverride().

ThreadPoolClock defaults to DefaultTickClock in this CL but will be
overridden under ScopedTaskEnvironment's upcoming multi-threaded
MOCK_TIME mode @
https://chromium-review.googlesource.com/c/chromium/src/+/1686776

Ultimately, when MOCK_TIME mode always mocks TimeTicks::Now(), we will
be able to remove this and use TimeTicks::Now() again directly for
these calls.

Bonus #1: Gets rid of a call to DeprecatedGetMockTickClock().

Bonus #2: Added thread annotations to DelayedTaskManager::queue_lock_.

R=fdoray@chromium.org

Bug: 946657, 789079, 905412
Change-Id: Ief86338388b670ad9ddafb926e6817df6d96a95a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1687028
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675695}

base/BUILD.gn

@@ -862,6 +862,8 @@ jumbo_component("base") {
     "task/thread_pool/thread_group_native_win.h",
     "task/thread_pool/thread_pool.cc",
     "task/thread_pool/thread_pool.h",
+    "task/thread_pool/thread_pool_clock.cc",
+    "task/thread_pool/thread_pool_clock.h",
     "task/thread_pool/thread_pool_impl.cc",
     "task/thread_pool/thread_pool_impl.h",
     "task/thread_pool/tracked_ref.h",

base/task/thread_pool/delayed_task_manager.cc

@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool/task.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 #include "base/task_runner.h"
 
 namespace base {
@@ -46,14 +47,10 @@ void DelayedTaskManager::DelayedTask::SetScheduled() {
   scheduled_ = true;
 }
 
-DelayedTaskManager::DelayedTaskManager(
-    std::unique_ptr<const TickClock> tick_clock)
+DelayedTaskManager::DelayedTaskManager()
     : process_ripe_tasks_closure_(
           BindRepeating(&DelayedTaskManager::ProcessRipeTasks,
-                        Unretained(this))),
-      tick_clock_(std::move(tick_clock)) {
-  DCHECK(tick_clock_);
-}
+                        Unretained(this))) {}
 
 DelayedTaskManager::~DelayedTaskManager() = default;
 
@@ -101,7 +98,7 @@ void DelayedTaskManager::ProcessRipeTasks() {
 
   {
     CheckedAutoLock auto_lock(queue_lock_);
-    const TimeTicks now = tick_clock_->NowTicks();
+    const TimeTicks now = ThreadPoolClock::Now();
     while (!delayed_task_queue_.empty() &&
            delayed_task_queue_.Min().task.delayed_run_time <= now) {
       // The const_cast on top is okay since the DelayedTask is
@@ -139,7 +136,7 @@ void DelayedTaskManager::ScheduleProcessRipeTasksOnServiceThread(
   DCHECK(!next_delayed_task_run_time.is_null());
   if (next_delayed_task_run_time.is_max())
     return;
-  const TimeTicks now = tick_clock_->NowTicks();
+  const TimeTicks now = ThreadPoolClock::Now();
   TimeDelta delay = std::max(TimeDelta(), next_delayed_task_run_time - now);
   service_thread_task_runner_->PostDelayedTask(
       FROM_HERE, process_ripe_tasks_closure_, delay);

base/task/thread_pool/delayed_task_manager.h

@@ -17,8 +17,7 @@
 #include "base/task/common/checked_lock.h"
 #include "base/task/common/intrusive_heap.h"
 #include "base/task/thread_pool/task.h"
-#include "base/time/default_tick_clock.h"
-#include "base/time/tick_clock.h"
+#include "base/thread_annotations.h"
 
 namespace base {
 
@@ -34,9 +33,7 @@ class BASE_EXPORT DelayedTaskManager {
   // Posts |task| for execution immediately.
   using PostTaskNowCallback = OnceCallback<void(Task task)>;
 
-  // |tick_clock| can be specified for testing.
-  DelayedTaskManager(std::unique_ptr<const TickClock> tick_clock =
-                         std::make_unique<DefaultTickClock>());
+  DelayedTaskManager();
   ~DelayedTaskManager();
 
   // Starts the delayed task manager, allowing past and future tasks to be
@@ -95,7 +92,8 @@ class BASE_EXPORT DelayedTaskManager {
   // Get the time at which to schedule the next |ProcessRipeTasks()| execution,
   // or TimeTicks::Max() if none needs to be scheduled (i.e. no task, or next
   // task already scheduled).
-  TimeTicks GetTimeToScheduleProcessRipeTasksLockRequired();
+  TimeTicks GetTimeToScheduleProcessRipeTasksLockRequired()
+      EXCLUSIVE_LOCKS_REQUIRED(queue_lock_);
 
   // Schedule |ProcessRipeTasks()| on the service thread to be executed at the
   // given |process_ripe_tasks_time|, provided the given time is not
@@ -105,19 +103,17 @@ class BASE_EXPORT DelayedTaskManager {
 
   const RepeatingClosure process_ripe_tasks_closure_;
 
-  const std::unique_ptr<const TickClock> tick_clock_;
-
-  scoped_refptr<TaskRunner> service_thread_task_runner_;
-
-  IntrusiveHeap<DelayedTask> delayed_task_queue_;
-
   // Synchronizes access to |delayed_task_queue_| and the setting of
-  // |service_thread_task_runner|. Once |service_thread_task_runner_| is set,
+  // |service_thread_task_runner_|. Once |service_thread_task_runner_| is set,
   // it is never modified. It is therefore safe to access
   // |service_thread_task_runner_| without synchronization once it is observed
   // that it is non-null.
   CheckedLock queue_lock_;
 
+  scoped_refptr<TaskRunner> service_thread_task_runner_;
+
+  IntrusiveHeap<DelayedTask> delayed_task_queue_ GUARDED_BY(queue_lock_);
+
   DISALLOW_COPY_AND_ASSIGN(DelayedTaskManager);
 };
 

base/task/thread_pool/delayed_task_manager_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task/thread_pool/task.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread.h"
@@ -47,20 +48,17 @@ Task ConstructMockedTask(testing::StrictMock<MockTask>& mock_task,
 
 class ThreadPoolDelayedTaskManagerTest : public testing::Test {
  protected:
-  ThreadPoolDelayedTaskManagerTest()
-      : delayed_task_manager_(
-            service_thread_task_runner_->DeprecatedGetMockTickClock()),
-        task_(ConstructMockedTask(
-            mock_task_,
-            service_thread_task_runner_->GetMockTickClock()->NowTicks(),
-            kLongDelay)) {}
+  ThreadPoolDelayedTaskManagerTest() = default;
   ~ThreadPoolDelayedTaskManagerTest() override = default;
 
   const scoped_refptr<TestMockTimeTaskRunner> service_thread_task_runner_ =
       MakeRefCounted<TestMockTimeTaskRunner>();
+  ThreadPoolClock thread_pool_clock_{
+      service_thread_task_runner_->GetMockTickClock()};
   DelayedTaskManager delayed_task_manager_;
   testing::StrictMock<MockTask> mock_task_;
-  Task task_;
+  Task task_{
+      ConstructMockedTask(mock_task_, ThreadPoolClock::Now(), kLongDelay)};
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ThreadPoolDelayedTaskManagerTest);
@@ -156,19 +154,16 @@ TEST_F(ThreadPoolDelayedTaskManagerTest, DelayedTasksRunAfterDelay) {
   delayed_task_manager_.Start(service_thread_task_runner_);
 
   testing::StrictMock<MockTask> mock_task_a;
-  Task task_a = ConstructMockedTask(
-      mock_task_a, service_thread_task_runner_->GetMockTickClock()->NowTicks(),
-      TimeDelta::FromHours(1));
+  Task task_a = ConstructMockedTask(mock_task_a, ThreadPoolClock::Now(),
+                                    TimeDelta::FromHours(1));
 
   testing::StrictMock<MockTask> mock_task_b;
-  Task task_b = ConstructMockedTask(
-      mock_task_b, service_thread_task_runner_->GetMockTickClock()->NowTicks(),
-      TimeDelta::FromHours(2));
+  Task task_b = ConstructMockedTask(mock_task_b, ThreadPoolClock::Now(),
+                                    TimeDelta::FromHours(2));
 
   testing::StrictMock<MockTask> mock_task_c;
-  Task task_c = ConstructMockedTask(
-      mock_task_c, service_thread_task_runner_->GetMockTickClock()->NowTicks(),
-      TimeDelta::FromHours(1));
+  Task task_c = ConstructMockedTask(mock_task_c, ThreadPoolClock::Now(),
+                                    TimeDelta::FromHours(1));
 
   // Send tasks to the DelayedTaskManager.
   delayed_task_manager_.AddDelayedTask(std::move(task_a), BindOnce(&RunTask),

base/task/thread_pool/sequence.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/task/task_features.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 #include "base/time/time.h"
 
 namespace base {
@@ -40,7 +41,7 @@ void Sequence::Transaction::PushTask(Task task) {
   DCHECK(task.queue_time.is_null());
 
   bool should_be_queued = WillPushTask();
-  task.queue_time = base::TimeTicks::Now();
+  task.queue_time = ThreadPoolClock::Now();
 
   task.task = sequence()->traits_.shutdown_behavior() ==
                       TaskShutdownBehavior::BLOCK_SHUTDOWN

base/task/thread_pool/service_thread.cc

@@ -13,6 +13,7 @@
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool/task_tracker.h"
 #include "base/task/thread_pool/thread_pool.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 
 namespace base {
 namespace internal {
@@ -103,8 +104,8 @@ void ServiceThread::PerformHeartbeatLatencyReport() const {
       FROM_HERE, profiled_traits,
       BindOnce(
           &TaskTracker::RecordHeartbeatLatencyAndTasksRunWhileQueuingHistograms,
-          Unretained(task_tracker_), task_priority, may_block, TimeTicks::Now(),
-          task_tracker_->GetNumTasksRun()));
+          Unretained(task_tracker_), task_priority, may_block,
+          ThreadPoolClock::Now(), task_tracker_->GetNumTasksRun()));
 }
 
 }  // namespace internal

base/task/thread_pool/task.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 
 namespace base {
 namespace internal {
@@ -20,10 +21,11 @@ AtomicSequenceNumber g_sequence_nums_for_tracing;
 Task::Task() = default;
 
 Task::Task(const Location& posted_from, OnceClosure task, TimeDelta delay)
-    : PendingTask(posted_from,
-                  std::move(task),
-                  delay.is_zero() ? TimeTicks() : TimeTicks::Now() + delay,
-                  Nestable::kNonNestable) {
+    : PendingTask(
+          posted_from,
+          std::move(task),
+          delay.is_zero() ? TimeTicks() : ThreadPoolClock::Now() + delay,
+          Nestable::kNonNestable) {
   // ThreadPoolImpl doesn't use |sequence_num| but tracing (toplevel.flow)
   // relies on it being unique. While this subtle dependency is a bit
   // overreaching, ThreadPoolImpl is the only task system that doesn't use

base/task/thread_pool/task_tracker.cc

@@ -20,6 +20,7 @@
 #include "base/sequence_token.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/task/scoped_set_task_priority_for_current_thread.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 #include "base/threading/sequence_local_storage_map.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_restrictions.h"
@@ -511,7 +512,7 @@ void TaskTracker::RecordLatencyHistogram(
     LatencyHistogramType latency_histogram_type,
     TaskTraits task_traits,
     TimeTicks posted_time) const {
-  const TimeDelta task_latency = TimeTicks::Now() - posted_time;
+  const TimeDelta task_latency = ThreadPoolClock::Now() - posted_time;
 
   DCHECK(latency_histogram_type == LatencyHistogramType::TASK_LATENCY ||
          latency_histogram_type == LatencyHistogramType::HEARTBEAT_LATENCY);

base/task/thread_pool/thread_group_impl.cc

@@ -30,6 +30,7 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time/time_override.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
@@ -724,7 +725,7 @@ bool ThreadGroupImpl::WorkerThreadDelegateImpl::CanCleanupLockRequired(
 
   const TimeTicks last_used_time = worker->GetLastUsedTime();
   return !last_used_time.is_null() &&
-         TimeTicks::Now() - last_used_time >=
+         subtle::TimeTicksNowIgnoringOverride() - last_used_time >=
              outer_->after_start().suggested_reclaim_time &&
          (outer_->workers_.size() > outer_->after_start().initial_max_tasks ||
           !FeatureList::IsEnabled(kNoDetachBelowInitialCapacity)) &&
@@ -737,7 +738,7 @@ void ThreadGroupImpl::WorkerThreadDelegateImpl::CleanupLockRequired(
 
   outer_->num_tasks_before_detach_histogram_->Add(
       worker_only().num_tasks_since_last_detach);
-  outer_->cleanup_timestamps_.push(TimeTicks::Now());
+  outer_->cleanup_timestamps_.push(subtle::TimeTicksNowIgnoringOverride());
   worker->Cleanup();
   outer_->idle_workers_stack_.Remove(worker);
 
@@ -876,7 +877,7 @@ void ThreadGroupImpl::WorkerThreadDelegateImpl::MayBlockEntered() {
 
   DCHECK(!incremented_max_tasks_since_blocked_);
   DCHECK(read_worker().may_block_start_time.is_null());
-  write_worker().may_block_start_time = TimeTicks::Now();
+  write_worker().may_block_start_time = subtle::TimeTicksNowIgnoringOverride();
   ++outer_->num_unresolved_may_block_;
   if (read_worker().is_running_best_effort_task)
     ++outer_->num_unresolved_best_effort_may_block_;
@@ -933,7 +934,8 @@ bool ThreadGroupImpl::WorkerThreadDelegateImpl::
     MustIncrementMaxTasksLockRequired() {
   if (!incremented_max_tasks_since_blocked_ &&
       !read_any().may_block_start_time.is_null() &&
-      TimeTicks::Now() - read_any().may_block_start_time >=
+      subtle::TimeTicksNowIgnoringOverride() -
+              read_any().may_block_start_time >=
           outer_->after_start().may_block_threshold) {
     incremented_max_tasks_since_blocked_ = true;
 
@@ -994,7 +996,7 @@ ThreadGroupImpl::CreateAndRegisterWorkerLockRequired(
   DCHECK_LE(workers_.size(), max_tasks_);
 
   if (!cleanup_timestamps_.empty()) {
-    detach_duration_histogram_->AddTime(TimeTicks::Now() -
+    detach_duration_histogram_->AddTime(subtle::TimeTicksNowIgnoringOverride() -
                                         cleanup_timestamps_.top());
     cleanup_timestamps_.pop();
   }

base/task/thread_pool/thread_pool_clock.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/task/thread_pool/thread_pool_clock.h"
+
+#include "base/logging.h"
+#include "base/time/tick_clock.h"
+
+namespace base {
+namespace internal {
+
+namespace {
+const TickClock* g_tick_clock = nullptr;
+}
+
+ThreadPoolClock::ThreadPoolClock(const TickClock* tick_clock) {
+  DCHECK(!g_tick_clock);
+  g_tick_clock = tick_clock;
+}
+
+ThreadPoolClock::~ThreadPoolClock() {
+  DCHECK(g_tick_clock);
+  g_tick_clock = nullptr;
+}
+
+// static
+TimeTicks ThreadPoolClock::Now() {
+  // Allow |g_tick_clock| to be null so simple thread_pool/ unit tests don't
+  // need to install one.
+  return g_tick_clock ? g_tick_clock->NowTicks() : TimeTicks::Now();
+}
+
+}  // namespace internal
+}  // namespace base

base/task/thread_pool/thread_pool_clock.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TASK_THREAD_POOL_THREAD_POOL_CLOCK_H_
+#define BASE_TASK_THREAD_POOL_THREAD_POOL_CLOCK_H_
+
+#include "base/base_export.h"
+#include "base/macros.h"
+#include "base/time/time.h"
+
+namespace base {
+
+class TickClock;
+
+namespace internal {
+
+class BASE_EXPORT ThreadPoolClock {
+ public:
+  // |tick_clock| will service ThreadPoolClock::Now() in the scope of this
+  // ThreadPoolClock.
+  ThreadPoolClock(const TickClock* tick_clock);
+  ~ThreadPoolClock();
+
+  // Returns the current TimeTicks::Now(). All calls to TimeTicks::Now() in
+  // base/task/thread_pool should use this or
+  // subtle::TimeTicksNowIgnoringOverride() depending on whether they want to
+  // respect mock time (e.g. delayed tasks) or need real-time timeouts (e.g.
+  // recycling threads). TODO(gab): Make MOCK_TIME always mock TimeTicks::Now()
+  // and get rid of this indirection.
+  static TimeTicks Now();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ThreadPoolClock);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_THREAD_POOL_THREAD_POOL_CLOCK_H_

base/task/thread_pool/thread_pool_impl.cc

@@ -27,6 +27,7 @@
 #include "base/task/thread_pool/task_source.h"
 #include "base/task/thread_pool/thread_group_impl.h"
 #include "base/threading/platform_thread.h"
+#include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 
 #if defined(OS_WIN)
@@ -67,7 +68,8 @@ ThreadPoolImpl::ThreadPoolImpl(StringPiece histogram_label)
 
 ThreadPoolImpl::ThreadPoolImpl(StringPiece histogram_label,
                                std::unique_ptr<TaskTrackerImpl> task_tracker)
-    : task_tracker_(std::move(task_tracker)),
+    : thread_pool_clock_(DefaultTickClock::GetInstance()),
+      task_tracker_(std::move(task_tracker)),
       service_thread_(std::make_unique<ServiceThread>(
           task_tracker_.get(),
           BindRepeating(&ThreadPoolImpl::ReportHeartbeatMetrics,

base/task/thread_pool/thread_pool_impl.h

@@ -29,6 +29,7 @@
 #include "base/task/thread_pool/thread_group.h"
 #include "base/task/thread_pool/thread_group_impl.h"
 #include "base/task/thread_pool/thread_pool.h"
+#include "base/task/thread_pool/thread_pool_clock.h"
 #include "base/updateable_sequenced_task_runner.h"
 #include "build/build_config.h"
 
@@ -128,6 +129,10 @@ class BASE_EXPORT ThreadPoolImpl : public ThreadPoolInstance,
   void UpdatePriority(scoped_refptr<TaskSource> task_source,
                       TaskPriority priority) override;
 
+  // The clock instance used by all classes in base/task/thread_pool. Must
+  // outlive everything else to ensure no discrepancy in Now().
+  ThreadPoolClock thread_pool_clock_;
+
   const std::unique_ptr<TaskTrackerImpl> task_tracker_;
   std::unique_ptr<Thread> service_thread_;
   DelayedTaskManager delayed_task_manager_;

base/task/thread_pool/worker_thread.cc

@@ -14,6 +14,7 @@
 #include "base/task/thread_pool/environment_config.h"
 #include "base/task/thread_pool/task_tracker.h"
 #include "base/task/thread_pool/worker_thread_observer.h"
+#include "base/time/time_override.h"
 #include "base/trace_event/trace_event.h"
 
 #if defined(OS_MACOSX)
@@ -133,7 +134,7 @@ void WorkerThread::Cleanup() {
 void WorkerThread::BeginUnusedPeriod() {
   CheckedAutoLock auto_lock(thread_lock_);
   DCHECK(last_used_time_.is_null());
-  last_used_time_ = TimeTicks::Now();
+  last_used_time_ = subtle::TimeTicksNowIgnoringOverride();
 }
 
 void WorkerThread::EndUnusedPeriod() {