Cache total_delayable_count for in_flight_requests in ResourceScheduler

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index 15c70270033dc8dbf563752b7aa50be12496b047..546401e7f7b33b0d207b7aa0af05000877b11578 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -178,13 +178,51 @@ class ResourceScheduler::ScheduledResourceRequest
 
 // Each client represents a tab.
 struct ResourceScheduler::Client {

[James Simonsen, 2014/03/05 02:30:59]

This isn't a struct anymore. To keep these methods, you'd need to make it a
class, make all the variables private, and add appropriate accessors, so nobody
misuses it by bypassing the member functions.

If we were to turn this into a class, it'd probably be best to move most of the
logic in this file to Client. Basically, there are (supposed to be) two levels
of scheduling: between tabs and within a tab. We only do the latter right now,
though we've long talked about prioritizing the foreground tab above the rest.
With two classes, we should probably divide up the work so that the
ResourceScheduler is responsible for between tab priority and Client is
responsible for within tab priority.

-  Client() : has_body(false), using_spdy_proxy(false) {}
+  Client()
+      : has_body(false),
+        using_spdy_proxy(false),
+        total_delayable_count(0) {}
   ~Client() {}
 
   bool has_body;
   bool using_spdy_proxy;
   RequestQueue pending_requests;
   RequestSet in_flight_requests;
+  size_t total_delayable_count;
+
+  bool IsDelayableRequest(ScheduledResourceRequest* request) {
+    if (request->url_request()->priority() < net::LOW) {
+      net::HostPortPair host_port_pair =
+          net::HostPortPair::FromURL(request->url_request()->url());
+      const net::HttpServerProperties& http_server_properties =
+          *request->url_request()->context()->http_server_properties();
+      if (!http_server_properties.SupportsSpdy(host_port_pair)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  void InsertFlightRequests(ScheduledResourceRequest* request) {

[James Simonsen, 2014/03/05 02:30:59]

This should be InsertInFlightRequest().

+    in_flight_requests.insert(request);
+    if (IsDelayableRequest(request))
+      total_delayable_count++;
+  }
+
+  size_t EraseFlightRequests(ScheduledResourceRequest* request) {

[James Simonsen, 2014/03/05 02:30:59]

And EraseInFlightRequest().

+    size_t erased = in_flight_requests.erase(request);
+    if (in_flight_requests.size() == 0) {
+      total_delayable_count = 0;
+    } else if (IsDelayableRequest(request)) {
+      total_delayable_count--;
+    }
+    return erased;
+  }
+
+  void ClearFlightRequests() {

[James Simonsen, 2014/03/05 02:30:59]

ClearInFlightRequests()

+    in_flight_requests.clear();
+    total_delayable_count = 0;
+  }
 };
 
 ResourceScheduler::ResourceScheduler() {
@@ -242,7 +280,7 @@ void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
     client->pending_requests.Erase(request);
     DCHECK(!ContainsKey(client->in_flight_requests, request));
   } else {
-    size_t erased = client->in_flight_requests.erase(request);
+    size_t erased = client->EraseFlightRequests(request);
     DCHECK(erased);
 
     // Removing this request may have freed up another to load.
@@ -275,7 +313,7 @@ void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
        it != client->in_flight_requests.end(); ++it) {
     unowned_requests_.insert(*it);
   }
-  client->in_flight_requests.clear();
+  client->ClearFlightRequests();
 
   delete client;
   client_map_.erase(it);
@@ -331,7 +369,7 @@ void ResourceScheduler::OnReceivedSpdyProxiedHttpResponse(
 
 void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
                                      Client* client) {
-  client->in_flight_requests.insert(request);
+  client->InsertFlightRequests(request);
   request->Start();
 }
 
@@ -355,6 +393,10 @@ void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
   Client *client = client_it->second;
   if (!client->pending_requests.IsQueued(request)) {
     DCHECK(ContainsKey(client->in_flight_requests, request));
+    if (new_priority >= net::LOW) {
+      if (client->IsDelayableRequest(request))
+        client->total_delayable_count--;

[James Simonsen, 2014/03/05 02:30:59]

Please add a unit test that shows the old code was broken and that this fixes
it. That'll prevent someone from screwing it up again.

[James Simonsen, 2014/03/05 02:30:59]

I don't like that total_delayable_count is sometimes modified directly and
sometimes modified via member functions on Client. It either needs to be one
style or the other.

+    }
     // Request has already started.
     return;
   }
@@ -406,15 +448,11 @@ void ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) {
   }
 }
 
-void ResourceScheduler::GetNumDelayableRequestsInFlight(
+size_t ResourceScheduler::GetNumSameHostRequestsInFlight(
     Client* client,
-    const net::HostPortPair& active_request_host,
-    size_t* total_delayable,
-    size_t* total_for_active_host) const {
-  DCHECK(client != NULL && total_delayable != NULL &&
-         total_for_active_host != NULL);
+    const net::HostPortPair& active_request_host) const {
+  DCHECK(client != NULL);
 
-  size_t total_delayable_count = 0;
   size_t same_host_count = 0;
   for (RequestSet::iterator it = client->in_flight_requests.begin();
        it != client->in_flight_requests.end(); ++it) {
@@ -424,18 +462,8 @@ void ResourceScheduler::GetNumDelayableRequestsInFlight(
     if (active_request_host.Equals(host_port_pair)) {
       same_host_count++;
     }
-
-    if ((*it)->url_request()->priority() < net::LOW) {
-      const net::HttpServerProperties& http_server_properties =
-          *(*it)->url_request()->context()->http_server_properties();
-
-      if (!http_server_properties.SupportsSpdy(host_port_pair)) {
-        ++total_delayable_count;
-      }
-    }
   }
-  *total_delayable = total_delayable_count;
-  *total_for_active_host = same_host_count;
+  return same_host_count;
 }
 
 // ShouldStartRequest is the main scheduling algorithm.
@@ -490,11 +518,9 @@ ResourceScheduler::ShouldStartReqResult ResourceScheduler::ShouldStartRequest(
     return START_REQUEST;
   }
 
-  size_t num_delayable_requests_in_flight = 0;
-  size_t num_requests_in_flight_for_host = 0;
-  GetNumDelayableRequestsInFlight(client, host_port_pair,
-                                  &num_delayable_requests_in_flight,
-                                  &num_requests_in_flight_for_host);
+  size_t num_delayable_requests_in_flight = client->total_delayable_count;
+  size_t num_requests_in_flight_for_host =
+      GetNumSameHostRequestsInFlight(client, host_port_pair);
 
   if (num_delayable_requests_in_flight >= kMaxNumDelayableRequestsPerClient) {
     return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;