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 18 matching lines @@
 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),
+        total_critical_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;
@@ skipping 20 matching lines @@
       // 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);
+    // Clear any special state that we were tracking for this request.
+    SetRequestCategory(request, NORMAL_REQUEST);
     DCHECK_LE(total_delayable_count_, in_flight_requests_.size());
+    DCHECK_LE(total_critical_count_, in_flight_requests_.size());

[mmenke, 2014/08/13 17:55:11]

Maybe these would get more coverage if at the end of SetRequestCategory instead?
 Could even make these more robust, by actually counting the requests in a
category.  i.e.

DCHECK_EQ(CountRequestsInCategory(DELAYABLE_REQUEST), total_delayable_count_);
and then another one for critical requests as well.

The accounting here isn't that complicated, but I'm paranoid about it
regressing.

[Pat Meenan, 2014/08/13 19:24:55]

Done.

   }
 
   void ClearInFlightRequests() {
     in_flight_requests_.clear();
     total_delayable_count_ = 0;
+    total_critical_count_ = 0;
   }
 
-  bool IsDelayableRequest(ScheduledResourceRequest* request) {
+  void SetRequestCategory(ScheduledResourceRequest* request,
+                          RequestCategory category) {
+    RequestCategory old_category = request->category();
+    if (old_category == category)
+      return;
+
+    if (old_category == DELAYABLE_REQUEST)
+      total_delayable_count_--;
+    if (old_category == CRITICAL_REQUEST)
+      total_critical_count_--;
+
+    if (category == DELAYABLE_REQUEST)
+      total_delayable_count_++;
+    if (category == CRITICAL_REQUEST)
+      total_critical_count_++;
+
+    request->set_category(category);
+  }
+
+  RequestCategory GetRequestCategory(ScheduledResourceRequest* request) {
+    if (!has_body_ && request->url_request()->priority() >= net::LOW)
+      return CRITICAL_REQUEST;

[mmenke, 2014/08/13 17:55:11]

Do critical requests prevent has_body_ from becoming true?  If not, we'll make
them non-critical once has_body_ is true, and they're redirected, since we
re-evaluating their priority on redirect.

[Pat Meenan, 2014/08/13 19:24:55]

No, they do not - good catch.  I changed the logic to not reset existing
critical requests as long as the request priority hasn't dropped to make it a
low (can't think of a case where someone would do that but better safe than
sorry).

     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. Critical requests:
+  //   * High-priority requests initiated before the renderer has a <body>.

[mmenke, 2014/08/13 17:55:11]

I think it's kinda weird (And very non-intuitive) that "critical" requests can
have lower priority than "high priority" requests.  This can theoretically have
implications for when we start loading delayable requests as well.

[Pat Meenan, 2014/08/13 19:24:55]

Would it be less worrying if I called them render-blocking instead of critical? 
We don't start loading delayable requests until all of the critical requests
have finished (outside of the one-at-a-time gate we allow when we are in the
critical phase).  All high-priority requests started before we hit the body are
critical.

I guess it's theoretically possible for the renderer to give a medium priority
to head resources and then something in the body gets a higher priority which
would cause it to compete with the per-host and connection limits against the
critical request but it won't break anything.  That feels like logic that the
renderer can decide to be smarter about if it really wants to though - I'm not
sure we want to have the extra logic on the net side of the fence.

+  //
+  // 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

[mmenke, 2014/08/13 17:55:11]

nit:  While you're here, SDPY -> SPDY

[Pat Meenan, 2014/08/13 19:24:55]

Done.

-  //     immediately
+  //     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 toload at a time while critical requests
+  //     are loading.
+  //   * Once all critical 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 53 matching lines @@
     }
 
     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_ &&
+    if (have_immediate_requests_in_flight &&
+        total_critical_count_ != 0 &&

[mmenke, 2014/08/13 17:55:11]

Shouldn't we check if the current request is critical somewhere?  Or am I
missing something?

[Pat Meenan, 2014/08/13 19:24:55]

I added a comment to the priority >= net::LOW check above which is where
critical requests would get scheduled (basically always requested as long as
they meet the host limit).

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

Oops - forgot to delete that comment once I figured out what was going on.

         num_delayable_requests_in_flight != 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:
@@ skipping 35 matching lines @@
   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_;
+  // The number of critical in-flight requests.
+  size_t total_critical_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