Skip to content

Commit

Permalink
Revert of QUIC - enable "delay_tcp_race" parameter by default. This f…
Browse files Browse the repository at this point in the history
…eature showed (patchset #3 id:60001 of https://codereview.chromium.org/1916783003/ )

Reason for revert:
Reverting this CL so that we could experiment delay_tcp_race.

Original issue's description:
> QUIC - enable "delay_tcp_race" parameter by default. This feature showed
> positive results in stable channel and it will be enabled by default
> via finch trial in Stable and Beta (M50 and M51).
>
> R=rch@chromium.org,xunjieli@chromium.org
>
> Committed: https://crrev.com/634050c3f304a48f62c6ceb022a7f83945ea1968
> Cr-Commit-Position: refs/heads/master@{#390468}

TBR=rch@chromium.org,xunjieli@chromium.org

Review-Url: https://codereview.chromium.org/1977303002
Cr-Commit-Position: refs/heads/master@{#393778}
  • Loading branch information
rtenneti authored and Commit bot committed May 16, 2016
1 parent 5bd243e commit b8e80fb
Show file tree
Hide file tree
Showing 19 changed files with 150 additions and 10 deletions.
8 changes: 8 additions & 0 deletions chrome/browser/io_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,7 @@ void IOThread::NetworkSessionConfigurator::ConfigureQuicParams(
if (receive_buffer_size != 0) {
params->quic_socket_receive_buffer_size = receive_buffer_size;
}
params->quic_delay_tcp_race = ShouldQuicDelayTcpRace(quic_trial_params);
float load_server_info_timeout_srtt_multiplier =
GetQuicLoadServerInfoTimeoutSrttMultiplier(quic_trial_params);
if (load_server_info_timeout_srtt_multiplier != 0) {
Expand Down Expand Up @@ -1447,6 +1448,13 @@ int IOThread::NetworkSessionConfigurator::GetQuicSocketReceiveBufferSize(
return 0;
}

// static
bool IOThread::NetworkSessionConfigurator::ShouldQuicDelayTcpRace(
const VariationParameters& quic_trial_params) {
return base::LowerCaseEqualsASCII(
GetVariationParam(quic_trial_params, "delay_tcp_race"), "true");
}

// static
bool IOThread::NetworkSessionConfigurator::ShouldQuicCloseSessionsOnIpChange(
const VariationParameters& quic_trial_params) {
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/io_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ class IOThread : public content::BrowserThreadDelegate {
static int GetQuicSocketReceiveBufferSize(
const VariationParameters& quic_trial_params);

// Returns true if QUIC should delay TCP connection when QUIC works.
static bool ShouldQuicDelayTcpRace(
const VariationParameters& quic_trial_params);

// Returns true if QUIC should close sessions when any of the client's IP
// addresses change.
static bool ShouldQuicCloseSessionsOnIpChange(
Expand Down
12 changes: 12 additions & 0 deletions chrome/browser/io_thread_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ TEST_F(NetworkSessionConfiguratorTest, EnableQuicFromFieldTrialGroup) {
EXPECT_FALSE(params_.enable_alternative_service_with_different_host);
EXPECT_EQ(0, params_.quic_max_number_of_lossy_connections);
EXPECT_EQ(1.0f, params_.quic_packet_loss_threshold);
EXPECT_FALSE(params_.quic_delay_tcp_race);
EXPECT_FALSE(params_.quic_close_sessions_on_ip_change);
EXPECT_EQ(net::kIdleConnectionTimeoutSeconds,
params_.quic_idle_connection_timeout_seconds);
Expand Down Expand Up @@ -576,6 +577,17 @@ TEST_F(NetworkSessionConfiguratorTest, QuicReceiveBufferSize) {
EXPECT_EQ(2097152, params_.quic_socket_receive_buffer_size);
}

TEST_F(NetworkSessionConfiguratorTest, QuicDelayTcpConnection) {
std::map<std::string, std::string> field_trial_params;
field_trial_params["delay_tcp_race"] = "true";
variations::AssociateVariationParams("QUIC", "Enabled", field_trial_params);
base::FieldTrialList::CreateFieldTrial("QUIC", "Enabled");

ParseFieldTrials();

EXPECT_TRUE(params_.quic_delay_tcp_race);
}

TEST_F(NetworkSessionConfiguratorTest, QuicOriginsToForceQuicOn) {
base::CommandLine::ForCurrentProcess()->AppendSwitch("enable-quic");
base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ protected void setUp() throws Exception {
.put("connection_options", "PACE,IW10,FOO,DEADBEEF")
.put("host_whitelist", "test.example.com")
.put("max_server_configs_stored_in_properties", 2)
.put("delay_tcp_race", true)
.put("max_number_of_lossy_connections", 10)
.put("packet_loss_threshold", 0.5)
.put("idle_connection_timeout_seconds", 300)
Expand Down
6 changes: 6 additions & 0 deletions components/cronet/url_request_context_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const char kQuicStoreServerConfigsInProperties[] =
"store_server_configs_in_properties";
const char kQuicMaxServerConfigsStoredInProperties[] =
"max_server_configs_stored_in_properties";
const char kQuicDelayTcpRace[] = "delay_tcp_race";
const char kQuicMaxNumberOfLossyConnections[] =
"max_number_of_lossy_connections";
const char kQuicPacketLossThreshold[] = "packet_loss_threshold";
Expand Down Expand Up @@ -108,6 +109,11 @@ void ParseAndSetExperimentalOptions(
static_cast<size_t>(quic_max_server_configs_stored_in_properties));
}

bool quic_delay_tcp_race = false;
if (quic_args->GetBoolean(kQuicDelayTcpRace, &quic_delay_tcp_race)) {
context_builder->set_quic_delay_tcp_race(quic_delay_tcp_race);
}

int quic_max_number_of_lossy_connections = 0;
if (quic_args->GetInteger(kQuicMaxNumberOfLossyConnections,
&quic_max_number_of_lossy_connections)) {
Expand Down
4 changes: 4 additions & 0 deletions components/cronet/url_request_context_config_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ TEST(URLRequestContextConfigTest, SetQuicExperimentalOptions) {
"fake agent",
// JSON encoded experimental options.
"{\"QUIC\":{\"max_server_configs_stored_in_properties\":2,"
"\"delay_tcp_race\":true,"
"\"max_number_of_lossy_connections\":10,"
"\"prefer_aes\":true,"
"\"user_agent_id\":\"Custom QUIC UAID\","
Expand Down Expand Up @@ -80,6 +81,9 @@ TEST(URLRequestContextConfigTest, SetQuicExperimentalOptions) {
// Check max_server_configs_stored_in_properties.
EXPECT_EQ(2u, params->quic_max_server_configs_stored_in_properties);

// Check delay_tcp_race.
EXPECT_TRUE(params->quic_delay_tcp_race);

// Check prefer_aes.
EXPECT_TRUE(params->quic_prefer_aes);

Expand Down
5 changes: 5 additions & 0 deletions ios/chrome/browser/ios_chrome_io_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ class IOSChromeIOThread : public web::WebThreadDelegate {
Optional<int> quic_max_number_of_lossy_connections;
Optional<float> quic_packet_loss_threshold;
Optional<int> quic_socket_receive_buffer_size;
Optional<bool> quic_delay_tcp_race;
Optional<size_t> quic_max_packet_length;
net::QuicTagVector quic_connection_options;
Optional<std::string> quic_user_agent_id;
Expand Down Expand Up @@ -318,6 +319,10 @@ class IOSChromeIOThread : public web::WebThreadDelegate {
static int GetQuicSocketReceiveBufferSize(
const VariationParameters& quic_trial_params);

// Returns true if QUIC should delay TCP connection when QUIC works.
static bool ShouldQuicDelayTcpRace(
const VariationParameters& quic_trial_params);

// Returns true if QUIC should close sessions when any of the client's
// IP addresses change.
static bool ShouldQuicCloseSessionsOnIpChange(
Expand Down
8 changes: 8 additions & 0 deletions ios/chrome/browser/ios_chrome_io_thread.mm
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,7 @@ void OnNetworkChanged(
&params->quic_packet_loss_threshold);
globals.quic_socket_receive_buffer_size.CopyToIfSet(
&params->quic_socket_receive_buffer_size);
globals.quic_delay_tcp_race.CopyToIfSet(&params->quic_delay_tcp_race);
params->enable_quic_port_selection = false;
globals.quic_max_packet_length.CopyToIfSet(&params->quic_max_packet_length);
globals.quic_user_agent_id.CopyToIfSet(&params->quic_user_agent_id);
Expand Down Expand Up @@ -768,6 +769,7 @@ void OnNetworkChanged(
if (receive_buffer_size != 0) {
globals->quic_socket_receive_buffer_size.set(receive_buffer_size);
}
globals->quic_delay_tcp_race.set(ShouldQuicDelayTcpRace(quic_trial_params));
float load_server_info_timeout_srtt_multiplier =
GetQuicLoadServerInfoTimeoutSrttMultiplier(quic_trial_params);
if (load_server_info_timeout_srtt_multiplier != 0) {
Expand Down Expand Up @@ -930,6 +932,12 @@ void OnNetworkChanged(
return 0;
}

bool IOSChromeIOThread::ShouldQuicDelayTcpRace(
const VariationParameters& quic_trial_params) {
return base::LowerCaseEqualsASCII(
GetVariationParam(quic_trial_params, "delay_tcp_race"), "true");
}

bool IOSChromeIOThread::ShouldQuicCloseSessionsOnIpChange(
const VariationParameters& quic_trial_params) {
return base::LowerCaseEqualsASCII(
Expand Down
4 changes: 3 additions & 1 deletion net/http/http_network_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ HttpNetworkSession::Params::Params()
quic_max_number_of_lossy_connections(0),
quic_packet_loss_threshold(1.0f),
quic_socket_receive_buffer_size(kQuicSocketReceiveBufferSize),
quic_delay_tcp_race(false),
quic_max_server_configs_stored_in_properties(0u),
quic_clock(NULL),
quic_random(NULL),
Expand Down Expand Up @@ -178,6 +179,7 @@ HttpNetworkSession::HttpNetworkSession(const Params& params)
params.quic_threshold_public_resets_post_handshake,
params.quic_threshold_timeouts_streams_open,
params.quic_socket_receive_buffer_size,
params.quic_delay_tcp_race,
params.quic_max_server_configs_stored_in_properties,
params.quic_close_sessions_on_ip_change,
params.disable_quic_on_timeout_with_open_streams,
Expand Down Expand Up @@ -331,7 +333,7 @@ std::unique_ptr<base::Value> HttpNetworkSession::QuicInfoToValue() const {
dict->SetInteger("max_number_of_lossy_connections",
params_.quic_max_number_of_lossy_connections);
dict->SetDouble("packet_loss_threshold", params_.quic_packet_loss_threshold);
dict->SetBoolean("delay_tcp_race", true);
dict->SetBoolean("delay_tcp_race", params_.quic_delay_tcp_race);
dict->SetInteger("max_server_configs_stored_in_properties",
params_.quic_max_server_configs_stored_in_properties);
dict->SetInteger("idle_connection_timeout_seconds",
Expand Down
3 changes: 3 additions & 0 deletions net/http/http_network_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ class NET_EXPORT HttpNetworkSession
float quic_packet_loss_threshold;
// Size in bytes of the QUIC DUP socket receive buffer.
int quic_socket_receive_buffer_size;
// Delay starting a TCP connection when QUIC believes it can speak
// 0-RTT to a server.
bool quic_delay_tcp_race;
// Maximum number of server configs that are to be stored in
// HttpServerProperties, instead of the disk cache.
size_t quic_max_server_configs_stored_in_properties;
Expand Down
4 changes: 2 additions & 2 deletions net/http/http_stream_factory_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1641,9 +1641,9 @@ TEST_P(HttpStreamFactoryBidirectionalQuicTest,
EXPECT_EQ(OK, stream_impl->ReadData(buffer.get(), 1));
EXPECT_EQ(kProtoQUIC1SPDY3, stream_impl->GetProtocol());
EXPECT_EQ("200", delegate.response_headers().find(":status")->second);
EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetTransportSocketPool(
EXPECT_EQ(1, GetSocketPoolGroupCount(session()->GetTransportSocketPool(
HttpNetworkSession::NORMAL_SOCKET_POOL)));
EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetSSLSocketPool(
EXPECT_EQ(1, GetSocketPoolGroupCount(session()->GetSSLSocketPool(
HttpNetworkSession::NORMAL_SOCKET_POOL)));
EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetTransportSocketPool(
HttpNetworkSession::WEBSOCKET_SOCKET_POOL)));
Expand Down
Loading

0 comments on commit b8e80fb

Please sign in to comment.