TaskScheduler: Bump thread priority during shutdown.

base/task_scheduler/scheduler_worker.cc

Index: base/task_scheduler/scheduler_worker.cc
diff --git a/base/task_scheduler/scheduler_worker.cc b/base/task_scheduler/scheduler_worker.cc
index 5a2a7d6a92b96272e1d148edeaec2c4a1782b672..cc1e68d63f5e98001cdb68496edc567fa7a3e341 100644
--- a/base/task_scheduler/scheduler_worker.cc
+++ b/base/task_scheduler/scheduler_worker.cc
@@ -28,6 +28,9 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate {
 
   // PlatformThread::Delegate.
   void ThreadMain() override {
+    // True once the thread's priority has been bumped to accelerate shutdown.
+    bool thread_priority_bumped_for_shutdown = false;

[robliao, 2016/07/19 22:41:28]

I wonder if it might be more extensible to keep track of the current priority,
and that would change your if statement below to...

if (outer_->task_tracker->IsShutdownInProgress() &&
    thread_priority_ != ThreadPriority::NORMAL) {
  thread_priority_ = ThreadPriority::NORMAL;
  PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
}

If GetCurrentPriority is fast, I'd go with that, but I speculate it comes with
non-trivial cost.

[fdoray, 2016/07/20 18:22:21]

I like the idea of keeping track of |thread_priority_|. Not sure why the speed
of GetCurrentPriority matters?

FYI, some benchmarks:

Windows - GetCurrentThreadPriority: 127ns
Windows - SetCurrentThreadPriority: 124ns
Mac - GetCurrentThreadPriority: 68ns
Mac - SetCurrentThreadPriority: 936ns

class Timer {
 public:
  ~Timer() {
    const TimeDelta elapsed_time = TimeTicks::Now() - start;
    LOG(ERROR) << "Elapsed time: " << elapsed_time;
  }

 private:
  const TimeTicks start = TimeTicks::Now();
};

constexpr size_t kNumIterations = 100000000;
volatile int counter = 0;

TEST(ThreadPriorityBenchmark, Counter) {
  Timer timer;
  for (size_t i = 0; i < kNumIterations; ++i) {
    ++counter;
  }
}

TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) {
  Timer timer;
  for (size_t i = 0; i < kNumIterations; ++i) {
    PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
    ++counter;
  }
}

TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) {
  Timer timer;
  for (size_t i = 0; i < kNumIterations; ++i) {
    PlatformThread::GetCurrentThreadPriority();
    ++counter;
  }
}

[robliao, 2016/07/20 19:14:57]

If GetCurrentThreadPriority is nearly as fast as reading the variable, we can
just do that instead of storing the state ourselves (and opening the door to
potential synchronization bugs).

[gab, 2016/07/20 20:30:09]

I much prefer a local variable to a call to the OS here.

[fdoray, 2016/07/20 20:30:37]

GetCurrentThreadPriority() is fast but not sure that 127ns is "nearly as fast"
as reading a variable that can be in cache. Do you think we should still use it?

[robliao, 2016/07/20 22:31:48]

This state is already stored in the TEB, so I'd rather reference that than
duplicate the state here. 127ns is a bit too long for my liking, so let's move
the SetCurrentThreadPriority call and actual_thread_priority_ update into a
separate method to avoid desynchronization. 

[fdoray, 2016/07/21 13:21:56]

Let me know if I understood your comment correctly.

[robliao, 2016/07/21 17:05:51]

Yup! Thanks.

+
     // Set if this thread was detached.
     std::unique_ptr<Thread> detached_thread;
 
@@ -39,6 +42,15 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate {
     while (!outer_->task_tracker_->IsShutdownComplete() &&
            !outer_->ShouldExitForTesting()) {
       DCHECK(outer_);
+
+      // Bump the thread's priority during shutdown.
+      if (outer_->thread_priority_ == ThreadPriority::BACKGROUND &&
+          !thread_priority_bumped_for_shutdown &&
+          outer_->task_tracker_->IsShutdownInProgress()) {
+        PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
+        thread_priority_bumped_for_shutdown = true;
+      }
+
       // Get the sequence containing the next task to execute.
       scoped_refptr<Sequence> sequence = outer_->delegate_->GetWork(outer_);
       if (!sequence) {