New issue
Advanced search Search tips

Issue 667471 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 682041



Sign in to add a comment

Allow at most one in-flight preconnect to proxy servers that support request priorities

Project Member Reported by tbansal@chromium.org, Nov 21 2016

Issue description

Currently, when a user starts a navigation, the //net/predictor may trigger multiple preconnects based on its previous learning. Each of these preconnects may open up a new connection to a proxy server (depending on proxy resolution). For the users with configured proxy servers, this causes Chromium to open up to 32 connections to the proxy server. These preconnections are useful for cases when the server does not support request priorities. However, for HTTP2 proxy servers, these preconnections (except the first one) are unnecessary since HTTP2 is designed to be used on only one transport layer connection.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0844a896c0334b3541315e79d1048789ea14cc71

commit 0844a896c0334b3541315e79d1048789ea14cc71
Author: tbansal <tbansal@chromium.org>
Date: Tue Dec 13 16:00:01 2016

Allow at most one preconnect to HTTP2 proxy servers

If Chromium is using a HTTP2 proxy server, then the preconnect jobs
are skipped if a preconnect request is already pending.

This prevents Chromium from unnecessarily establishing multiple
preconnections to an HTTP2 proxy. The new behavior is guarded
behind a field trial so that the performance benefits can be
evaluated.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG= 667471 

Review-Url: https://codereview.chromium.org/2522703002
Cr-Commit-Position: refs/heads/master@{#438179}

[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/0844a896c0334b3541315e79d1048789ea14cc71/tools/metrics/histograms/histograms.xml

Summary: Allow at most one in-flight preconnect to proxy servers that support request priorities (was: Preconnects should be disabled for data saver HTTP requests when QUIC is in use)
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7450edf914e2fedef46171ec5741d17b127df0f0

commit 7450edf914e2fedef46171ec5741d17b127df0f0
Author: tbansal <tbansal@chromium.org>
Date: Wed Dec 28 21:12:53 2016

Cleanup the preconnect to proxy code and Job controller code

(1) Add a boolean |restrict_to_one_preconnect_for_proxies| to network
session params. This allows us to run multiple experiments using a single
field trial. Next CL will add the variation param
|race_preconnects_to_proxies| to experiment with racing the alternative
and main jobs for proxy preconnects.

(2) Remove an extra parameter in
HttpStreamFactoryImpl::Job::Delegate::OnStreamReady() method.

BUG= 667471 ,671291
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2600943002
Cr-Commit-Position: refs/heads/master@{#440893}

[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_network_session.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_network_session.h
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/7450edf914e2fedef46171ec5741d17b127df0f0/net/spdy/spdy_test_util_common.cc

Description: Show this description
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37e33d58c076a55df84e0187538d6ec8996c206b

commit 37e33d58c076a55df84e0187538d6ec8996c206b
Author: tbansal <tbansal@chromium.org>
Date: Mon Jan 23 17:02:15 2017

Allow a preconnecting job to a HTTP2 proxy server based on privacy mode

Currently, one preconnect job is allowed in-flight for one proxy server.
This CL changes it to allow one in-flight preconnect job for one
(proxy server, privacy mode) tuple. This is needed since requests that
use different privacy modes cannot share the same connection.

BUG= 667471 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2648593002
Cr-Commit-Position: refs/heads/master@{#445392}

[modify] https://crrev.com/37e33d58c076a55df84e0187538d6ec8996c206b/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/37e33d58c076a55df84e0187538d6ec8996c206b/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/37e33d58c076a55df84e0187538d6ec8996c206b/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/37e33d58c076a55df84e0187538d6ec8996c206b/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/37e33d58c076a55df84e0187538d6ec8996c206b/net/http/http_stream_factory_impl_unittest.cc

Labels: Merge-Request-57
Requesting merge for the CL in Comment #6.
Project Member

Comment 8 by sheriffbot@chromium.org, Feb 7 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Approved -Merge-Approved-57
Labels: Merge-Request-57
Requesting merge for the CL in Comment #6.
Project Member

Comment 11 by sheriffbot@chromium.org, Feb 22 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 22 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9de7914b0e652585d4ac07303a949fbf1b19f09c

commit 9de7914b0e652585d4ac07303a949fbf1b19f09c
Author: Tarun Bansal <tbansal@google.com>
Date: Wed Feb 22 23:25:05 2017

Allow a preconnecting job to a HTTP2 proxy server based on privacy mode

Currently, one preconnect job is allowed in-flight for one proxy server.
This CL changes it to allow one in-flight preconnect job for one
(proxy server, privacy mode) tuple. This is needed since requests that
use different privacy modes cannot share the same connection.

BUG= 667471 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2648593002
Cr-Commit-Position: refs/heads/master@{#445392}
(cherry picked from commit 37e33d58c076a55df84e0187538d6ec8996c206b)

Review-Url: https://codereview.chromium.org/2707053006 .
Cr-Commit-Position: refs/branch-heads/2987@{#649}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/9de7914b0e652585d4ac07303a949fbf1b19f09c/net/http/http_stream_factory_impl.cc
[modify] https://crrev.com/9de7914b0e652585d4ac07303a949fbf1b19f09c/net/http/http_stream_factory_impl.h
[modify] https://crrev.com/9de7914b0e652585d4ac07303a949fbf1b19f09c/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/9de7914b0e652585d4ac07303a949fbf1b19f09c/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/9de7914b0e652585d4ac07303a949fbf1b19f09c/net/http/http_stream_factory_impl_unittest.cc

Labels: M-57
Blocking: 682041
Status: Fixed (was: Started)
Marking as fixed. The fix is behind a field trial. Issue 682041 will track the experiment.

Sign in to add a comment