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..82edd6f62a67fd0e80a6ac7792e8031be7389eb5 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -4,8 +4,9 @@
 
 #include <set>
 
-#include "content/browser/loader/resource_scheduler.h"
-
+#include "base/base_switches.h"
+#include "base/basictypes.h"
+#include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
@@ -14,9 +15,11 @@
 #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"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.h"
+#include "content/public/common/content_switches.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -303,7 +306,8 @@ class ResourceScheduler::Client {
  public:
   explicit Client(ResourceScheduler* scheduler,
                   bool is_visible,
-                  bool is_audible)
+                  bool is_audible,
+                  int fetch_mode)
       : is_audible_(is_audible),
         is_visible_(is_visible),
         is_loaded_(false),
@@ -314,7 +318,8 @@ class ResourceScheduler::Client {
         scheduler_(scheduler),
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0),
-        throttle_state_(ResourceScheduler::THROTTLED) {}
+        throttle_state_(ResourceScheduler::THROTTLED),
+        fetch_mode_(fetch_mode) {}
 
   ~Client() {
     // Update to default state and pause to ensure the scheduler has a
@@ -624,22 +629,43 @@ 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) {
-      return LAYOUT_BLOCKING_REQUEST;
-    }
+    if (fetch_mode_ == 2) {
+      if (request->classification() == LAYOUT_BLOCKING_REQUEST &&
+          request->url_request()->priority() > net::MEDIUM) {
+        return LAYOUT_BLOCKING_REQUEST;
+      }
 
-    if (!has_body_ && request->url_request()->priority() > net::LOW)
-      return LAYOUT_BLOCKING_REQUEST;
+      if (!has_body_ && request->url_request()->priority() > net::MEDIUM)
+        return LAYOUT_BLOCKING_REQUEST;
+
+      if (request->url_request()->priority() < net::MEDIUM) {
+        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;
+        }
+      }
+    } else {
+      if (request->classification() == LAYOUT_BLOCKING_REQUEST &&
+          request->url_request()->priority() > net::LOW) {
+        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 (!has_body_ && request->url_request()->priority() > net::LOW)
+        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;
+        }
       }
     }
     return NORMAL_REQUEST;
@@ -769,8 +795,14 @@ class ResourceScheduler::Client {
     }
 
     // High-priority and layout-blocking requests.
-    if (url_request.priority() >= net::LOW) {
-      return START_REQUEST;
+    if (fetch_mode_ == 2) {
+      if (url_request.priority() >= net::MEDIUM) {
+        return START_REQUEST;
+      }
+    } else {
+      if (url_request.priority() >= net::LOW) {
+        return START_REQUEST;
+      }
     }
 
     if (in_flight_delayable_count_ >= kMaxNumDelayableRequestsPerClient) {
@@ -783,15 +815,49 @@ 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 (fetch_mode_ == 2) {
+      // If we are in render-blocking more make sure at least 2 requests
+      // are in-flight. High priority requests would have alreay
+      // been allowed so this just restricts low-priority requests.
+      if ((!has_body_ || total_layout_blocking_count_ != 0) &&
+          in_flight_requests_.size() >= 2) {

[Bryan McQuade, 2015/07/29 17:53:47]

it's hard for me to say with certainty that this is the right thing for all
pages. i'd want to be able to test just this change in isolation (same for
previous comment) to really see if this is the right thing. could we separate
this change out from the rest of your prioritization change and try it out as
part of a separate patch?

[Pat Meenan, 2015/08/07 15:53:53]

This has all been ripped out and replaced with finch-controllable parameters.

+        return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+      }
+
+      // Allow 1 lower priority resource while 1 higher priority resource
+      // is pending.  If more than 1 higher priority resources are in flight
+      // hold back from issuing any more lower priority requests.
+      // This balances keeping the pipe from going idle when the last high
+      // priority request completes but also limits the cases where low priority
+      // requests will be contending with them for bandwidth.
+      size_t higher_priority = 0;

[Bryan McQuade, 2015/07/29 17:53:47]

same as above (can we test this change separately from all of the other changes
in a different patch)?

[Pat Meenan, 2015/08/07 15:53:53]

Also replaced and finch-controllable now.

+      size_t equal_priority = 0;
+      net::RequestPriority priority = url_request.priority();
+      for (RequestSet::const_iterator it = in_flight_requests_.begin();
+           it != in_flight_requests_.end(); ++it) {
+        net::RequestPriority pending_priority =
+            (*it)->get_request_priority_params().priority;
+        if (pending_priority > priority) {
+          higher_priority++;
+        } else if (pending_priority == priority) {
+          equal_priority++;
+        }
+      }
+      if (higher_priority > 1 || (higher_priority == 1 && equal_priority > 0)) {
+        return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
+      }
+
+    } else {
+      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;
+      }
     }
 
     return START_REQUEST;
@@ -851,6 +917,7 @@ class ResourceScheduler::Client {
   // The number of layout-blocking in-flight requests.
   size_t total_layout_blocking_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
+  int fetch_mode_;
 };
 
 ResourceScheduler::ResourceScheduler()
@@ -860,6 +927,7 @@ ResourceScheduler::ResourceScheduler()
       coalesced_clients_(0),
       limit_outstanding_requests_(false),
       outstanding_request_limit_(0),
+      fetch_mode_(0),
       coalescing_timer_(new base::Timer(true /* retain_user_task */,
                                         true /* is_repeating */)) {
   std::string throttling_trial_group =
@@ -884,6 +952,15 @@ ResourceScheduler::ResourceScheduler()
     limit_outstanding_requests_ = true;
     outstanding_request_limit_ = outstanding_limit;
   }
+
+  std::string fetch_mode_cmd =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kFetchMode);
+  if (!fetch_mode_cmd.empty()) {
+    int temp;
+    if (base::StringToInt(fetch_mode_cmd, &temp))
+      fetch_mode_ = temp;
+  }
 }
 
 ResourceScheduler::~ResourceScheduler() {
@@ -957,7 +1034,7 @@ 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, fetch_mode_);
   client_map_[client_id] = client;
 
   client->UpdateThrottleState();
@@ -1187,6 +1264,10 @@ void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
 
   DCHECK(old_priority_params != new_priority_params);
 
+  // only allow priority promotions
+  if (fetch_mode_ == 2 && old_priority_params.GreaterThan(new_priority_params))

[Bryan McQuade, 2015/07/29 17:53:47]

are there cases where we demote priorities today? what are some examples and why
do you want to disable them in fetch mode 2? I think I'd be more inclined to
instead change the code that does the deprioritization to not do that instead,
rather than ignoring the request to deprioritize silently here. That would
likely be a surprise to anyone writing code and expecting a resource to get
deprioritized.

[Pat Meenan, 2015/08/07 15:53:53]

I'll debug this case separately and not make it part of this test.  I saw it
happen a few times in testing but there are supposed to be upstream protections
to prevent it - like you said, better to go fix those.  The case where it is
supposed to be guarded against is when the same image is used in both visible
and hidden parts of the page - to prevent the hidden version from lowering the
priority of the visible one.  That logic belongs on the blink side though.

+    return;
+
   ClientMap::iterator client_it = client_map_.find(request->client_id());
   if (client_it == client_map_.end()) {
     // The client was likely deleted shortly before we received this IPC.