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..e538154f6aa4af671db630dc6897e99fcd608c99 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -4,8 +4,6 @@
 
 #include <set>
 
-#include "content/browser/loader/resource_scheduler.h"
-
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
@@ -14,6 +12,7 @@
 #include "base/time/time.h"
 #include "content/common/resource_messages.h"
 #include "content/browser/loader/resource_message_delegate.h"
+#include "content/browser/loader/resource_scheduler.h"

[mmenke, 2015/08/07 16:55:10]

This should actually go at the top, before the <set>

[Pat Meenan, 2015/08/07 20:48:10]

Done.

 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.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
@@ -77,6 +78,8 @@ static const size_t kCoalescedTimerPeriod = 5000;
 static const size_t kMaxNumDelayableRequestsPerClient = 10;
 static const size_t kMaxNumDelayableRequestsPerHost = 6;
 static const size_t kMaxNumThrottledRequestsPerClient = 1;

[mmenke, 2015/08/07 16:55:10]

+Default

[Pat Meenan, 2015/08/07 20:48:10]

Done.

+static const size_t kMaxNumDelayableWhileLayoutBlocking = 1;

[mmenke, 2015/08/07 16:55:11]

+Default

[Pat Meenan, 2015/08/07 20:48:09]

Done.

+static const net::RequestPriority kLayoutBlockingPriorityThreshold = net::LOW;

[mmenke, 2015/08/07 16:55:11]

+Default

[Pat Meenan, 2015/08/07 20:48:10]

Done.

 
 struct ResourceScheduler::RequestPriorityParams {
   RequestPriorityParams()
@@ -314,7 +317,46 @@ class ResourceScheduler::Client {
         scheduler_(scheduler),
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0),
-        throttle_state_(ResourceScheduler::THROTTLED) {}
+        throttle_state_(ResourceScheduler::THROTTLED),
+        max_num_delayable_requests_(kMaxNumDelayableRequestsPerClient),
+        max_num_delayable_while_layout_blocking_(
+            kMaxNumDelayableWhileLayoutBlocking),
+        enable_layout_blocking_threshold_(false),
+        in_flight_layout_blocking_threshold_(0),
+        layout_blocking_priority_threshold_(kLayoutBlockingPriorityThreshold) {
+    // 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> split_group(
+        base::SplitString(resource_priorities_trial_group, "_",
+                          base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL));
+    if (split_group.size() == 5 && split_group[1].length() == 5) {
+      // fetchIncreasePriorities
+      if (split_group[1].at(3) == '1')
+        layout_blocking_priority_threshold_ = net::MEDIUM;
+      enable_layout_blocking_threshold_ = split_group[1].at(4) == '1';
+      size_t numeric_value = 0;
+      if (base::StringToSizeT(split_group[2], &numeric_value))
+        in_flight_layout_blocking_threshold_ = numeric_value;
+      if (base::StringToSizeT(split_group[3], &numeric_value))
+        max_num_delayable_while_layout_blocking_ = numeric_value;
+      if (base::StringToSizeT(split_group[4], &numeric_value))
+        max_num_delayable_requests_ = numeric_value;
+    }

[mmenke, 2015/08/07 16:55:11]

Why are these per-client variables, and not in the scheduler itself?  Also, not
a big fan of having duplicate code to parse these.

All this stuff also needs to be tested - both the implementation of the new
rules, and the parsing of the field trial.

[Pat Meenan, 2015/08/07 20:48:10]

Moved to the scheduler itself and pass into each client at construction.  I was
trying to keep the changes as small as possible with the hopes that it will all
be ripped out once the field trial is done and a decision is made but it's not
that much more to rip out the added code.

As far as duplicate code for parsing, I could try to drop the actual code some
place that both could reach though right now it's not 100% duplicate since they
pull out different settings.

+  }
 
   ~Client() {
     // Update to default state and pause to ensure the scheduler has a
@@ -625,22 +667,20 @@ 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_body_ &&
+        request->url_request()->priority() >
+        layout_blocking_priority_threshold_)
       return LAYOUT_BLOCKING_REQUEST;

[mmenke, 2015/08/07 16:55:11]

Add braces

[mmenke, 2015/08/07 16:55:11]

Perhaps we should rename this?  Either a request blocks layout, or it doesn't. 
Having a squishier meaning here seems like it calls for a squishier term.

[Pat Meenan, 2015/08/07 20:48:09]

There are effectively 3 levels of groupings that we track.

LAYOUT_BLOCKING_REQUEST (green columns on the doc) - This is basically CSS and
is for tracking things that block layout even after we have made it to the body
though head scripts could go in here if needed and wouldn't change the behavior.

<Middle group> (yellow in the doc) - These are usually scripts and are resources
that we allow to load while we are in "layout-blocking mode".  This middle
un-labled group comes into play below and is the difference between immediate
and layout-blocking counts.

