Delay resource scheduling decisions until network access.

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 cda8a6c787bf5e679b9242456898468373ac750f..379c7c78f83a8d0b0e250207e358bb3603b8fd35 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -32,9 +32,9 @@ namespace content {
 
 namespace {
 
-enum StartMode {
-  START_SYNC,
-  START_ASYNC
+enum ResumeMode {
+  RESUME_SYNC,
+  RESUME_ASYNC
 };
 
 // Field trial constants
@@ -151,8 +151,7 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
                            bool is_async)
       : client_id_(client_id),
         request_(request),
-        ready_(false),
-        deferred_(false),
+        paused_(false),
         is_async_(is_async),
         attributes_(kAttributeNone),
         scheduler_(scheduler),
@@ -164,43 +163,39 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
   }
 
   ~ScheduledResourceRequest() override {
-    request_->RemoveUserData(kUserDataKey);
-    scheduler_->RemoveRequest(this);
-  }
+      request_->RemoveUserData(kUserDataKey);
+      scheduler_->RemoveRequest(this);
+    }

[mmenke, 2016/02/25 18:09:51]

Fix indent (Or just run git cl format)

 
   static ScheduledResourceRequest* ForRequest(net::URLRequest* request) {
     return static_cast<UnownedPointer*>(request->GetUserData(kUserDataKey))
         ->get();
   }
 
