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_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;
+
 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();
     }
   }
 
+  // |queue_pointer| keeps track of the position of |this| in its client's
+  // |pending_requests| queue.
+  ResourceScheduler::RequestQueue::Pointer queue_pointer() {
+    return queue_pointer_;
+  }
+  void set_queue_pointer(ResourceScheduler::RequestQueue::Pointer pointer) {
+    queue_pointer_ = pointer;
+  }
+  void clear_queue_pointer() {
+    queue_pointer_.Reset();
+  }
+
   const ClientId& client_id() const { return client_id_; }
   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();
+    if (new_priority != old_priority) {

[willchan no longer on Chromium, 2013/03/18 07:30:33]

When is this ever false? Sounds like a renderer bug if that ever happens. Should
we DCHECK to catch this?

[James Simonsen, 2013/03/18 23:50:13]

Agreed. Done.

+      request_->set_priority(new_priority);
+      scheduler_->ReprioritizeRequest(this);
     }
   }
 
   ClientId client_id_;
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
   ResourceScheduler* scheduler_;
+  ResourceScheduler::RequestQueue::Pointer queue_pointer_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
 
 ResourceScheduler::ResourceScheduler() {
 }
 
 ResourceScheduler::~ResourceScheduler() {
   for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin();
        it != client_map_.end(); ++it) {
-    DCHECK(it->second->pending_requests.empty());
+    DCHECK(!it->second->pending_requests.size());
     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);
+    RequestQueue::Pointer pointer = client->pending_requests.Insert(
+        request.get(), url_request->priority());
+    request->set_queue_pointer(pointer);
   }
   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);
-    DCHECK(!ContainsKey(client->in_flight_requests, request));
-  } else {
+
+  if (request->queue_pointer().is_null()) {
     size_t erased = client->in_flight_requests.erase(request);
     DCHECK(erased);
+  } else {
+    client->pending_requests.Erase(request->queue_pointer());
+    request->clear_queue_pointer();
+    DCHECK(!ContainsKey(client->in_flight_requests, request));
   }
 
-  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.

[willchan no longer on Chromium, 2013/03/18 07:30:33]

Isn't this only true if we remove an in flight request? Why would removing a
pending request free up another to load?

[James Simonsen, 2013/03/18 23:50:13]

Done.

+  LoadPendingRequests(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;
 }
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(ContainsKey(client_map_, client_id));
   ClientMap::iterator it = client_map_.find(client_id);
   Client* client = it->second;
 
   // FYI, |in_flight_requests| should only be non-empty in the case of a
   // cross-renderer navigation.

[willchan no longer on Chromium, 2013/03/18 07:30:33]

How does this work? We move all the requests into unowned_requests_...so when
does the new renderer take over ownership? What happens if it sends
reprioritization IPCs for these requests?

[James Simonsen, 2013/03/18 23:50:13]

It doesn't get picked up in the new renderer. I should fix that. I'll do it in
another CL.

If it's not found, the priority change is ignored (it's already in flight
anyway, so it really doesn't matter).

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

Do we want to ignore it in debug mode? Are there any valid cases for this to
happen? Do we want to be able to DCHECK on this condition?

   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();
 
   delete client;
   client_map_.erase(it);
 }
 
@@ skipping 28 matching lines @@
     LoadPendingRequests(client);
   }
 }
 
 void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
                                      Client* client) {
   client->in_flight_requests.insert(request);
   request->Start();
 }
 
+void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request) {
+  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/18 07:30:33]

How? The comment for OnClientDeleted() says it happens when the renderer is
destroyed. Where are we getting an IPC from if the renderer is destroyed?

[James Simonsen, 2013/03/18 23:50:13]

RenderViewHosts can be deleted by user action on the UI thread. As that's
happening, the renderer might send us an IPC. Sometimes, we'll get the Task from
the UI thread before we process the IPC.

I've documented this in the header.

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

Why are we listening for the signal that the RenderViewHost is gone instead of
the signal that the RenderView (the client) is actually gone? Why do we have to
accept this race?

