Cache total_delayable_count for in_flight_requests in ResourceScheduler

content/browser/loader/resource_scheduler.cc

 // 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.
 
 #include "content/browser/loader/resource_scheduler.h"
 
 #include "base/stl_util.h"
 #include "content/common/resource_messages.h"
 #include "content/browser/loader/resource_message_delegate.h"
 #include "content/public/browser/resource_controller.h"
@@ skipping 159 matching lines @@
   ClientId client_id_;
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
   ResourceScheduler* scheduler_;
 
   DISALLOW_COPY_AND_ASSIGN(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() {
 }
 
 ResourceScheduler::~ResourceScheduler() {
   DCHECK(unowned_requests_.empty());
   DCHECK(client_map_.empty());
 }
 
@@ skipping 37 matching lines @@
   if (client_it == client_map_.end()) {
     return;
   }
 
   Client* client = client_it->second;
 
   if (client->pending_requests.IsQueued(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.
     LoadAnyStartablePendingRequests(client);
   }
 }
 
 void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
@@ skipping 12 matching lines @@
 
   Client* client = it->second;
 
   // FYI, ResourceDispatcherHost cancels all of the requests after this function
   // is called. It should end up canceling all of the requests except for a
   // cross-renderer navigation.
   for (RequestSet::iterator it = client->in_flight_requests.begin();
        it != client->in_flight_requests.end(); ++it) {
     unowned_requests_.insert(*it);
   }
-  client->in_flight_requests.clear();
+  client->ClearFlightRequests();
 
   delete client;
   client_map_.erase(it);
 }
 
 void ResourceScheduler::OnNavigate(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
 
   ClientMap::iterator it = client_map_.find(client_id);
@@ skipping 35 matching lines @@
   Client* client = client_it->second;
 
   if (!client->using_spdy_proxy) {
     client->using_spdy_proxy = true;
     LoadAnyStartablePendingRequests(client);
   }
 }
 
 void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
                                      Client* client) {
-  client->in_flight_requests.insert(request);
+  client->InsertFlightRequests(request);
   request->Start();
 }
 
 void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
                                             net::RequestPriority new_priority) {
   if (request->url_request()->load_flags() & net::LOAD_IGNORE_LIMITS) {
     // We should not be re-prioritizing requests with the
     // IGNORE_LIMITS flag.
     NOTREACHED();
     return;
   }
   net::RequestPriority old_priority = request->url_request()->priority();
   DCHECK_NE(new_priority, old_priority);
   request->url_request()->SetPriority(new_priority);
   ClientMap::iterator client_it = client_map_.find(request->client_id());
   if (client_it == client_map_.end()) {
     // The client was likely deleted shortly before we received this IPC.
     return;
   }
 
   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;
   }
 
   client->pending_requests.Erase(request);
   client->pending_requests.Insert(request,
                                   request->url_request()->priority());
 
   if (new_priority > old_priority) {
     // Check if this request is now able to load at its new priority.
@@ skipping 31 matching lines @@
     } else if (query_result == DO_NOT_START_REQUEST_AND_KEEP_SEARCHING) {
       ++request_iter;
       continue;
     } else {
       DCHECK(query_result == DO_NOT_START_REQUEST_AND_STOP_SEARCHING);
       break;
     }
   }
 }
 
-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) {
     net::HostPortPair host_port_pair =
         net::HostPortPair::FromURL((*it)->url_request()->url());
 
     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.
 //
 // Requests are categorized into two categories:
 //
 // 1. Immediately issued requests, which are:
 //
 //   * Higher priority requests (>= net::LOW).
 //   * Synchronous requests.
@@ skipping 34 matching lines @@
   net::HostPortPair host_port_pair =
       net::HostPortPair::FromURL(url_request.url());
 
   // TODO(willchan): We should really improve this algorithm as described in
   // crbug.com/164101. Also, theoretically we should not count a SPDY request
   // against the delayable requests limit.
   if (http_server_properties.SupportsSpdy(host_port_pair)) {
     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;
   }
 
   if (num_requests_in_flight_for_host >= kMaxNumDelayableRequestsPerHost) {
     // There may be other requests for other hosts we'd allow, so keep checking.
     return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
   }
 
   bool have_immediate_requests_in_flight =
       client->in_flight_requests.size() > num_delayable_requests_in_flight;
   if (have_immediate_requests_in_flight && !client->has_body &&
       num_delayable_requests_in_flight != 0) {
     return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
   }
 
   return START_REQUEST;
 }
 
 ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
     int child_id, int route_id) {
   return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
 }  // namespace content