Fix Resource Priorities and Scheduling (Chrome Side)

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/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #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"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
 #include "net/base/request_priority.h"
 #include "net/http/http_server_properties.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 
 namespace content {
 
 namespace {
 
 // Field trial constants
 const char kThrottleCoalesceFieldTrial[] = "RequestThrottlingAndCoalescing";
 const char kThrottleCoalesceFieldTrialThrottle[] = "Throttle";
 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
 // Else:
 //   ResourceScheduler.base_name.histogram_name.histogram_suffix
 void PostHistogram(const char* base_name,
                    const char* histogram_name,
                    const char* histogram_suffix,
                    base::TimeDelta time) {
   std::string histogram =
@@ skipping 19 matching lines @@
   else if (num_clients <= 30)
     return "Max30Clients";
   return "Over30Clients";
 }
 
 }  // namespace
 
 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()
     : priority(net::DEFAULT_PRIORITY),
       intra_priority(0) {
   }
 
   RequestPriorityParams(net::RequestPriority priority, int intra_priority)
     : priority(priority),
       intra_priority(intra_priority) {
@@ skipping 217 matching lines @@
       : is_audible_(is_audible),
         is_visible_(is_visible),
         is_loaded_(false),
         is_paused_(false),
         has_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_(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
     // correct count of relevant types of clients.
     is_visible_ = false;
     is_audible_ = false;
     is_paused_ = true;
     UpdateThrottleState();
   }
 
@@ skipping 290 matching lines @@
         CountRequestsWithClassification(IN_FLIGHT_DELAYABLE_REQUEST, false),
         in_flight_delayable_count_);
     DCHECK_EQ(CountRequestsWithClassification(LAYOUT_BLOCKING_REQUEST, 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) {
+        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;
   }
 
   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 =
@@ skipping 108 matching lines @@
     }
 
     if (throttle_state_ == THROTTLED &&
         in_flight_requests_.size() >= kMaxNumThrottledRequestsPerClient) {
       // There may still be request-priority-capable requests that should be
       // issued.
       return DO_NOT_START_REQUEST_AND_KEEP_SEARCHING;
     }
 
     // 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;
     }
 
     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() > 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;
   }
 
   void LoadAnyStartablePendingRequests() {
     // We iterate through all the pending requests, starting with the highest
     // priority one. For each entry, one of three things can happen:
     // 1) We start the request, remove it from the list, and keep checking.
     // 2) We do NOT start the request, but ShouldStartRequest() signals us that
@@ skipping 26 matching lines @@
         DCHECK(query_result == DO_NOT_START_REQUEST_AND_STOP_SEARCHING);
         break;
       }
     }
   }
 
   bool is_audible_;
   bool is_visible_;
   bool is_loaded_;
   bool is_paused_;
   bool has_body_;

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

Please add a description here, and maybe rename it to something more intuitive
(HEAD responses, for example, have no bodies, but that's not what this is
referring to)

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

Done.

   bool using_spdy_proxy_;
   RequestQueue pending_requests_;
   RequestSet in_flight_requests_;
   base::TimeTicks load_started_time_;
   // The last time the client switched state between active and background.
   base::TimeTicks last_active_switch_time_;
   ResourceScheduler* scheduler_;
   // The number of delayable in-flight requests.
   size_t in_flight_delayable_count_;
   // 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()
     : should_coalesce_(false),
       should_throttle_(false),
       active_clients_loading_(0),
       coalesced_clients_(0),
       limit_outstanding_requests_(false),
       outstanding_request_limit_(0),
       coalescing_timer_(new base::Timer(true /* retain_user_task */,
@@ skipping 338 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