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

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 <set>
 
 #include "content/browser/loader/resource_scheduler.h"
 
 #include "base/stl_util.h"
 #include "content/common/resource_messages.h"
@@ skipping 110 matching lines @@
                            ResourceScheduler* scheduler,
                            const RequestPriorityParams& priority)
       : ResourceMessageDelegate(request),
         client_id_(client_id),
         request_(request),
         ready_(false),
         deferred_(false),
         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());
   }
 
   virtual ~ScheduledResourceRequest() {
     scheduler_->RemoveRequest(this);
   }
 
   void Start() {
     TRACE_EVENT_ASYNC_STEP_PAST0("net", "URLRequest", request_, "Queued");
@@ skipping 10 matching lines @@
   const RequestPriorityParams& get_request_priority_params() const {
     return priority_;
   }
   const ClientId& client_id() const { return client_id_; }
   net::URLRequest* url_request() { return request_; }
   const net::URLRequest* url_request() const { return request_; }
   uint32 fifo_ordering() const { return fifo_ordering_; }
   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:
   // ResourceMessageDelegate interface:
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE {
     bool handled = true;
     IPC_BEGIN_MESSAGE_MAP(ScheduledResourceRequest, message)
       IPC_MESSAGE_HANDLER(ResourceHostMsg_DidChangePriority, DidChangePriority)
       IPC_MESSAGE_UNHANDLED(handled = false)
     IPC_END_MESSAGE_MAP()
@@ skipping 11 matching lines @@
 
   void DidChangePriority(int request_id, net::RequestPriority new_priority,
                          int intra_priority_value) {
     scheduler_->ReprioritizeRequest(this, new_priority, intra_priority_value);
   }
 
   ClientId client_id_;
   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);
 };
 
 bool ResourceScheduler::ScheduledResourceSorter::operator()(
     const ScheduledResourceRequest* a,
     const ScheduledResourceRequest* b) const {
   // Want the set to be ordered first by decreasing priority, then by
   // decreasing intra_priority.
   // ie. with (priority, intra_priority)
@@ skipping 17 matching lines @@
 // Each client represents a tab.
 class ResourceScheduler::Client {
  public:
   explicit Client(ResourceScheduler* scheduler)
       : is_audible_(false),
         is_visible_(false),
         is_loaded_(false),
         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;
   }
 
   ~Client() {
     // Update to default state and pause to ensure the scheduler has a
     // correct count of relevant types of clients.
     is_visible_ = false;
     is_audible_ = false;
     is_paused_ = true;
     UpdateThrottleState();
   }
 
   void ScheduleRequest(
       net::URLRequest* url_request,
       ScheduledResourceRequest* request) {
     if (ShouldStartRequest(request) == START_REQUEST) {
       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?

     }
   }
 
   void RemoveRequest(ScheduledResourceRequest* request) {
     if (pending_requests_.IsQueued(request)) {
       pending_requests_.Erase(request);
       DCHECK(!ContainsKey(in_flight_requests_, request));
     } else {
       EraseInFlightRequest(request);
 
       // Removing this request may have freed up another to load.
       LoadAnyStartablePendingRequests();
     }
   }
 
   RequestSet RemoveAllRequests() {
     RequestSet unowned_requests;
     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;
   }
 
   bool is_active() const { return is_visible_ || is_audible_; }
 
   bool is_loaded() const { return is_loaded_; }
 
   void OnAudibilityChanged(bool is_audible) {
@@ skipping 75 matching lines @@
 
   void ReprioritizeRequest(ScheduledResourceRequest* request,
                            RequestPriorityParams old_priority_params,
                            RequestPriorityParams new_priority_params) {
     request->url_request()->SetPriority(new_priority_params.priority);
     request->set_request_priority_params(new_priority_params);
     if (!pending_requests_.IsQueued(request)) {
       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;
     }
 
     pending_requests_.Erase(request);
     pending_requests_.Insert(request);
 
     if (new_priority_params.priority > old_priority_params.priority) {
       // Check if this request is now able to load at its new priority.
       LoadAnyStartablePendingRequests();
@@ skipping 46 matching lines @@
 
  private:
   enum ShouldStartReqResult {
     DO_NOT_START_REQUEST_AND_STOP_SEARCHING,
     DO_NOT_START_REQUEST_AND_KEEP_SEARCHING,
     START_REQUEST,
   };
 
   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(
       const net::HostPortPair& active_request_host) const {
     size_t same_host_count = 0;
     for (RequestSet::const_iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       net::HostPortPair host_port_pair =
           net::HostPortPair::FromURL((*it)->url_request()->url());
       if (active_request_host.Equals(host_port_pair)) {
         same_host_count++;
         if (same_host_count >= kMaxNumDelayableRequestsPerHost)
           return true;
       }
     }
     return false;
   }
 
   void StartRequest(ScheduledResourceRequest* request) {
     InsertInFlightRequest(request);
     request->Start();
   }
 
   // ShouldStartRequest is the main scheduling algorithm.
   //
-  // Requests are categorized into three categories:
+  // Requests are categorized into five categories:
   //
   // 1. Non-delayable requests:
   //   * Synchronous requests.
   //   * Non-HTTP[S] requests.
   //
   // 2. Requests to SPDY-capable origin servers.
   //
   // 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.
   //   * At most one non-SPDY request will be issued per THROTTLED Client
   //   * If no high priority requests are in flight, start loading low priority
   //     requests.
   //
   //  COALESCED Clients never load requests, with the following exceptions:
   //   * Non-delayable requests are issued imediately.
   //   * On a (currently 5 second) heart beat, they load all requests as an
@@ skipping 36 matching lines @@
     if (http_server_properties.SupportsSpdy(host_port_pair)) {
       return START_REQUEST;
     }
 
     if (throttle_state_ == THROTTLED &&
         in_flight_requests_.size() >= kMaxNumThrottledRequestsPerClient) {
       // There may still be SPDY-capable requests that should be issued.
       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;
     }
 
     if (ShouldKeepSearching(host_port_pair)) {
       // There may be other requests for other hosts we'd allow,
       // so keep checking.
       return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
     }
 
     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;
     }
 
     return START_REQUEST;
   }
 
   void LoadAnyStartablePendingRequests() {
     // We iterate through all the pending requests, starting with the highest
     // priority one. For each entry, one of three things can happen:
     // 1) We start the request, remove it from the list, and keep checking.
@@ skipping 33 matching lines @@
   bool is_audible_;
   bool is_visible_;
   bool is_loaded_;
   bool is_paused_;
   bool has_body_;
   bool using_spdy_proxy_;
   RequestQueue pending_requests_;
   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_;
 };
 
 ResourceScheduler::ResourceScheduler()
     : should_coalesce_(false),
       should_throttle_(false),
       active_clients_loading_(0),
       coalesced_clients_(0),
       coalescing_timer_(new base::Timer(true /* retain_user_task */,
                                         true /* is_repeating */)) {
@@ skipping 289 matching lines @@
   client->ReprioritizeRequest(
       request, old_priority_params, new_priority_params);
 }
 
 ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
     int child_id, int route_id) {
   return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
 }  // namespace content