Limit to only 10 image requests per page 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"
+#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"
 #include "net/base/request_priority.h"
 #include "net/url_request/url_request.h"
 
 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 {
  public:
   ScheduledResourceRequest(const ClientId& client_id,
                            net::URLRequest* request,
                            ResourceScheduler* scheduler)
       : ResourceMessageDelegate(request),
         client_id_(client_id),
         request_(request),
         ready_(false),
         deferred_(false),
         scheduler_(scheduler) {
   }
 
   virtual ~ScheduledResourceRequest() {
     scheduler_->RemoveRequest(this);
   }
 
   void Start() {
     ready_ = true;
     if (deferred_ && request_->status().is_success()) {
       deferred_ = false;
       controller()->Resume();
     }
   }
 
   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:
   virtual bool OnMessageReceived(const IPC::Message& message,
                                  bool* message_was_ok) OVERRIDE {
     bool handled = true;
     IPC_BEGIN_MESSAGE_MAP_EX(ScheduledResourceRequest, message, *message_was_ok)
       IPC_MESSAGE_HANDLER(ResourceHostMsg_DidChangePriority, DidChangePriority)
       IPC_MESSAGE_UNHANDLED(handled = false)
     IPC_END_MESSAGE_MAP_EX()
     return handled;
   }
 
   // ResourceThrottle interface:
   virtual void WillStartRequest(bool* defer) OVERRIDE {
     deferred_ = *defer = !ready_;
   }
 
   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_;
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
   ResourceScheduler* scheduler_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
 
 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());
   DCHECK(client_map_.empty());
 }
 
 scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
     int child_id,
     int route_id,
     net::URLRequest* url_request) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   scoped_ptr<ScheduledResourceRequest> request(
       new ScheduledResourceRequest(client_id, url_request, this));
 
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end()) {
     // There are several ways this could happen:
     // 1. <a ping> requests don't have a route_id.
     // 2. Most unittests don't send the IPCs needed to register Clients.
     // 3. The tab is closed while a RequestResource IPC is in flight.
     unowned_requests_.insert(request.get());
     request->Start();
     return request.PassAs<ResourceThrottle>();
   }
 
   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());
+  if (ShouldStartRequest(request.get(), client)) {
+    StartRequest(request.get(), client);
   } else {
-    StartRequest(request.get(), client);
+    client->pending_requests.Insert(request.get(), url_request->priority());
   }
   return request.PassAs<ResourceThrottle>();
 }
 
 void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
   DCHECK(CalledOnValidThread());
   if (ContainsKey(unowned_requests_, request)) {
     unowned_requests_.erase(request);
     return;
   }
 
   ClientMap::iterator client_it = client_map_.find(request->client_id());
   if (client_it == client_map_.end()) {
     return;
   }
 
   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);
   }
 }
 
 void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!ContainsKey(client_map_, client_id));
 
   client_map_[client_id] = new Client;
 }
@@ skipping 39 matching lines @@
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end()) {
     // The client was likely deleted shortly before we received this IPC.
     return;
   }
 
   Client* client = it->second;
   client->has_body = false;
   if (!client->has_body) {
     client->has_body = true;
-    LoadPendingRequests(client);
+    LoadAnyStartablePendingRequests(client);
   }
 }
 
 void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
                                      Client* client) {
   client->in_flight_requests.insert(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;
+    }
+  }
+}
+
+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;
+}
+
+// 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::ClientId ResourceScheduler::MakeClientId(
     int child_id, int route_id) {
   return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
-ResourceScheduler::Client::Client()
-    : has_body(false) {
-}
-
-ResourceScheduler::Client::~Client() {
-}
-
 }  // namespace content