DELAYABLE_REQUEST (red in the doc) - Images, Async scripts, etc.  We limit the
loading of these while in layout-blocking mode.

I'm happy to change the names, I just don't have a good suggestion for what to
call them.  The LAYOUT_BLOCKING requests effectively keep us in layout-blocking
mode beyond the point where has_body is flipped.

[Pat Meenan, 2015/08/07 20:48:09]

Done.

 
-    if (request->url_request()->priority() < net::LOW) {
-      net::HostPortPair host_port_pair =

[Bryan McQuade, 2015/08/07 18:02:26]

why did the hostportpair logic get removed?

[Pat Meenan, 2015/08/07 20:48:09]

It was doing more harm than good.  If the resource is from a H2/SPDY server then
it will get issued long before any limit checks or counters are checked.  If
we're ONLY talking to a H2 server then all of the requests will get issued no
matter what.

If we are talking to a mix of servers, the request will still be issued to the
H2 server but wouldn't have been accounted for or tracked as an in-flight
delayable request so we'd allow an additional request on a H1 connection as
well.

Removing the check doesn't do anything in a H2 case and brings us a step closer
to sanity in a mixed connection mode and behaves more like we'd like.

-          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 +809,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 +823,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;
+    if (!has_body_ || total_layout_blocking_count_ != 0) {
+      // Layout-blocking phase of resource loading

[mmenke, 2015/08/07 16:55:10]

Add period (More consistent with other comments here, even though it's not a
sentence, and makes it clearer that it's the end of the comment)

[mmenke, 2015/08/07 16:55:11]

Suggest comment above the if (I find it easier to parse, and matches the
"High-priority and layout-blocking requests." comment above)

[Pat Meenan, 2015/08/07 20:48:09]

Done.

+      size_t immediate_requests_in_flight_count =

[mmenke, 2015/08/07 16:55:11]

non_delayable_...?

[Pat Meenan, 2015/08/07 20:48:09]

Done.

+          in_flight_requests_.size() - in_flight_delayable_count_;
+      if (enable_layout_blocking_threshold_) {
+        if (immediate_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 &&

[mmenke, 2015/08/07 16:55:11]

The other branch only does this if immediate_requests_in_flight_count > 0, while
this branch does this if there's any request in flight.  I don't suppose we
could just apply that logic here, too, which would let us remove the else and
merge the branches?  This method is really a nightmare of hack on hack, would
really like to prevent it from growing too much worse.

I'm also not sure the difference between the two is at all significant.

[Pat Meenan, 2015/08/07 20:48:09]

The difference between the two is pretty significant in that the other branch
(the existing code basically) skips the logic entirely if the only thing in
flight is delayable requests.  That ends up causing a problem when head js does
something like a document.write triggering more resource loads but where the
non-delayable count could drop to zero while still in the head.  The existing
code would open the floodgates and allow for 10 delayable requests to get issued
in that lull even though we are still in layout-blocking mode (I saw this happen
on a few sites I was testing which is what made me change it in the new code).

I REALLY want to rip the branching out and collapse the logic together but for
the field trial to be fair I need to keep the old behavior intact. The new path
looks really good in lab testing so as soon as the field trial concludes the
hope is that we can rip out all of the conditional branching around the new
settings and make it a lot cleaner.

+            // 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 (immediate_requests_in_flight_count > 0 &&

[mmenke, 2015/08/07 16:55:11]

if "total_layout_blocking_count_" is greater than 0, isn't
immediate_requests_in_flight_count *also* necessarily greater than 0?

[Pat Meenan, 2015/08/07 20:48:09]

Sadly, no.  This is where the middle group of requests come into play (scripts
basically) that are allowed to be requested while in layout-blocking mode but
aren't categorized as layout-blocking themselves and they are not subjected to
the same limits as the delayable requests.

[mmenke, 2015/08/07 20:56:02]

Oh, I missed that this block is "!has_body_ || total_layout_blocking_count_ !=
0" and not "!has_body_ && total_layout_blocking_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;
@@ -851,6 +912,11 @@ class ResourceScheduler::Client {
   // The number of layout-blocking in-flight requests.
   size_t total_layout_blocking_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
+  size_t max_num_delayable_requests_;
+  size_t max_num_delayable_while_layout_blocking_;
+  bool enable_layout_blocking_threshold_;
+  size_t in_flight_layout_blocking_threshold_;
+  net::RequestPriority layout_blocking_priority_threshold_;

[mmenke, 2015/08/07 16:55:10]

These really need docs, and could use clearer names.

"in_flight_layout_blocking_threshold_", for example, sounds like the number of
in flight requests to block layout for.

[Pat Meenan, 2015/08/07 20:48:10]

Added docs.  Let me know if threshold still doesn't make sense to you.

 };
 
 ResourceScheduler::ResourceScheduler()