New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: ----

Blocking:
issue 545123



Sign in to add a comment
link

Issue 545118: Add experimental configuration options to CronetEngine.Builder.

Reported by mef@chromium.org, Oct 19 2015 Project Member

Issue description

There are different experimental settings (e.g. Async DNS, Caching 5 MRU Server Configs) that need to be configurable by the application.

Currently CronetEngine.Builder has a method
setExperimentalQuicConnectionOptions(String quicConnectionOptions) specifically for QUIC experiments.

We should provide an API for safe and flexible configuration of other experiments, for example:

setExperimentalOptions(String experimentalOptions)

We could start with having experimentalOptions a comma-separated list of options, similar to quicConnectionOptions or come up with something more elaborate.
 

Comment 1 by mef@chromium.org, Oct 19 2015

Cc: rtenneti@chromium.org

Comment 2 by mef@chromium.org, Oct 19 2015

Blocking: chromium:545123

Comment 3 by xunji...@chromium.org, Oct 27 2015

Owner: xunji...@chromium.org
Status: Assigned
Looks like a crbug already exists here. Raman, what kind of plumbing is needed on the Cronet side to enable the experiment on caching 5 MRU Server Configs? Is there any relevant code that you could point me to?

Comment 4 by rtenneti@chromium.org, Oct 27 2015

An option is to do something similar to what we had done in io_thread.cc. API call accepts a  const std::map<std::string, std::string>& params which is a name, value pairs and we implement parsing of the value based on the name and set the value in Params.

GetVariationParam() and do something similar to the way we had set the Params there.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_thread.cc&rcl=1445945869&l=1363

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_thread.cc&l=257

Another option is to use protobuf and do the same.

Comment 5 by xunji...@chromium.org, Oct 27 2015

Talked to Raman offline. We need to pass field trial params using a Cronet API, parse those in the native code, and set appropriate HttpNetworkSession::Param.

Each field trial has its own map of key value pairs. So I propose the following format to encode the field trial in a string and pass it using setExperimentalOptions(String experimentalOptions) API.

