TaskScheduler [5/9] Task Tracker

base/task_scheduler/task_tracker.cc

+// Copyright 2016 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_scheduler/task_tracker.h"
+
+#include "base/callback.h"
+#include "base/debug/task_annotator.h"
+#include "base/metrics/histogram_macros.h"
+
+namespace base {
+namespace internal {
+
+namespace {
+const char kQueueFunctionName[] = "base::PostTask";
+}  // namespace
+
+TaskTracker::TaskTracker() = default;
+TaskTracker::~TaskTracker() = default;
+
+void TaskTracker::Shutdown() {
+  size_t num_tasks_blocking_shutdown_copy;
+
+  {
+    AutoSchedulerLock auto_lock(lock_);
+
+    DCHECK(!shutdown_completed_ && !shutdown_cv_)
+        << "TaskTracker::Shutdown() should only be invoked once.";
+
+    shutdown_cv_ = lock_.CreateConditionVariable();
+
+    // Wait until the number of tasks blocking shutdown is zero.
+    while (num_tasks_blocking_shutdown_ != 0)
+      shutdown_cv_->Wait();
+
+    shutdown_cv_.reset();
+    shutdown_completed_ = true;
+
+    num_tasks_blocking_shutdown_copy = num_tasks_blocking_shutdown_;

[gab, 2016/03/21 17:42:30]

UMA is extremely optimized, I don't think it's worth making a copy just to
release the lock barely earlier.

[fdoray, 2016/03/21 19:08:07]

Done because it makes the code simpler.

However, Dana asked me to release the Sequence's lock before
constructing+returning a SequenceSortKey here
https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se....
Maybe she'll ask me to do a similar optimization here.

[gab, 2016/03/21 19:53:25]

Right, I saw that, and I agree that made sense there because of the construction
of an object. But logging UMA is really just a ++ in a static memory region
IIRC.

+  }
+
+  UMA_HISTOGRAM_COUNTS_100(

[gab, 2016/03/21 17:42:30]

UMA_HISTOGRAM_COUNTS_100 sounds low given current 10000 restriction on
BlockingPool.

Also, this will not catch an infinite shutdown loop.

How about using UMA_HISTOGRAM_COUNTS_CUSTOM with a self-defined max and also
logging a metric if the max is busted during shutdown?

[fdoray, 2016/03/21 19:08:07]

Done.

The current restriction for BlockingPool is 1000 I believe
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...).

[gab, 2016/03/21 19:53:25]

Ah, yes :-), then let's use that.

+      "TaskScheduler.BlockShutdownTasksPostedDuringShutdown",
+      num_tasks_blocking_shutdown_copy);
+}
+
+void TaskTracker::PostTask(
+    const Callback<void(scoped_ptr<Task>)>& post_task_callback,
+    scoped_ptr<Task> task) {
+  DCHECK(!post_task_callback.is_null());
+  DCHECK(task);
+
+  if (!BeforePostTask(task->traits.shutdown_behavior()))
+    return;
+
+  debug::TaskAnnotator task_annotator;
+  task_annotator.DidQueueTask(kQueueFunctionName, *task);
+
+  post_task_callback.Run(std::move(task));
+}
+
+void TaskTracker::RunTask(const Task* task) {
+  DCHECK(task);
+
+  const TaskShutdownBehavior shutdown_behavior =
+      task->traits.shutdown_behavior();
+  if (!BeforeRunTask(shutdown_behavior))
+    return;
+
+  debug::TaskAnnotator task_annotator;
+  task_annotator.RunTask(kQueueFunctionName, *task);
+
+  AfterRunTask(shutdown_behavior);
+}
+
+bool TaskTracker::IsShuttingDownForTesting() const {
+  AutoSchedulerLock auto_lock(lock_);
+  return !!shutdown_cv_;
+}
+
+bool TaskTracker::BeforePostTask(TaskShutdownBehavior shutdown_behavior) {
+  AutoSchedulerLock auto_lock(lock_);
+
+  if (shutdown_completed_) {
+    // A BLOCK_SHUTDOWN task posted after shutdown has completed is an ordering
+    // bug. This DCHECK aims to catch those early.
+    DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior);
+
+    // No task is allowed to be posted after shutdown.
+    return false;
+  }
+
+  if (shutdown_behavior == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
+    // BLOCK_SHUTDOWN tasks block shutdown between the moment they are posted
+    // and the moment they complete their execution.
+    ++num_tasks_blocking_shutdown_;
+
+    if (shutdown_cv_)
+      ++num_block_shutdown_tasks_posted_during_shutdown_;
+
+    return true;
+  }
+
+  return !shutdown_cv_;
+}
+
+bool TaskTracker::BeforeRunTask(TaskShutdownBehavior shutdown_behavior) {
+  AutoSchedulerLock auto_lock(lock_);
+
+  if (shutdown_completed_) {
+    // A BLOCK_SHUTDOWN task run after shutdown has completed is an ordering
+    // bug. This DCHECK aims to catch those early.

[gab, 2016/03/21 17:42:30]

Posting is an ordering bug, running is just unexpected as it either shouldn't
have been posted if shutdown completed or should be blocking shutdown if was
posted before it did.

Actually, shouldn't this just be NOTREACHED()? No task should ever request to
run after shutdown completed, right?

[fdoray, 2016/03/21 19:08:07]

It is not an error if a WorkerThread tries to run a non BLOCK_SHUTDOWN task
after shutdown. For example:

Thread 1: WorkerThread extracts a SKIP_SHUTDOWN task from a PriorityQueue.
Thread 2: TaskTracker::Shutdown() returns because no task is blocking shutdown.
Thread 1: WorkerThread tries to run its task via the TaskTracker. The
TaskTracker should prevent that, but it shouldn't crash.

[gab, 2016/03/21 19:53:25]

Ah right, okay.

+    DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior);
+    return false;
+  }
+
+  switch (shutdown_behavior) {
+    case TaskShutdownBehavior::BLOCK_SHUTDOWN:
+      return true;
+
+    case TaskShutdownBehavior::SKIP_ON_SHUTDOWN:
+      if (shutdown_cv_)
+        return false;
+
+      // SKIP_ON_SHUTDOWN tasks block shutdown while they are running.
+      ++num_tasks_blocking_shutdown_;
+      return true;
+
+    case TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN:
+      return !shutdown_cv_;
+  }
+
+  NOTREACHED();
+  return false;
+}
+
+void TaskTracker::AfterRunTask(TaskShutdownBehavior shutdown_behavior) {
+  if (shutdown_behavior == TaskShutdownBehavior::BLOCK_SHUTDOWN ||
+      shutdown_behavior == TaskShutdownBehavior::SKIP_ON_SHUTDOWN) {
+    AutoSchedulerLock auto_lock(lock_);
+    DCHECK_GT(num_tasks_blocking_shutdown_, 0U);
+    --num_tasks_blocking_shutdown_;
+    if (num_tasks_blocking_shutdown_ == 0 && shutdown_cv_)
+      shutdown_cv_->Signal();
+  }
+}
+
+}  // namespace internal
+}  // namespace base