Readability review

net/base/prioritized_dispatcher.h

 // Copyright (c) 2012 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 NET_BASE_PRIORITY_DISPATCH_H_
-#define NET_BASE_PRIORITY_DISPATCH_H_
+#ifndef NET_BASE_PRIORITIZED_DISPATCHER_H_
+#define NET_BASE_PRIORITIZED_DISPATCHER_H_
 #pragma once
 
 #include <vector>
 
 #include "net/base/net_export.h"
 #include "net/base/priority_queue.h"
 
 namespace net {
 
 // A priority-based dispatcher of jobs. Dispatch order is by priority (highest
 // first) and then FIFO. The dispatcher enforces limits on the number of running
 // jobs. It never revokes a job once started. The job must call OnJobFinished
 // once it finishes in order to dispatch further jobs.
 //
-// All operations are O(p) time for p priority levels. The class is fully
-// reentrant: it is safe to execute any method (incl. destructor) from within
-// Job callbacks. However, this class is NOT thread-safe, which is enforced
-// by the underlying non-thread-safe PriorityQueue.
+// This class is NOT thread-safe which is enforced by the underlying
+// non-thread-safe PriorityQueue. All operations are O(p) time for p priority
+// levels. It is safe to execute any method, including destructor, from within
+// Job::Start.
 //
 class NET_EXPORT_PRIVATE PrioritizedDispatcher {
  public:
   class Job;
   typedef PriorityQueue<Job*>::Priority Priority;
 
   // Describes the limits for the number of jobs started by the dispatcher.
-  // For example, |total_jobs| = 30 and |reserved_slots| = { 5, 10, 5 }
+  // For example, |total_jobs| = 30 and |reserved_slots| = { 0, 5, 10, 5 }
   // allow for at most 30 running jobs in total. If there are already 24 jobs

[mmenke, 2012/06/01 17:45:34]

optional nit:  Suggest you add ", and no jobs at priority 0 will ever be
started", After "in total".

While higher priority -> more important may be intuitive, first item in a list
is the highest priority is also intuitive, so the comment draws a little more
attention to the fact that lowest priority is first in the list.

[szym, 2012/06/01 18:15:01]

Those are reserved slots. Only 20 slots are reserved, so there could be up to 10
jobs at priority 0. I added comment.

[mmenke, 2012/06/01 18:30:05]

Ah, right, I forgot about the 10.

-  // running, then there can be 6 more jobs started of which at most 1 can be
-  // at priority 1 or 2, but the rest have to be at 2.
+  // running, then only 6 more jobs can start. No jobs at priority 1 or below
+  // can start. After one more job starts, no jobs at priority 2 or below can
+  // start, since the remaining 5 slots are reserved for priority 3 or above.
   struct NET_EXPORT_PRIVATE Limits {
     Limits(Priority num_priorities, size_t total_jobs);
     ~Limits();
 
     // Total allowed running jobs.
     size_t total_jobs;
     // Number of slots reserved for each priority and higher.
     // Sum of |reserved_slots| must be no greater than |total_jobs|.
     std::vector<size_t> reserved_slots;
   };
 
   // An interface to the job dispatched by PrioritizedDispatcher. The dispatcher
   // does not own the Job but expects it to live as long as the Job is queued.
   // Use Cancel to remove Job from queue before it is dispatched. The Job can be
   // deleted after it is dispatched or canceled, or the dispatcher is destroyed.
   class Job {
    public:
-    // Note: PriorityDispatch will never delete a Job.
+    // Note: PrioritizedDispatcher will never delete a Job.
     virtual ~Job() {}
-    // Called when the dispatcher starts the job. Must call OnJobFinished when
-    // done.
+    // Called when the dispatcher starts the job. Once the job finishes, it must
+    // call OnJobFinished.
     virtual void Start() = 0;
   };
 
   // A handle to the enqueued job. The handle becomes invalid when the job is
   // canceled, updated, or started.
   typedef PriorityQueue<Job*>::Pointer Handle;
 
   // Creates a dispatcher enforcing |limits| on number of running jobs.
-  PrioritizedDispatcher(const Limits& limits);
+  explicit PrioritizedDispatcher(const Limits& limits);
 
   ~PrioritizedDispatcher();
 
   size_t num_running_jobs() const { return num_running_jobs_; }
   size_t num_queued_jobs() const { return queue_.size(); }
   size_t num_priorities() const { return max_running_jobs_.size(); }
 
   // Adds |job| with |priority| to the dispatcher. If limits permit, |job| is
   // started immediately. Returns handle to the job or null-handle if the job is
-  // started.
+  // started. The dispatcher does not own |job|, but must be outlived by |job|.

[mmenke, 2012/06/01 17:45:34]

Shouldn't this be "but must outlive |job|"?

[szym, 2012/06/01 18:15:01]

The Job must outlive the dispatcher so that it can be manipulated by it. I
clarified the comment.

[mmenke, 2012/06/01 18:30:05]

But you say Jobs must call OnJobFinished after being started, and they're
generally deleted at some point after that, so they don't outlive the
dispatcher.

Maybe "|job| must outlive the dispatcher unless cancelled or started?"

[mmenke, 2012/06/01 18:32:49]

Err...Or evicted.

Ok...Maybe "|jobs| that are still queued cannot be deleted until the dispatcher
is destroyed or they are no longer queued?"

   Handle Add(Job* job, Priority priority);
 
   // Removes the job with |handle| from the queue. Invalidates |handle|.
   // Note: a Handle is valid iff the job is in the queue, i.e. has not Started.
   void Cancel(const Handle& handle);
 
   // Removes and returns the oldest-lowest Job from the queue invalidating any
   // handles to it. Returns NULL if the queue is empty.
   Job* EvictOldestLowest();
 
@@ skipping 18 matching lines @@
   // another job cannot be started.
   std::vector<size_t> max_running_jobs_;
   // Total number of running jobs.
   size_t num_running_jobs_;
 
   DISALLOW_COPY_AND_ASSIGN(PrioritizedDispatcher);
 };
 
 }  // namespace net
 
-#endif  // NET_BASE_PRIORITY_DISPATCH_H_
+#endif  // NET_BASE_PRIORITIZED_DISPATCHER_H_
 
+

[mmenke, 2012/06/01 17:45:34]

nit:  Remove extra blank line (Goes for other files, too)

[szym, 2012/06/01 18:15:01]

Done.