Changed resource scheduler to block until all critical resources actually load.

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 85adbdb30e275631b53af88286e75b658e73a16c..0b6678d2ef8570cfcf1e0c7319cbdf03648e4209 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -128,7 +128,7 @@ class ResourceScheduler::ScheduledResourceRequest
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0),
-        accounted_as_delayable_request_(false) {
+        category_(NORMAL_REQUEST) {
     TRACE_EVENT_ASYNC_BEGIN1("net", "URLRequest", request_,
                              "url", request->url().spec());
   }
@@ -159,11 +159,11 @@ class ResourceScheduler::ScheduledResourceRequest
   void set_fifo_ordering(uint32 fifo_ordering) {
     fifo_ordering_ = fifo_ordering;
   }
-  bool accounted_as_delayable_request() const {
-    return accounted_as_delayable_request_;
+  RequestCategory category() const {
+    return category_;
   }
-  void set_accounted_as_delayable_request(bool accounted) {
-    accounted_as_delayable_request_ = accounted;
+  void set_category(RequestCategory category) {
+    category_ = category;
   }
 
  private:
@@ -195,11 +195,10 @@ class ResourceScheduler::ScheduledResourceRequest
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
+  RequestCategory category_;
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32 fifo_ordering_;
-  // True if the request is delayable in |in_flight_requests_|.
-  bool accounted_as_delayable_request_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
@@ -237,7 +236,8 @@ class ResourceScheduler::Client {
         is_paused_(false),
         has_body_(false),
         using_spdy_proxy_(false),
-        total_delayable_count_(0),
+        in_flight_delayable_count_(0),
+        total_layout_blocking_count_(0),
         throttle_state_(ResourceScheduler::THROTTLED) {
     scheduler_ = scheduler;
   }
@@ -258,6 +258,13 @@ class ResourceScheduler::Client {
       StartRequest(request);
     } else {
       pending_requests_.Insert(request);
+      // We need to keep track of all layout-blocking requests, not just ones
+      // that are in-flight.  StartRequest() updates the accounting for ones
+      // that are in-flight and this catches the ones that are queued.  We also
+      // need to do the accounting after it is in the queue so the DCHECK to
+      // validate the counts is accurate.

[mmenke, 2014/08/15 19:53:40]

nit:  Don't use we in comments, since it's ambiguous (x2).

[Pat Meenan, 2014/08/15 21:39:31]

Done.

+      if (GetRequestCategory(request) == LAYOUT_BLOCKING_REQUEST)
+        SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST);

[mmenke, 2014/08/15 19:53:40]

Think SetRequestCategory(GetRequestCategory()) is a little weird.  Maybe
Set->Compute or Calculate or somesuch?

[mmenke, 2014/08/15 19:53:40]

I think it's weird that layout blocking requests are layout blocking for life,
but delayable requests have a category() of delayable only once they've been
started, despite having a GetRequestCategory of delayable.

Having trouble coming up with a clean solution to this - I understand this is
for accounting purposes, but it results in a confusing API.

[Pat Meenan, 2014/08/15 21:39:31]

Agreed.  I could set the category on everything all of the time but then we'd
have to move the accounting out and that would get distributed again (and it's
logic would be just about as confusing because it would be doing separate
accounting depending on if the request is in-flight or not).  I'm good with
doing it either way but I think this might be simpler even though it's still
confusing.

[mmenke, 2014/08/18 16:30:20]

I'm on the fence...  Clarity reduces the change of regression, and the other
approach is clearer...  But it's also more complicated, which increases the
chance of regressions.

If we keep the current behavior, however, we should have comments about this
weirdness (Probably in a couple places - above the category_ itself, in
SetRequestCategory, and in the code that only sets the LAYOUT_BLOCKING
category), and SetRequestCategory should have a DCHECK that when we set a
request as delayable, it's in in_flight_requests_.

[Pat Meenan, 2014/08/18 18:29:57]

I think I found a cleaner way to solve it.  I changed the delayable designation
to in-flight delayable, always set the category for every request and check to
see if it is in-flight at the point where it gets set.  I also changed
"category" to "classification" because it seems closer to how we actually treat
the requests.  All of the logic is contained in the set/get classification calls
and we just need to make sure it is called when requests are scheduled,
priorities changed or started (all of which are already covered).

Do you think we need comments about making sure to call
SetRequestClassification() at all of these points explicitly?

     }
   }
 
@@ -278,7 +285,7 @@ class ResourceScheduler::Client {
     for (RequestSet::iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       unowned_requests.insert(*it);
-      (*it)->set_accounted_as_delayable_request(false);
+      (*it)->set_category(NORMAL_REQUEST);
     }
     ClearInFlightRequests();
     return unowned_requests;
@@ -374,7 +381,7 @@ class ResourceScheduler::Client {
       DCHECK(ContainsKey(in_flight_requests_, request));
       // The priority and SPDY support may have changed, so update the
       // delayable count.
-      SetRequestDelayable(request, IsDelayableRequest(request));
+      SetRequestCategory(request, GetRequestCategory(request));
       // Request has already started.
       return;
     }
@@ -441,44 +448,84 @@ class ResourceScheduler::Client {
 
   void InsertInFlightRequest(ScheduledResourceRequest* request) {
     in_flight_requests_.insert(request);
-    if (IsDelayableRequest(request))
-      SetRequestDelayable(request, true);
+    SetRequestCategory(request, GetRequestCategory(request));
   }
 
   void EraseInFlightRequest(ScheduledResourceRequest* request) {
     size_t erased = in_flight_requests_.erase(request);
     DCHECK_EQ(1u, erased);
-    SetRequestDelayable(request, false);
-    DCHECK_LE(total_delayable_count_, in_flight_requests_.size());
+    // Clear any special state that we were tracking for this request.
+    SetRequestCategory(request, NORMAL_REQUEST);
   }
 
   void ClearInFlightRequests() {
     in_flight_requests_.clear();
-    total_delayable_count_ = 0;
+    in_flight_delayable_count_ = 0;
+    total_layout_blocking_count_ = 0;
   }
 
-  bool IsDelayableRequest(ScheduledResourceRequest* request) {
+  size_t CountRequestsInCategory(RequestCategory category,
+                                 bool include_pending) {

[mmenke, 2014/08/15 19:53:40]

nit:  const

[Pat Meenan, 2014/08/15 21:39:32]

Done.

+    size_t category_request_count = 0;
+    for (RequestSet::const_iterator it = in_flight_requests_.begin();
+         it != in_flight_requests_.end(); ++it) {
+      if ((*it)->category() == category)
+        category_request_count++;
+    }
+    if (include_pending) {
+      for (RequestQueue::NetQueue::const_iterator
+           it = pending_requests_.GetNextHighestIterator();
+           it != pending_requests_.End(); ++it) {
+        if ((*it)->category() == category)
+          category_request_count++;
+      }
+    }
+    return category_request_count;
+  }
+
+  void SetRequestCategory(ScheduledResourceRequest* request,
+                          RequestCategory category) {
+    RequestCategory old_category = request->category();
+    if (old_category == category)
+      return;
+
+    if (old_category == DELAYABLE_REQUEST)
+      in_flight_delayable_count_--;
+    if (old_category == LAYOUT_BLOCKING_REQUEST)
+      total_layout_blocking_count_--;
+
+    if (category == DELAYABLE_REQUEST)
+      in_flight_delayable_count_++;
+    if (category == LAYOUT_BLOCKING_REQUEST)
+      total_layout_blocking_count_++;
+
+    request->set_category(category);
+    DCHECK_EQ(CountRequestsInCategory(DELAYABLE_REQUEST, false),
+              in_flight_delayable_count_);
+    DCHECK_EQ(CountRequestsInCategory(LAYOUT_BLOCKING_REQUEST, true),
+              total_layout_blocking_count_);
+  }
+
+  RequestCategory GetRequestCategory(ScheduledResourceRequest* request) {
+    // If a request is already marked as layout-blocking make sure to keep the
+    // category across redirects unless we chose to lower the priority of the

[mmenke, 2014/08/15 19:53:40]

nit:  Don't use we in comments.

[Pat Meenan, 2014/08/15 21:39:31]

Done.

+    // request.
+    if (request->category() == LAYOUT_BLOCKING_REQUEST &&
+        request->url_request()->priority() >= net::LOW)
+      return LAYOUT_BLOCKING_REQUEST;

[mmenke, 2014/08/15 19:53:40]

nit:  Use braces when the conditional takes up more than one line.

[Pat Meenan, 2014/08/15 21:39:31]

Done.

+
+    if (!has_body_ && request->url_request()->priority() >= net::LOW)
+      return LAYOUT_BLOCKING_REQUEST;
     if (request->url_request()->priority() < net::LOW) {
       net::HostPortPair host_port_pair =
           net::HostPortPair::FromURL(request->url_request()->url());
       net::HttpServerProperties& http_server_properties =
           *request->url_request()->context()->http_server_properties();
       if (!http_server_properties.SupportsSpdy(host_port_pair)) {
-        return true;
+        return DELAYABLE_REQUEST;
       }
     }
-    return false;
-  }
-
-  void SetRequestDelayable(ScheduledResourceRequest* request,
-                           bool delayable) {
-    if (request->accounted_as_delayable_request() == delayable)
-      return;
-    if (delayable)
-      total_delayable_count_++;
-    else
-      total_delayable_count_--;
-    request->set_accounted_as_delayable_request(delayable);
+    return NORMAL_REQUEST;
   }
 
   bool ShouldKeepSearching(
@@ -504,7 +551,7 @@ class ResourceScheduler::Client {
 
   // ShouldStartRequest is the main scheduling algorithm.
   //
-  // Requests are categorized into three categories:
+  // Requests are categorized into five categories:
   //
   // 1. Non-delayable requests:
   //   * Synchronous requests.
@@ -515,20 +562,25 @@ class ResourceScheduler::Client {
   // 3. High-priority requests:
   //   * Higher priority requests (>= net::LOW).
   //
-  // 4. Low priority requests
+  // 4. Layout-blocking requests:
+  //   * High-priority requests initiated before the renderer has a <body>.
+  //
+  // 5. Low priority requests
   //
   //  The following rules are followed:
   //
   //  ACTIVE_AND_LOADING and UNTHROTTLED Clients follow these rules:
-  //   * Non-delayable, High-priority and SDPY capable requests are issued
-  //     immediately
+  //   * Non-delayable, High-priority and SPDY capable requests are issued
+  //     immediately.
   //   * If no high priority requests are in flight, start loading low priority
   //     requests.
   //   * Low priority requests are delayable.
-  //   * Once the renderer has a <body>, start loading delayable requests.
+  //   * Allow one delayable request to load at a time while layout-blocking
+  //     requests are loading.
+  //   * Once all layout-blocking requests have finished loading, start loading
+  //     delayable requests.
   //   * Never exceed 10 delayable requests in flight per client.
   //   * Never exceed 6 delayable requests for a given host.
-  //   * Prior to <body>, allow one delayable request to load at a time.
   //
   //  THROTTLED Clients follow these rules:
   //   * Non-delayable and SPDY-capable requests are issued immediately.
@@ -585,12 +637,12 @@ class ResourceScheduler::Client {
       return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
     }
 
+    // High-priority and layout-blocking requests.
     if (url_request.priority() >= net::LOW) {
       return START_REQUEST;
     }
 
-    size_t num_delayable_requests_in_flight = total_delayable_count_;
-    if (num_delayable_requests_in_flight >= kMaxNumDelayableRequestsPerClient) {
+    if (in_flight_delayable_count_ >= kMaxNumDelayableRequestsPerClient) {
       return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
     }
 
@@ -601,9 +653,10 @@ class ResourceScheduler::Client {
     }
 
     bool have_immediate_requests_in_flight =
-        in_flight_requests_.size() > num_delayable_requests_in_flight;
-    if (have_immediate_requests_in_flight && !has_body_ &&
-        num_delayable_requests_in_flight != 0) {
+        in_flight_requests_.size() > in_flight_delayable_count_;

[mmenke, 2014/08/15 19:53:40]

Random comment, no need to worry about in this CL:  SPDY requests count towards
in_flight_requests_.size here.  Should they?  Seems weird that lower priority
SPDY requests jump to the front of the queue, potentially delaying the start of
low-priority non-SPDY requests.

[Pat Meenan, 2014/08/15 21:39:31]

SPDY handling across multiple connections in general is pretty weak (and by weak
I mean non-existent).  We just dump all requests into a SPDY connection
regardless of state or anything else and expect the server to handle
prioritization but that only works when you're only talking to a single server
which isn't the case for just about any site I've ever looked at, including
Google's.

It's probably worth re-visiting that and applying the same multi-phase logic
across SPDY connections as well (probably don't want to take it further than
that).

+    if (have_immediate_requests_in_flight &&
+        total_layout_blocking_count_ != 0 &&
+        in_flight_delayable_count_ != 0) {
       return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
     }
 
@@ -657,7 +710,9 @@ class ResourceScheduler::Client {
   RequestSet in_flight_requests_;
   ResourceScheduler* scheduler_;
   // The number of delayable in-flight requests.
-  size_t total_delayable_count_;
+  size_t in_flight_delayable_count_;
+  // The number of layout-blocking in-flight requests.
+  size_t total_layout_blocking_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
 };