Skip to content

GitLab

T
tangled
Commit 3818510e authored by Scott Violet's avatar Scott Violet Committed by Commit Bot
Browse files
Options

Revert "Add feature flag for disabling local predictions" 

This reverts commit 564fad0b.

Reason for revert: This has broken a number of builders, such as https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/35679 . I believe you misspelled DISABLED_, you have DISABLE_. I would fix it, but it'll take longer to cycle through the CQ then a straight revert.

Original change's description:
> Add feature flag for disabling local predictions
> 
> This allows us to run the Client-Side Reactive Preconnect in the below
> configs:
> 1) Default - Local predictions only
> 2) Local predictions with optimization guide filling in
> 3) Optimization guide only
> 4) No predictions
> 
> Bug: 1050101
> Change-Id: I9eb87170aa0079f3384f1797b03cd3d850b1228e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2094246
> Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Commit-Queue: Sophie Chang <sophiechang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#748290}

TBR=tbansal@chromium.org,sophiechang@chromium.org,ryansturm@chromium.org

Change-Id: I434d86687d555ece46617b62d59cbe492904a5b0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1050101
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2094542Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748316}
parent 897e3eb6
Hide whitespace changes
Inline Side-by-side
Showing with 15 additions and 158 deletions
chrome/browser/predictors/loading_predictor.cc
View file @ 3818510e
......@@ -11,7 +11,6 @@
#include "chrome/browser/predictors/loading_data_collector.h"
#include "chrome/browser/predictors/loading_stats_collector.h"
#include "chrome/browser/predictors/navigation_id.h"
#include "chrome/browser/predictors/predictors_features.h"
#include "chrome/browser/predictors/resource_prefetch_predictor.h"
#include "content/public/browser/browser_thread.h"
#include "net/base/network_isolation_key.h"
......@@ -86,12 +85,8 @@ bool LoadingPredictor::PrepareForPageLoad(
}
PreconnectPrediction prediction;
bool has_local_preconnect_prediction = false;
if (features::ShouldUseLocalPredictions()) {
has_local_preconnect_prediction =
resource_prefetch_predictor_->PredictPreconnectOrigins(url,
&prediction);
}
bool has_local_preconnect_prediction =
resource_prefetch_predictor_->PredictPreconnectOrigins(url, &prediction);
if (active_hints_.find(url) != active_hints_.end() &&
has_local_preconnect_prediction) {
// We are currently preconnecting using the local preconnect prediction. Do
......
chrome/browser/predictors/loading_predictor_browsertest.cc
View file @ 3818510e
......@@ -1519,21 +1519,13 @@ IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithProxy,
}
class LoadingPredictorBrowserTestWithOptimizationGuide
: public ::testing::WithParamInterface<bool>,
public LoadingPredictorBrowserTest {
: public LoadingPredictorBrowserTest {
public:
LoadingPredictorBrowserTestWithOptimizationGuide() {
feature_list_.InitWithFeatures(
{features::kLoadingPredictorUseOptimizationGuide,
optimization_guide::features::kOptimizationHints},
{});
if (GetParam()) {
local_predictions_feature_list_.InitAndEnableFeature(
features::kLoadingPredictorUseLocalPredictions);
} else {
local_predictions_feature_list_.InitAndDisableFeature(
features::kLoadingPredictorUseLocalPredictions);
}
}
void SetUpCommandLine(base::CommandLine* command_line) override {
......@@ -1573,19 +1565,12 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
optimization_guide::switches::kHintsProtoOverride, config_string);
}
bool IsLocalPredictionEnabled() const { return GetParam(); }
private:
base::test::ScopedFeatureList feature_list_;
base::test::ScopedFeatureList local_predictions_feature_list_;
};
INSTANTIATE_TEST_SUITE_P(UseLocalPrediction,
LoadingPredictorBrowserTestWithOptimizationGuide,
::testing::Bool());
IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
NavigationHasLocalPredictionNoOptimizationHint) {
IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithOptimizationGuide,
LocalPredictionTakesPrecedence) {
// Navigate the first time to fill the predictor's database and the HTTP
// cache.
GURL url = embedded_test_server()->GetURL(
......@@ -1599,24 +1584,13 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
auto observer = NavigateToURLAsync(url);
EXPECT_TRUE(observer->WaitForRequestStart());
for (auto* const host : kHtmlSubresourcesHosts) {
if (!IsLocalPredictionEnabled() && host != url.host()) {
// We don't expect local predictions to be preconnected to.
continue;
}
preconnect_manager_observer()->WaitUntilHostLookedUp(host,
network_isolation_key);
EXPECT_TRUE(
preconnect_manager_observer()->HostFound(host, network_isolation_key));
}
size_t expected_connections;
if (IsLocalPredictionEnabled()) {
// 2 connections to the main frame host + 1 connection per host for others.
expected_connections = base::size(kHtmlSubresourcesHosts) + 1;
} else {
// There should always be 2 connections to the main frame host.
expected_connections = 2;
}
// 2 connections to the main frame host + 1 connection per host for others.
const size_t expected_connections = base::size(kHtmlSubresourcesHosts) + 1;
connection_tracker()->WaitForAcceptedConnections(expected_connections);
EXPECT_EQ(expected_connections,
connection_tracker()->GetAcceptedSocketCount());
......@@ -1626,70 +1600,14 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
// https://crbug.com/1056693
#if (defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS))
#define DISABLE_ON_WIN_MAC_CHROMEOS(x) DISABLE_##x
#define MAYBE_UsesPredictionsFromOptimizationGuideIfAvailable \
DISABLED_UsesPredictionsFromOptimizationGuideIfAvailable
#else
#define DISABLE_ON_WIN_MAC_CHROMEOS(x) x
#define MAYBE_UsesPredictionsFromOptimizationGuideIfAvailable \
UsesPredictionsFromOptimizationGuideIfAvailable
#endif
IN_PROC_BROWSER_TEST_P(
LoadingPredictorBrowserTestWithOptimizationGuide,
DISABLE_ON_WIN_MAC_CHROMEOS(
NavigationWithBothLocalPredictionAndOptimizationHint)) {
base::HistogramTester histogram_tester;
GURL url = embedded_test_server()->GetURL(
"m.hints.com",
GetPathWithPortReplacement(kHtmlSubresourcesPath,
embedded_test_server()->port()));
url::Origin origin = url::Origin::Create(url);
net::NetworkIsolationKey network_isolation_key(origin, origin);
ui_test_utils::NavigateToURL(browser(), url);
RetryForHistogramUntilCountReached(
histogram_tester, optimization_guide::kLoadedHintLocalHistogramString, 1);
ResetNetworkState();
auto observer = NavigateToURLAsync(url);
EXPECT_TRUE(observer->WaitForRequestStart());
// The initial URL should be preconnected to.
preconnect_manager_observer()->WaitUntilHostLookedUp(url.host(),
network_isolation_key);
EXPECT_TRUE(preconnect_manager_observer()->HostFound(url.host(),
network_isolation_key));
EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
origin.GetURL()));
// Both subresource hosts should be preconnected to.
std::vector<std::string> expected_subresource_hosts;
if (IsLocalPredictionEnabled()) {
// Should use subresources that were learned.
expected_subresource_hosts = {"baz.com", "foo.com"};
} else {
// Should use subresources from optimization hint.
expected_subresource_hosts = {"subresource.com", "otherresource.com"};
}
for (const auto& host : expected_subresource_hosts) {
preconnect_manager_observer()->WaitUntilHostLookedUp(host,
network_isolation_key);
EXPECT_TRUE(
preconnect_manager_observer()->HostFound(host, network_isolation_key));
GURL expected_origin;
if (IsLocalPredictionEnabled()) {
// The locally learned origins are expected to have a port.
expected_origin = embedded_test_server()->GetURL(host, "/");
} else {
// The optimization hints learned origins do not have a port.
expected_origin = GURL(base::StringPrintf("http://%s", host.c_str()));
}
EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
expected_origin));
}
}
IN_PROC_BROWSER_TEST_P(
LoadingPredictorBrowserTestWithOptimizationGuide,
DISABLE_ON_WIN_MAC_CHROMEOS(
NavigationWithNoLocalPredictionsButHasOptimizationHint)) {
IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithOptimizationGuide,
MAYBE_UsesPredictionsFromOptimizationGuideIfAvailable) {
base::HistogramTester histogram_tester;
GURL url = embedded_test_server()->GetURL("m.hints.com", "/simple.html");
......@@ -1746,7 +1664,7 @@ IN_PROC_BROWSER_TEST_P(
"LoadingPredictor.PreconnectLearningCount.OptimizationGuide", 2, 1);
}
IN_PROC_BROWSER_TEST_P(
IN_PROC_BROWSER_TEST_F(
LoadingPredictorBrowserTestWithOptimizationGuide,
OptimizationGuidePredictionsNotAppliedForAlreadyCommittedNavigation) {
GURL url = embedded_test_server()->GetURL("hints.com", "/");
......@@ -1762,7 +1680,7 @@ IN_PROC_BROWSER_TEST_P(
"otheresource.com", network_isolation_key));
}
IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithOptimizationGuide,
OptimizationGuidePredictionsNotAppliedForRedirect) {
GURL destination_url =
embedded_test_server()->GetURL("hints.com", "/cachetime");
......@@ -1780,46 +1698,4 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
"otheresource.com", network_isolation_key));
}
class LoadingPredictorBrowserTestWithNoLocalPredictions
: public LoadingPredictorBrowserTest {
public:
LoadingPredictorBrowserTestWithNoLocalPredictions() {
feature_list_.InitAndDisableFeature(
features::kLoadingPredictorUseLocalPredictions);
}
private:
base::test::ScopedFeatureList feature_list_;
};
IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithNoLocalPredictions,
ShouldNotActOnLocalPrediction) {
// Navigate the first time to fill the predictor's database and the HTTP
// cache.
GURL url = embedded_test_server()->GetURL(
"test.com", GetPathWithPortReplacement(kHtmlSubresourcesPath,
embedded_test_server()->port()));
url::Origin origin = url::Origin::Create(url);
net::NetworkIsolationKey network_isolation_key(origin, origin);
ui_test_utils::NavigateToURL(browser(), url);
ResetNetworkState();
auto observer = NavigateToURLAsync(url);
EXPECT_TRUE(observer->WaitForRequestStart());
// The initial URL should be preconnected to.
preconnect_manager_observer()->WaitUntilHostLookedUp(url.host(),
network_isolation_key);
EXPECT_TRUE(preconnect_manager_observer()->HostFound(url.host(),
network_isolation_key));
EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
origin.GetURL()));
// 2 connections to the main frame host.
const size_t expected_connections = 2;
connection_tracker()->WaitForAcceptedConnections(expected_connections);
EXPECT_EQ(expected_connections,
connection_tracker()->GetAcceptedSocketCount());
// No reads since all resources should be cached.
EXPECT_EQ(0u, connection_tracker()->GetReadSocketCount());
}
} // namespace predictors
chrome/browser/predictors/predictors_features.cc
View file @ 3818510e
......@@ -6,10 +6,6 @@
namespace features {
// Whether local predictions should be used to make preconnect predictions.
const base::Feature kLoadingPredictorUseLocalPredictions{
"LoadingPredictorUseLocalPredictions", base::FEATURE_ENABLED_BY_DEFAULT};
// Modifies loading predictor so that it only learns about subresources and
// origins that are high priority.
const base::Feature kLoadingOnlyLearnHighPriorityResources{
......@@ -35,8 +31,4 @@ const base::Feature kLoadingPredictorDisregardAlwaysAccessesNetwork{
const base::Feature kLoadingPredictorUseOptimizationGuide{
"LoadingPredictorUseOptimizationGuide", base::FEATURE_DISABLED_BY_DEFAULT};
bool ShouldUseLocalPredictions() {
return base::FeatureList::IsEnabled(kLoadingPredictorUseLocalPredictions);
}
} // namespace features
chrome/browser/predictors/predictors_features.h
View file @ 3818510e
......@@ -12,8 +12,6 @@ namespace features {
// All features in alphabetical order. The features should be documented
// alongside the definition of their values in the .cc file.
extern const base::Feature kLoadingPredictorUseLocalPredictions;
extern const base::Feature kLoadingOnlyLearnHighPriorityResources;
extern const base::Feature kLoadingPredictorTableConfig;
......@@ -24,10 +22,6 @@ extern const base::Feature kLoadingPredictorDisregardAlwaysAccessesNetwork;
extern const base::Feature kLoadingPredictorUseOptimizationGuide;
// Returns whether local predictions should be used to make preconnect
// predictions.
bool ShouldUseLocalPredictions();
} // namespace features
#endif // CHROME_BROWSER_PREDICTORS_PREDICTORS_FEATURES_H_
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment