NQE: Set min_requests for throughput computation to 5

Keep min payload size to be 32 KB, but make that a field trial param. Also, move |use_small_responses_| to the NQE params class. Bug: 764145 Change-Id: I8bfd37d602705367d294fff037a47ba72c93f2f7 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/657202 Commit-Queue: Tarun Bansal <[email protected]> Reviewed-by: Ryan Sturm <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#501153}(cherry picked from commit fc13d59) Reviewed-on: https://chromium-review.googlesource.com/667127 Reviewed-by: Tarun Bansal <[email protected]> Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#228} Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
tenta-browser · Sep 14, 2017 · fa7ad19 · fa7ad19
1 parent 665eeaa
commit fa7ad19
Show file tree

Hide file tree

Showing 8 changed files with 137 additions and 46 deletions.
diff --git a/net/nqe/network_quality_estimator.cc b/net/nqe/network_quality_estimator.cc
@@ -218,7 +218,6 @@ NetworkQualityEstimator::NetworkQualityEstimator(
     NetLog* net_log)
     : params_(std::move(params)),
       use_localhost_requests_(false),
-      use_small_responses_(false),
       disable_offline_check_(false),
       add_default_platform_observations_(true),
       tick_clock_(new base::DefaultTickClock()),
@@ -677,8 +676,7 @@ void NetworkQualityEstimator::SetUseLocalHostRequestsForTesting(
 void NetworkQualityEstimator::SetUseSmallResponsesForTesting(
     bool use_small_responses) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  use_small_responses_ = use_small_responses;
-  throughput_analyzer_->SetUseSmallResponsesForTesting(use_small_responses_);
+  params_->SetUseSmallResponsesForTesting(use_small_responses);
 }
 
 void NetworkQualityEstimator::DisableOfflineCheckForTesting(

diff --git a/net/nqe/network_quality_estimator.h b/net/nqe/network_quality_estimator.h
@@ -526,11 +526,6 @@ class NET_EXPORT NetworkQualityEstimator
   // network quality. Set to true only for tests.
   bool use_localhost_requests_;
 
-  // Determines if the responses smaller than |kMinTransferSizeInBytes|
-  // or shorter than |kMinTransferSizeInBytes| can be used in estimating the
-  // network quality. Set to true only for tests.
-  bool use_small_responses_;
-
   // When set to true, the device offline check is disabled when computing the
   // effective connection type or when writing the prefs. Set to true only for
   // testing.

diff --git a/net/nqe/network_quality_estimator_params.cc b/net/nqe/network_quality_estimator_params.cc
@@ -375,7 +375,11 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
       throughput_min_requests_in_flight_(
           GetValueForVariationParam(params_,
                                     "throughput_min_requests_in_flight",
-                                    1)),
+                                    5)),
+      throughput_min_transfer_size_kilobytes_(
+          GetValueForVariationParam(params_,
+                                    "throughput_min_transfer_size_kilobytes",
+                                    32)),
       weight_multiplier_per_second_(GetWeightMultiplierPerSecond(params_)),
       weight_multiplier_per_signal_strength_level_(
           GetDoubleValueForVariationParamWithDefaultValue(
@@ -418,7 +422,8 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
           GetDoubleValueForVariationParamWithDefaultValue(
               params_,
               "historical_time_threshold",
-              60000))) {
+              60000))),
+      use_small_responses_(false) {
   DCHECK_LE(0.0, correlation_uma_logging_probability_);
   DCHECK_GE(1.0, correlation_uma_logging_probability_);
   DCHECK(lower_bound_http_rtt_transport_rtt_multiplier_ == -1 ||
@@ -447,6 +452,17 @@ NetworkQualityEstimatorParams::NetworkQualityEstimatorParams(
 NetworkQualityEstimatorParams::~NetworkQualityEstimatorParams() {
 }
 
+void NetworkQualityEstimatorParams::SetUseSmallResponsesForTesting(
+    bool use_small_responses) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  use_small_responses_ = use_small_responses;
+}
+
+bool NetworkQualityEstimatorParams::use_small_responses() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return use_small_responses_;
+};
+
 // static
 NetworkQualityEstimatorParams::EffectiveConnectionTypeAlgorithm
 NetworkQualityEstimatorParams::GetEffectiveConnectionTypeAlgorithmFromString(
@@ -479,6 +495,22 @@ NetworkQualityEstimatorParams::GetEffectiveConnectionTypeAlgorithmFromString(
   return kDefaultEffectiveConnectionTypeAlgorithm;
 }
 
+size_t NetworkQualityEstimatorParams::throughput_min_requests_in_flight()
+    const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // If |use_small_responses_| is set to true for testing, then consider one
+  // request as sufficient for taking throughput sample.
+  return use_small_responses_ ? 1 : throughput_min_requests_in_flight_;
+}
+
+int64_t NetworkQualityEstimatorParams::GetThroughputMinTransferSizeBits()
+    const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return static_cast<int64_t>(throughput_min_transfer_size_kilobytes_) * 8 *
+         1000;
+}
+
 // static
 const char* NetworkQualityEstimatorParams::GetNameForConnectionType(
     NetworkChangeNotifier::ConnectionType connection_type) {

diff --git a/net/nqe/network_quality_estimator_params.h b/net/nqe/network_quality_estimator_params.h
@@ -68,9 +68,11 @@ class NET_EXPORT NetworkQualityEstimatorParams {
   // Returns the minimum number of requests in-flight to consider the network
   // fully utilized. A throughput observation is taken only when the network is
   // considered as fully utilized.
-  size_t throughput_min_requests_in_flight() const {
-    return throughput_min_requests_in_flight_;
-  }
+  size_t throughput_min_requests_in_flight() const;
+
+  // Tiny transfer sizes may give inaccurate throughput results.
+  // Minimum size of the transfer over which the throughput is computed.
+  int64_t GetThroughputMinTransferSizeBits() const;
 
   // Returns the weight multiplier per second, which represents the factor by
   // which the weight of an observation reduces every second.
@@ -173,12 +175,23 @@ class NET_EXPORT NetworkQualityEstimatorParams {
     return historical_time_threshold_;
   }
 
+  // Determines if the responses smaller than |kMinTransferSizeInBytes|
+  // or shorter than |kMinTransferSizeInBytes| can be used in estimating the
+  // network quality. Set to true only for tests.
+  bool use_small_responses() const;
+
+  // |use_small_responses| should only be true when testing.
+  // Allows the responses smaller than |kMinTransferSizeInBits| to be used for
+  // network quality estimation.
+  void SetUseSmallResponsesForTesting(bool use_small_responses);
+
  private:
   // Map containing all field trial parameters related to
   // NetworkQualityEstimator field trial.
   const std::map<std::string, std::string> params_;
 
   const size_t throughput_min_requests_in_flight_;
+  const int throughput_min_transfer_size_kilobytes_;
   const double weight_multiplier_per_second_;
   const double weight_multiplier_per_signal_strength_level_;
   const double correlation_uma_logging_probability_;
@@ -191,6 +204,8 @@ class NET_EXPORT NetworkQualityEstimatorParams {
   const base::TimeDelta recent_time_threshold_;
   const base::TimeDelta historical_time_threshold_;
 
+  bool use_small_responses_;
+
   EffectiveConnectionTypeAlgorithm effective_connection_type_algorithm_;
 
   // Default network quality observations obtained from |params_|.

diff --git a/net/nqe/network_quality_estimator_unittest.cc b/net/nqe/network_quality_estimator_unittest.cc
@@ -195,6 +195,7 @@ TEST(NetworkQualityEstimatorTest, TestKbpsRTTUpdates) {
   base::HistogramTester histogram_tester;
   // Enable requests to local host to be used for network quality estimation.
   std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
   TestNetworkQualityEstimator estimator(variation_params);
 
   estimator.SimulateNetworkChange(
@@ -347,6 +348,7 @@ TEST(NetworkQualityEstimatorTest, Caching) {
         NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET}) {
     base::HistogramTester histogram_tester;
     std::map<std::string, std::string> variation_params;
+    variation_params["throughput_min_requests_in_flight"] = "1";
     TestNetworkQualityEstimator estimator(variation_params);
 
     const std::string connection_id =
@@ -465,6 +467,7 @@ TEST(NetworkQualityEstimatorTest, CachingDisabled) {
   std::map<std::string, std::string> variation_params;
   // Do not set |persistent_cache_reading_enabled| variation param.
   variation_params["persistent_cache_reading_enabled"] = "false";
+  variation_params["throughput_min_requests_in_flight"] = "1";
   TestNetworkQualityEstimator estimator(variation_params);
 
   estimator.SimulateNetworkChange(
@@ -545,7 +548,9 @@ TEST(NetworkQualityEstimatorTest, QuicObservations) {
 }
 
 TEST(NetworkQualityEstimatorTest, StoreObservations) {
-  TestNetworkQualityEstimator estimator;
+  std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
+  TestNetworkQualityEstimator estimator(variation_params);
 
   base::TimeDelta rtt;
   int32_t kbps;
@@ -582,7 +587,9 @@ TEST(NetworkQualityEstimatorTest, StoreObservations) {
 // throughput and RTT percentiles are checked for correctness by doing simple
 // verifications.
 TEST(NetworkQualityEstimatorTest, ComputedPercentiles) {
-  TestNetworkQualityEstimator estimator;
+  std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
+  TestNetworkQualityEstimator estimator(variation_params);
 
   std::vector<NetworkQualityObservationSource> disallowed_observation_sources;
   disallowed_observation_sources.push_back(
@@ -1508,6 +1515,7 @@ TEST(NetworkQualityEstimatorTest, TestExternalEstimateProviderMergeEstimates) {
       test_external_estimate_provider);
 
   std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
   TestNetworkQualityEstimator estimator(variation_params,
                                         std::move(external_estimate_provider));
   estimator.SimulateNetworkChange(net::NetworkChangeNotifier::CONNECTION_WIFI,
@@ -1553,6 +1561,7 @@ TEST(NetworkQualityEstimatorTest, TestExternalEstimateProviderMergeEstimates) {
 TEST(NetworkQualityEstimatorTest, TestThroughputNoRequestOverlap) {
   base::HistogramTester histogram_tester;
   std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
 
   static const struct {
     bool allow_small_localhost_requests;
@@ -2060,7 +2069,11 @@ TEST(NetworkQualityEstimatorTest,
 TEST(NetworkQualityEstimatorTest, TestRttThroughputObservers) {
   TestRTTObserver rtt_observer;
   TestThroughputObserver throughput_observer;
-  TestNetworkQualityEstimator estimator;
+
+  std::map<std::string, std::string> variation_params;
+  variation_params["throughput_min_requests_in_flight"] = "1";
+  TestNetworkQualityEstimator estimator(variation_params);
+
   estimator.AddRTTObserver(&rtt_observer);
   estimator.AddThroughputObserver(&throughput_observer);
 
@@ -2162,6 +2175,7 @@ TEST(NetworkQualityEstimatorTest, MAYBE_TestTCPSocketRTT) {
 
   std::map<std::string, std::string> variation_params;
   variation_params["persistent_cache_reading_enabled"] = "true";
+  variation_params["throughput_min_requests_in_flight"] = "1";
   TestNetworkQualityEstimator estimator(
       nullptr, variation_params, true, true,
       true /* add_default_platform_observations */,

diff --git a/net/nqe/throughput_analyzer.cc b/net/nqe/throughput_analyzer.cc
@@ -26,10 +26,6 @@ namespace {
 // degrade accuracy held in the memory.
 static const size_t kMaxRequestsSize = 300;
 
-// Tiny transfer sizes may give inaccurate throughput results.
-// Minimum size of the transfer over which the throughput is computed.
-static const int kMinTransferSizeInBits = 32 * 8 * 1000;
-
 }  // namespace
 
 namespace nqe {
@@ -49,7 +45,6 @@ ThroughputAnalyzer::ThroughputAnalyzer(
       bits_received_at_window_start_(0),
       disable_throughput_measurements_(false),
       use_localhost_requests_for_tests_(false),
-      use_small_responses_for_tests_(false),
       net_log_(net_log) {
   DCHECK(params_);
   DCHECK(task_runner_);
@@ -203,8 +198,10 @@ bool ThroughputAnalyzer::MaybeGetThroughputObservation(
 
   // Ignore tiny/short transfers, which will not produce accurate rates. Skip
   // the checks if |use_small_responses_| is true.
-  if (!use_small_responses_for_tests_ && bits_received < kMinTransferSizeInBits)
+  if (!params_->use_small_responses() &&
+      bits_received < params_->GetThroughputMinTransferSizeBits()) {
     return false;
+  }
 
   double downstream_kbps_double =
       (bits_received * 1.0f) / duration.InMillisecondsF();
@@ -243,12 +240,6 @@ void ThroughputAnalyzer::SetUseLocalHostRequestsForTesting(
   use_localhost_requests_for_tests_ = use_localhost_requests;
 }
 
-void ThroughputAnalyzer::SetUseSmallResponsesForTesting(
-    bool use_small_responses) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  use_small_responses_for_tests_ = use_small_responses;
-}
-
 int64_t ThroughputAnalyzer::GetBitsReceived() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return NetworkActivityMonitor::GetInstance()->GetBytesReceived() * 8;

diff --git a/net/nqe/throughput_analyzer.h b/net/nqe/throughput_analyzer.h
@@ -77,12 +77,6 @@ class NET_EXPORT_PRIVATE ThroughputAnalyzer {
   // quality estimation.
   void SetUseLocalHostRequestsForTesting(bool use_localhost_requests);
 
-  // |use_smaller_responses_for_tests| should only be true when testing, and
-  // allows the responses smaller than |kMinTransferSizeInBits| or shorter than
-  // |kMinRequestDurationMicroseconds| to be used for network quality
-  // estimation.
-  void SetUseSmallResponsesForTesting(bool use_small_responses);
-
   // Returns true if throughput is currently tracked by a throughput
   // observation window.
   bool IsCurrentlyTrackingThroughput() const;
@@ -171,11 +165,6 @@ class NET_EXPORT_PRIVATE ThroughputAnalyzer {
   // network quality. Set to true only for tests.
   bool use_localhost_requests_for_tests_;
 
-  // Determines if the responses smaller than |kMinTransferSizeInBits|
-  // or shorter than |kMinTransferSizeInBits| can be used in estimating the
-  // network quality. Set to true only for tests.
-  bool use_small_responses_for_tests_;
-
   base::ThreadChecker thread_checker_;
 
   NetLogWithSource net_log_;

diff --git a/net/nqe/throughput_analyzer_unittest.cc b/net/nqe/throughput_analyzer_unittest.cc
@@ -136,6 +136,53 @@ TEST(ThroughputAnalyzerTest, MaximumRequests) {
   }
 }
 
+// Tests that the throughput observation is taken only if there are sufficient
+// number of requests in-flight.
+TEST(ThroughputAnalyzerTest, TestMinRequestsForThroughputSample) {
+  std::map<std::string, std::string> variation_params;
+  NetworkQualityEstimatorParams params(variation_params);
+
+  for (size_t num_requests = 1;
+       num_requests <= params.throughput_min_requests_in_flight() + 1;
+       ++num_requests) {
+    TestThroughputAnalyzer throughput_analyzer(&params);
+    TestDelegate test_delegate;
+    TestURLRequestContext context;
+    throughput_analyzer.AddIPAddressResolution(&context);
+    std::vector<std::unique_ptr<URLRequest>> requests_not_local;
+
+    for (size_t i = 0; i < num_requests; ++i) {
+      std::unique_ptr<URLRequest> request_not_local(context.CreateRequest(
+          GURL("http://example.com/echo.html"), DEFAULT_PRIORITY,
+          &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS));
+      request_not_local->Start();
+      requests_not_local.push_back(std::move(request_not_local));
+    }
+
+    base::RunLoop().Run();
+
+    EXPECT_EQ(0, throughput_analyzer.throughput_observations_received());
+
+    for (size_t i = 0; i < requests_not_local.size(); ++i) {
+      throughput_analyzer.NotifyStartTransaction(*requests_not_local.at(i));
+    }
+
+    // Increment the bytes received count to emulate the bytes received for
+    // |request_local| and |requests_not_local|.
+    throughput_analyzer.IncrementBitsReceived(100 * 1000 * 8);
+
+    for (size_t i = 0; i < requests_not_local.size(); ++i) {
+      throughput_analyzer.NotifyRequestCompleted(*requests_not_local.at(i));
+    }
+    base::RunLoop().RunUntilIdle();
+
+    int expected_throughput_observations =
+        num_requests >= params.throughput_min_requests_in_flight() ? 1 : 0;
+    EXPECT_EQ(expected_throughput_observations,
+              throughput_analyzer.throughput_observations_received());
+  }
+}
+
 // Tests if the throughput observation is taken correctly when local and network
 // requests overlap.
 TEST(ThroughputAnalyzerTest, TestThroughputWithMultipleRequestsOverlap) {
@@ -167,10 +214,15 @@ TEST(ThroughputAnalyzerTest, TestThroughputWithMultipleRequestsOverlap) {
 
     std::unique_ptr<URLRequest> request_local;
 
-    std::unique_ptr<URLRequest> request_not_local(context.CreateRequest(
-        GURL("http://example.com/echo.html"), DEFAULT_PRIORITY, &test_delegate,
-        TRAFFIC_ANNOTATION_FOR_TESTS));
-    request_not_local->Start();
+    std::vector<std::unique_ptr<URLRequest>> requests_not_local;
+
+    for (size_t i = 0; i < params.throughput_min_requests_in_flight(); ++i) {
+      std::unique_ptr<URLRequest> request_not_local(context.CreateRequest(
+          GURL("http://example.com/echo.html"), DEFAULT_PRIORITY,
+          &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS));
+      request_not_local->Start();
+      requests_not_local.push_back(std::move(request_not_local));
+    }
 
     if (test.start_local_request) {
       request_local = context.CreateRequest(GURL("http://127.0.0.1/echo.html"),
@@ -190,18 +242,23 @@ TEST(ThroughputAnalyzerTest, TestThroughputWithMultipleRequestsOverlap) {
     // at all times.
     if (test.start_local_request)
       throughput_analyzer.NotifyStartTransaction(*request_local);
-    throughput_analyzer.NotifyStartTransaction(*request_not_local);
+
+    for (size_t i = 0; i < requests_not_local.size(); ++i) {
+      throughput_analyzer.NotifyStartTransaction(*requests_not_local.at(i));
+    }
 
     if (test.local_request_completes_first) {
       ASSERT_TRUE(test.start_local_request);
       throughput_analyzer.NotifyRequestCompleted(*request_local);
     }
 
     // Increment the bytes received count to emulate the bytes received for
-    // |request_local| and |request_not_local|.
+    // |request_local| and |requests_not_local|.
     throughput_analyzer.IncrementBitsReceived(100 * 1000 * 8);
 
-    throughput_analyzer.NotifyRequestCompleted(*request_not_local);
+    for (size_t i = 0; i < requests_not_local.size(); ++i) {
+      throughput_analyzer.NotifyRequestCompleted(*requests_not_local.at(i));
+    }
     if (test.start_local_request && !test.local_request_completes_first)
       throughput_analyzer.NotifyRequestCompleted(*request_local);