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..d962e46dbadf82d127a925f3e63f9b2624875217 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,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 +76,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()
@@ -308,13 +313,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
@@ -350,10 +356,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 +476,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();
   }
 
@@ -623,16 +629,19 @@ class ResourceScheduler::Client {
 
   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) {
+    // classification across redirects.
+    if (request->classification() == LAYOUT_BLOCKING_REQUEST) {
       return LAYOUT_BLOCKING_REQUEST;
     }

[mmenke, 2015/08/12 17:30:08]

nit:  Remove braces, to be more consistent with the rest of this file

[Pat Meenan, 2015/08/13 12:11:03]

Done.

 
-    if (!has_body_ && request->url_request()->priority() > net::LOW)
+    if (!has_html_body_ &&
+        request->url_request()->priority() >
+        scheduler_->layout_blocking_priority_threshold()) {
       return LAYOUT_BLOCKING_REQUEST;
+    }
 
-    if (request->url_request()->priority() < net::LOW) {
+    if (request->url_request()->priority() <
+        scheduler_->layout_blocking_priority_threshold()) {
       net::HostPortPair host_port_pair =
           net::HostPortPair::FromURL(request->url_request()->url());
       net::HttpServerProperties& http_server_properties =
@@ -769,11 +778,13 @@ class ResourceScheduler::Client {
     }
 
     // High-priority and layout-blocking requests.
-    if (url_request.priority() >= net::LOW) {
+    if (url_request.priority() >=
+        scheduler_->layout_blocking_priority_threshold()) {
       return START_REQUEST;
     }
 
-    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 +794,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_;
+      if (scheduler_->enable_layout_blocking_threshold()) {
+        if (non_delayable_requests_in_flight_count >
+            scheduler_->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 &&

[mmenke, 2015/08/12 17:30:08]

I think we can remove this line - if in_flight_requests_.size() is 0, then
total_layout_blocking_count_ will also be 0, so we won't reach this point.  Or
should we reconsider the conditions of the first if?  It seems kinda weird that
if we have a layout blocking request, high priority requests count against low
priority requests, but if we don't, they don't.

[mmenke, 2015/08/12 17:33:59]

Oh, right...the no body yet case.  Think I forgot about that before.

[Pat Meenan, 2015/08/13 12:11:03]

Not sure if it helps any but I beefed up the comment for this chunk of logic to
hopefully be clearer.  It might also help if I factor out the check for the
layout-blocking loading pahse into a separate function to make it clearer and
bury the actual logic (though this is the only place it has to make that check).

+            (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 +870,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 +894,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 +925,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() {