["QUIC": {"enable_connection_racing": "true", "enable_non_blocking_io": "true", "disable_disk_cache": "true", ...}, fieldTrialB": {"foo":"bar", .. }]

It's a JSON array containing field trials keyed on trial names. Each trial has a JSON object representing a map of key-value pairs. Embedders can add their own trials and params in the above format, and we parse those we understand.

Any thought?

Comment 6 by mef@chromium.org, Oct 27 2015

Does it mean that Cronet will need to somehow choose between fieldTrialA and fieldTrialB?

Also, per our discussion, we would want to have CronetEngine.Builder.setExperimentalQuicConnectionOptions superseded by this method.

Comment 7 by xunji...@chromium.org, Oct 27 2015

I believe there can be several field trials going on at the same time, and Cronet does not need to choose a particular one. See kQuicFieldTrialName, kSpdyFieldTrialName, kNetworkQualityEstimatorFieldTrialName https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_thread.cc&rcl=1445945869&l=136

Comment 9 by bugdroid1@chromium.org, Nov 19 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fde0b72c603cd111c36ca4cc416d82a7395bcf6c

commit fde0b72c603cd111c36ca4cc416d82a7395bcf6c
Author: xunjieli <xunjieli@chromium.org>
Date: Thu Nov 19 00:38:10 2015

[Cronet] Add QUIC experimental params

This CL plumbs four QUIC experimental params (
quic_store_server_configs_in_properties,
quic_delay_tcp_race,
quic_max_number_of_lossy_connections,
quic_packet_loss_threshold) from Cronet's
setExperimentalOptions API to net::HttpNetworkSession.

This CL also adds a unittests target to run the unittests.
A followup CL will enable the unittests on the cronet bots.

BUG= 545118 

Review URL: https://codereview.chromium.org/1448583003

Cr-Commit-Position: refs/heads/master@{#360454}

[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/components_tests.gyp
[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/cronet.gypi
[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[add] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/cronet/url_request_context_config.cc
[add] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c/net/url_request/url_request_context_builder.h

Comment 10 by xunji...@chromium.org, Nov 19 2015

Status: Fixed
Marking this as fixed now. Will file a separate bug to enable unittests on build bots.

Comment 11 by bugdroid1@chromium.org, Nov 19 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d15ad369c19ff184a0cc633bf133a18440d6c13a

commit d15ad369c19ff184a0cc633bf133a18440d6c13a
Author: bruening <bruening@chromium.org>
Date: Thu Nov 19 03:10:02 2015

Revert of [Cronet] Add QUIC experimental params (patchset #7 id:140001 of https://codereview.chromium.org/1448583003/ )

Reason for revert:
[MemSheriff] Reverting CL that contains uninitialized reads that show up in multiple tests on MSan and Valgrind MFYI bots, e.g.: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests/builds/11562/steps/browser_tests/logs/ExtensionPreferenceApiTest.DataReductionProxy

Original issue's description:
> [Cronet] Add QUIC experimental params
>
> This CL plumbs four QUIC experimental params (
> quic_store_server_configs_in_properties,
> quic_delay_tcp_race,
> quic_max_number_of_lossy_connections,
> quic_packet_loss_threshold) from Cronet's
> setExperimentalOptions API to net::HttpNetworkSession.
>
> This CL also adds a unittests target to run the unittests.
> A followup CL will enable the unittests on the cronet bots.
>
> BUG= 545118 
>
> Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
> Cr-Commit-Position: refs/heads/master@{#360454}

TBR=rtenneti@google.com,rtenneti@chromium.org,mef@chromium.org,xunjieli@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 545118 

Review URL: https://codereview.chromium.org/1454333002

Cr-Commit-Position: refs/heads/master@{#360509}

[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/components/components_tests.gyp
[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/components/cronet.gypi
[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[delete] http://crrev.com/ba6f68ca9e93654011350088c8a986dd3d0938ac/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/components/cronet/url_request_context_config.cc
[delete] http://crrev.com/ba6f68ca9e93654011350088c8a986dd3d0938ac/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/d15ad369c19ff184a0cc633bf133a18440d6c13a/net/url_request/url_request_context_builder.h

Comment 12 by pauljensen@chromium.org, Nov 19 2015

Status: Started
Reopen as fix reverted

Comment 13 by xunji...@chromium.org, Nov 19 2015

I am working on a crasher now which I introduced yesterday, I will try landing the CL tomorrow. Sorry, Raman. Feel free to take over the CL if it's urgent. I believe I forgot to initialize the variables.

Comment 14 by bugdroid1@chromium.org, Nov 20 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ece3aa6845350c1971a3e824bf148f3e8de3253

commit 8ece3aa6845350c1971a3e824bf148f3e8de3253
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Nov 20 19:22:37 2015

[Cronet] Add QUIC experimental params

This CL plumbs four QUIC experimental params (
quic_store_server_configs_in_properties,
quic_delay_tcp_race,
quic_max_number_of_lossy_connections,
quic_packet_loss_threshold) from Cronet's
setExperimentalOptions API to net::HttpNetworkSession.

This CL also adds a unittests target to run the unittests.
A followup CL will enable the unittests on the cronet bots.

BUG= 545118 

Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
Cr-Commit-Position: refs/heads/master@{#360454}

Review URL: https://codereview.chromium.org/1448583003

Cr-Commit-Position: refs/heads/master@{#360875}

[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/components_tests.gyp
[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/cronet.gypi
[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[add] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/cronet/url_request_context_config.cc
[add] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253/net/url_request/url_request_context_builder.h

Comment 15 by bugdroid1@chromium.org, Nov 20 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/505f20a95df63e77c8207a7d5a19f5719f3dc3c6

commit 505f20a95df63e77c8207a7d5a19f5719f3dc3c6
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Nov 20 19:49:39 2015

Revert of [Cronet] Add QUIC experimental params (patchset #9 id:180001 of https://codereview.chromium.org/1448583003/ )

Reason for revert:
gyp dependency is missing. This broke one internal bot.

Original issue's description:
> [Cronet] Add QUIC experimental params
>
> This CL plumbs four QUIC experimental params (
> quic_store_server_configs_in_properties,
> quic_delay_tcp_race,
> quic_max_number_of_lossy_connections,
> quic_packet_loss_threshold) from Cronet's
> setExperimentalOptions API to net::HttpNetworkSession.
>
> This CL also adds a unittests target to run the unittests.
> A followup CL will enable the unittests on the cronet bots.
>
> BUG= 545118 
>
> Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
> Cr-Commit-Position: refs/heads/master@{#360454}
>
> Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253
> Cr-Commit-Position: refs/heads/master@{#360875}

TBR=rtenneti@google.com,rtenneti@chromium.org,mef@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 545118 

Review URL: https://codereview.chromium.org/1464903002

Cr-Commit-Position: refs/heads/master@{#360890}

[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/components/components_tests.gyp
[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/components/cronet.gypi
[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[delete] http://crrev.com/d9a11c0f75c929d2e7ef88f32b076213dfe78772/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/components/cronet/url_request_context_config.cc
[delete] http://crrev.com/d9a11c0f75c929d2e7ef88f32b076213dfe78772/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/505f20a95df63e77c8207a7d5a19f5719f3dc3c6/net/url_request/url_request_context_builder.h

Comment 16 by bugdroid1@chromium.org, Nov 23 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f24ee5f5afeb59d2a9134a7b986efb6483629fe5

commit f24ee5f5afeb59d2a9134a7b986efb6483629fe5
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Nov 23 18:05:26 2015

[Cronet] Add QUIC experimental params

This CL plumbs four QUIC experimental params (
quic_store_server_configs_in_properties,
quic_delay_tcp_race,
quic_max_number_of_lossy_connections,
quic_packet_loss_threshold) from Cronet's
setExperimentalOptions API to net::HttpNetworkSession.

This CL also adds a unittests target to run the unittests.
A followup CL will enable the unittests on the cronet bots.

BUG= 545118 

Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
Cr-Commit-Position: refs/heads/master@{#360454}

Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253
Cr-Commit-Position: refs/heads/master@{#360875}

Review URL: https://codereview.chromium.org/1448583003

Cr-Commit-Position: refs/heads/master@{#361138}

[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/components_tests.gyp
[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/cronet.gypi
[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[add] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/cronet/url_request_context_config.cc
[add] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5/net/url_request/url_request_context_builder.h

Comment 17 by xunji...@chromium.org, Nov 30 2015

Labels: Merge-Request-48
tinazh@: Can we have this CL merged into M48?

Comment 18 by tin...@google.com, Nov 30 2015

Labels: -Merge-Request-48 Merge-Approved-48 Hotlist-Merge-Approved
Congrats your change is auto-approved for M48 (branch: 2564)

Comment 19 by bugdroid1@chromium.org, Nov 30 2015

Project Member
Labels: -Merge-Approved-48 merge-merged-2564
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0499800c94b313392d7e30b976389da92ed04554

commit 0499800c94b313392d7e30b976389da92ed04554
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Nov 30 18:19:02 2015

[Cronet] Replace setExperimentalQuicConnectionOptions with a more general API

This CL replaces CronetEngine.Builder#setExperimentalQuicConnectionOptions with
a more general API to set experimental options.

BUG= 545118 

NOTRY=true
NOPRESUBMIT=true

Review URL: https://codereview.chromium.org/1414053008

Cr-Commit-Position: refs/heads/master@{#360187}
(cherry picked from commit 61b1eaaab7d5f38c76a3fd41d23a737fe177d211)

Review URL: https://codereview.chromium.org/1487723002

Cr-Commit-Position: refs/branch-heads/2564@{#160}
Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700}

[modify] http://crrev.com/0499800c94b313392d7e30b976389da92ed04554/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] http://crrev.com/0499800c94b313392d7e30b976389da92ed04554/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[modify] http://crrev.com/0499800c94b313392d7e30b976389da92ed04554/components/cronet/url_request_context_config.cc
[modify] http://crrev.com/0499800c94b313392d7e30b976389da92ed04554/components/cronet/url_request_context_config.h
[modify] http://crrev.com/0499800c94b313392d7e30b976389da92ed04554/components/cronet/url_request_context_config_list.h

Comment 20 by bugdroid1@chromium.org, Nov 30 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21daec09bda89713ab3865b66f66fb95c172bd55

commit 21daec09bda89713ab3865b66f66fb95c172bd55
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Nov 30 19:13:58 2015

[Cronet] Add QUIC experimental params

This CL plumbs four QUIC experimental params (
quic_store_server_configs_in_properties,
quic_delay_tcp_race,
quic_max_number_of_lossy_connections,
quic_packet_loss_threshold) from Cronet's
setExperimentalOptions API to net::HttpNetworkSession.

This CL also adds a unittests target to run the unittests.
A followup CL will enable the unittests on the cronet bots.

BUG= 545118 
NOTRY=true
NOPRESUBMIT=true

Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
Cr-Commit-Position: refs/heads/master@{#360454}

Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253
Cr-Commit-Position: refs/heads/master@{#360875}

Review URL: https://codereview.chromium.org/1448583003

Cr-Commit-Position: refs/heads/master@{#361138}
(cherry picked from commit f24ee5f5afeb59d2a9134a7b986efb6483629fe5)

Review URL: https://codereview.chromium.org/1483103002

Cr-Commit-Position: refs/branch-heads/2564@{#161}
Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700}

[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/components_tests.gyp
[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/cronet.gypi
[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[add] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/cronet/run_all_unittests.cc
[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/cronet/url_request_context_config.cc
[add] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/components/cronet/url_request_context_config_unittest.cc
[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/net/url_request/url_request_context_builder.cc
[modify] http://crrev.com/21daec09bda89713ab3865b66f66fb95c172bd55/net/url_request/url_request_context_builder.h

Comment 21 by xunji...@chromium.org, Nov 30 2015

Labels: Merge-Merged
Status: Fixed

Comment 22 by bugdroid1@chromium.org, Dec 1 2015

Project Member

Comment 23 by bugdroid1@chromium.org, Dec 1 2015

Project Member

Sign in to add a comment