[ResourceScheduler] Yield after starting several requests

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 6d68cf1c438dd7b22621500e2dcc70af6fa917b0..ac5bebf605ff8c51a8d0c89fa721c1059b7df5f6 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -13,6 +13,7 @@
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/supports_user_data.h"
@@ -39,6 +40,16 @@ namespace {
 const base::Feature kPrioritySupportedRequestsDelayable{
     "PrioritySupportedRequestsDelayable", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// In the event that many resource requests are started quickly, this feature
+// will periodically yield (e.g., delaying starting of requests) by posting a
+// task and waiting for the task to run to resume. This allows other
+// operations that rely on the IO thread (e.g., already running network
+// requests) to make progress.
+const base::Feature kNetworkSchedulerYielding{
+    "NetworkSchedulerYielding", base::FEATURE_DISABLED_BY_DEFAULT};
+const char kMaxRequestsBeforeYieldingParam[] = "MaxRequestsBeforeYieldingParam";
+const int kMaxRequestsBeforeYieldingDefault = 5;
+
 enum StartMode {
   START_SYNC,
   START_ASYNC
@@ -61,6 +72,7 @@ enum class RequestStartTrigger {
   CLIENT_KILL,
   SPDY_PROXY_DETECTED,
   REQUEST_REPRIORITIZED,
+  START_YIELDED,

[Charlie Harrison, 2017/02/09 19:08:04]

nit: I think START_WAS_YIELDED is a bit better.

[jkarlin, 2017/02/09 19:48:12]

Done.

 };
 
 const char* RequestStartTriggerString(RequestStartTrigger trigger) {
@@ -79,6 +91,8 @@ const char* RequestStartTriggerString(RequestStartTrigger trigger) {
       return "SPDY_PROXY_DETECTED";
     case RequestStartTrigger::REQUEST_REPRIORITIZED:
       return "REQUEST_REPRIORITIZED";
+    case RequestStartTrigger::START_YIELDED:
+      return "START_YIELDED";
   }
   NOTREACHED();
   return "Unknown";
@@ -348,7 +362,9 @@ void ResourceScheduler::RequestQueue::Insert(
 // Each client represents a tab.
 class ResourceScheduler::Client {
  public:
-  explicit Client(bool priority_requests_delayable)
+  Client(bool priority_requests_delayable,
+         bool yielding_scheduler,
+         int max_requests_before_yielding)
       : is_loaded_(false),
         has_html_body_(false),
         using_spdy_proxy_(false),
@@ -356,6 +372,9 @@ class ResourceScheduler::Client {
         total_layout_blocking_count_(0),
         priority_requests_delayable_(priority_requests_delayable),
         has_pending_start_task_(false),
+        requests_since_yielding_(0),
+        yielding_scheduler_(yielding_scheduler),
+        max_requests_before_yielding_(max_requests_before_yielding),
         weak_ptr_factory_(this) {}
 
   ~Client() {}
@@ -363,7 +382,8 @@ class ResourceScheduler::Client {
   void ScheduleRequest(net::URLRequest* url_request,
                        ScheduledResourceRequest* request) {
     SetRequestAttributes(request, DetermineRequestAttributes(request));
-    if (ShouldStartRequest(request) == START_REQUEST) {
+    if (ShouldStartRequest(request) == START_REQUEST &&

[Charlie Harrison, 2017/02/09 19:08:04]

Eek, I am a little worried about sync XHRs here. Maybe not a big deal in
practice?

[jkarlin, 2017/02/09 19:48:13]

Nice catch.  I've changed it to not yield for sync requests and added a test.

+        !ShouldYieldScheduler()) {
       // New requests can be started synchronously without issue.
       StartRequest(request, START_SYNC, RequestStartTrigger::NONE);
     } else {
@@ -738,6 +758,28 @@ class ResourceScheduler::Client {
                               weak_ptr_factory_.GetWeakPtr(), trigger));
   }
 
+  void ResumeAfterYielding() {
+    requests_since_yielding_ = 0;

[Charlie Harrison, 2017/02/09 19:08:04]

// A single request can trigger this method, so don't continue searching 
// unless we really yielded.
bool should_search_for_requests = requests_since_yielding_ >
max_requests_before_yielding_;
requests_since_yielding_ = 0;
if (should_search_for_requests)
  LoadAnyStartable...

[jkarlin, 2017/02/09 19:48:13]

Thanks, I forgot to add that back when I switched from starting task when first
yielding to first request.

+    LoadAnyStartablePendingRequests(RequestStartTrigger::START_YIELDED);
+  }
+
+  // Returns true if the scheduler should not start the next request and
+  // instead return so that other tasks can run on the IO thread (e.g.,
+  // existing network requests). If it returns true, ShouldYieldScheduler will
+  // have already scheduled a task to resume after yielding.
+  bool ShouldYieldScheduler() {
+    if (!yielding_scheduler_)
+      return false;
+
+    if (requests_since_yielding_ == 0) {

[Charlie Harrison, 2017/02/09 19:08:04]

The fact that we post a task immediately deserves some explanation.

[jkarlin, 2017/02/09 19:48:12]

Done.

+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&Client::ResumeAfterYielding,
+                                weak_ptr_factory_.GetWeakPtr()));
+    }
+
+    return requests_since_yielding_++ >= max_requests_before_yielding_;

[Charlie Harrison, 2017/02/09 19:08:04]

optional: Can you put the increment on a separate line? This adds cognitive
burden for me.

[jkarlin, 2017/02/09 19:48:12]

Done.

+  }
+
   void LoadAnyStartablePendingRequests(RequestStartTrigger trigger) {
     // We iterate through all the pending requests, starting with the highest
     // priority one. For each entry, one of three things can happen:
@@ -756,6 +798,9 @@ class ResourceScheduler::Client {
       ShouldStartReqResult query_result = ShouldStartRequest(request);
 
       if (query_result == START_REQUEST) {
+        if (ShouldYieldScheduler())
+          break;
+
         pending_requests_.Erase(request);
         StartRequest(request, START_ASYNC, trigger);
 
@@ -794,12 +839,28 @@ class ResourceScheduler::Client {
 
   bool has_pending_start_task_;
 
+  // The number of requests that have been started since the last
+  // ResumeAfterYielding task was posted.
+  int requests_since_yielding_;
+
+  // Whether or not to periodically yield when starting lots of requests.
+  bool yielding_scheduler_;
+
+  // The number of requests that can start before yielding.
+  int max_requests_before_yielding_;
+
   base::WeakPtrFactory<ResourceScheduler::Client> weak_ptr_factory_;
 };
 
 ResourceScheduler::ResourceScheduler()
     : priority_requests_delayable_(
-          base::FeatureList::IsEnabled(kPrioritySupportedRequestsDelayable)) {}
+          base::FeatureList::IsEnabled(kPrioritySupportedRequestsDelayable)),
+      yielding_scheduler_(
+          base::FeatureList::IsEnabled(kNetworkSchedulerYielding)),
+      max_requests_before_yielding_(base::GetFieldTrialParamByFeatureAsInt(
+          kNetworkSchedulerYielding,
+          kMaxRequestsBeforeYieldingParam,
+          kMaxRequestsBeforeYieldingDefault)) {}
 
 ResourceScheduler::~ResourceScheduler() {
   DCHECK(unowned_requests_.empty());
@@ -856,7 +917,8 @@ void ResourceScheduler::OnClientCreated(int child_id,
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!base::ContainsKey(client_map_, client_id));
 
-  Client* client = new Client(priority_requests_delayable_);
+  Client* client = new Client(priority_requests_delayable_, yielding_scheduler_,
+                              max_requests_before_yielding_);
   client_map_[client_id] = client;
 }