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 ef95f2a34f9cb79b962246ed847e81e70f2ef110..8d510f34849a8d4049523f2cdf6f7bd42566a3af 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -16,6 +16,8 @@
 
 namespace content {
 
+static const int kMaxNumLowPriorityRequestsPerClient = 10;
+
 class ResourceScheduler::ScheduledResourceRequest
     : public ResourceMessageDelegate,
       public ResourceThrottle {
@@ -43,6 +45,18 @@ class ResourceScheduler::ScheduledResourceRequest
     }
   }
 
+  // |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_; }
 
@@ -65,9 +79,9 @@ 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();
+    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);
     }
   }
 
@@ -76,6 +90,7 @@ class ResourceScheduler::ScheduledResourceRequest
   bool ready_;
   bool deferred_;
   ResourceScheduler* scheduler_;
+  ResourceScheduler::RequestQueue::Pointer queue_pointer_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
@@ -86,7 +101,7 @@ 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());
@@ -114,17 +129,12 @@ 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 {
+    RequestQueue::Pointer pointer = client->pending_requests.Insert(
+        request.get(), url_request->priority());
+    request->set_queue_pointer(pointer);
   }
   return request.PassAs<ResourceThrottle>();
 }
@@ -142,30 +152,18 @@ 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);
-    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) {
@@ -233,26 +231,93 @@ void ResourceScheduler::StartRequest(ScheduledResourceRequest* 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