Readability review

net/base/prioritized_dispatcher.h

Index: net/base/prioritized_dispatcher.h
diff --git a/net/base/prioritized_dispatcher.h b/net/base/prioritized_dispatcher.h
index 7e59d4e67e1bf505ffe460b044ecc64ec942e060..fa50019da086c9748f3a12288132c42c61ded8d9 100644
--- a/net/base/prioritized_dispatcher.h
+++ b/net/base/prioritized_dispatcher.h
@@ -20,7 +20,7 @@ namespace net {
 //
 // 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

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

Ok, let me be more clear:  can you provide an example of how to take advantage
of the fact that this class is re-entrant, but not thread-safe.

I have never encountered code that claims to be re-entrant but not thread-safe
(and most discussion I've found online, people assume all reentrant code IS
threadsafe, the only non-example of it is in a wikipedia article just showing
you can actually write such).  

I don't know why it would be useful.  I certainly don't know why you would
highlight the re-entrant property over the NOT thread-safe property.

To be more specific - in order to make a re-entrant call, I must have called
you, and you called me back somehow before that call returned, and now I can
call you back.  If you're not thread-safe, either I'm using a mutex (in which
case, why would I care at all about re-entrancy?), or I know it's all in the
same thread, which seems like a pretty huge encapsulation violation (ie I'm
assuming things about how you called me back).

From a readability perspective, it seems this talk of reentrancy is beside the
point and simply confusing; just say it's not thread-safe and leave it at that. 
If there's some case where reentrancy is important, mention it AFTER saying it's
not threadsafe (non-thread-safety seems hugely more important), and explain why
it matters because for the average engineer it won't be at all obvious.  In
fact, I'd wager the average engineer will assume it's a typo in the comment (if
they even bother to pay attention to it)!

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

Understood. I should remove "re-entrant" and leave the explicit statement: "It
is safe to execute any method, including destructor, from within a callback."
after highlighting it's non thread safe.
That's exactly the scenario. With callback passing there is often the case when
we want to take action on the caller. For example, a Job that has been started
might want to add another Job to the PrioritizedDispatcher, or it might destroy
it.

Consider:
struct QuitJob : public Job {
  virtual void Start() {
    delete dispatcher;
  }
  PrioritizedDispatcher* dispatcher;
};
PrioritizedDispatcher* dispatcher = ...
QuitJob quit = { dispatcher; }
dispatcher->AddJob(&quit, IDLE);
// AddJob calls QuitJob::Start
// which deletes dispatcher

We don't normally guarantee that it's safe to act on the caller from the
callback. In most cases, this is enabled by posting the callback to a
MessageLoop, so that the caller is the MessageLoop. In this case, by careful
design, it's safe to call on PrioritizedDispatcher from Job::Start.
In Chromium's net this is the common assumption (that it's on the single
thread). Classes which are thread-safe are typically explicitly documented as
such. Classes are only explicitly documented as NonThreadSafe when there is
reasonable concern that different threads might incorrectly call into them.

-// Job callbacks. However, this class is NOT thread-safe, which is enforced
+// Job::Start. However, this class is NOT thread-safe, which is enforced
 // by the underlying non-thread-safe PriorityQueue.
 //
 class NET_EXPORT_PRIVATE PrioritizedDispatcher {
@@ -29,10 +29,11 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   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
-  // 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();
@@ -50,10 +51,10 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   // 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;
   };
 
@@ -62,7 +63,7 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   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();
 
@@ -72,7 +73,7 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
   // 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|.
   Handle Add(Job* job, Priority priority);

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

parameter ordering still needs to be fixed

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

I see the second part of my comment does not show up in Rietveld unless
expanded:

"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)."

|job| is hardly an output argument. Add does not write to |job|. I would
normally use a Callback (essentially passed by value), but EvictOldestLowest
requires using something identifiable.

[stevez, 2012/05/22 14:22:40]

Ok on the parameter order, but can you explain what you mean here about using a
callback?  If you don't need a job class at all it'd be good to get rid of it. 
I don't see what you mean by EvictOldestLowest needing something identifiable. 
(not least because the Job interface doesn't provide any more identifiability
than a callback does).

[szym, 2012/05/23 01:26:49]

PrioritizedDispatcher::Job has only one method, so the preferred solution is to
use Callback (see:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/callback.h ). However,
in some use cases PrioritizedDispatcher::EvictOldestLowest only provides utility
if the caller can identify the Job (or Callback) that has been evicted. See:
http://code.google.com/searchframe#OAMlx_jo-ck/src/net/base/host_resolver_imp...

Since Callback is passed by value (const& for efficiency only), it would not
provide such identification.

[stevez, 2012/05/30 17:12:45]

Does chromium use a different callback library than google3?  You can certainly
pass callbacks around by pointer, that is the standard way of doing so.

The usage you point to requires casting, which is unfortunate.  You might enrich
the job interface instead.  Or, you might return a Handle from EvictOldestLowest
as a different mechanism to match up its return value to a corresponding Add
call (assuming it has suitable semantics, I didn't look; at any rate it seems
reasonable that the job manipulation methods are all in terms of handles).

In any case this is advisory only, feel free to change or not.

[szym, 2012/05/30 23:53:24]

Yes, Chromium uses different callback library. If curious, see callback.h:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/callback.h
In the original review, we found that calling Job::OnEvicted from
PrioritizedDispatcher is awkward because the Job could be evicted on addition.
We decided that queue limits and eviction are better handled at the caller
(HostResolverImpl).
PriorityQueue::Pointer is only valid for objects still in the queue. But more
importantly, Handle::value() returns the generic PrioritizedDispatcher::Job, so
static_cast is inevitable unless PrioritizedDispatcher becomes a template.

 
   // Removes the job with |handle| from the queue. Invalidates |handle|.
@@ -113,3 +114,4 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
 #endif  // NET_BASE_PRIORITY_DISPATCH_H_
 
+