Fix Resource Priorities and Scheduling (Chrome Side)

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 1d118f0ffbc2ae3be243e75f8eb227a1073edfcf..1e7a4888e00c3e2dd986a58553d70ecff340fee2 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -2,10 +2,10 @@
 // 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 <set>
+
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
@@ -37,6 +37,16 @@ const char kThrottleCoalesceFieldTrialCoalesce[] = "Coalesce";
 const char kRequestLimitFieldTrial[] = "OutstandingRequestLimiting";
 const char kRequestLimitFieldTrialGroupPrefix[] = "Limit";
 
+const char kResourcePrioritiesFieldTrial[] = "ResourcePriorities";
+
+// Flags identifying various attributes of the request that are used
+// when making scheduling decisions.
+typedef uint8_t RequestAttributes;

[mmenke, 2015/08/13 21:27:44]

nit:  include <stdint.h>

[mmenke, 2015/08/13 21:27:45]

I think "using RequestAttributes = uint8_t;" is now preferred

[Pat Meenan, 2015/08/14 13:52:47]

Done.

[Pat Meenan, 2015/08/14 13:52:47]

Done.

+const RequestAttributes kAttributeNone = 0x00;
+const RequestAttributes kAttributeInFlight = 0x01;
+const RequestAttributes kAttributeDelayable = 0x02;
+const RequestAttributes kAttributeLayoutBlocking = 0x04;
+
 // Post ResourceScheduler histograms of the following forms:
 // If |histogram_suffix| is NULL or the empty string:
 //   ResourceScheduler.base_name.histogram_name
@@ -74,9 +84,12 @@ const char* GetNumClientsString(size_t num_clients) {
 }  // namespace
 
 static const size_t kCoalescedTimerPeriod = 5000;
-static const size_t kMaxNumDelayableRequestsPerClient = 10;
+static const size_t kDefaultMaxNumDelayableRequestsPerClient = 10;
 static const size_t kMaxNumDelayableRequestsPerHost = 6;
 static const size_t kMaxNumThrottledRequestsPerClient = 1;