-  // Starts the request. If |start_mode| is START_ASYNC, the request will not
-  // be started immediately.
-  void Start(StartMode start_mode) {
-    DCHECK(!ready_);
-
+  // Resumes the request. If |resume_mode| is RESUME_ASYNC, the request will not
+  // be resumed immediately.
+  void Resume(ResumeMode resume_mode) {
     // If the request was cancelled, do nothing.
     if (!request_->status().is_success())
       return;
 
-    // If the request was deferred, need to start it.  Otherwise, will just not
-    // defer starting it in the first place, and the value of |start_mode|
+    // If the request was paused, need to resume it.  Otherwise, will just not
+    // pause it in the first place, and the value of |resume_mode|
     // makes no difference.
-    if (deferred_) {
+    if (paused_) {
       // If can't start the request synchronously, post a task to start the
       // request.
-      if (start_mode == START_ASYNC) {
+      if (resume_mode == RESUME_ASYNC) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE,
-            base::Bind(&ScheduledResourceRequest::Start,
+            base::Bind(&ScheduledResourceRequest::Resume,
                        weak_ptr_factory_.GetWeakPtr(),
-                       START_SYNC));
+                       RESUME_SYNC));
         return;
       }
-      deferred_ = false;
+      paused_ = false;
       controller()->Resume();
     }
-
-    ready_ = true;
   }
 
   void set_request_priority_params(const RequestPriorityParams& priority) {
@@ -241,16 +236,15 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
   static const void* const kUserDataKey;
 
   // ResourceThrottle interface:
-  void WillStartRequest(bool* defer) override {
-    deferred_ = *defer = !ready_;
+  void WillStartUsingNetwork(bool* defer) override {
+    paused_ = *defer = !scheduler_->ShouldSendRequest(this);
   }
 
   const char* GetNameForLogging() const override { return "ResourceScheduler"; }
 
   const ClientId client_id_;
   net::URLRequest* request_;
-  bool ready_;
-  bool deferred_;
+  bool paused_;
   bool is_async_;
   RequestAttributes attributes_;
   ResourceScheduler* scheduler_;
@@ -304,25 +298,30 @@ class ResourceScheduler::Client {
 
   void ScheduleRequest(net::URLRequest* url_request,

[mmenke, 2016/02/25 18:09:51]

SchedulerRequest -> AddRequest?  This is no longer doing any actual scheduling.

                        ScheduledResourceRequest* request) {
+    all_requests_.insert(request);
     SetRequestAttributes(request, DetermineRequestAttributes(request));
+  }
+
+  bool ShouldSendRequest(ScheduledResourceRequest* request) {

[mmenke, 2016/02/25 18:09:51]

Think this needs a description

[mmenke, 2016/02/25 18:09:51]

This should be renamed.  "Should" implies no side effects, and doing nothing but
returning a bool.

     if (ShouldStartRequest(request) == START_REQUEST) {
-      // New requests can be started synchronously without issue.
-      StartRequest(request, START_SYNC);
+      InsertInFlightRequest(request);
+      return true;
     } else {
       pending_requests_.Insert(request);

[mmenke, 2016/02/25 18:09:51]

pending_requests_ -> paused_requests_, maybe?

+      return false;
     }
   }
 
   void RemoveRequest(ScheduledResourceRequest* request) {
-    if (pending_requests_.IsQueued(request)) {
+    if (pending_requests_.IsQueued(request))
       pending_requests_.Erase(request);

[mmenke, 2016/02/25 18:09:51]

Why did you remove the DCHECK here?

-      DCHECK(!ContainsKey(in_flight_requests_, request));
-    } else {
+    else if (ContainsKey(in_flight_requests_, request))
       EraseInFlightRequest(request);

[mmenke, 2016/02/25 18:09:51]

nit:  Network stack team generally uses braces for all else-ifs.  Since we're
now the owns who now own this file, should probably stick with our style.

-
-      // Removing this request may have freed up another to load.
-      LoadAnyStartablePendingRequests();
-    }
+    SetRequestAttributes(request, kAttributeNone);
+    all_requests_.erase(request);

[mmenke, 2016/02/25 18:09:51]

We should update the list before calling SetRequestAttributes, like we do
elsewhere, since you remove the current_request parameter of
SetRequestAttributes.

This order happens not to cause the DCHECKs at the end of SetRequestAttributes
to trigger, but...

+    // Removing this request may have freed up another to load or may have
+    // cleared out the queue of layout-blocking requests.
+    LoadAnyStartablePendingRequests();
   }
 
   RequestSet StartAndRemoveAllRequests() {
@@ -334,10 +333,9 @@ class ResourceScheduler::Client {
     while (!pending_requests_.IsEmpty()) {
       ScheduledResourceRequest* request =
           *pending_requests_.GetNextHighestIterator();
-      pending_requests_.Erase(request);
-      // Starting requests asynchronously ensures no side effects, and avoids
-      // starting a bunch of requests that may be about to be deleted.
-      StartRequest(request, START_ASYNC);
+      // Resuming requests asynchronously ensures no side effects, and avoids
+      // resuming a bunch of requests that may be about to be deleted.
+      ResumeRequest(request, RESUME_ASYNC);
     }
     RequestSet unowned_requests;
     for (RequestSet::iterator it = in_flight_requests_.begin();
@@ -379,8 +377,7 @@ class ResourceScheduler::Client {
     request->set_request_priority_params(new_priority_params);
     SetRequestAttributes(request, DetermineRequestAttributes(request));
     if (!pending_requests_.IsQueued(request)) {
-      DCHECK(ContainsKey(in_flight_requests_, request));
-      // Request has already started.
+      // Request is not waiting to be sent.
       return;
     }
 
@@ -418,30 +415,18 @@ class ResourceScheduler::Client {
     total_layout_blocking_count_ = 0;
   }
 
-  size_t CountRequestsWithAttributes(
-      const RequestAttributes attributes,
-      ScheduledResourceRequest* current_request) {
+  size_t CountRequestsWithAttributes(const RequestAttributes attributes) {
     size_t matching_request_count = 0;
-    for (RequestSet::const_iterator it = in_flight_requests_.begin();
-         it != in_flight_requests_.end(); ++it) {
-      if (RequestAttributesAreSet((*it)->attributes(), attributes))
-        matching_request_count++;
-    }
-    if (!RequestAttributesAreSet(attributes, kAttributeInFlight)) {
-      bool current_request_is_pending = false;
-      for (RequestQueue::NetQueue::const_iterator
-           it = pending_requests_.GetNextHighestIterator();
-           it != pending_requests_.End(); ++it) {
+    if (RequestAttributesAreSet(attributes, kAttributeInFlight)) {
+      for (RequestSet::const_iterator it = in_flight_requests_.begin();
+           it != in_flight_requests_.end(); ++it) {

[mmenke, 2016/02/25 18:09:51]

Switch to range loops while you're here, on both of these loops?

         if (RequestAttributesAreSet((*it)->attributes(), attributes))
           matching_request_count++;
-        if (*it == current_request)
-          current_request_is_pending = true;
       }
-      // Account for the current request if it is not in one of the lists yet.
-      if (current_request &&
-          !ContainsKey(in_flight_requests_, current_request) &&
-          !current_request_is_pending) {
-        if (RequestAttributesAreSet(current_request->attributes(), attributes))
+    } else {
+      for (RequestSet::const_iterator it = all_requests_.begin();
+           it != all_requests_.end(); ++it) {
+        if (RequestAttributesAreSet((*it)->attributes(), attributes))
           matching_request_count++;
       }
     }
@@ -475,9 +460,9 @@ class ResourceScheduler::Client {
 
     request->set_attributes(attributes);
     DCHECK_EQ(CountRequestsWithAttributes(
-        kAttributeInFlight | kAttributeDelayable, request),
-        in_flight_delayable_count_);
-    DCHECK_EQ(CountRequestsWithAttributes(kAttributeLayoutBlocking, request),
+              kAttributeInFlight | kAttributeDelayable),
+              in_flight_delayable_count_);
+    DCHECK_EQ(CountRequestsWithAttributes(kAttributeLayoutBlocking),
               total_layout_blocking_count_);
   }
 
@@ -531,10 +516,12 @@ class ResourceScheduler::Client {
     return false;
   }
 
-  void StartRequest(ScheduledResourceRequest* request,
-                    StartMode start_mode) {
+  void ResumeRequest(ScheduledResourceRequest* request,
+                     ResumeMode resume_mode) {
+    DCHECK(pending_requests_.IsQueued(request));
+    pending_requests_.Erase(request);
     InsertInFlightRequest(request);
-    request->Start(start_mode);
+    request->Resume(resume_mode);
   }
 
   // ShouldStartRequest is the main scheduling algorithm.
@@ -677,8 +664,7 @@ class ResourceScheduler::Client {
       ShouldStartReqResult query_result = ShouldStartRequest(request);
 
       if (query_result == START_REQUEST) {
-        pending_requests_.Erase(request);
-        StartRequest(request, START_ASYNC);
+        ResumeRequest(request, RESUME_ASYNC);
 
         // StartRequest can modify the pending list, so we (re)start evaluation
         // from the currently highest priority request. Avoid copying a singular
@@ -702,6 +688,7 @@ class ResourceScheduler::Client {
   // layout-blocking resources.
   bool has_html_body_;
   bool using_spdy_proxy_;
+  RequestSet all_requests_;

[mmenke, 2016/02/25 18:09:51]

Hrm...  I really don't like keeping around a third set of requests, instead of
the original two.  Can we just get rid of one of the old ones?  Could even get
rid of both, I suppose, though that may have performance implications.

   RequestQueue pending_requests_;
   RequestSet in_flight_requests_;
   ResourceScheduler* scheduler_;
@@ -794,7 +781,6 @@ scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
     // 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(START_SYNC);
     return std::move(request);
   }
 
@@ -803,6 +789,17 @@ scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
   return std::move(request);
 }
 
+bool ResourceScheduler::ShouldSendRequest(ScheduledResourceRequest* request) {

[mmenke, 2016/02/25 18:09:51]

Per earlier comment, the number of side effects this has makes "Should*" a
confusing name for this method.

+  DCHECK(CalledOnValidThread());
+  ClientMap::iterator client_it = client_map_.find(request->client_id());
+  if (client_it == client_map_.end()) {

[mmenke, 2016/02/25 18:09:51]

Think this warrants a comment.

+    return true;
+  }

[mmenke, 2016/02/25 18:09:51]

nit:  Remove braces.

+
+  Client* client = client_it->second;
+  return client->ShouldSendRequest(request);
+}
+
 void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
   DCHECK(CalledOnValidThread());
   if (ContainsKey(unowned_requests_, request)) {