Remove yielding experiment code in resource scheduler

This experiment has not been enabled for a while
(see http://shortn/_sn5eJWqtjL), and there are
no plans to revive it (already checked with relevant
experiment owners).

Removing the code since I plan to start an experiment which
has some overlaps with this existing code.

Note that resource_scheduler.cc has other experiments too
which are disabled. This CL onl cleans up a single experiment.

Change-Id: I58fd890a522e561942eb4e9ae3f7a437737242de
Reviewed-on: https://chromium-review.googlesource.com/c/1343606Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609750}

services/network/resource_scheduler.cc

@@ -53,18 +53,6 @@ const base::Feature kPrioritySupportedRequestsDelayable{
 const base::Feature kHeadPrioritySupportedRequestsDelayable{
     "HeadPriorityRequestsDelayable", 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;
-const char kYieldMsParam[] = "MaxYieldMs";
-const int kYieldMsDefault = 0;
-
 enum StartMode { START_SYNC, START_ASYNC };
 
 // Flags identifying various attributes of the request that are used
@@ -84,7 +72,6 @@ enum class RequestStartTrigger {
   CLIENT_KILL,
   SPDY_PROXY_DETECTED,
   REQUEST_REPRIORITIZED,
-  START_WAS_YIELDED,
   LONG_QUEUED_REQUESTS_TIMER_FIRED,
 };
 
@@ -104,8 +91,6 @@ const char* RequestStartTriggerString(RequestStartTrigger trigger) {
       return "SPDY_PROXY_DETECTED";
     case RequestStartTrigger::REQUEST_REPRIORITIZED:
       return "REQUEST_REPRIORITIZED";
-    case RequestStartTrigger::START_WAS_YIELDED:
-      return "START_WAS_YIELDED";
     case RequestStartTrigger::LONG_QUEUED_REQUESTS_TIMER_FIRED:
       return "LONG_QUEUED_REQUESTS_TIMER_FIRED";
   }
@@ -397,8 +382,6 @@ class ResourceScheduler::Client {
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0),
         num_skipped_scans_due_to_scheduled_start_(0),
-        started_requests_since_yielding_(0),
-        did_scheduler_yield_(false),
         network_quality_estimator_(network_quality_estimator),
         resource_scheduler_(resource_scheduler),
         tick_clock_(tick_clock),
@@ -426,8 +409,6 @@ class ResourceScheduler::Client {
       StartRequest(request, START_SYNC, RequestStartTrigger::NONE);
     } else {
       pending_requests_.Insert(request);
-      if (should_start == YIELD_SCHEDULER)
-        did_scheduler_yield_ = true;
     }
   }
 
@@ -523,8 +504,7 @@ class ResourceScheduler::Client {
   enum ShouldStartReqResult {
     DO_NOT_START_REQUEST_AND_STOP_SEARCHING,
     DO_NOT_START_REQUEST_AND_KEEP_SEARCHING,
-    START_REQUEST,
-    YIELD_SCHEDULER
+    START_REQUEST
   };
 
   // Records the metrics related to number of requests in flight.
@@ -712,21 +692,6 @@ class ResourceScheduler::Client {
   void StartRequest(ScheduledResourceRequestImpl* request,
                     StartMode start_mode,
                     RequestStartTrigger trigger) {
-    if (resource_scheduler_->yielding_scheduler_enabled()) {
-      started_requests_since_yielding_ += 1;
-      if (started_requests_since_yielding_ == 1) {
-        // This is the first started request since last yielding. Post a task to
-        // reset the counter and start any yielded tasks if necessary. We post
-        // this now instead of when we first yield so that if there is a pause
-        // between requests the counter is reset.
-        resource_scheduler_->task_runner()->PostDelayedTask(
-            FROM_HERE,
-            base::BindOnce(&Client::ResumeIfYielded,
-                           weak_ptr_factory_.GetWeakPtr()),
-            resource_scheduler_->yield_time());
-      }
-    }
-
     // Only log on requests that were blocked by the ResourceScheduler.
     if (start_mode == START_ASYNC) {
       DCHECK_NE(RequestStartTrigger::NONE, trigger);
@@ -827,7 +792,7 @@ class ResourceScheduler::Client {
       // https://crbug.com/164101. Also, theoretically we should not count a
       // request-priority capable request against the delayable requests limit.
       if (supports_priority)
-        return ShouldStartOrYieldRequest(request);
+        return START_REQUEST;
     }
 
     // Non-delayable requests.
@@ -896,34 +861,6 @@ class ResourceScheduler::Client {
     num_skipped_scans_due_to_scheduled_start_ += 1;
   }
 
-  void ResumeIfYielded() {
-    bool yielded = did_scheduler_yield_;
-    started_requests_since_yielding_ = 0;
-    did_scheduler_yield_ = false;
-
-    if (yielded)
-      LoadAnyStartablePendingRequests(RequestStartTrigger::START_WAS_YIELDED);
-  }
-
-  // For a request that is ready to start, return START_REQUEST if the
-  // scheduler doesn't need to yield, else YIELD_SCHEDULER.
-  ShouldStartReqResult ShouldStartOrYieldRequest(
-      ScheduledResourceRequestImpl* request) const {
-    DCHECK_GE(started_requests_since_yielding_, 0);
-
-    // Don't yield if:
-    // 1. The yielding scheduler isn't enabled
-    // 2. The resource is high priority
-    // 3. There haven't been enough recent requests to warrant yielding.
-    if (!resource_scheduler_->yielding_scheduler_enabled() ||
-        request->url_request()->priority() >= kDelayablePriorityThreshold ||
-        started_requests_since_yielding_ <
-            resource_scheduler_->max_requests_before_yielding()) {
-      return START_REQUEST;
-    }
-    return YIELD_SCHEDULER;
-  }
-
   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:
@@ -960,9 +897,6 @@ class ResourceScheduler::Client {
       } else if (query_result == DO_NOT_START_REQUEST_AND_KEEP_SEARCHING) {
         ++request_iter;
         continue;
-      } else if (query_result == YIELD_SCHEDULER) {
-        did_scheduler_yield_ = true;
-        break;
       } else {
         DCHECK(query_result == DO_NOT_START_REQUEST_AND_STOP_SEARCHING);
         break;
@@ -986,14 +920,6 @@ class ResourceScheduler::Client {
   // to smarter task scheduling around reprioritization.
   int num_skipped_scans_due_to_scheduled_start_;
 
-  // The number of started requests since the last ResumeIfYielded task was
-  // run.
-  int started_requests_since_yielding_;
-
-  // If the scheduler had to yield the start of a request since the last
-  // ResumeIfYielded task was run.
-  bool did_scheduler_yield_;
-
   // Network quality estimator for network aware resource scheudling. This may
   // be null.
   const net::NetworkQualityEstimator* const network_quality_estimator_;
@@ -1022,16 +948,6 @@ ResourceScheduler::ResourceScheduler(bool enabled,
           base::FeatureList::IsEnabled(kPrioritySupportedRequestsDelayable)),
       head_priority_requests_delayable_(base::FeatureList::IsEnabled(
           kHeadPrioritySupportedRequestsDelayable)),
-      yielding_scheduler_enabled_(
-          base::FeatureList::IsEnabled(kNetworkSchedulerYielding)),
-      max_requests_before_yielding_(base::GetFieldTrialParamByFeatureAsInt(
-          kNetworkSchedulerYielding,
-          kMaxRequestsBeforeYieldingParam,
-          kMaxRequestsBeforeYieldingDefault)),
-      yield_time_(base::TimeDelta::FromMilliseconds(
-          base::GetFieldTrialParamByFeatureAsInt(kNetworkSchedulerYielding,
-                                                 kYieldMsParam,
-                                                 kYieldMsDefault))),
       task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   DCHECK(tick_clock_);
 

services/network/resource_scheduler.h

@@ -148,23 +148,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ResourceScheduler {
   bool head_priority_requests_delayable() const {
     return head_priority_requests_delayable_;
   }
-  bool yielding_scheduler_enabled() const {
-    return yielding_scheduler_enabled_;
-  }
-  int max_requests_before_yielding() const {
-    return max_requests_before_yielding_;
-  }
-  base::TimeDelta yield_time() const { return yield_time_; }
   base::SequencedTaskRunner* task_runner() { return task_runner_.get(); }
 
   // Testing setters
-  void SetMaxRequestsBeforeYieldingForTesting(
-      int max_requests_before_yielding) {
-    max_requests_before_yielding_ = max_requests_before_yielding;
-  }
-  void SetYieldTimeForTesting(base::TimeDelta yield_time) {
-    yield_time_ = yield_time;
-  }
   void SetTaskRunnerForTesting(
       scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner) {
     task_runner_ = std::move(sequenced_task_runner);
@@ -230,12 +216,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ResourceScheduler {
   // be delayed while the parser is in head.
   bool head_priority_requests_delayable_;
 
-  // True if the scheduler should yield between several successive calls to
-  // start resource requests.
-  bool yielding_scheduler_enabled_;
-  int max_requests_before_yielding_;
-  base::TimeDelta yield_time_;
-
   ResourceSchedulerParamsManager resource_scheduler_params_manager_;
 
   // The TaskRunner to post tasks on. Can be overridden for tests.

services/network/resource_scheduler_unittest.cc

@@ -60,35 +60,8 @@ const char kPrioritySupportedRequestsDelayable[] =
     "PrioritySupportedRequestsDelayable";
 const char kHeadPrioritySupportedRequestsDelayable[] =
     "HeadPriorityRequestsDelayable";
-const char kNetworkSchedulerYielding[] = "NetworkSchedulerYielding";
 const size_t kMaxNumDelayableRequestsPerHostPerClient = 6;
 
-void ConfigureYieldFieldTrial(
-    int max_requests_before_yielding,
-    int max_yield_ms,
-    base::test::ScopedFeatureList* scoped_feature_list) {
-  const std::string kTrialName = "TrialName";
-  const std::string kGroupName = "GroupName";  // Value not used
-  const std::string kNetworkSchedulerYielding = "NetworkSchedulerYielding";
-
-  scoped_refptr<base::FieldTrial> trial =
-      base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
-
-  std::map<std::string, std::string> params;
-  params["MaxRequestsBeforeYieldingParam"] =
-      base::IntToString(max_requests_before_yielding);
-  params["MaxYieldMs"] = base::IntToString(max_yield_ms);
-  ASSERT_TRUE(
-      base::FieldTrialParamAssociator::GetInstance()->AssociateFieldTrialParams(
-          kTrialName, kGroupName, params));
-
-  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
-  feature_list->RegisterFieldTrialOverride(
-      kNetworkSchedulerYielding, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
-      trial.get());
-  scoped_feature_list->InitWithFeatureList(std::move(feature_list));
-}
-
 class TestRequest {
  public:
   TestRequest(std::unique_ptr<net::URLRequest> url_request,
@@ -495,189 +468,6 @@ TEST_F(ResourceSchedulerTest, OneLowLoadsUntilCriticalComplete) {
       2);
 }
 
-TEST_F(ResourceSchedulerTest, SchedulerYieldsOnSpdy) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(kNetworkSchedulerYielding, "");
-  InitializeScheduler();
-
-  // The second low-priority request should yield.
-  scheduler_->SetMaxRequestsBeforeYieldingForTesting(1);
-
-  // Set a custom yield time.
-  scheduler_->SetYieldTimeForTesting(base::TimeDelta::FromMilliseconds(42));
-
-  // Use a testing task runner so that we can control time.
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
-  scheduler_->SetTaskRunnerForTesting(task_runner);
-
-  http_server_properties_.SetSupportsSpdy(
-      url::SchemeHostPort("https", "spdyhost", 443), true);
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request3(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-
-  // Just before the yield task runs, only the first request should have
-  // started.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(41));
-  EXPECT_TRUE(request->started());
-  EXPECT_FALSE(request2->started());
-  EXPECT_FALSE(request3->started());
-
-  // Yield is done, run the next task.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
-  EXPECT_TRUE(request2->started());
-  EXPECT_FALSE(request3->started());
-
-  // Just before the yield task runs, only the first two requests should have
-  // started.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(41));
-  EXPECT_FALSE(request3->started());
-
-  // Yield is done, run the next task.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
-  EXPECT_TRUE(request3->started());
-}
-
-// Same as SchedulerYieldsOnSpdy but uses FieldTrial Parameters for
-// configuration.
-TEST_F(ResourceSchedulerTest, SchedulerYieldFieldTrialParams) {
-  base::test::ScopedFeatureList scoped_feature_list;
-
-  ConfigureYieldFieldTrial(1 /* requests before yielding */,
-                           42 /* yield time */, &scoped_feature_list);
-  InitializeScheduler();
-
-  // Make sure the parameters were properly set.
-  EXPECT_EQ(42, scheduler_->yield_time().InMilliseconds());
-  EXPECT_EQ(1, scheduler_->max_requests_before_yielding());
-
-  // Use a testing task runner so that we can control time.
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
-  scheduler_->SetTaskRunnerForTesting(task_runner);
-
-  http_server_properties_.SetSupportsSpdy(
-      url::SchemeHostPort("https", "spdyhost", 443), true);
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-
-  // Just before the yield task runs, only the first request should have
-  // started.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(41));
-  EXPECT_TRUE(request->started());
-  EXPECT_FALSE(request2->started());
-
-  // Yield is done, run the next task.
-  task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
-  EXPECT_TRUE(request2->started());
-}
-
-TEST_F(ResourceSchedulerTest, YieldingDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine("", kNetworkSchedulerYielding);
-  InitializeScheduler();
-
-  // We're setting a yield parameter, but no yielding will happen since it's
-  // disabled.
-  scheduler_->SetMaxRequestsBeforeYieldingForTesting(1);
-
-  http_server_properties_.SetSupportsSpdy(
-      url::SchemeHostPort("https", "spdyhost", 443), true);
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  EXPECT_TRUE(request->started());
-  EXPECT_TRUE(request2->started());
-}
-
-TEST_F(ResourceSchedulerTest, SchedulerDoesNotYieldH1) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(kNetworkSchedulerYielding, "");
-  InitializeScheduler();
-  SetMaxDelayableRequests(1);
-
-  // Use a testing task runner so that we can control time.
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
-  scheduler_->SetTaskRunnerForTesting(task_runner);
-
-  // Yield after each request.
-  scheduler_->SetMaxRequestsBeforeYieldingForTesting(1);
-  scheduler_->SetYieldTimeForTesting(base::TimeDelta::FromMilliseconds(42));
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("https://host/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("https://host/low", net::LOWEST));
-
-  EXPECT_TRUE(request->started());
-  EXPECT_FALSE(request2->started());
-
-  // Finish the first task so that the second can start.
-  request = nullptr;
-
-  // Run tasks without advancing time, if there were yielding the next task
-  // wouldn't start.
-  task_runner->RunUntilIdle();
-
-  // The next task started, so there was no yielding.
-  EXPECT_TRUE(request2->started());
-}
-
-TEST_F(ResourceSchedulerTest, SchedulerDoesNotYieldAltSchemes) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(kNetworkSchedulerYielding, "");
-  InitializeScheduler();
-
-  // Yield after each request.
-  scheduler_->SetMaxRequestsBeforeYieldingForTesting(1);
-  scheduler_->SetYieldTimeForTesting(base::TimeDelta::FromMilliseconds(42));
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("yyy://host/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("zzz://host/low", net::LOWEST));
-
-  EXPECT_TRUE(request->started());
-  EXPECT_TRUE(request2->started());
-}
-
-TEST_F(ResourceSchedulerTest, SchedulerDoesNotYieldSyncRequests) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(kNetworkSchedulerYielding, "");
-  InitializeScheduler();
-
-  // The second low-priority request should yield.
-  scheduler_->SetMaxRequestsBeforeYieldingForTesting(1);
-
-  // Use spdy so that we don't throttle.
-  http_server_properties_.SetSupportsSpdy(
-      url::SchemeHostPort("https", "spdyhost", 443), true);
-
-  std::unique_ptr<TestRequest> request(
-      NewRequest("https://spdyhost/low", net::LOWEST));
-  std::unique_ptr<TestRequest> request2(
-      NewRequest("https://spdyhost/low", net::LOWEST));  // yields
-
-  // Add a synchronous request, it shouldn't yield.
-  std::unique_ptr<TestRequest> sync_request(
-      NewSyncRequest("http://spdyhost/low", net::LOWEST));
-
-  EXPECT_TRUE(request->started());
-  EXPECT_FALSE(request2->started());
-  EXPECT_TRUE(sync_request->started());  // The sync request started.
-
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(request2->started());
-}
-
 TEST_F(ResourceSchedulerTest, MaxRequestsPerHostForSpdyWhenNotDelayable) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitFromCommandLine("",
@@ -805,11 +595,6 @@ TEST_F(ResourceSchedulerTest, CancelOtherRequestsWhileResuming) {
 }
 
 TEST_F(ResourceSchedulerTest, LimitedNumberOfDelayableRequestsInFlight) {
-  // The yielding feature will sometimes yield requests before they get a
-  // chance to start, which conflicts this test. So disable the feature.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine("", kNetworkSchedulerYielding);
-
   // Throw in one high priority request to make sure that's not a factor.
   std::unique_ptr<TestRequest> high(
       NewRequest("http://host/high", net::HIGHEST));