TaskScheduler: Change Sequence::PeekTask to Sequence::TakeTask.
Sequence::TakeTask() transfers ownership of the Task in the front slot of
the Sequence to the caller. This makes the front slot empty. The empty
front slot can be removed with Sequence::RemoveFrontSlot().
Benefits of transfering ownership of a Task to code that runs it:
- The code that runs (and owns) the Task can modify it. This is
important if we want to use OnceClosure in TaskScheduler.
- It is now clear that a single thread has a pointer to a given
Task at a time (before, multiple threads calling PeekTask on
the same Sequence could theoretically get pointers to the same
Task).
BUG=553459
Review-Url: https://codereview.chromium.org/2399213005
Cr-Commit-Position: refs/heads/master@{#424719}
CrOS-Libchrome-Original-Commit: 0c97aa3cb8c7710bdbc7d22c260a4d871f41e264
diff --git a/base/task_scheduler/scheduler_service_thread.cc b/base/task_scheduler/scheduler_service_thread.cc
index 9f6936b..e592907 100644
--- a/base/task_scheduler/scheduler_service_thread.cc
+++ b/base/task_scheduler/scheduler_service_thread.cc
@@ -35,7 +35,8 @@
return nullptr;
}
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override {
+ void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) override {
NOTREACHED()
<< "GetWork() never returns a sequence so no task should ever run.";
}
diff --git a/base/task_scheduler/scheduler_worker.cc b/base/task_scheduler/scheduler_worker.cc
index 2bed780..1f5625c 100644
--- a/base/task_scheduler/scheduler_worker.cc
+++ b/base/task_scheduler/scheduler_worker.cc
@@ -70,12 +70,13 @@
continue;
}
- const Task* task = sequence->PeekTask();
- const TimeTicks start_time = TimeTicks::Now();
- if (outer_->task_tracker_->RunTask(task, sequence->token()))
- outer_->delegate_->DidRunTask(task, start_time - task->sequenced_time);
+ std::unique_ptr<Task> task = sequence->TakeTask();
+ const TaskPriority task_priority = task->traits.priority();
+ const TimeDelta task_latency = TimeTicks::Now() - task->sequenced_time;
+ if (outer_->task_tracker_->RunTask(std::move(task), sequence->token()))
+ outer_->delegate_->DidRunTaskWithPriority(task_priority, task_latency);
- const bool sequence_became_empty = sequence->PopTask();
+ const bool sequence_became_empty = sequence->Pop();
// If |sequence| isn't empty immediately after the pop, re-enqueue it to
// maintain the invariant that a non-empty Sequence is always referenced
diff --git a/base/task_scheduler/scheduler_worker.h b/base/task_scheduler/scheduler_worker.h
index eee2f4a..ec49eb8 100644
--- a/base/task_scheduler/scheduler_worker.h
+++ b/base/task_scheduler/scheduler_worker.h
@@ -53,10 +53,11 @@
// run a Task.
virtual scoped_refptr<Sequence> GetWork(SchedulerWorker* worker) = 0;
- // Called by the SchedulerWorker after it ran |task|. |task_latency| is the
- // time elapsed between when the task was posted and when it started to run.
- virtual void DidRunTask(const Task* task,
- const TimeDelta& task_latency) = 0;
+ // Called by the SchedulerWorker after it ran a task with |task_priority|.
+ // |task_latency| is the time elapsed between when the task was posted and
+ // when it started to run.
+ virtual void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) = 0;
// Called when |sequence| isn't empty after the SchedulerWorker pops a Task
// from it. |sequence| is the last Sequence returned by GetWork().
diff --git a/base/task_scheduler/scheduler_worker_pool_impl.cc b/base/task_scheduler/scheduler_worker_pool_impl.cc
index ed8e84a..dc04030 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl.cc
@@ -238,7 +238,8 @@
void OnMainEntry(SchedulerWorker* worker,
const TimeDelta& detach_duration) override;
scoped_refptr<Sequence> GetWork(SchedulerWorker* worker) override;
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override;
+ void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) override;
void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override;
TimeDelta GetSleepTimeout() override;
bool CanDetach(SchedulerWorker* worker) override;
@@ -588,12 +589,12 @@
return sequence;
}
-void SchedulerWorkerPoolImpl::SchedulerWorkerDelegateImpl::DidRunTask(
- const Task* task,
- const TimeDelta& task_latency) {
+void SchedulerWorkerPoolImpl::SchedulerWorkerDelegateImpl::
+ DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) {
++num_tasks_since_last_wait_;
- const int priority_index = static_cast<int>(task->traits.priority());
+ const int priority_index = static_cast<int>(task_priority);
// As explained in the header file, histograms are allocated on demand. It
// doesn't matter if an element of |task_latency_histograms_| is set multiple
@@ -603,7 +604,7 @@
subtle::Acquire_Load(&outer_->task_latency_histograms_[priority_index]));
if (!task_latency_histogram) {
task_latency_histogram =
- GetTaskLatencyHistogram(outer_->name_, task->traits.priority());
+ GetTaskLatencyHistogram(outer_->name_, task_priority);
subtle::Release_Store(
&outer_->task_latency_histograms_[priority_index],
reinterpret_cast<subtle::AtomicWord>(task_latency_histogram));
diff --git a/base/task_scheduler/scheduler_worker_stack_unittest.cc b/base/task_scheduler/scheduler_worker_stack_unittest.cc
index c56bc8a..022e196 100644
--- a/base/task_scheduler/scheduler_worker_stack_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_stack_unittest.cc
@@ -27,8 +27,9 @@
scoped_refptr<Sequence> GetWork(SchedulerWorker* worker) override {
return nullptr;
}
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override {
- ADD_FAILURE() << "Unexpected call to DidRunTask()";
+ void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) override {
+ ADD_FAILURE() << "Unexpected call to DidRunTaskWithPriority()";
}
void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override {
ADD_FAILURE() << "Unexpected call to ReEnqueueSequence()";
diff --git a/base/task_scheduler/scheduler_worker_unittest.cc b/base/task_scheduler/scheduler_worker_unittest.cc
index 47a35cb..0d29bcd 100644
--- a/base/task_scheduler/scheduler_worker_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_unittest.cc
@@ -46,8 +46,9 @@
scoped_refptr<Sequence> GetWork(SchedulerWorker* worker) override {
return nullptr;
}
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override {
- ADD_FAILURE() << "Unexpected call to DidRunTask()";
+ void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) override {
+ ADD_FAILURE() << "Unexpected call to DidRunTaskWithPriority()";
}
void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override {
ADD_FAILURE() << "Unexpected call to ReEnqueueSequence()";
@@ -126,7 +127,7 @@
: outer_(outer) {}
~TestSchedulerWorkerDelegate() override {
- EXPECT_FALSE(IsCallToDidRunTaskExpected());
+ EXPECT_FALSE(IsCallToDidRunTaskWithPriorityExpected());
}
// SchedulerWorker::Delegate:
@@ -134,7 +135,7 @@
const TimeDelta& detach_duration) override {
outer_->worker_set_.Wait();
EXPECT_EQ(outer_->worker_.get(), worker);
- EXPECT_FALSE(IsCallToDidRunTaskExpected());
+ EXPECT_FALSE(IsCallToDidRunTaskWithPriorityExpected());
// Without synchronization, OnMainEntry() could be called twice without
// generating an error.
@@ -144,7 +145,7 @@
}
scoped_refptr<Sequence> GetWork(SchedulerWorker* worker) override {
- EXPECT_FALSE(IsCallToDidRunTaskExpected());
+ EXPECT_FALSE(IsCallToDidRunTaskWithPriorityExpected());
EXPECT_EQ(outer_->worker_.get(), worker);
{
@@ -174,7 +175,7 @@
sequence->PushTask(std::move(task));
}
- ExpectCallToDidRunTask(sequence->PeekTask());
+ ExpectCallToDidRunTaskWithPriority(sequence->PeekTaskTraits().priority());
{
// Add the Sequence to the vector of created Sequences.
@@ -185,11 +186,13 @@
return sequence;
}
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override {
- AutoSchedulerLock auto_lock(expect_did_run_task_lock_);
- EXPECT_EQ(expect_did_run_task_, task);
- expect_did_run_task_ = nullptr;
+ void DidRunTaskWithPriority(TaskPriority task_priority,
+ const TimeDelta& task_latency) override {
+ AutoSchedulerLock auto_lock(expect_did_run_task_with_priority_lock_);
+ EXPECT_TRUE(expect_did_run_task_with_priority_);
+ EXPECT_EQ(expected_task_priority_, task_priority);
EXPECT_FALSE(task_latency.is_max());
+ expect_did_run_task_with_priority_ = false;
}
// This override verifies that |sequence| contains the expected number of
@@ -197,15 +200,14 @@
// EnqueueSequence implementation, it doesn't reinsert |sequence| into a
// queue for further execution.
void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override {
- EXPECT_FALSE(IsCallToDidRunTaskExpected());
+ EXPECT_FALSE(IsCallToDidRunTaskWithPriorityExpected());
EXPECT_GT(outer_->TasksPerSequence(), 1U);
// Verify that |sequence| contains TasksPerSequence() - 1 Tasks.
for (size_t i = 0; i < outer_->TasksPerSequence() - 1; ++i) {
- EXPECT_TRUE(sequence->PeekTask());
- sequence->PopTask();
+ EXPECT_TRUE(sequence->TakeTask());
+ EXPECT_EQ(i == outer_->TasksPerSequence() - 2, sequence->Pop());
}
- EXPECT_FALSE(sequence->PeekTask());
// Add |sequence| to |re_enqueued_sequences_|.
AutoSchedulerLock auto_lock(outer_->lock_);
@@ -215,27 +217,31 @@
}
private:
- // Expect a call to DidRunTask() with |task| as argument before the next
- // call to any other method of this delegate.
- void ExpectCallToDidRunTask(const Task* task) {
- AutoSchedulerLock auto_lock(expect_did_run_task_lock_);
- expect_did_run_task_ = task;
+ // Expect a call to DidRunTaskWithPriority() with |task_priority| as
+ // argument before the next call to any other method of this delegate.
+ void ExpectCallToDidRunTaskWithPriority(TaskPriority task_priority) {
+ AutoSchedulerLock auto_lock(expect_did_run_task_with_priority_lock_);
+ expect_did_run_task_with_priority_ = true;
+ expected_task_priority_ = task_priority;
}
- bool IsCallToDidRunTaskExpected() const {
- AutoSchedulerLock auto_lock(expect_did_run_task_lock_);
- return expect_did_run_task_ != nullptr;
+ bool IsCallToDidRunTaskWithPriorityExpected() const {
+ AutoSchedulerLock auto_lock(expect_did_run_task_with_priority_lock_);
+ return expect_did_run_task_with_priority_;
}
TaskSchedulerWorkerTest* outer_;
- // Synchronizes access to |expect_did_run_task_|.
- mutable SchedulerLock expect_did_run_task_lock_;
+ // Synchronizes access to |expect_did_run_task_with_priority_| and
+ // |expected_task_priority_|.
+ mutable SchedulerLock expect_did_run_task_with_priority_lock_;
- // Expected task for the next call to DidRunTask(). DidRunTask() should not
- // be called when this is nullptr. No method other than DidRunTask() should
- // be called on this delegate when this is not nullptr.
- const Task* expect_did_run_task_ = nullptr;
+ // Whether the next method called on this delegate should be
+ // DidRunTaskWithPriority().
+ bool expect_did_run_task_with_priority_ = false;
+
+ // Expected priority for the next call to DidRunTaskWithPriority().
+ TaskPriority expected_task_priority_ = TaskPriority::BACKGROUND;
};
void RunTaskCallback() {
@@ -384,7 +390,8 @@
return sequence;
}
- void DidRunTask(const Task* task, const TimeDelta& task_latency) override {}
+ void DidRunTaskWithPriority(TaskPriority task,
+ const TimeDelta& task_latency) override {}
bool CanDetach(SchedulerWorker* worker) override {
detach_requested_.Signal();
diff --git a/base/task_scheduler/sequence.cc b/base/task_scheduler/sequence.cc
index 86e99f0..601b540 100644
--- a/base/task_scheduler/sequence.cc
+++ b/base/task_scheduler/sequence.cc
@@ -26,37 +26,32 @@
return queue_.size() == 1;
}
-const Task* Sequence::PeekTask() const {
+std::unique_ptr<Task> Sequence::TakeTask() {
AutoSchedulerLock auto_lock(lock_);
+ DCHECK(!queue_.empty());
+ DCHECK(queue_.front());
- if (queue_.empty())
- return nullptr;
+ const int priority_index =
+ static_cast<int>(queue_.front()->traits.priority());
+ DCHECK_GT(num_tasks_per_priority_[priority_index], 0U);
+ --num_tasks_per_priority_[priority_index];
- return queue_.front().get();
+ return std::move(queue_.front());
}
-bool Sequence::PopTask() {
- // Delete the popped task outside the scope of |lock_|. This prevents a double
- // acquisition of |lock_| if the task's destructor tries to post a task to
- // this Sequence and reduces contention.
- std::unique_ptr<Task> delete_outside_lock_scope;
- bool sequence_empty_after_pop = false;
+TaskTraits Sequence::PeekTaskTraits() const {
+ AutoSchedulerLock auto_lock(lock_);
+ DCHECK(!queue_.empty());
+ DCHECK(queue_.front());
+ return queue_.front()->traits;
+}
- {
- AutoSchedulerLock auto_lock(lock_);
- DCHECK(!queue_.empty());
-
- const int priority_index =
- static_cast<int>(queue_.front()->traits.priority());
- DCHECK_GT(num_tasks_per_priority_[priority_index], 0U);
- --num_tasks_per_priority_[priority_index];
-
- delete_outside_lock_scope = std::move(queue_.front());
- queue_.pop();
- sequence_empty_after_pop = queue_.empty();
- }
-
- return sequence_empty_after_pop;
+bool Sequence::Pop() {
+ AutoSchedulerLock auto_lock(lock_);
+ DCHECK(!queue_.empty());
+ DCHECK(!queue_.front());
+ queue_.pop();
+ return queue_.empty();
}
SequenceSortKey Sequence::GetSortKey() const {
diff --git a/base/task_scheduler/sequence.h b/base/task_scheduler/sequence.h
index 8717336..408d99f 100644
--- a/base/task_scheduler/sequence.h
+++ b/base/task_scheduler/sequence.h
@@ -22,7 +22,10 @@
namespace base {
namespace internal {
-// A sequence holds tasks that must be executed in posting order.
+// A Sequence holds slots each containing up to a single Task that must be
+// executed in posting order.
+//
+// In comments below, an "empty Sequence" is a Sequence with no slot.
//
// Note: there is a known refcounted-ownership cycle in the Scheduler
// architecture: Sequence -> Task -> TaskRunner -> Sequence -> ...
@@ -41,20 +44,27 @@
public:
Sequence();
- // Adds |task| at the end of the sequence's queue. Returns true if the
- // sequence was empty before this operation.
+ // Adds |task| in a new slot at the end of the Sequence. Returns true if the
+ // Sequence was empty before this operation.
bool PushTask(std::unique_ptr<Task> task);
- // Returns the task in front of the sequence's queue, if any.
- const Task* PeekTask() const;
+ // Transfers ownership of the Task in the front slot of the Sequence to the
+ // caller. The front slot of the Sequence will be nullptr and remain until
+ // Pop(). Cannot be called on an empty Sequence or a Sequence whose front slot
+ // is already nullptr.
+ std::unique_ptr<Task> TakeTask();
- // Removes the task in front of the sequence's queue. Returns true if the
- // sequence is empty after this operation. Cannot be called on an empty
- // sequence.
- bool PopTask();
+ // Returns the TaskTraits of the Task in front of the Sequence. Cannot be
+ // called on an empty Sequence or on a Sequence whose front slot is empty.
+ TaskTraits PeekTaskTraits() const;
- // Returns a SequenceSortKey representing the priority of the sequence. Cannot
- // be called on an empty sequence.
+ // Removes the front slot of the Sequence. The front slot must have been
+ // emptied by TakeTask() before this is called. Cannot be called on an empty
+ // Sequence. Returns true if the Sequence is empty after this operation.
+ bool Pop();
+
+ // Returns a SequenceSortKey representing the priority of the Sequence. Cannot
+ // be called on an empty Sequence.
SequenceSortKey GetSortKey() const;
// Returns a token that uniquely identifies this Sequence.
@@ -72,7 +82,7 @@
// Queue of tasks to execute.
std::queue<std::unique_ptr<Task>> queue_;
- // Number of tasks contained in the sequence for each priority.
+ // Number of tasks contained in the Sequence for each priority.
size_t num_tasks_per_priority_[static_cast<int>(TaskPriority::HIGHEST) + 1] =
{};
diff --git a/base/task_scheduler/sequence_unittest.cc b/base/task_scheduler/sequence_unittest.cc
index 41a9794..ba020cb 100644
--- a/base/task_scheduler/sequence_unittest.cc
+++ b/base/task_scheduler/sequence_unittest.cc
@@ -9,6 +9,7 @@
#include "base/bind.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
+#include "base/test/gtest_util.h"
#include "base/time/time.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -17,30 +18,6 @@
namespace {
-// A class that pushes a Task to a Sequence in its destructor.
-class PushTaskInDestructor {
- public:
- explicit PushTaskInDestructor(scoped_refptr<Sequence> sequence)
- : sequence_(std::move(sequence)) {}
- PushTaskInDestructor(PushTaskInDestructor&&) = default;
- PushTaskInDestructor& operator=(PushTaskInDestructor&&) = default;
-
- ~PushTaskInDestructor() {
- // |sequence_| may be nullptr in a temporary instance of this class.
- if (sequence_) {
- EXPECT_FALSE(sequence_->PeekTask());
- sequence_->PushTask(WrapUnique(
- new Task(FROM_HERE, Closure(), TaskTraits(), TimeDelta())));
- }
- }
-
- private:
- scoped_refptr<Sequence> sequence_;
-
- DISALLOW_COPY_AND_ASSIGN(PushTaskInDestructor);
-};
-
-void DoNothing(const PushTaskInDestructor&) {}
class TaskSchedulerSequenceTest : public testing::Test {
public:
@@ -99,54 +76,54 @@
} // namespace
-TEST_F(TaskSchedulerSequenceTest, PushPopPeek) {
+TEST_F(TaskSchedulerSequenceTest, PushTakeRemove) {
scoped_refptr<Sequence> sequence(new Sequence);
// Push task A in the sequence. Its sequenced time should be updated and it
// should be in front of the sequence.
EXPECT_TRUE(sequence->PushTask(std::move(task_a_owned_)));
EXPECT_FALSE(task_a_->sequenced_time.is_null());
- EXPECT_EQ(task_a_, sequence->PeekTask());
+ EXPECT_EQ(task_a_->traits.priority(), sequence->PeekTaskTraits().priority());
// Push task B, C and D in the sequence. Their sequenced time should be
// updated and task A should always remain in front of the sequence.
EXPECT_FALSE(sequence->PushTask(std::move(task_b_owned_)));
EXPECT_FALSE(task_b_->sequenced_time.is_null());
- EXPECT_EQ(task_a_, sequence->PeekTask());
+ EXPECT_EQ(task_a_->traits.priority(), sequence->PeekTaskTraits().priority());
EXPECT_FALSE(sequence->PushTask(std::move(task_c_owned_)));
EXPECT_FALSE(task_c_->sequenced_time.is_null());
- EXPECT_EQ(task_a_, sequence->PeekTask());
+ EXPECT_EQ(task_a_->traits.priority(), sequence->PeekTaskTraits().priority());
EXPECT_FALSE(sequence->PushTask(std::move(task_d_owned_)));
EXPECT_FALSE(task_d_->sequenced_time.is_null());
- EXPECT_EQ(task_a_, sequence->PeekTask());
+ EXPECT_EQ(task_a_->traits.priority(), sequence->PeekTaskTraits().priority());
- // Pop task A. Task B should now be in front.
- EXPECT_FALSE(sequence->PopTask());
- EXPECT_EQ(task_b_, sequence->PeekTask());
+ // Get the task in front of the sequence. It should be task A.
+ EXPECT_EQ(task_a_, sequence->TakeTask().get());
- // Pop task B. Task C should now be in front.
- EXPECT_FALSE(sequence->PopTask());
- EXPECT_EQ(task_c_, sequence->PeekTask());
+ // Remove the empty slot. Task B should now be in front.
+ EXPECT_FALSE(sequence->Pop());
+ EXPECT_EQ(task_b_, sequence->TakeTask().get());
- // Pop task C. Task D should now be in front.
- EXPECT_FALSE(sequence->PopTask());
- EXPECT_EQ(task_d_, sequence->PeekTask());
+ // Remove the empty slot. Task C should now be in front.
+ EXPECT_FALSE(sequence->Pop());
+ EXPECT_EQ(task_c_, sequence->TakeTask().get());
- // Push task E in the sequence. Its sequenced time should be updated and
- // task D should remain in front.
+ // Remove the empty slot. Task D should now be in front.
+ EXPECT_FALSE(sequence->Pop());
+ EXPECT_EQ(task_d_, sequence->TakeTask().get());
+
+ // Push task E in the sequence. Its sequenced time should be updated.
EXPECT_FALSE(sequence->PushTask(std::move(task_e_owned_)));
EXPECT_FALSE(task_e_->sequenced_time.is_null());
- EXPECT_EQ(task_d_, sequence->PeekTask());
- // Pop task D. Task E should now be in front.
- EXPECT_FALSE(sequence->PopTask());
- EXPECT_EQ(task_e_, sequence->PeekTask());
+ // Remove the empty slot. Task E should now be in front.
+ EXPECT_FALSE(sequence->Pop());
+ EXPECT_EQ(task_e_, sequence->TakeTask().get());
- // Pop task E. The sequence should now be empty.
- EXPECT_TRUE(sequence->PopTask());
- EXPECT_EQ(nullptr, sequence->PeekTask());
+ // Remove the empty slot. The sequence should now be empty.
+ EXPECT_TRUE(sequence->Pop());
}
TEST_F(TaskSchedulerSequenceTest, GetSortKey) {
@@ -181,21 +158,24 @@
// Pop task A. The highest priority is still USER_BLOCKING. The task in front
// of the sequence is now task B.
- sequence->PopTask();
+ sequence->TakeTask();
+ sequence->Pop();
EXPECT_EQ(
SequenceSortKey(TaskPriority::USER_BLOCKING, task_b_->sequenced_time),
sequence->GetSortKey());
// Pop task B. The highest priority is still USER_BLOCKING. The task in front
// of the sequence is now task C.
- sequence->PopTask();
+ sequence->TakeTask();
+ sequence->Pop();
EXPECT_EQ(
SequenceSortKey(TaskPriority::USER_BLOCKING, task_c_->sequenced_time),
sequence->GetSortKey());
// Pop task C. The highest priority is still USER_BLOCKING. The task in front
// of the sequence is now task D.
- sequence->PopTask();
+ sequence->TakeTask();
+ sequence->Pop();
EXPECT_EQ(
SequenceSortKey(TaskPriority::USER_BLOCKING, task_d_->sequenced_time),
sequence->GetSortKey());
@@ -209,25 +189,37 @@
// Pop task D. The highest priority is now from task E (BACKGROUND). The
// task in front of the sequence is now task E.
- sequence->PopTask();
+ sequence->TakeTask();
+ sequence->Pop();
EXPECT_EQ(SequenceSortKey(TaskPriority::BACKGROUND, task_e_->sequenced_time),
sequence->GetSortKey());
}
-TEST_F(TaskSchedulerSequenceTest, CanPushTaskInTaskDestructor) {
+// Verify that a DCHECK fires if Pop() is called on a sequence whose front slot
+// isn't empty.
+TEST_F(TaskSchedulerSequenceTest, PopNonEmptyFrontSlot) {
scoped_refptr<Sequence> sequence(new Sequence);
- sequence->PushTask(MakeUnique<Task>(
- FROM_HERE, Bind(&DoNothing, PushTaskInDestructor(sequence)), TaskTraits(),
- TimeDelta()));
+ sequence->PushTask(
+ MakeUnique<Task>(FROM_HERE, Bind(&DoNothing), TaskTraits(), TimeDelta()));
- // PushTask() is invoked on |sequence| when the popped Task is destroyed. If
- // PopTask() destroys the Task outside the scope of its lock as expected, no
- // deadlock will occur when PushTask() tries to acquire the Sequence's lock.
- sequence->PopTask();
+ EXPECT_DCHECK_DEATH({ sequence->Pop(); });
+}
- // Verify that |sequence| contains exactly one Task.
- EXPECT_TRUE(sequence->PeekTask());
- EXPECT_TRUE(sequence->PopTask());
+// Verify that a DCHECK fires if TakeTask() is called on a sequence whose front
+// slot is empty.
+TEST_F(TaskSchedulerSequenceTest, TakeEmptyFrontSlot) {
+ scoped_refptr<Sequence> sequence(new Sequence);
+ sequence->PushTask(
+ MakeUnique<Task>(FROM_HERE, Bind(&DoNothing), TaskTraits(), TimeDelta()));
+
+ EXPECT_TRUE(sequence->TakeTask());
+ EXPECT_DCHECK_DEATH({ sequence->TakeTask(); });
+}
+
+// Verify that a DCHECK fires if TakeTask() is called on an empty sequence.
+TEST_F(TaskSchedulerSequenceTest, TakeEmptySequence) {
+ scoped_refptr<Sequence> sequence(new Sequence);
+ EXPECT_DCHECK_DEATH({ sequence->TakeTask(); });
}
} // namespace internal
diff --git a/base/task_scheduler/task_scheduler_impl.cc b/base/task_scheduler/task_scheduler_impl.cc
index 70a71ba..af9639b 100644
--- a/base/task_scheduler/task_scheduler_impl.cc
+++ b/base/task_scheduler/task_scheduler_impl.cc
@@ -118,12 +118,12 @@
DCHECK(sequence);
const SequenceSortKey sort_key = sequence->GetSortKey();
- TaskTraits traits(sequence->PeekTask()->traits);
- // Update the priority of |traits| so that the next task in |sequence| runs
- // with the highest priority in |sequence| as opposed to the next task's
- // specific priority.
- traits.WithPriority(sort_key.priority());
+ // The next task in |sequence| should run in a worker pool suited for its
+ // traits, except for the priority which is adjusted to the highest priority
+ // in |sequence|.
+ const TaskTraits traits =
+ sequence->PeekTaskTraits().WithPriority(sort_key.priority());
GetWorkerPoolForTraits(traits)->ReEnqueueSequence(std::move(sequence),
sort_key);
diff --git a/base/task_scheduler/task_tracker.cc b/base/task_scheduler/task_tracker.cc
index eeb8e39..68e606b 100644
--- a/base/task_scheduler/task_tracker.cc
+++ b/base/task_scheduler/task_tracker.cc
@@ -205,7 +205,7 @@
return true;
}
-bool TaskTracker::RunTask(const Task* task,
+bool TaskTracker::RunTask(std::unique_ptr<Task> task,
const SequenceToken& sequence_token) {
DCHECK(task);
DCHECK(sequence_token.IsValid());
diff --git a/base/task_scheduler/task_tracker.h b/base/task_scheduler/task_tracker.h
index 93769f1..8c19456 100644
--- a/base/task_scheduler/task_tracker.h
+++ b/base/task_scheduler/task_tracker.h
@@ -57,7 +57,7 @@
// |sequence_token| is the token identifying the sequence from which |task|
// was extracted. Returns true if |task| ran. WillPostTask() must have allowed
// |task| to be posted before this is called.
- bool RunTask(const Task* task, const SequenceToken& sequence_token);
+ bool RunTask(std::unique_ptr<Task> task, const SequenceToken& sequence_token);
// Returns true once shutdown has started (Shutdown() has been called but
// might not have returned). Note: sequential consistency with the thread
diff --git a/base/task_scheduler/task_tracker_unittest.cc b/base/task_scheduler/task_tracker_unittest.cc
index 57dca1f..3a1ff78 100644
--- a/base/task_scheduler/task_tracker_unittest.cc
+++ b/base/task_scheduler/task_tracker_unittest.cc
@@ -7,6 +7,7 @@
#include <stdint.h>
#include <memory>
+#include <utility>
#include <vector>
#include "base/bind.h"
@@ -77,7 +78,26 @@
tracker_(tracker),
task_(task),
action_(action),
- expect_post_succeeds_(expect_post_succeeds) {}
+ expect_post_succeeds_(expect_post_succeeds) {
+ EXPECT_TRUE(task_);
+
+ // Ownership of the Task is required to run it.
+ EXPECT_NE(Action::RUN, action_);
+ EXPECT_NE(Action::WILL_POST_AND_RUN, action_);
+ }
+
+ ThreadPostingAndRunningTask(TaskTracker* tracker,
+ std::unique_ptr<Task> task,
+ Action action,
+ bool expect_post_succeeds)
+ : SimpleThread("ThreadPostingAndRunningTask"),
+ tracker_(tracker),
+ task_(task.get()),
+ owned_task_(std::move(task)),
+ action_(action),
+ expect_post_succeeds_(expect_post_succeeds) {
+ EXPECT_TRUE(task_);
+ }
private:
void Run() override {
@@ -88,12 +108,14 @@
}
if (post_succeeded &&
(action_ == Action::RUN || action_ == Action::WILL_POST_AND_RUN)) {
- tracker_->RunTask(task_, SequenceToken::Create());
+ EXPECT_TRUE(owned_task_);
+ tracker_->RunTask(std::move(owned_task_), SequenceToken::Create());
}
}
TaskTracker* const tracker_;
Task* const task_;
+ std::unique_ptr<Task> owned_task_;
const Action action_;
const bool expect_post_succeeds_;
@@ -229,7 +251,7 @@
// Run the task.
EXPECT_EQ(0U, NumTasksExecuted());
- EXPECT_TRUE(tracker_.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(task), SequenceToken::Create()));
EXPECT_EQ(1U, NumTasksExecuted());
// Shutdown() shouldn't block.
@@ -240,16 +262,17 @@
// Create a task that will block until |event| is signaled.
WaitableEvent event(WaitableEvent::ResetPolicy::AUTOMATIC,
WaitableEvent::InitialState::NOT_SIGNALED);
- Task blocked_task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)),
- TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ auto blocked_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
// Inform |task_tracker_| that |blocked_task| will be posted.
- EXPECT_TRUE(tracker_.WillPostTask(&blocked_task));
+ EXPECT_TRUE(tracker_.WillPostTask(blocked_task.get()));
// Run the task asynchronouly.
ThreadPostingAndRunningTask thread_running_task(
- &tracker_, &blocked_task, ThreadPostingAndRunningTask::Action::RUN,
- false);
+ &tracker_, std::move(blocked_task),
+ ThreadPostingAndRunningTask::Action::RUN, false);
thread_running_task.Start();
// Initiate shutdown while the task is running.
@@ -291,13 +314,14 @@
// should be discarded.
EXPECT_EQ(0U, NumTasksExecuted());
const bool should_run = GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN;
- EXPECT_EQ(should_run, tracker_.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_EQ(should_run,
+ tracker_.RunTask(std::move(task), SequenceToken::Create()));
EXPECT_EQ(should_run ? 1U : 0U, NumTasksExecuted());
VERIFY_ASYNC_SHUTDOWN_IN_PROGRESS();
// Unblock shutdown by running the remaining BLOCK_SHUTDOWN task.
- EXPECT_TRUE(
- tracker_.RunTask(block_shutdown_task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(block_shutdown_task),
+ SequenceToken::Create()));
EXPECT_EQ(should_run ? 2U : 1U, NumTasksExecuted());
WAIT_FOR_ASYNC_SHUTDOWN_COMPLETED();
}
@@ -315,7 +339,7 @@
VERIFY_ASYNC_SHUTDOWN_IN_PROGRESS();
// Run the task to unblock shutdown.
- EXPECT_TRUE(tracker_.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(task), SequenceToken::Create()));
EXPECT_EQ(1U, NumTasksExecuted());
WAIT_FOR_ASYNC_SHUTDOWN_COMPLETED();
@@ -326,7 +350,7 @@
WAIT_FOR_ASYNC_SHUTDOWN_COMPLETED();
// The task shouldn't be allowed to run after shutdown.
- EXPECT_FALSE(tracker_.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_FALSE(tracker_.RunTask(std::move(task), SequenceToken::Create()));
EXPECT_EQ(0U, NumTasksExecuted());
}
}
@@ -349,7 +373,7 @@
// Run the BLOCK_SHUTDOWN task.
EXPECT_EQ(0U, NumTasksExecuted());
- EXPECT_TRUE(tracker_.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(task), SequenceToken::Create()));
EXPECT_EQ(1U, NumTasksExecuted());
} else {
// It shouldn't be allowed to post a non BLOCK_SHUTDOWN task.
@@ -361,8 +385,8 @@
// Unblock shutdown by running |block_shutdown_task|.
VERIFY_ASYNC_SHUTDOWN_IN_PROGRESS();
- EXPECT_TRUE(
- tracker_.RunTask(block_shutdown_task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(block_shutdown_task),
+ SequenceToken::Create()));
EXPECT_EQ(GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN ? 2U : 1U,
NumTasksExecuted());
WAIT_FOR_ASYNC_SHUTDOWN_COMPLETED();
@@ -400,10 +424,10 @@
// Running the task should fail iff the task isn't allowed to use singletons.
if (can_use_singletons) {
- EXPECT_TRUE(tracker.RunTask(task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker.RunTask(std::move(task), SequenceToken::Create()));
} else {
EXPECT_DCHECK_DEATH(
- { tracker.RunTask(task.get(), SequenceToken::Create()); });
+ { tracker.RunTask(std::move(task), SequenceToken::Create()); });
}
}
@@ -418,7 +442,8 @@
EXPECT_FALSE(ThreadTaskRunnerHandle::IsSet());
EXPECT_FALSE(SequencedTaskRunnerHandle::IsSet());
- EXPECT_TRUE(tracker->RunTask(verify_task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(
+ tracker->RunTask(std::move(verify_task), SequenceToken::Create()));
// TaskRunnerHandle state is reset outside of task's scope.
EXPECT_FALSE(ThreadTaskRunnerHandle::IsSet());
@@ -498,10 +523,10 @@
}
TEST_P(TaskSchedulerTaskTrackerTest, FlushPendingUndelayedTask) {
- const Task undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&undelayed_task);
+ auto undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(undelayed_task.get());
// Flush() shouldn't return before the undelayed task runs.
CallFlushAsync();
@@ -509,15 +534,15 @@
VERIFY_ASYNC_FLUSH_IN_PROGRESS();
// Flush() should return after the undelayed task runs.
- tracker_.RunTask(&undelayed_task, SequenceToken::Create());
+ tracker_.RunTask(std::move(undelayed_task), SequenceToken::Create());
WAIT_FOR_ASYNC_FLUSH_RETURNED();
}
TEST_P(TaskSchedulerTaskTrackerTest, PostTaskDuringFlush) {
- const Task undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&undelayed_task);
+ auto undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(undelayed_task.get());
// Flush() shouldn't return before the undelayed task runs.
CallFlushAsync();
@@ -525,33 +550,33 @@
VERIFY_ASYNC_FLUSH_IN_PROGRESS();
// Simulate posting another undelayed task.
- const Task other_undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&other_undelayed_task);
+ auto other_undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(other_undelayed_task.get());
// Run the first undelayed task.
- tracker_.RunTask(&undelayed_task, SequenceToken::Create());
+ tracker_.RunTask(std::move(undelayed_task), SequenceToken::Create());
// Flush() shouldn't return before the second undelayed task runs.
PlatformThread::Sleep(TestTimeouts::tiny_timeout());
VERIFY_ASYNC_FLUSH_IN_PROGRESS();
// Flush() should return after the second undelayed task runs.
- tracker_.RunTask(&other_undelayed_task, SequenceToken::Create());
+ tracker_.RunTask(std::move(other_undelayed_task), SequenceToken::Create());
WAIT_FOR_ASYNC_FLUSH_RETURNED();
}
TEST_P(TaskSchedulerTaskTrackerTest, RunDelayedTaskDuringFlush) {
// Simulate posting a delayed and an undelayed task.
- const Task delayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta::FromDays(1));
- tracker_.WillPostTask(&delayed_task);
- const Task undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&undelayed_task);
+ auto delayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta::FromDays(1));
+ tracker_.WillPostTask(delayed_task.get());
+ auto undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(undelayed_task.get());
// Flush() shouldn't return before the undelayed task runs.
CallFlushAsync();
@@ -559,7 +584,7 @@
VERIFY_ASYNC_FLUSH_IN_PROGRESS();
// Run the delayed task.
- tracker_.RunTask(&delayed_task, SequenceToken::Create());
+ tracker_.RunTask(std::move(delayed_task), SequenceToken::Create());
// Flush() shouldn't return since there is still a pending undelayed
// task.
@@ -567,7 +592,7 @@
VERIFY_ASYNC_FLUSH_IN_PROGRESS();
// Run the undelayed task.
- tracker_.RunTask(&undelayed_task, SequenceToken::Create());
+ tracker_.RunTask(std::move(undelayed_task), SequenceToken::Create());
// Flush() should now return.
WAIT_FOR_ASYNC_FLUSH_RETURNED();
@@ -578,10 +603,10 @@
return;
// Simulate posting a task.
- const Task undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&undelayed_task);
+ auto undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(undelayed_task.get());
// Shutdown() should return immediately since there are no pending
// BLOCK_SHUTDOWN tasks.
@@ -597,10 +622,10 @@
return;
// Simulate posting a task.
- const Task undelayed_task(FROM_HERE, Bind(&DoNothing),
- TaskTraits().WithShutdownBehavior(GetParam()),
- TimeDelta());
- tracker_.WillPostTask(&undelayed_task);
+ auto undelayed_task = base::MakeUnique<Task>(
+ FROM_HERE, Bind(&DoNothing),
+ TaskTraits().WithShutdownBehavior(GetParam()), TimeDelta());
+ tracker_.WillPostTask(undelayed_task.get());
// Flush() shouldn't return before the undelayed task runs or
// shutdown completes.
@@ -641,36 +666,33 @@
// when a Task runs.
TEST_F(TaskSchedulerTaskTrackerTest, CurrentSequenceToken) {
const SequenceToken sequence_token(SequenceToken::Create());
- Task task(FROM_HERE, Bind(&ExpectSequenceToken, sequence_token), TaskTraits(),
- TimeDelta());
- tracker_.WillPostTask(&task);
+ auto task = base::MakeUnique<Task>(FROM_HERE,
+ Bind(&ExpectSequenceToken, sequence_token),
+ TaskTraits(), TimeDelta());
+ tracker_.WillPostTask(task.get());
EXPECT_FALSE(SequenceToken::GetForCurrentThread().IsValid());
- EXPECT_TRUE(tracker_.RunTask(&task, sequence_token));
+ EXPECT_TRUE(tracker_.RunTask(std::move(task), sequence_token));
EXPECT_FALSE(SequenceToken::GetForCurrentThread().IsValid());
}
TEST_F(TaskSchedulerTaskTrackerTest, LoadWillPostAndRunBeforeShutdown) {
// Post and run tasks asynchronously.
- std::vector<std::unique_ptr<Task>> tasks;
std::vector<std::unique_ptr<ThreadPostingAndRunningTask>> threads;
for (size_t i = 0; i < kLoadTestNumIterations; ++i) {
- tasks.push_back(CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, true));
threads.back()->Start();
- tasks.push_back(CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, true));
threads.back()->Start();
- tasks.push_back(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, true));
threads.back()->Start();
}
@@ -720,9 +742,9 @@
// Run tasks asynchronously.
std::vector<std::unique_ptr<ThreadPostingAndRunningTask>> run_threads;
- for (const auto& task : tasks) {
+ for (auto& task : tasks) {
run_threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, task.get(), ThreadPostingAndRunningTask::Action::RUN,
+ &tracker_, std::move(task), ThreadPostingAndRunningTask::Action::RUN,
false));
run_threads.back()->Start();
}
@@ -747,25 +769,21 @@
CallShutdownAsync();
// Post and run tasks asynchronously.
- std::vector<std::unique_ptr<Task>> tasks;
std::vector<std::unique_ptr<ThreadPostingAndRunningTask>> threads;
for (size_t i = 0; i < kLoadTestNumIterations; ++i) {
- tasks.push_back(CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, false));
threads.back()->Start();
- tasks.push_back(CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::SKIP_ON_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, false));
threads.back()->Start();
- tasks.push_back(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
threads.push_back(MakeUnique<ThreadPostingAndRunningTask>(
- &tracker_, tasks.back().get(),
+ &tracker_, CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN),
ThreadPostingAndRunningTask::Action::WILL_POST_AND_RUN, true));
threads.back()->Start();
}
@@ -780,8 +798,8 @@
VERIFY_ASYNC_SHUTDOWN_IN_PROGRESS();
// Unblock shutdown by running |block_shutdown_task|.
- EXPECT_TRUE(
- tracker_.RunTask(block_shutdown_task.get(), SequenceToken::Create()));
+ EXPECT_TRUE(tracker_.RunTask(std::move(block_shutdown_task),
+ SequenceToken::Create()));
EXPECT_EQ(kLoadTestNumIterations + 1, NumTasksExecuted());
WAIT_FOR_ASYNC_SHUTDOWN_COMPLETED();
}