TaskScheduler [3/9] Task and Sequence

base/task_scheduler/sequence.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/sequence.h"
+
+#include <utility>
+
+#include "base/logging.h"
+#include "base/time/time.h"
+
+namespace base {
+namespace internal {
+
+Sequence::Sequence() = default;
+
+bool Sequence::PushTask(scoped_ptr<Task> task) {
+  DCHECK(task->sequenced_time.is_null());
+  task->sequenced_time = base::TimeTicks::Now();
+
+  AutoSchedulerLock auto_lock(lock_);
+
+  const TaskPriorityUnderlyingType priority_index =
+      static_cast<TaskPriorityUnderlyingType>(task->traits.priority());

[danakj, 2016/03/15 23:16:31]

Can you put code that doesn't access members above the auto_lock?

[fdoray, 2016/03/16 19:11:43]

|priority_index| was there to make the code simpler. Now that I cast to int
instead of TaskPriorityUnderlyingType, everything fits on 1 line and I don't
think that |priority_index| is required anymore. WDYT?

+  ++num_tasks_per_priority_[priority_index];
+
+  queue_.push(std::move(task));
+
+  // Return true if the sequence was empty before the push.
+  return queue_.size() == 1;
+}
+
+const Task* Sequence::PeekTask() const {
+  AutoSchedulerLock auto_lock(lock_);
+
+  if (queue_.empty())
+    return nullptr;
+
+  return queue_.front().get();
+}
+
+bool Sequence::PopTask() {
+  AutoSchedulerLock auto_lock(lock_);
+  DCHECK(!queue_.empty());
+
+  const TaskPriorityUnderlyingType priority_index =
+      static_cast<TaskPriorityUnderlyingType>(
+          queue_.front()->traits.priority());
+  --num_tasks_per_priority_[priority_index];

[danakj, 2016/03/15 23:16:31]

maybe DCHECK that num_tasks_per_priority > 0 before this?

[fdoray, 2016/03/16 19:11:43]

Done.

+
+  queue_.pop();
+  return queue_.empty();
+}
+
+SequenceSortKey Sequence::GetSortKey() const {
+  AutoSchedulerLock auto_lock(lock_);
+  DCHECK(!queue_.empty());
+
+  // Find the highest task priority in the sequence.
+  static_assert(static_cast<TaskPriorityUnderlyingType>(
+                    TaskPriority::HIGHEST) == internal::kNumTaskPriorities - 1,

[danakj, 2016/03/15 23:16:32]

Do you even needs kNumTaskPriorities? Why not just use HIGHEST (or HIGHEST+1)
instead?

[fdoray, 2016/03/16 19:11:43]

Done. kNumTaskPriorities is not needed.

+                "The value of TaskPriority::HIGHEST must be the number of "
+                "priorities minus 1.");
+  TaskPriority priority = TaskPriority::LOWEST;
+  for (TaskPriorityUnderlyingType i =
+           static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST);

[danakj, 2016/03/15 23:16:31]

This would probably be a lot easier to read if you made some temp vars for
highest and lowest outside the loop and used those here instead of
static_casting inside the for declaration.

[fdoray, 2016/03/16 19:11:43]

Done.

+       i > static_cast<TaskPriorityUnderlyingType>(TaskPriority::LOWEST); --i) {
+    if (num_tasks_per_priority_[i] > 0) {
+      priority = static_cast<TaskPriority>(i);
+      break;
+    }
+  }
+
+  return SequenceSortKey(priority, queue_.front()->sequenced_time);

[danakj, 2016/03/15 23:16:31]

this is a bit pendantic, but you could put the sequenced time into a local
variable, then construct and return the SequenceSortKey after releasing the
lock_.

[fdoray, 2016/03/16 19:11:43]

Done.

+}
+
+Sequence::~Sequence() = default;
+
+}  // namespace internal
+}  // namespace base