net: Socket pools prioritize requests with ignore_limits

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
===================================================================
--- net/socket/client_socket_pool_base.cc	(revision 203448)
+++ net/socket/client_socket_pool_base.cc	(working copy)
@@ -19,6 +19,8 @@
 
 using base::TimeDelta;
 
+namespace net {
+
 namespace {
 
 // Indicate whether we should enable idle socket cleanup timer. When timer is
@@ -37,10 +39,30 @@
 // after a certain timeout has passed without receiving an ACK.
 bool g_connect_backup_jobs_enabled = true;
 
+// Compares the effective priority of two results, and returns 1 if |request1|
+// has greater effective priority than |request2|, 0 if they have the same
+// effective priority, and -1 if |request2| has the greater effective priority.
+// Requests with |ignore_limits| set have higher effective pririoty than those

[eroman, 2013/06/10 17:48:00]

typo priority

[mmenke, 2013/06/10 18:17:26]

Done.

+// without.  If both requests have |ignore_limits| set/unset, then the request
+// with the highest Pririoty has the highest effective priority.  Does not take
+// into account the fact that Requests are serviced in FIFO order if they would
+// otherwise have the same priority.
+int CompareEffectiveRequestPriority(
+    const internal::ClientSocketPoolBaseHelper::Request& request1,
+    const internal::ClientSocketPoolBaseHelper::Request& request2) {
+  if (request1.ignore_limits() && !request2.ignore_limits())
+    return 1;
+  if (!request1.ignore_limits() && request2.ignore_limits())
+    return -1;
+  if (request1.priority() > request2.priority())
+    return 1;
+  if (request1.priority() < request2.priority())
+    return -1;
+  return 0;
+}
+
 }  // namespace
 
-namespace net {
-
 ConnectJob::ConnectJob(const std::string& group_name,
                        base::TimeDelta timeout_duration,
                        Delegate* delegate,
@@ -187,16 +209,16 @@
 
 ClientSocketPoolBaseHelper::CallbackResultPair::~CallbackResultPair() {}
 
-// InsertRequestIntoQueue inserts the request into the queue based on
-// priority.  Highest priorities are closest to the front.  Older requests are
-// prioritized over requests of equal priority.
-//
 // static
 void ClientSocketPoolBaseHelper::InsertRequestIntoQueue(
     const Request* r, RequestQueue* pending_requests) {
   RequestQueue::iterator it = pending_requests->begin();
-  while (it != pending_requests->end() && r->priority() <= (*it)->priority())
+  // TODO(mmenke):  Should the network stack require requests with
+  //                |ignore_limits| have the highest priority?
+  while (it != pending_requests->end() &&
+         CompareEffectiveRequestPriority(*r, *(*it)) <= 0) {

[eroman, 2013/06/10 17:48:00]

Given that the result is only compared <=0, would it be sufficient to define
"CompareEffectiveRequestPriority" as a 'less than' comparator instead?

[mmenke, 2013/06/10 18:17:26]

I could, but to maintain FIFO order, I'd have to either make a less-or-equal
comparator, or I'd have to negate it.  Or I suppose I could walk through the
list backwards.

Negating a comparator seems ugly to me.  Making it less-or-equal seems a bit
confusing.  Walking backwards also doesn't seem as clear as this is.  Thoughts?

[eroman, 2013/06/10 18:31:49]

I guess you could do something like
  !LessThan(b,a)

(both negating, and also flipping the order of |a| and |b|).

However I agree that is harder to read. As is is fine. I would also be in favor
of having a boolean comparator that returns either "greater than" or "less than
equal to" for simplicity.

I haven't thought this through, but <algorithm>'s std::upper_bound() might also
be applicable. (Although probably even harder to read).

LGTM whichever option you choose.

     ++it;
+  }
   pending_requests->insert(it, r);
 }
 
@@ -494,8 +516,10 @@
       req->net_log().AddEvent(NetLog::TYPE_CANCELLED);
       req->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
 
-      // We let the job run, unless we're at the socket limit.
-      if (group->jobs().size() && ReachedMaxSocketsLimit()) {
+      // We let the job run, unless we're at the socket limit and there is
+      // not another request waiting on the job.
+      if (group->jobs().size() > group->pending_requests().size() &&
+          ReachedMaxSocketsLimit()) {
         RemoveConnectJob(*group->jobs().begin(), group);
         CheckForStalledSocketGroups();
       }