+static const size_t kDefaultMaxNumDelayableWhileLayoutBlocking = 1;
+static const net::RequestPriority
+    kDefaultLayoutBlockingPriorityThreshold = net::LOW;
 
 struct ResourceScheduler::RequestPriorityParams {
   RequestPriorityParams()
@@ -177,7 +190,7 @@ class ResourceScheduler::ScheduledResourceRequest
         request_(request),
         ready_(false),
         deferred_(false),
-        classification_(NORMAL_REQUEST),
+        attributes_(kAttributeNone),
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0) {
@@ -230,11 +243,11 @@ class ResourceScheduler::ScheduledResourceRequest
   void set_fifo_ordering(uint32 fifo_ordering) {
     fifo_ordering_ = fifo_ordering;
   }
-  RequestClassification classification() const {
-    return classification_;
+  RequestAttributes attributes() const {
+    return attributes_;
   }
-  void set_classification(RequestClassification classification) {
-    classification_ = classification;
+  void set_attributes(RequestAttributes attributes) {
+    attributes_ = attributes;
   }
 
  private:
@@ -266,7 +279,7 @@ class ResourceScheduler::ScheduledResourceRequest
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
-  RequestClassification classification_;
+  RequestAttributes attributes_;
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32 fifo_ordering_;
@@ -308,13 +321,14 @@ class ResourceScheduler::Client {
         is_visible_(is_visible),
         is_loaded_(false),
         is_paused_(false),
-        has_body_(false),
+        has_html_body_(false),
         using_spdy_proxy_(false),
         load_started_time_(base::TimeTicks::Now()),
         scheduler_(scheduler),
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0),
-        throttle_state_(ResourceScheduler::THROTTLED) {}
+        throttle_state_(ResourceScheduler::THROTTLED) {
+  }
 
   ~Client() {
     // Update to default state and pause to ensure the scheduler has a
@@ -328,11 +342,11 @@ class ResourceScheduler::Client {
   void ScheduleRequest(
       net::URLRequest* url_request,
       ScheduledResourceRequest* request) {
+    SetRequestAttributes(request, DetermineRequestAttributes(request));
     if (ShouldStartRequest(request) == START_REQUEST)
       StartRequest(request);
     else
       pending_requests_.Insert(request);
-    SetRequestClassification(request, ClassifyRequest(request));
   }
 
   void RemoveRequest(ScheduledResourceRequest* request) {
@@ -350,10 +364,10 @@ class ResourceScheduler::Client {
   RequestSet StartAndRemoveAllRequests() {
     // First start any pending requests so that they will be moved into
     // in_flight_requests_. This may exceed the limits
-    // kMaxNumDelayableRequestsPerClient, kMaxNumDelayableRequestsPerHost and
-    // kMaxNumThrottledRequestsPerClient, so this method must not do anything
-    // that depends on those limits before calling ClearInFlightRequests()
-    // below.
+    // kDefaultMaxNumDelayableRequestsPerClient, kMaxNumDelayableRequestsPerHost
+    // and kMaxNumThrottledRequestsPerClient, so this method must not do
+    // anything that depends on those limits before calling
+    // ClearInFlightRequests() below.
     while (!pending_requests_.IsEmpty()) {
       ScheduledResourceRequest* request =
           *pending_requests_.GetNextHighestIterator();
@@ -365,7 +379,7 @@ class ResourceScheduler::Client {
     for (RequestSet::iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       unowned_requests.insert(*it);
-      (*it)->set_classification(NORMAL_REQUEST);
+      (*it)->set_attributes(kAttributeNone);
     }
     ClearInFlightRequests();
     return unowned_requests;
@@ -470,12 +484,12 @@ class ResourceScheduler::Client {
   }
 
   void OnNavigate() {
-    has_body_ = false;
+    has_html_body_ = false;
     is_loaded_ = false;
   }
 
   void OnWillInsertBody() {
-    has_body_ = true;
+    has_html_body_ = true;
     LoadAnyStartablePendingRequests();
   }
 
@@ -491,11 +505,9 @@ class ResourceScheduler::Client {
                            RequestPriorityParams new_priority_params) {
     request->url_request()->SetPriority(new_priority_params.priority);
     request->set_request_priority_params(new_priority_params);
+    SetRequestAttributes(request, DetermineRequestAttributes(request));
     if (!pending_requests_.IsQueued(request)) {
       DCHECK(ContainsKey(in_flight_requests_, request));
-      // The priority of the request and priority support of the server may
-      // have changed, so update the delayable count.
-      SetRequestClassification(request, ClassifyRequest(request));
       // Request has already started.
       return;
     }
@@ -562,14 +574,14 @@ class ResourceScheduler::Client {
 
   void InsertInFlightRequest(ScheduledResourceRequest* request) {
     in_flight_requests_.insert(request);
-    SetRequestClassification(request, ClassifyRequest(request));
+    SetRequestAttributes(request, DetermineRequestAttributes(request));
   }
 
   void EraseInFlightRequest(ScheduledResourceRequest* request) {
     size_t erased = in_flight_requests_.erase(request);
     DCHECK_EQ(1u, erased);
     // Clear any special state that we were tracking for this request.
-    SetRequestClassification(request, NORMAL_REQUEST);
+    SetRequestAttributes(request, kAttributeNone);
   }
 
   void ClearInFlightRequests() {
@@ -578,71 +590,91 @@ class ResourceScheduler::Client {
     total_layout_blocking_count_ = 0;
   }
 
-  size_t CountRequestsWithClassification(
-      const RequestClassification classification, const bool include_pending) {
-    size_t classification_request_count = 0;
+  size_t CountRequestsWithAttributes(
+      const RequestAttributes attributes,
+      const bool include_pending) {
+    size_t matching_request_count = 0;
     for (RequestSet::const_iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
-      if ((*it)->classification() == classification)
-        classification_request_count++;
+      if (RequestAttributesAreSet((*it)->attributes(), attributes))
+        matching_request_count++;
     }
     if (include_pending) {
       for (RequestQueue::NetQueue::const_iterator
            it = pending_requests_.GetNextHighestIterator();
            it != pending_requests_.End(); ++it) {
-        if ((*it)->classification() == classification)
-          classification_request_count++;
+        if (RequestAttributesAreSet((*it)->attributes(), attributes))
+          matching_request_count++;
       }
     }
-    return classification_request_count;
+    return matching_request_count;
+  }
+
+  bool RequestAttributesAreSet(RequestAttributes request_attributes,
+                               RequestAttributes matching_attributes) const {
+    return (request_attributes & matching_attributes) == matching_attributes;
   }
 
-  void SetRequestClassification(ScheduledResourceRequest* request,
-                                RequestClassification classification) {
-    RequestClassification old_classification = request->classification();
-    if (old_classification == classification)
+  void SetRequestAttributes(ScheduledResourceRequest* request,
+                            RequestAttributes attributes) {
+    RequestAttributes old_attributes = request->attributes();
+    if (old_attributes == attributes)
       return;
 
-    if (old_classification == IN_FLIGHT_DELAYABLE_REQUEST)
+    if (RequestAttributesAreSet(old_attributes,
+                               kAttributeInFlight | kAttributeDelayable)) {

[mmenke, 2015/08/13 21:27:45]

nit: +1 indent

[Pat Meenan, 2015/08/14 13:52:47]

Done.

       in_flight_delayable_count_--;
-    if (old_classification == LAYOUT_BLOCKING_REQUEST)
+    }
+    if (RequestAttributesAreSet(old_attributes, kAttributeLayoutBlocking))
       total_layout_blocking_count_--;
 
-    if (classification == IN_FLIGHT_DELAYABLE_REQUEST)
+    if (RequestAttributesAreSet(attributes,
+                               kAttributeInFlight | kAttributeDelayable)) {

[mmenke, 2015/08/13 21:27:45]

nit:  +1 indent

[Pat Meenan, 2015/08/14 13:52:47]

Done.

       in_flight_delayable_count_++;
-    if (classification == LAYOUT_BLOCKING_REQUEST)
+    }
+    if (RequestAttributesAreSet(attributes, kAttributeLayoutBlocking))
       total_layout_blocking_count_++;
 
-    request->set_classification(classification);
-    DCHECK_EQ(
-        CountRequestsWithClassification(IN_FLIGHT_DELAYABLE_REQUEST, false),
+    request->set_attributes(attributes);
+    DCHECK_EQ(CountRequestsWithAttributes(
+        kAttributeInFlight | kAttributeDelayable, false),
         in_flight_delayable_count_);
-    DCHECK_EQ(CountRequestsWithClassification(LAYOUT_BLOCKING_REQUEST, true),
+    DCHECK_EQ(CountRequestsWithAttributes(kAttributeLayoutBlocking, true),
               total_layout_blocking_count_);
   }
 
-  RequestClassification ClassifyRequest(ScheduledResourceRequest* request) {
-    // If a request is already marked as layout-blocking make sure to keep the
-    // classification across redirects unless the priority was lowered.
-    if (request->classification() == LAYOUT_BLOCKING_REQUEST &&
-        request->url_request()->priority() > net::LOW) {
-      return LAYOUT_BLOCKING_REQUEST;
-    }
-
-    if (!has_body_ && request->url_request()->priority() > net::LOW)
-      return LAYOUT_BLOCKING_REQUEST;
-
-    if (request->url_request()->priority() < net::LOW) {
+  RequestAttributes DetermineRequestAttributes(
+      ScheduledResourceRequest* request) {
+    RequestAttributes attributes = kAttributeNone;
+
+    if (ContainsKey(in_flight_requests_, request))
+      attributes = attributes | kAttributeInFlight;

[mmenke, 2015/08/13 21:27:45]

Any reason not to use |= on all of these?

[Pat Meenan, 2015/08/14 13:52:47]

I wasn't sure if it would look too much like != and thought it would be easier
to read when skimming but since it's stand-alone it looks readable enough with
|=.

Done.

+
+    if (RequestAttributesAreSet(request->attributes(),
+                                kAttributeLayoutBlocking)) {
+      // If a request is already marked as layout-blocking make sure to keep the
+      // attribute across redirects.
+      attributes = attributes | kAttributeLayoutBlocking;
+    } else if (!has_html_body_ &&
+               request->url_request()->priority() >
+               scheduler_->non_delayable_threshold()) {
+      // Requests that are above the non_delayable threshold before the HTML
+      // body has been parsed are inferred to be layout-blocking.
+      attributes = attributes | kAttributeLayoutBlocking;
+    } else if (request->url_request()->priority() <
+               scheduler_->non_delayable_threshold()) {
+      // Resources below the non_delayable_threshold that are being requested
+      // from a server that does not support native prioritization are
+      // considered delayable.
       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.SupportsRequestPriority(host_port_pair) &&
-          ContainsKey(in_flight_requests_, request)) {
-        return IN_FLIGHT_DELAYABLE_REQUEST;
-      }
+      if (!http_server_properties.SupportsRequestPriority(host_port_pair))
+        attributes = attributes | kAttributeDelayable;
     }
-    return NORMAL_REQUEST;
+
+    return attributes;
   }
 
   bool ShouldKeepSearching(
@@ -768,12 +800,13 @@ class ResourceScheduler::Client {
       return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
     }
 
-    // High-priority and layout-blocking requests.
-    if (url_request.priority() >= net::LOW) {
+    // non-delayable requests.

[mmenke, 2015/08/13 21:27:45]

Nit:  "Non-delayable requests." (capitalize)

[Pat Meenan, 2015/08/14 13:52:47]

Done.

+    if (!RequestAttributesAreSet(request->attributes(), kAttributeDelayable)) {
       return START_REQUEST;
     }

[mmenke, 2015/08/13 21:27:45]

nit:  Remove braces

[Pat Meenan, 2015/08/14 13:52:47]

I had them to be consistent with the rest of the function which had braces for
all of the checks but to be consistent with the whole file and rest of code I
also cleaned up the other instances in the function.

Done.

 
-    if (in_flight_delayable_count_ >= kMaxNumDelayableRequestsPerClient) {
+    if (in_flight_delayable_count_ >=
+        scheduler_->max_num_delayable_requests()) {
       return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
     }
 
@@ -783,15 +816,40 @@ class ResourceScheduler::Client {
       return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
     }
 
-    bool have_immediate_requests_in_flight =
-        in_flight_requests_.size() > in_flight_delayable_count_;
-    if (have_immediate_requests_in_flight &&
-        (!has_body_ || total_layout_blocking_count_ != 0) &&
-        // Do not allow a low priority request through in parallel if
-        // we are in a limit field trial.
-        (scheduler_->limit_outstanding_requests() ||
-         in_flight_delayable_count_ != 0)) {
-      return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+    // The in-flight requests consist of layout-blocking requests,
+    // normal requests and delayable requests.  Everything except for
+    // delayable requests is handled above here so this is deciding what to
+    // do with a delayable request while we are in the layout-blocking phase
+    // of loading.
+    if (!has_html_body_ || total_layout_blocking_count_ != 0) {
+      size_t non_delayable_requests_in_flight_count =
+          in_flight_requests_.size() - in_flight_delayable_count_;
+      if (scheduler_->enable_in_flight_non_delayable_threshold()) {
+        if (non_delayable_requests_in_flight_count >
+            scheduler_->in_flight_non_delayable_threshold()) {
+          // Too many higher priority in-flight requests to allow lower priority
+          // requests through.
+          return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+        }
+        if (in_flight_requests_.size() > 0 &&
+            (scheduler_->limit_outstanding_requests() ||
+             in_flight_delayable_count_ >=
+             scheduler_->max_num_delayable_while_layout_blocking())) {
+          // Block the request if at least one request is in flight and the
+          // number of in-flight delayable requests has hit the configured
+          // limit.
+          return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+        }
+      } else if (non_delayable_requests_in_flight_count > 0 &&
+          (scheduler_->limit_outstanding_requests() ||
+           in_flight_delayable_count_ >=
+           scheduler_->max_num_delayable_while_layout_blocking())) {
+        // If there are no high-priority requests in flight the floodgates open.
+        // If there are high-priority requests in-flight then limit the number
+        // of lower-priority requests (or zero if a limit field trial is
+        // active).
+        return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+      }
     }
 
     return START_REQUEST;
@@ -838,7 +896,9 @@ class ResourceScheduler::Client {
   bool is_visible_;
   bool is_loaded_;
   bool is_paused_;
-  bool has_body_;
+  // Tracks if the main HTML parser has reached the body which marks the end of
+  // layout-blocking resources.
+  bool has_html_body_;
   bool using_spdy_proxy_;
   RequestQueue pending_requests_;
   RequestSet in_flight_requests_;
@@ -860,6 +920,13 @@ ResourceScheduler::ResourceScheduler()
       coalesced_clients_(0),
       limit_outstanding_requests_(false),
       outstanding_request_limit_(0),
+      non_delayable_threshold_(
+          kDefaultLayoutBlockingPriorityThreshold),
+      enable_in_flight_non_delayable_threshold_(false),
+      in_flight_non_delayable_threshold_(0),
+      max_num_delayable_while_layout_blocking_(
+          kDefaultMaxNumDelayableWhileLayoutBlocking),
+      max_num_delayable_requests_(kDefaultMaxNumDelayableRequestsPerClient),
       coalescing_timer_(new base::Timer(true /* retain_user_task */,
                                         true /* is_repeating */)) {
   std::string throttling_trial_group =
@@ -884,6 +951,42 @@ ResourceScheduler::ResourceScheduler()
     limit_outstanding_requests_ = true;
     outstanding_request_limit_ = outstanding_limit;
   }
+
+  // Set up the ResourceScheduling field trial options.
+  // The field trial parameters are also encoded into the group name since
+  // the variations component is not available from here and plumbing the
+  // options through the code is overkill for a short experiment.
+  //
+  // The group name encoding looks like this:
+  //  <descriptiveName>_ABCDE_E2_F_G
+  //    A - fetchDeferLateScripts (1 for true, 0 for false)
+  //    B - fetchIncreaseFontPriority (1 for true, 0 for false)
+  //    C - fetchIncreaseAsyncScriptPriority (1 for true, 0 for false)
+  //    D - fetchIncreasePriorities (1 for true, 0 for false)
+  //    E - fetchEnableLayoutBlockingThreshold (1 for true, 0 for false)
+  //    E2 - fetchLayoutBlockingThreshold (Numeric)
+  //    F - fetchMaxNumDelayableWhileLayoutBlocking (Numeric)
+  //    G - fetchMaxNumDelayableRequests (Numeric)
+  std::string resource_priorities_trial_group =
+      base::FieldTrialList::FindFullName(kResourcePrioritiesFieldTrial);
+  std::vector<std::string> resource_priorities_split_group(
+      base::SplitString(resource_priorities_trial_group, "_",
+                        base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL));
+  if (resource_priorities_split_group.size() == 5 &&
+      resource_priorities_split_group[1].length() == 5) {
+    // fetchIncreasePriorities
+    if (resource_priorities_split_group[1].at(3) == '1')
+      non_delayable_threshold_ = net::MEDIUM;
+    enable_in_flight_non_delayable_threshold_ =
+        resource_priorities_split_group[1].at(4) == '1';
+    size_t numeric_value = 0;
+    if (base::StringToSizeT(resource_priorities_split_group[2], &numeric_value))
+      in_flight_non_delayable_threshold_ = numeric_value;
+    if (base::StringToSizeT(resource_priorities_split_group[3], &numeric_value))
+      max_num_delayable_while_layout_blocking_ = numeric_value;
+    if (base::StringToSizeT(resource_priorities_split_group[4], &numeric_value))
+      max_num_delayable_requests_ = numeric_value;
+  }
 }
 
 ResourceScheduler::~ResourceScheduler() {