TaskScheduler: Avoid Sequence refcount bump in GetWork()

base/task_scheduler/scheduler_thread_pool_impl.cc

Index: base/task_scheduler/scheduler_thread_pool_impl.cc
diff --git a/base/task_scheduler/scheduler_thread_pool_impl.cc b/base/task_scheduler/scheduler_thread_pool_impl.cc
index 0eba0f7f0e13df7687fc995b3d7893abda4c9703..70d1f1c89ca75c72155357cdd2fa060577c4fb35 100644
--- a/base/task_scheduler/scheduler_thread_pool_impl.cc
+++ b/base/task_scheduler/scheduler_thread_pool_impl.cc
@@ -271,9 +271,8 @@ scoped_refptr<TaskRunner> SchedulerThreadPoolImpl::CreateTaskRunnerWithTraits(
 void SchedulerThreadPoolImpl::ReEnqueueSequence(
     scoped_refptr<Sequence> sequence,
     const SequenceSortKey& sequence_sort_key) {
-  shared_priority_queue_.BeginTransaction()->Push(
-      WrapUnique(new PriorityQueue::SequenceAndSortKey(std::move(sequence),
-                                                       sequence_sort_key)));
+  shared_priority_queue_.BeginTransaction()->Push(std::move(sequence),
+                                                  sequence_sort_key);
 
   // The thread calling this method just ran a Task from |sequence| and will
   // soon try to get another Sequence from which to run a Task. If the thread
@@ -338,9 +337,8 @@ void SchedulerThreadPoolImpl::PostTaskWithSequenceNow(
     // - A worker thread is running a Task from |sequence|. It will insert
     //   |sequence| in a PriorityQueue once it's done running the Task.
     const auto sequence_sort_key = sequence->GetSortKey();
-    priority_queue->BeginTransaction()->Push(
-        WrapUnique(new PriorityQueue::SequenceAndSortKey(std::move(sequence),
-                                                         sequence_sort_key)));
+    priority_queue->BeginTransaction()->Push(std::move(sequence),
+                                             sequence_sort_key);
 
     // Wake up a worker thread to process |sequence|.
     if (worker_thread)
@@ -383,20 +381,16 @@ SchedulerThreadPoolImpl::SchedulerWorkerThreadDelegateImpl::GetWork(
   DCHECK(ContainsWorkerThread(outer_->worker_threads_, worker_thread));
 
   scoped_refptr<Sequence> sequence;
-
   {
     std::unique_ptr<PriorityQueue::Transaction> shared_transaction(
         outer_->shared_priority_queue_.BeginTransaction());
-    const auto& shared_sequence_and_sort_key = shared_transaction->Peek();
 
     std::unique_ptr<PriorityQueue::Transaction> single_threaded_transaction(
         SINGLE_THREADED_PRIORITY_QUEUE_FROM_WORKER_THREAD(worker_thread)
             ->BeginTransaction());
-    const auto& single_threaded_sequence_and_sort_key =
-        single_threaded_transaction->Peek();
 
-    if (shared_sequence_and_sort_key.is_null() &&
-        single_threaded_sequence_and_sort_key.is_null()) {
+    if (shared_transaction->IsEmpty() &&
+        single_threaded_transaction->IsEmpty()) {
       single_threaded_transaction.reset();
 
       // |shared_transaction| is kept alive while |worker_thread| is added to
@@ -415,20 +409,19 @@ SchedulerThreadPoolImpl::SchedulerWorkerThreadDelegateImpl::GetWork(
       return nullptr;
     }
 
-    if (single_threaded_sequence_and_sort_key.is_null() ||
-        (!shared_sequence_and_sort_key.is_null() &&
-         single_threaded_sequence_and_sort_key.sort_key <
-             shared_sequence_and_sort_key.sort_key)) {
-      sequence = shared_sequence_and_sort_key.sequence;
-      shared_transaction->Pop();
+    if (single_threaded_transaction->IsEmpty() ||
+        (!shared_transaction->IsEmpty() &&
+         single_threaded_transaction->PeekSortKey() <
+             shared_transaction->PeekSortKey())) {
+      sequence = std::move(shared_transaction->PopSequence());
       last_sequence_is_single_threaded_ = false;
     } else {
-      DCHECK(!single_threaded_sequence_and_sort_key.is_null());
-      sequence = single_threaded_sequence_and_sort_key.sequence;
-      single_threaded_transaction->Pop();
+      DCHECK(!single_threaded_transaction->IsEmpty());
+      sequence = std::move(single_threaded_transaction->PopSequence());

[gab, 2016/04/26 23:32:45]

Are these moves required? I don't think so per the return value being an rvalue
but I'm always worried about scoped_refptr's copy constructor so just want to
double confirm with you guys..

[robliao, 2016/04/27 00:59:01]

http://en.cppreference.com/w/cpp/utility/move
The functions that accept rvalue reference parameters (including move
constructors, move assignment operators, and regular member functions such as
std::vector::push_back) are selected, by overload resolution, when called with
rvalue arguments (either prvalues such as a temporary objects or xvalues such as
the one produced by std::move).

PopSequence() is an rvalue, so it looks like that statement applies. 
Copy elision is also on your side. So it looks like you can remove the std::move
here and above.

[fdoray, 2016/04/27 01:05:10]

According to my test program [1], std::move() doesn't change anything here (i.e.
no extra copy or move constructor is invoked without it).

[1]
https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...

[gab, 2016/04/27 20:40:30]

Done.

       last_sequence_is_single_threaded_ = true;
     }
   }
+  DCHECK(sequence);
 
   outer_->RemoveFromIdleWorkerThreadsStack(worker_thread);
   return sequence;
@@ -441,8 +434,7 @@ void SchedulerThreadPoolImpl::SchedulerWorkerThreadDelegateImpl::
     // PriorityQueue from which it was extracted.
     const SequenceSortKey sequence_sort_key = sequence->GetSortKey();
     single_threaded_priority_queue_.BeginTransaction()->Push(
-        WrapUnique(new PriorityQueue::SequenceAndSortKey(std::move(sequence),
-                                                         sequence_sort_key)));
+        std::move(sequence), sequence_sort_key);
   } else {
     // |re_enqueue_sequence_callback_| will determine in which PriorityQueue
     // |sequence| must be enqueued.