+    return;
+  }
+
+  Client *client = client_it->second;
+  if (request->queue_pointer().is_null()) {
+    DCHECK(ContainsKey(client->in_flight_requests, request));
+    // Request has already started.

[willchan no longer on Chromium, 2013/03/18 07:30:33]

But don't we need to call set_priority()? OH, I see now. You're already calling
set_priority() before ReprioritizeRequest() is called. That's non-intuitive. Can
we move the set_priority() call into this function? Is there a reason to keep it
separate?

[James Simonsen, 2013/03/18 23:50:13]

Done.

+    return;
+  }
+
+  client->pending_requests.Erase(request->queue_pointer());

[willchan no longer on Chromium, 2013/03/18 07:30:33]

When I see stuff like this where the caller needs to remember to set or clear
the queue_pointer when inserting/erasing into/from the queue, I sorta feel like
the RequestQueue shouldn't be a typedef of the net::PriorityQueue, but rather a
class that composes a net::PriorityQueue.

I leave it up to you, but that's what I would do.

[James Simonsen, 2013/03/18 23:50:13]

Done.

+  RequestQueue::Pointer pointer = client->pending_requests.Insert(
+      request, request->url_request().priority());
+  request->set_queue_pointer(pointer);
+
+  // Check if this request is now able to load at its new priority.

[willchan no longer on Chromium, 2013/03/18 07:30:33]

We only want to do this if the new priority is higher than the old one, right?

[James Simonsen, 2013/03/18 23:50:13]

Done.

+  LoadPendingRequests(client);
+}
+
 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);
+  while (client->pending_requests.size()) {
+    ScheduledResourceRequest* request =
+        client->pending_requests.FirstMax().value();
+    if (ShouldStartRequest(request, client)) {
+      client->pending_requests.Erase(request->queue_pointer());
+      request->clear_queue_pointer();
+      StartRequest(request, client);
+    } else {
+      break;
+    }
   }
 }
 
+int ResourceScheduler::GetNumLowPriorityRequestsInFlight(Client* client) {
+  int 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;
+}
+
+bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request,

[willchan no longer on Chromium, 2013/03/18 07:30:33]

Please add a comment here explaining in plain English what the algorithm is.

[James Simonsen, 2013/03/18 23:50:13]

Done.

+                                           Client* client) {
+  bool is_synchronous =

[willchan no longer on Chromium, 2013/03/18 07:30:33]

I know you're just moving this, but this caught my eye. LOAD_IGNORE_LIMITS
smells like the wrong signal to use here. Should we be reading from the
ResourceRequestInfoImpl instead? It explicitly says whether or not this is
async.

[James Simonsen, 2013/03/18 23:50:13]

Done.

+      (request->url_request().load_flags() & net::LOAD_IGNORE_LIMITS) ==
+      net::LOAD_IGNORE_LIMITS;
+  bool is_low_priority =

[willchan no longer on Chromium, 2013/03/18 07:30:33]

Sorry again for nitting you when you're just moving stuff. This reads weird. I
think we should delete this local variable and just do the if check directly.
is_low_priority == priority < LOW && !is_synchronous is just...weird. Reads
like..."the priority is low when the priority is less than low and it's not
sync"?

[James Simonsen, 2013/03/18 23:50:13]

Done.

+      request->url_request().priority() < net::LOW && !is_synchronous;
+  if (!is_low_priority)
+    return true;
+
+  int num_low_priority_requests_in_flight =
+      GetNumLowPriorityRequestsInFlight(client);
+  if (num_low_priority_requests_in_flight >=
+      kMaxNumLowPriorityRequestsPerClient) {
+    return false;
+  }
+
+  int num_high_priority_requests_in_flight =

[willchan no longer on Chromium, 2013/03/18 07:30:33]

You don't need the number, you just need to know if there are any high priority
requests in flight. I suggest changing this to:
bool have_high_priority_requests_in_flight = client->in_flight_requests.size() >
num_low_priority_requests_in_flight;

[James Simonsen, 2013/03/18 23:50:13]

Done.

+      client->in_flight_requests.size() - num_low_priority_requests_in_flight;
+  if (num_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) {
+    : has_body(false),
+      pending_requests(net::NUM_PRIORITIES) {
 }
 
 ResourceScheduler::Client::~Client() {
   DCHECK(in_flight_requests.empty());
-  DCHECK(pending_requests.empty());
+  DCHECK(!pending_requests.size());
 }
 
 }  // namespace content