scheduler: Add support for tracing scheduler state

content/renderer/scheduler/renderer_scheduler_selector.h

 // 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.
 
 #ifndef CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_
 #define CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_
 
 #include <set>
 
+#include "base/memory/ref_counted.h"
 #include "base/threading/thread_checker.h"
 #include "content/renderer/scheduler/task_queue_selector.h"
 
+namespace base {
+namespace debug {
+class ConvertableToTraceFormat;
+class TracedValue;
+}
+}
+
 namespace content {
 
 // A RendererSchedulerSelector is a TaskQueueSelector which is used by the
 // RendererScheduler to enable prioritization of particular queues depending on
 // circumstances.
 class RendererSchedulerSelector : public TaskQueueSelector {
  public:
   enum QueuePriority {
     // Queues with control priority will run before any other queue, and will
     // explicitly starve other queues. Typically this should only be used for
@@ skipping 24 matching lines @@
   void EnableQueue(size_t queue_index, QueuePriority priority);
 
   // Disable the |queue_index|.
   void DisableQueue(size_t queue_index);
 
   // TaskQueueSelector implementation:
   void RegisterWorkQueues(
       const std::vector<const base::TaskQueue*>& work_queues) override;
   bool SelectWorkQueueToService(size_t* out_queue_index) override;
 
+  // Serialize the selector state into the given state dictionary for tracing.
+  void AsValueInto(base::debug::TracedValue* state) const;
+

[picksi1, 2014/10/28 11:01:40]

"AsValueInto" means nothing to me! Is there some idiom I am unaware of?

[Sami, 2014/10/28 12:57:47]

Agreed that it isn't very descriptive, but it's pretty common tracing vernacular
so I'll follow the pattern:
https://code.google.com/p/chromium/codesearch#search/&q=asvalueinto&sq=packag...

+  // Set the name |queue_index| for tracing purposes. |name| must be a pointer
+  // to a static string.
+  void SetQueueName(size_t queue_index, const char* name);
+
  private:
   // Returns true if queueA contains an older task than queueB.
   static bool IsOlder(const base::TaskQueue* queueA,
                       const base::TaskQueue* queueB);
 
   // Returns the priority which is next after |priority|.
   static QueuePriority NextPriority(QueuePriority priority);
 
   // Return true if |out_queue_index| indicates the index of the queue with
   // the oldest pending task from the set of queues of |priority|, or
   // false if all queues of that priority are empty.
   bool ChooseOldestWithPriority(QueuePriority priority,
                                 size_t* out_queue_index) const;
 
   // Returns true if |queue_index| is enabled with the given |priority|.
   bool QueueEnabledWithPriority(size_t queue_index,
                                 QueuePriority priority) const;
 
+  void DidSelectQueue(size_t selected_queue, QueuePriority priority);

[rmcilroy, 2014/10/27 17:51:28]

nit - comment when to call this.

[Sami, 2014/10/28 12:57:47]

Done.

+

[picksi1, 2014/10/28 11:01:40]

Possibly the ship has sailed, but it should the selected_queue index be a
typedef (e.g. "QueueIndex") or something other than a size_t?

[Sami, 2014/10/28 12:57:47]

I think we went back and forth with this a couple of times but ended up with
size_t since it is commonly used with indices. Unfortunately a typedef doesn't
really buy you much in terms of type correctness because it will get implicitly
converted to and from a size_t.

(btw, typedefs are now discouraged for new code; "using" is the new hotness.)

+  scoped_refptr<base::debug::ConvertableToTraceFormat> AsValueWithSelectedQueue(
+      size_t selected_queue) const;
+  static const char* PriorityToString(QueuePriority priority);
+
   // Number of high priority tasks which can be run before a normal priority
   // task should be selected to prevent starvation.
   // TODO(rmcilroy): Check if this is a good value.
   static const size_t kMaxStarvationTasks = 5;
 
+  struct Queue {
+    Queue();
+
+    const base::TaskQueue* work_queue;
+    const char* name;
+  };

[picksi1, 2014/10/28 11:01:40]

Does this structure need to hold a work_queue pointer, as this adds an extra
indirection to every queue use? Could we just keep the name here as a parallel
data structure to the original task queue list?

[Sami, 2014/10/28 12:57:47]

Yes, the work queues are not owned by this class so we need to hold a pointer.
It was the same before this patch. There really shouldn't be extra indirection
here -- just the element stride changes. I don't like having two parallel arrays
because you have to make sure their sizes are in sync.

Anyway, this is all academic because this struct is no more :)

+
   base::ThreadChecker main_thread_checker_;
-  std::vector<const base::TaskQueue*> work_queues_;
+  std::vector<Queue> queues_;
   std::set<size_t> queue_priorities_[kQueuePriorityCount];
   size_t starvation_count_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_