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/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"
 #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"
 #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"
 #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 {
 
@@ skipping 266 matching lines @@
   DCHECK(!ContainsKey(pointers_, request));
   request->set_fifo_ordering(MakeFifoOrderingId());
   pointers_[request] = queue_.insert(request);
 }
 
 // Each client represents a tab.
 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),
         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),
+        fetch_mode_(fetch_mode) {}
 
   ~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 289 matching lines @@
     DCHECK_EQ(
         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) {
-      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::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() < 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 (!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;
   }
 
   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) {
@@ skipping 109 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) {
-      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) {
       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 (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;
   }
 
   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 39 matching lines @@
   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_;
+  int fetch_mode_;
 };
 
 ResourceScheduler::ResourceScheduler()
     : should_coalesce_(false),
       should_throttle_(false),
       active_clients_loading_(0),
       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 =
       base::FieldTrialList::FindFullName(kThrottleCoalesceFieldTrial);
   if (throttling_trial_group == kThrottleCoalesceFieldTrialThrottle) {
     should_throttle_ = true;
   } else if (throttling_trial_group == kThrottleCoalesceFieldTrialCoalesce) {
     should_coalesce_ = true;
     should_throttle_ = true;
   }
 
   std::string outstanding_limit_trial_group =
       base::FieldTrialList::FindFullName(kRequestLimitFieldTrial);
   std::vector<std::string> split_group(
       base::SplitString(outstanding_limit_trial_group, "=",
                         base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL));
   int outstanding_limit = 0;
   if (split_group.size() == 2 &&
       split_group[0] == kRequestLimitFieldTrialGroupPrefix &&
       base::StringToInt(split_group[1], &outstanding_limit) &&
       outstanding_limit > 0) {
     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() {
   DCHECK(unowned_requests_.empty());
   DCHECK(client_map_.empty());
 }
 
 void ResourceScheduler::SetThrottleOptionsForTesting(bool should_throttle,
                                                      bool should_coalesce) {
   should_coalesce_ = should_coalesce;
@@ skipping 53 matching lines @@
 }
 
 void ResourceScheduler::OnClientCreated(int child_id,
                                         int route_id,
                                         bool is_visible,
                                         bool is_audible) {
   DCHECK(CalledOnValidThread());
   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();
 }
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(ContainsKey(client_map_, client_id));
   ClientMap::iterator it = client_map_.find(client_id);
@@ skipping 209 matching lines @@
     NOTREACHED();
     return;
   }
   RequestPriorityParams new_priority_params(new_priority,
       new_intra_priority_value);
   RequestPriorityParams old_priority_params =
       request->get_request_priority_params();
 
   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.
     request->url_request()->SetPriority(new_priority_params.priority);
     request->set_request_priority_params(new_priority_params);
     return;
   }
 
   if (old_priority_params == new_priority_params)
     return;
 
   Client *client = client_it->second;
   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