TaskScheduler [3/9] Task and Sequence

base/task_scheduler/sequence.cc

Index: base/task_scheduler/sequence.cc
diff --git a/base/task_scheduler/sequence.cc b/base/task_scheduler/sequence.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3cf44badee1812e4a3e0ef74ff806460c9a5b9c3
--- /dev/null
+++ b/base/task_scheduler/sequence.cc
@@ -0,0 +1,67 @@
+// 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() = default;
+
+bool Sequence::PushTask(scoped_ptr<Task> task) {
+  AutoSchedulerLock auto_lock(lock_);
+
+  ++num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>(

[robliao, 2016/02/19 02:33:43]

Maybe a temporary for the static_cast would make this more readable:
TaskPriorityUnderlyingType index_priority =
    static_cast<TaskPriorityUnderlyingType>(task->traits.priority());
++num_tasks_per_priority_[index_priority]

Same with below.

[fdoray, 2016/02/19 14:12:14]

Done.

+      task->traits.priority())];
+  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());

[robliao, 2016/02/19 02:33:43]

I'm thinking this should be a CHECK. It is undefined what happens when you pop
an empty queue (so no exception may be thrown). Ideally, it would crash. 

We don't rely on this behavior, do we?

[fdoray, 2016/02/19 14:12:14]

Done.

https://www.chromium.org/developers/coding-style says "Use CHECK() if the
consequence of a failed assertion would be a security vulnerability". It is an
error to call PopTask on an empty sequence. If it happens, something is wrong
and I guess it is better to crash than to have an undefined behavior.

[gab, 2016/02/19 16:50:47]

I don't think this should be a CHECK.

It's enforcing the method's contract and it would only fire if the algorithm is
wrong which is okay to only verify in debug builds IMO (i.e. it's not dynamic
based on user input or anything which is what the CHECK comment you linked to is
trying to articulate I think).

For example, Lock only verifies correct usage in DCHECK_IS_ON() builds despite
many things resulting in undefined behaviour (locking twice, releasing twice,
deleting an owned lock).

[fdoray, 2016/02/19 22:28:29]

Done. I do prefer DCHECK, but I agreed to change it to CHECK because not
crashing leads to an undefined behavior, which can be dangerous. For the usual
check that a pointer is not null before dereferencing, there is no undefined
behavior: we know that the process will crash.

It is still unclear to me when CHECK is appropriate. Clearly, for user input, it
is better to show an error than to provoke a crash with CHECK.

[gab, 2016/02/22 17:12:21]

Agreed that for identifiable user input errors a nice error is much prefered to
a CHECK.

Still I think CHECK doesn't belong anywhere in this file. This API is internal
to the task scheduler, and DCHECKs are sufficient to verify that the code uses
it correctly and then be confident that it can't be triggered the wrong way in
Release builds.

I guess CHECK applies when you can't be confident that the lack of DCHECK hits
in a debug build implies that the situation will never occur in a Release build
(and is a security issue if it does).

[fdoray, 2016/02/22 18:07:54]

Ack.

+
+  --num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>(
+      queue_.front()->traits.priority())];
+  queue_.pop();
+  return queue_.empty();
+}
+
+SequenceSortKey Sequence::GetSortKey() const {
+  AutoSchedulerLock auto_lock(lock_);
+  DCHECK(!queue_.empty());

[fdoray, 2016/02/19 14:12:14]

CHECK here too, because front() has an undefined behavior on an empty queue?

+
+  // Find the highest task priority in the sequence.
+  TaskPriority priority = TaskPriority::LOWEST;
+  for (size_t i =
+           static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST);
+       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);
+}
+
+Sequence::~Sequence() = default;
+
+}  // namespace internal
+}  // namespace base