Limit to only 10 image requests per page 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 0a171ad06923cbadc9d9dad2704a5fc84c34fb8a..9c465cb884604974d2dce5d359ec74fe7de1a41d 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -8,6 +8,7 @@
 #include "content/common/resource_messages.h"
 #include "content/browser/loader/resource_message_delegate.h"
 #include "content/public/browser/resource_controller.h"
+#include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/load_flags.h"
@@ -16,6 +17,64 @@
 
 namespace content {
 
+static const int kMaxNumLowPriorityRequestsPerClient = 10;
+
+// Each client represents a tab.
+struct ResourceScheduler::Client {
+  Client() : has_body(false) {}
+  ~Client() {}
+
+  bool has_body;
+  RequestQueue pending_requests;
+  RequestSet in_flight_requests;
+};
+
+// A thin wrapper around net::PriorityQueue that deals with
+// ScheduledResourceRequests instead of PriorityQueue::Pointers.
+class ResourceScheduler::RequestQueue {
+ public:
+  RequestQueue() : queue_(net::NUM_PRIORITIES) {}
+  ~RequestQueue() {}
+
+  // Adds |request| to the queue with given |priority|.
+  void Insert(ScheduledResourceRequest* request,
+              net::RequestPriority priority) {
+    DCHECK(!ContainsKey(pointers_, request));
+    NetQueue::Pointer pointer = queue_.Insert(request, priority);
+    pointers_[request] = pointer;
+  }
+
+  // Removes |request| from the queue.
+  void Erase(ScheduledResourceRequest* request) {
+    PointerMap::iterator it = pointers_.find(request);
+    DCHECK(it != pointers_.end());
+    queue_.Erase(it->second);
+    pointers_.erase(it);
+  }
+
+  // Returns the highest priority request that's queued, or NULL if none are.
+  ScheduledResourceRequest* FirstMax() {
+    return queue_.FirstMax().value();
+  }
+
+  // Returns true if |request| is queued.
+  bool IsQueued(ScheduledResourceRequest* request) const {
+    return ContainsKey(pointers_, request);
+  }
+
+  // Returns true if no requests are queued.
+  bool IsEmpty() const { return queue_.size() == 0; }
+
+ private:
+  typedef net::PriorityQueue<ScheduledResourceRequest*> NetQueue;
+  typedef std::map<ScheduledResourceRequest*, NetQueue::Pointer> PointerMap;
+
+  NetQueue queue_;
+  PointerMap pointers_;
+};
+
+// This is the handle we return to the ResourceDispatcherHostImpl so it can
+// interact with the request.
 class ResourceScheduler::ScheduledResourceRequest
     : public ResourceMessageDelegate,
       public ResourceThrottle {
@@ -44,7 +103,8 @@ class ResourceScheduler::ScheduledResourceRequest
   }
 
   const ClientId& client_id() const { return client_id_; }
-  const net::URLRequest& url_request() const { return *request_; }
+  net::URLRequest* url_request() { return request_; }
+  const net::URLRequest* url_request() const { return request_; }
 
  private:
   // ResourceMessageDelegate interface:
@@ -64,11 +124,7 @@ class ResourceScheduler::ScheduledResourceRequest
   }
 
   void DidChangePriority(int request_id, net::RequestPriority new_priority) {
-    net::RequestPriority old_priority = request_->priority();
-    request_->set_priority(new_priority);
-    if (new_priority > old_priority) {
-      Start();
-    }
+    scheduler_->ReprioritizeRequest(this, new_priority);
   }
 
   ClientId client_id_;
@@ -86,7 +142,7 @@ ResourceScheduler::ResourceScheduler() {
 ResourceScheduler::~ResourceScheduler() {
   for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin();

[willchan no longer on Chromium, 2013/03/19 20:53:53]

Sorry again for nitting on existing code. Why does this loop exist? We have a
DCHECK(client_map_.empty()) below.

[James Simonsen, 2013/03/19 23:00:46]

Done. It used to be more useful.

        it != client_map_.end(); ++it) {
-    DCHECK(it->second->pending_requests.empty());
+    DCHECK(it->second->pending_requests.IsEmpty());
     DCHECK(it->second->in_flight_requests.empty());
   }
   DCHECK(unowned_requests_.empty());
@@ -114,17 +170,10 @@ scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
   }
 
   Client* client = it->second;
-
-  bool is_synchronous = (url_request->load_flags() & net::LOAD_IGNORE_LIMITS) ==
-      net::LOAD_IGNORE_LIMITS;
-  bool is_low_priority =
-      url_request->priority() < net::LOW && !is_synchronous;
-
-  if (is_low_priority && !client->in_flight_requests.empty() &&
-      !client->has_body) {
-    client->pending_requests.push_back(request.get());
-  } else {
+  if (ShouldStartRequest(request.get(), client)) {
     StartRequest(request.get(), client);
+  } else {
+    client->pending_requests.Insert(request.get(), url_request->priority());
   }
   return request.PassAs<ResourceThrottle>();
 }
@@ -142,29 +191,16 @@ void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
   }
 
   Client* client = client_it->second;
-  RequestSet::iterator request_it = client->in_flight_requests.find(request);
-  if (request_it == client->in_flight_requests.end()) {
-    bool removed = false;
-    RequestQueue::iterator queue_it;
-    for (queue_it = client->pending_requests.begin();
-         queue_it != client->pending_requests.end(); ++queue_it) {
-      if (*queue_it == request) {
-        client->pending_requests.erase(queue_it);
-        removed = true;
-        break;
-      }
-    }
-    DCHECK(removed);
+
+  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);
     DCHECK(erased);
