diff --git a/net/nqe/network_quality_estimator.cc b/net/nqe/network_quality_estimator.cc index e75af88385d48..71a0f3476f2c7 100644 --- 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 index af987fe59ba33..3b7eb4d6213e9 100644 --- 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 index 23ce9f9f7a9c4..a6bc3bd10916a 100644 --- 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(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 index 5d02e97e4dd15..02b4e1dd85301 100644 --- 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 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 index 3f560907d0082..4549137c628b2 100644 --- 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 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 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 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 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 variation_params; + variation_params["throughput_min_requests_in_flight"] = "1"; + TestNetworkQualityEstimator estimator(variation_params); std::vector disallowed_observation_sources; disallowed_observation_sources.push_back( @@ -1508,6 +1515,7 @@ TEST(NetworkQualityEstimatorTest, TestExternalEstimateProviderMergeEstimates) { test_external_estimate_provider); std::map 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 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 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 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 index e1d2999778c6c..b54e5be3512c1 100644 --- 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 index b815ac5141c57..74c9849f7abf4 100644 --- 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 index 58afa8386bdc5..f4e2e251fbb82 100644 --- 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 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(¶ms); + TestDelegate test_delegate; + TestURLRequestContext context; + throughput_analyzer.AddIPAddressResolution(&context); + std::vector> requests_not_local; + + for (size_t i = 0; i < num_requests; ++i) { + std::unique_ptr 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 request_local; - std::unique_ptr 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> requests_not_local; + + for (size_t i = 0; i < params.throughput_min_requests_in_flight(); ++i) { + std::unique_ptr 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,7 +242,10 @@ 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); @@ -198,10 +253,12 @@ TEST(ThroughputAnalyzerTest, TestThroughputWithMultipleRequestsOverlap) { } // 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);