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..556e6d4835207f125390682046c042be524ecd93 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -2,9 +2,8 @@
 // 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"
@@ -37,6 +36,8 @@ const char kThrottleCoalesceFieldTrialCoalesce[] = "Coalesce";
 const char kRequestLimitFieldTrial[] = "OutstandingRequestLimiting";
 const char kRequestLimitFieldTrialGroupPrefix[] = "Limit";
 
+const char kResourcePrioritiesFieldTrial[] = "ResourcePriorities";
+
 // Post ResourceScheduler histograms of the following forms:
 // If |histogram_suffix| is NULL or the empty string:
 //   ResourceScheduler.base_name.histogram_name
@@ -74,9 +75,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()
@@ -303,18 +307,32 @@ class ResourceScheduler::Client {
  public:
   explicit Client(ResourceScheduler* scheduler,
                   bool is_visible,
-                  bool is_audible)
+                  bool is_audible,
+                  net::RequestPriority layout_blocking_priority_threshold,
+                  bool enable_layout_blocking_threshold,
+                  size_t in_flight_layout_blocking_threshold,
+                  size_t max_num_delayable_while_layout_blocking,
+                  size_t max_num_delayable_requests)
       : is_audible_(is_audible),
         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),
+        max_num_delayable_requests_(max_num_delayable_requests),
+        max_num_delayable_while_layout_blocking_(
+            max_num_delayable_while_layout_blocking),
+        enable_layout_blocking_threshold_(enable_layout_blocking_threshold),
+        in_flight_layout_blocking_threshold_(
+            in_flight_layout_blocking_threshold),
+        layout_blocking_priority_threshold_(
+            layout_blocking_priority_threshold) {
+  }
 
   ~Client() {
     // Update to default state and pause to ensure the scheduler has a
@@ -350,10 +368,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();
@@ -470,12 +488,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();
   }
 
@@ -625,22 +643,21 @@ class ResourceScheduler::Client {
     // 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) {
+        request->url_request()->priority() >
+        layout_blocking_priority_threshold_) {
       return LAYOUT_BLOCKING_REQUEST;
     }
 
-    if (!has_body_ && request->url_request()->priority() > net::LOW)
+    if (!has_html_body_ &&
+        request->url_request()->priority() >
+        layout_blocking_priority_threshold_) {
       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.SupportsRequestPriority(host_port_pair) &&
-          ContainsKey(in_flight_requests_, request)) {
-        return IN_FLIGHT_DELAYABLE_REQUEST;
-      }
+    if (request->url_request()->priority() <
+        layout_blocking_priority_threshold_ &&
+        ContainsKey(in_flight_requests_, request)) {
+      return IN_FLIGHT_DELAYABLE_REQUEST;
     }
     return NORMAL_REQUEST;
   }
@@ -769,11 +786,11 @@ class ResourceScheduler::Client {
     }
 
     // High-priority and layout-blocking requests.
-    if (url_request.priority() >= net::LOW) {
+    if (url_request.priority() >= layout_blocking_priority_threshold_) {
       return START_REQUEST;
     }
 
-    if (in_flight_delayable_count_ >= kMaxNumDelayableRequestsPerClient) {
+    if (in_flight_delayable_count_ >= max_num_delayable_requests_) {
       return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
     }
 
@@ -783,15 +800,36 @@ 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;
+    // Layout-blocking phase of resource 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_;

[Bryan McQuade, 2015/08/11 13:25:07]

on first glance this is potentially scary since, if in_flight_delayable_count_
is ever greater than in_flight_requests_.size(), the size_t wraps which could
lead to unexpected behavior. i get the sense that it'd be a bug if this ever
happened, but just to be more explicit about it, can we add
CHECK(in_flight_requests_.size() >= in_flight_delayable_count_); right before
this as a sanity check that this should never happen?

[mmenke, 2015/08/11 13:35:34]

The only place we modify the classification of requests
(SetRequestClassification) already has some sanity DCHECKs.  I'm fine with
adding more, if we want better DCHECKs.  I don't think we want checks for sanity
here - dchecks should be where we actually modify in_flight_delayable_count_
(And possibly in_flight_requests_.size()) make a lot more sense as sanity
checks, since they give more useful information.

If we just want a DCHECK for documentation purposes here (To say "this is true,
we've thought of this), I am actually fine with that, just don't think it makes
sense as a sanity check.

Should not use CHECKs, unless there's some particular concern here, as they
bloat up binary size.

+      if (enable_layout_blocking_threshold_) {
+        if (non_delayable_requests_in_flight_count >
+            in_flight_layout_blocking_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 &&
+            // Allow the request if nothing is in flight or if the limit of
+            // concurrent lower priority requests has not been hit.
+            (scheduler_->limit_outstanding_requests() ||
+             in_flight_delayable_count_ >=
+             max_num_delayable_while_layout_blocking_)) {
+          return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+        }
+      } else if (non_delayable_requests_in_flight_count > 0 &&
+          // 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).
+          (scheduler_->limit_outstanding_requests() ||
+           in_flight_delayable_count_ >=
+           max_num_delayable_while_layout_blocking_)) {
+        return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+      }
     }
 
     return START_REQUEST;
@@ -838,7 +876,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_;
@@ -851,6 +891,20 @@ class ResourceScheduler::Client {
   // The number of layout-blocking in-flight requests.
   size_t total_layout_blocking_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
+  // Maximum number of delayable requests to allow at a time, regardless of
+  // the number of servers (limit to X concurrent requests across all hosts).
+  size_t max_num_delayable_requests_;
+  // Maximum number of delayable requests to allow while in the layout-blocking
+  // phase of resource loading.
+  size_t max_num_delayable_while_layout_blocking_;
+  // Determines if in_flight_layout_blocking_threshold_ should be used.
+  bool enable_layout_blocking_threshold_;
+  // Number number of non-delayable requests above which no delayable requests
+  // should be issued while in the layout-blocking phase of resource loading.
+  size_t in_flight_layout_blocking_threshold_;
+  // Priority level above which resources are considered layout-blocking and
+  // below which they are considered delayable.
+  net::RequestPriority layout_blocking_priority_threshold_;
 };
 
 ResourceScheduler::ResourceScheduler()
@@ -860,6 +914,13 @@ ResourceScheduler::ResourceScheduler()
       coalesced_clients_(0),
       limit_outstanding_requests_(false),
       outstanding_request_limit_(0),
+      layout_blocking_priority_threshold_(
+          kDefaultLayoutBlockingPriorityThreshold),
+      enable_layout_blocking_threshold_(false),
+      in_flight_layout_blocking_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 +945,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')
+      layout_blocking_priority_threshold_ = net::MEDIUM;
+    enable_layout_blocking_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_layout_blocking_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() {
@@ -957,7 +1054,10 @@ void ResourceScheduler::OnClientCreated(int child_id,
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!ContainsKey(client_map_, client_id));
 
-  Client* client = new Client(this, is_visible, is_audible);
+  Client* client = new Client(this, is_visible, is_audible,
+      layout_blocking_priority_threshold_, enable_layout_blocking_threshold_,
+      in_flight_layout_blocking_threshold_,
+      max_num_delayable_while_layout_blocking_, max_num_delayable_requests_);
   client_map_[client_id] = client;
 
   client->UpdateThrottleState();