Add reprioritization to socket pools.

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 979225987c1d288f77b6a14e67b4bb63332b62ce..f0ccb573f4d31d9625d7a670614aaa21f4510724 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -279,7 +279,7 @@ void ClientSocketPoolBaseHelper::RemoveHigherLayeredPool(
 
 int ClientSocketPoolBaseHelper::RequestSocket(
     const std::string& group_name,
-    std::unique_ptr<const Request> request) {
+    std::unique_ptr<Request> request) {
   CHECK(!request->callback().is_null());
   CHECK(request->handle());
 
@@ -527,6 +527,24 @@ void ClientSocketPoolBaseHelper::LogBoundConnectJobToRequest(
                              connect_job_source.ToEventParametersCallback());
 }
 
+void ClientSocketPoolBaseHelper::SetPriority(const std::string& group_name,
+                                             ClientSocketHandle* handle,
+                                             RequestPriority priority) {
+  PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle);

[mmenke, 2016/04/22 17:04:14]

This seems a roundabout way of doing things, and while perf does matter, doing
two searches here seems unnecessary (And suppose it could increase binary size,
which matters more).

Can we just use group_map_.find(group_name), and if it's not found, DCHECK that
it's in pending_callback_map_?

[mmenke, 2016/04/22 17:05:19]

does matter -> does not matter

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Sure, that's fine by me.  Done.

I did it this way copying the pattern in CancelRequest; should I extend the
change there too?

[Randy Smith (Not in Mondays), 2016/05/08 01:36:20]

Acknowledged.

+  // The Request has already completed and been destroyed; nothing to
+  // reprioritize.
+  if (callback_it != pending_callback_map_.end())
+    return;
+
+  DCHECK(ContainsKey(group_map_, group_name));
+
+  Group* group = GetOrCreateGroup(group_name);
+  std::unique_ptr<Request> request = group->FindAndRemovePendingRequest(handle);
+  DCHECK(request);
+  request->set_priority(priority);

[mmenke, 2016/04/22 17:04:14]

Can we either DCHECK the priorities are not the same, or do nothing if they are
the same?  Just helps use keep FIFO ordering if we do nothing when they're the
same.

[mmenke, 2016/04/22 17:04:14]

DCHECK(!request->ignore_limits())?  Decreasing the priority of an ignore limits
request isn't really allowed.

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Hmmm.  I'm not certain how to do nothing if they are the same (which would be my
preference); I don't see the Group class as having lookup operators that don't
remove the request.  Am I missing something?   Should I add an interface like
that to Group?

It's probably just my rationalizing avoiding something that would be tricky to
do :-}, but I'm also not sure that I don't want the current semantics.  If we
ever document the behavior of requests within a priority group, we'll be saying
FIFO, with the call to SetPriority for movements between priorities being an
insert (i.e. putting things at the end of the queue).  It seems like not putting
the request in question at the end of the queue if you do a SetPriority to the
same priority would violate that pattern.

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Done.

[mmenke, 2016/05/09 18:41:03]

Looks like you're right.  You could just DCHECK they're not the same, and catch
them at the URLRequest::SetPriority (Looks like we already catch that case
there, actually).
But then again, we're already doing that, per above comment.  I'm fine with
changing behavior in the future, but then we should make sure it affects
everything that cares about priority.

+  group->InsertPendingRequest(std::move(request));
+}
+
 void ClientSocketPoolBaseHelper::CancelRequest(
     const std::string& group_name, ClientSocketHandle* handle) {
   PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle);
@@ -547,8 +565,7 @@ void ClientSocketPoolBaseHelper::CancelRequest(
   Group* group = GetOrCreateGroup(group_name);
 
   // Search pending_requests for matching handle.
-  std::unique_ptr<const Request> request =
-      group->FindAndRemovePendingRequest(handle);
+  std::unique_ptr<Request> request = group->FindAndRemovePendingRequest(handle);
   if (request) {
     request->net_log().AddEvent(NetLog::TYPE_CANCELLED);
     request->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
@@ -904,7 +921,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
   if (result == OK) {
     DCHECK(socket.get());
     RemoveConnectJob(job, group);
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<Request> request = group->PopNextPendingRequest();
     if (request) {
       LogBoundConnectJobToRequest(job_log.source(), *request);
       HandOutSocket(std::move(socket), ClientSocketHandle::UNUSED,
@@ -921,7 +938,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
     // If we got a socket, it must contain error information so pass that
     // up so that the caller can retrieve it.
     bool handed_out_socket = false;
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<Request> request = group->PopNextPendingRequest();
     if (request) {
       LogBoundConnectJobToRequest(job_log.source(), *request);
       job->GetAdditionalErrorState(request->handle());
@@ -990,7 +1007,7 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest(
 
   int rv = RequestSocketInternal(group_name, *next_request);
   if (rv != ERR_IO_PENDING) {
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<Request> request = group->PopNextPendingRequest();
     DCHECK(request);
     if (group->IsEmpty())
       RemoveGroup(group_name);
@@ -1066,7 +1083,7 @@ void ClientSocketPoolBaseHelper::CancelAllRequestsWithError(int error) {
     Group* group = i->second;
 
     while (true) {
-      std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+      std::unique_ptr<Request> request = group->PopNextPendingRequest();
       if (!request)
         break;
       InvokeUserCallbackLater(request->handle(), request->callback(), error);
@@ -1304,7 +1321,7 @@ bool ClientSocketPoolBaseHelper::Group::HasConnectJobForHandle(
 }
 
 void ClientSocketPoolBaseHelper::Group::InsertPendingRequest(
-    std::unique_ptr<const Request> request) {
+    std::unique_ptr<Request> request) {
   // This value must be cached before we release |request|.
   RequestPriority priority = request->priority();
   if (request->respect_limits() == ClientSocketPool::RespectLimits::DISABLED) {
@@ -1318,33 +1335,33 @@ void ClientSocketPoolBaseHelper::Group::InsertPendingRequest(
   }
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::PopNextPendingRequest() {
   if (pending_requests_.empty())
-    return std::unique_ptr<const ClientSocketPoolBaseHelper::Request>();
+    return std::unique_ptr<ClientSocketPoolBaseHelper::Request>();
   return RemovePendingRequest(pending_requests_.FirstMax());
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::FindAndRemovePendingRequest(
     ClientSocketHandle* handle) {
   for (RequestQueue::Pointer pointer = pending_requests_.FirstMax();
        !pointer.is_null();
        pointer = pending_requests_.GetNextTowardsLastMin(pointer)) {
     if (pointer.value()->handle() == handle) {
-      std::unique_ptr<const Request> request = RemovePendingRequest(pointer);
+      std::unique_ptr<Request> request = RemovePendingRequest(pointer);
       return request;
     }
   }
-  return std::unique_ptr<const ClientSocketPoolBaseHelper::Request>();
+  return std::unique_ptr<ClientSocketPoolBaseHelper::Request>();
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::RemovePendingRequest(
     const RequestQueue::Pointer& pointer) {
   // TODO(eroman): Temporary for debugging http://crbug.com/467797.
   CHECK(!pointer.is_null());
-  std::unique_ptr<const Request> request(pointer.value());
+  std::unique_ptr<Request> request(pointer.value());
   pending_requests_.Erase(pointer);
   // If there are no more requests, kill the backup timer.
   if (pending_requests_.empty())