content: Add RendererScheduler.

content/renderer/scheduler/renderer_scheduler_selector.cc

Index: content/renderer/scheduler/renderer_scheduler_selector.cc
diff --git a/content/renderer/scheduler/renderer_scheduler_selector.cc b/content/renderer/scheduler/renderer_scheduler_selector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7d312b8a096afb4558af3c8e40240dd58cfe0f7d
--- /dev/null
+++ b/content/renderer/scheduler/renderer_scheduler_selector.cc
@@ -0,0 +1,115 @@
+// Copyright 2014 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 "content/renderer/scheduler/renderer_scheduler_selector.h"
+
+#include "base/logging.h"
+#include "base/pending_task.h"
+
+namespace content {
+
+namespace {
+  int kBitsInByte = 8;
+}

[picksi1, 2014/10/20 11:42:04]

Does this exist elsewhere as a constant already?

[rmcilroy, 2014/10/21 17:21:27]

Changed to CHAR_BITS as suggested by Petr.

+
+RendererSchedulerSelector::RendererSchedulerSelector() { }
+
+RendererSchedulerSelector::~RendererSchedulerSelector() { }
+
+void RendererSchedulerSelector::RegisterWorkQueues(
+    const std::vector<const base::TaskQueue*>& work_queues) {
+  main_thread_checker_.CalledOnValidThread();
+  DCHECK(work_queues.size() < queue_priorities_[kNormalPriority] * kBitsInByte);
+  work_queues_ = work_queues;
+  // By default, all work queues are set to normal priority.
+  queue_priorities_[kControlPriority] = 0;
+  queue_priorities_[kHighPriority] = 0;
+  queue_priorities_[kNormalPriority] = (1 << work_queues.size()) - 1;

[picksi1, 2014/10/20 11:42:04]

Is it worth doing setting the initial bits here to all zero's and then having a
for loop that calls EnableQueue() setting all queues as kNormalPriority? I know
it would be (much) slower but it would make it clearer than the base
2-voodoo(!), and helps 'encapsulate' the implementation details.

[rmcilroy, 2014/10/21 17:21:27]

I could do this but I'm not sure it's worth it.  How about encapsolating the
Voodoo in an EnableAllQueues(Priority) function?

+  queue_priorities_[kBestEffortPriority] = 0;
+}
+
+RendererSchedulerSelector::QueuePriority RendererSchedulerSelector::NextPriorty(
+    QueuePriority priority) {
+  DCHECK(priority < kQueuePriorityCount);
+  return static_cast<QueuePriority>(static_cast<int>(priority) + 1);
+}
+
+void RendererSchedulerSelector::SetQueuePriority(
+    size_t queue_id, QueuePriority set_priority) {
+  main_thread_checker_.CalledOnValidThread();
+  DCHECK(queue_id < work_queues_.size());
+  DCHECK(set_priority < kQueuePriorityCount);
+  int64_t queue_id_mask_ = 1 << queue_id;
+  for (QueuePriority priority = kControlPriority;
+       priority < kQueuePriorityCount;
+       priority = NextPriorty(priority)) {
+    if (priority == set_priority) {
+      queue_priorities_[priority] |= queue_id_mask_;
+    } else {
+      queue_priorities_[priority] &= ~queue_id_mask_;
+    }
+  }

[picksi1, 2014/10/20 11:42:04]

Does each queue only exist in a single 'priority' list? If so we could track
where each queue was enabled and then this loop could change into something
like:

// Remove old flag
queue_priorities_[ owner[queue_id] ] &= ~queue_id_mask_;
// Change where this flag is set
owner[queue_id] = set_priority;
// Set the flag
queue_priorities_[ owner[queue_id] ] |= queue_id_mask_;


... or ...

is it worth simplifying this function to:

// Clear from all priorities
DisableQueue(queue_id);
// Set for the priority requested
queue_priorities_[priority] |= queue_id_mask_;

[rmcilroy, 2014/10/21 17:21:27]

Good point, I went with the second option. Done.

+}
+
+void RendererSchedulerSelector::EnableQueue(
+    size_t queue_id, QueuePriority priority) {
+  SetQueuePriority(queue_id, priority);
+}
+
+void RendererSchedulerSelector::DisableQueue(size_t queue_id) {
+  main_thread_checker_.CalledOnValidThread();
+  DCHECK(queue_id < work_queues_.size());
+  int64_t queue_id_mask_ = 1 << queue_id;
+  for (QueuePriority priority = kControlPriority;
+       priority < kQueuePriorityCount;
+       priority = NextPriorty(priority)) {
+    queue_priorities_[priority] &= ~queue_id_mask_;
+  }
+}
+
+bool RendererSchedulerSelector::QueueEnabledWithPriority(
+    size_t queue_id, QueuePriority priority) {
+  DCHECK(queue_id < work_queues_.size());
+  DCHECK(priority < kQueuePriorityCount);
+  return queue_priorities_[priority] & (1 << queue_id);
+}

[picksi1, 2014/10/20 11:42:04]

There's a lot of (1<<queue_id)'s in the code. Should we wrap them as a function
in case we (go mad and) decide to change how we allocate bits to queues?

[rmcilroy, 2014/10/21 17:21:27]

Done.

+
+bool RendererSchedulerSelector::ChooseOldestWithPriority(
+    QueuePriority priority, size_t* out_queue_index) {
+  bool found_non_empty_queue = false;
+  size_t chosen_queue = 0;
+  for (size_t queue_id = 0; queue_id < work_queues_.size(); queue_id++) {
+    if (!QueueEnabledWithPriority(queue_id, priority) ||
+        work_queues_[queue_id]->empty()) {
+      continue;
+    }

[picksi1, 2014/10/20 11:42:04]

For clarity it might be worth breaking this 'if' statement into 2 instead of
or'ing ~unrelated tests together. If the empty check was done first it would be
clearer too.

[rmcilroy, 2014/10/21 17:21:27]

I changed the order, but I'm not sure I agree that breaking the if into 2 would
be clearer.  Would you be are you OK with this now?

+    if (!found_non_empty_queue ||
+        work_queues_[queue_id]->front() < work_queues_[chosen_queue]->front()) {
+      found_non_empty_queue = true;
+      chosen_queue = queue_id;
+    }
+  }
+
+  if (found_non_empty_queue) {
+    *out_queue_index = chosen_queue;
+  }
+  return found_non_empty_queue;
+}
+
+bool RendererSchedulerSelector::SelectWorkQueueToService(
+    size_t* out_queue_index) {
+  main_thread_checker_.CalledOnValidThread();
+  DCHECK(work_queues_.size());
+  for (QueuePriority priority = kControlPriority;
+       priority < kQueuePriorityCount;
+       priority = NextPriorty(priority)) {
+    if (ChooseOldestWithPriority(priority, out_queue_index)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+
+}  // namespace content