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"
+
+namespace base {
+namespace internal {
+
+Sequence::Sequence() : num_tasks_per_priority_() {}

[gab, 2016/02/18 03:00:45]

Does |num_task_per_priority_| really need to be explicitly initialized here?

If so use "Class Member Initializers" (ref: https://chromium-cpp.appspot.com/)

[fdoray, 2016/02/18 14:56:12]

http://stackoverflow.com/questions/15212261/default-initialization-of-pod-typ...
says that pod types are not initialized by default. I now use a class member
initializer.

+
+void Sequence::PushTask(scoped_ptr<Task> task, size_t* prev_num_tasks) {
+  AutoSchedulerLock auto_lock(lock_);
+
+  ++num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>(
+      task->traits.priority())];
+  *prev_num_tasks = queue_.size();
+  queue_.push(std::move(task));
+}
+
+const Task* Sequence::PeekTask() {
+  AutoSchedulerLock auto_lock(lock_);
+
+  if (queue_.empty())
+    return nullptr;
+
+  return queue_.front().get();
+}
+
+void Sequence::PopTask(size_t* new_num_tasks) {
+  DCHECK(new_num_tasks);
+  AutoSchedulerLock auto_lock(lock_);

[fdoray, 2016/02/18 01:46:11]

Put AutoSchedulerLock first, to be consistent with other methods.

[gab, 2016/02/18 03:00:45]

I actually like having it second but would put an empty line after the DCHECK.

The DCHECK verifies the contract of the method and should always come first
(i.e. not even be considered part of the method's body, it's like the
verification of the API contract before the body, e.g. what annotations are
meant for in Java)

Edit: Now I see that the next line is the DCHECK(!queue_.empty()); call which is
also verifying the contract but requires the lock... sigh...! In that light I
guess I agree and prefer the AutoLock at the top..!

[fdoray, 2016/02/18 14:56:12]

Done (new_num_tasks is gone)

+  DCHECK(!queue_.empty());
+
+  --num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>(
+      queue_.front()->traits.priority())];
+  queue_.pop();
+  *new_num_tasks = queue_.size();
+}
+
+SequenceSortKey Sequence::GetSortKey() {
+  AutoSchedulerLock auto_lock(lock_);
+  DCHECK(!queue_.empty());
+
+  // Find the highest task priority in the sequence.
+  TaskPriority priority = TaskPriority::BACKGROUND;

[fdoray, 2016/02/18 01:46:11]

This loop relies on the fact that the priority values are in [0,
kNumTaskPriorities - 1]. Maybe we should add this in task_traits.h:

// sequence.h has an array of counters indexed by the underlying values of
TaskPriority. That means that the priority values must be in [0,
kNumTaskPriorities - 1].
static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::BACKGROUND)
== 0, "The value of the lower priority must be 0.");
static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::USER_BLOCKING)
== kNumTaskPriorities - 1, "The value of the highest priority must be
kNumTaskPriorities - 1.");

[gab, 2016/02/18 03:00:45]

Agreed, also line 57 in header ( size_t
num_tasks_per_priority_[kNumTaskPriorities]; ) relies on them being sequential.

Should we instead declare it as :

namespace internal {

using TaskPriorityUnderlyingType = uint8;

}  // namespace internal

TaskPriority : TaskPriorityUnderlyingType {
  BACKGROUND = 0,
  USER_VISIBLE,
  USER_BLOCKING,
  HIGHEST = USER_BLOCKING
}

const TaskPriorityUnderlyingType kNumTaskPriorities = TaskPriority::HIGHEST + 1;

and let it just be a zero-based list without assigning explicit value and thus
not requiring tons of static assert?

This design also nicely allows a task to pick TaskPriority::HIGHEST as a
priority if it thinks it really wants to always be at the highest possible
priority should we add one above USER_BLOCKING as discussed earlier today.

Note that I also just had this thought that TaskPriorityUnderlyingType should
probably be in the internal namespace (i.e. it's meant for us to manage internal
state, it's not meant for people to use the underlying value from the API --
otherwise we'd just make it an enum, not an enum class).

PS: Let's make these changes in this CL and let the other one land.

+  for (TaskPriorityUnderlyingType i = kNumTaskPriorities - 1; i >= 0; --i) {

[fdoray, 2016/02/18 01:46:11]

static_assert(std::is_signed<TaskPriorityUnderlyingType>::value, "");

[gab, 2016/02/18 03:00:45]

Hmmm, why? I'm actually proposing using uint8 above. |kNumTaskPriorities - 1| is
only unsafe is we ever think kNumTaskPriorities would be 0 which would be
utterly crazy IMO!

[fdoray, 2016/02/18 14:56:12]

|i| needs to be signed. Otherwise, the i >=0 condition will always be true. But
you're right that the enum type doesn't have to be signed.

+    if (num_tasks_per_priority_[i] > 0) {
+      priority = static_cast<TaskPriority>(i);
+      break;
+    }
+  }
+
+  return SequenceSortKey(priority, queue_.front()->post_time);
+}
+
+Sequence::~Sequence() = default;
+
+}  // namespace internal
+}  // namespace base