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_
 #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 (lowest

[stevez, 2012/04/11 16:13:58]

If it's possible to reverse the decision of lowest number == highest priority, I
would suggest doing so.  You'll have to explain that everywhere you use
priorities, and it's very easy to get wrong, as it's unnatural to read code like
this:
  if (priority1 < priority2) { 
    // priority one is higher priority, not lower
  }
Basically you're introducing cognitive load every time priorities are used.

Another way around that is to explicitly define some readable interface to the
priorities which does the right thing with the numbers, eg
  if (IsHigherPriority(priority1, priority2))

[szym, 2012/05/14 22:56:02]

Took some work, but done. 

 // 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

[stevez, 2012/04/11 16:13:58]

It's not clear what all this means.

Perhaps give some examples of what is safe or not safe to do, or document it
down at the method level.  eg, it's very unclear to me how one can safely call
Cancel (especially if OnJobFinished might be called from some other thread).

[szym, 2012/05/14 22:56:02]

That's not allowed. The class is NOT thread-safe.

 // by the underlying non-thread-safe PriorityQueue.
 //
 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 }
   // allow for at most 30 running jobs in total. If there are already 24 jobs
   // running, then there can be 6 more jobs started of which at most 1 can be
   // at priority 1 or 0, but the rest has to be at 0.

[stevez, 2012/04/11 16:13:58]

Can you explain this comment more, it doesn't make sense to me.  Perhaps there's
some assumptions about how the priorities work that are not stated here; or
simply a typo.

[szym, 2012/05/14 22:56:02]

There's no typo, I updated the comment after priority reordering. Is it still
unclear?

[stevez, 2012/05/17 17:57:07]

The typo was 'at priority 1 or 0' -> 'at priority 1 or 2'.  The new comment is
fine.

[szym, 2012/05/17 19:52:01]

That was not a typo. That one slot could be used by priority 1 or "higher" (0).
Fortunately, the higher/lower confusion is gone now.

   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.

[stevez, 2012/04/11 16:13:58]

What is PriorityDispatch?

[szym, 2012/05/14 22:56:02]

Done.

     virtual ~Job() {}
     // Called when the dispatcher starts the job. Must call OnJobFinished when
     // done.

[stevez, 2012/04/11 16:13:58]

When done what?  done starting?

[szym, 2012/05/14 22:56:02]

Fixed wording.

     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);

[stevez, 2012/04/11 16:13:58]

Single arg constructors must be declared explicit.

[szym, 2012/05/14 22:56:02]

Done.

 
   ~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.
   Handle Add(Job* job, Priority priority);

[stevez, 2012/04/11 16:13:58]

Document ownership semantics when passing mutable pointers.

Also, fix parameter ordering.

[szym, 2012/05/14 22:56:02]

Done.
 
I prefer to leave this one as is. This is a very common pattern in Chromium
codebase: http://goo.gl/4lnej (in src/net alone).

 
   // 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();
 
   // Moves the queued job with |handle| to the end of all values with priority
@@ skipping 19 matching lines @@
   // Total number of running jobs.
   size_t num_running_jobs_;
 
   DISALLOW_COPY_AND_ASSIGN(PrioritizedDispatcher);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_PRIORITY_DISPATCH_H_
 
+