-  }
 
-  if (client->in_flight_requests.empty()) {
-    // Since the network is now idle, we may as well load some of the low
-    // priority requests.
-    LoadPendingRequests(client);
+    // Removing this request may have freed up another to load.
+    LoadAnyStartablePendingRequests(client);
   }
 }
 
@@ -224,7 +260,7 @@ void ResourceScheduler::OnWillInsertBody(int child_id, int route_id) {
   client->has_body = false;
   if (!client->has_body) {
     client->has_body = true;
-    LoadPendingRequests(client);
+    LoadAnyStartablePendingRequests(client);
   }
 }
 
@@ -234,24 +270,94 @@ void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
   request->Start();
 }
 
-void ResourceScheduler::LoadPendingRequests(Client* client) {
-  while (!client->pending_requests.empty()) {
-    ScheduledResourceRequest* request = client->pending_requests.front();
-    client->pending_requests.erase(client->pending_requests.begin());
-    StartRequest(request, client);
+void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
+                                            net::RequestPriority new_priority) {
+  net::RequestPriority old_priority = request->url_request()->priority();
+  DCHECK(new_priority != old_priority);

[willchan no longer on Chromium, 2013/03/19 20:53:53]

Does DCHECK_NE() not work?

[James Simonsen, 2013/03/19 23:00:46]

Done.

+  request->url_request()->set_priority(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.

[willchan no longer on Chromium, 2013/03/19 20:53:53]

I think this RenderViewHost deletion signal seems wrong if it allows this. We
should just track when the IPC channel is dead. If Darin's around tomorrow, I'm
going to bug him about this.

[James Simonsen, 2013/03/19 23:00:46]

There can be several renderers on the same channel. Watching the channel isn't
sufficient to know when the renderers have come and gone. The UI thread is the
authority on that.

[willchan no longer on Chromium, 2013/03/19 23:08:32]

Sorry, forgot about that. But why don't we get a RenderView shutdown
notification? I assume we do. We should use that signal instead of
RenderViewHost deletion.

What I'm really saying is that this race is silly. Whenever we know for sure the
peer (RenderView) is dead, then we know it's safe to delete the client.
RenderViewHost deletion smells wrong, since that corresponds to a UI thread
object. Frankly, the whole UI+IO thread usage of RVH smells weird to me.

+    return;
+  }
+
+  Client *client = client_it->second;
+  if (!client->pending_requests.IsQueued(request)) {
+    DCHECK(ContainsKey(client->in_flight_requests, request));
+    // 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.
+    LoadAnyStartablePendingRequests(client);
+  }
+}
+
+void ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) {
+  while (!client->pending_requests.IsEmpty()) {
+    ScheduledResourceRequest* request = client->pending_requests.FirstMax();
+    if (ShouldStartRequest(request, client)) {
+      client->pending_requests.Erase(request);
+      StartRequest(request, client);
+    } else {
+      break;
+    }
   }
 }
 
-ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
-    int child_id, int route_id) {
-  return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
+size_t ResourceScheduler::GetNumLowPriorityRequestsInFlight(
+    Client* client) const {
+  size_t count = 0;
+  for (RequestSet::iterator it = client->in_flight_requests.begin();
+       it != client->in_flight_requests.end(); ++it) {
+    if ((*it)->url_request()->priority() < net::LOW) {
+      ++count;
+    }
+  }
+  return count;
 }
 
-ResourceScheduler::Client::Client()
-    : has_body(false) {
+// ShouldStartRequest is the main scheduling algorithm. It works as follows:
+//
+// 1. High priority requests (>= net::LOW are always issued.

[willchan no longer on Chromium, 2013/03/19 20:53:53]

This is very unfortunately named. Can we fix this somehow? See the above
GetNumLowPriorityRequestsInFlight() which does < net::LOW. Hah!

[James Simonsen, 2013/03/19 23:00:46]

The whole notion of using priority names is ridiculous. We should fix that
instead. I've changed the wording here in the meantime.

+// 2. Synchronous requests are always high priority.

[willchan no longer on Chromium, 2013/03/19 20:53:53]

I'd say instead that synchronous requests are always issued.

[James Simonsen, 2013/03/19 23:00:46]

Done.

+//
+// The remaining, low priority follow these rules:
+//
+// 1. If no high priority requests are in flight, start loading low priority
+//    requests.
+// 2. Once the renderer has a <body>, start loading low priority requests.
+// 3. Never exceed 10 low priority requests in flight per client.
+bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request,
+                                           Client* client) const {
+  if (request->url_request()->priority() >= net::LOW ||
+      !ResourceRequestInfo::ForRequest(request->url_request())->IsAsync()) {
+    return true;
+  }
+
+  size_t num_low_priority_requests_in_flight =
+      GetNumLowPriorityRequestsInFlight(client);
+  if (num_low_priority_requests_in_flight >=
+      kMaxNumLowPriorityRequestsPerClient) {
+    return false;
+  }
+
+  bool have_high_priority_requests_in_flight =
+      client->in_flight_requests.size() > num_low_priority_requests_in_flight;
+  if (have_high_priority_requests_in_flight && !client->has_body) {
+    return false;
+  }
+
+  return true;
 }
 
-ResourceScheduler::Client::~Client() {
+ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
+    int child_id, int route_id) {
+  return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
 }  // namespace content