Chrome still opens up to 6 TCP/SSL connections when HttpServerProperties knows that the server speaks H2 |
||||||||||||
Issue description[This issue is found in Cronet, but applies to Chrome as well.] It appears that we dutifully sets HttpServerProperties::SetSupportsSpdy(), but we never uses this information other than for preconnects, and in content/browser/loader/resource_scheduler.cc (which uses SupportsRequestPriority() that calls GetSupportsSpdy() indirectly). This leads to unnecessary resource usage in negotiating the extra TCP/SSL connections (up to 5 for each host that speaks H2 if there isn't an existing H2 connection).
,
May 4 2017
Spot https://codereview.chromium.org/7827033 which introduces the method SupportsSpdy(which was renamed to GetSupportsSpdy). It doesn't seem to be a regression.
,
May 4 2017
Do I the network stack does the right thing with preconnects, and Chrome does the right thing for real requests because of resource_scheduler? But cronet doesn't have the resource_scheduler, so is this really a cronet problem but not a Chrome problem?
,
May 5 2017
It is a Chrome problem. (1) resource_scheduler.cc only deal with renderer-initiated requests. (2) resource_scheduler.cc didn't not throttle low-priority H2/QUIC requests until Issue 655585. Even with Issue 655585, we only throttle *low-priority* H2 requests. If we have a bunch of high priority H2 requests going to the same host, I think we will still have the same issue. +jkarlin to confirm
,
May 5 2017
> (1) resource_scheduler.cc only deal with renderer-initiated requests. Good point :) Presumably, that's the majority of requests? > (2) If we have a bunch of high priority H2 requests going to the same host, I think we will still have the same issue. Ah, I see. Also a good point! :)
,
May 5 2017
Sorry, I'm not following. The resource scheduler just limits the number of parallel requests per origin and per page load. Not sure what that has to do with there being 6 H2 sockets. Seems like lower in the net stack preconnects for H2 need to be ignored if an H2 connection already exists for the origin?
,
May 5 2017
Re#6. I think Ryan was trying to see how big an impact this has, given that resource scheduler already throttles some H2 requests going to the same origin. Yes, the fix will be in HttpStreamFactory/FactoryImpl::Job layer. We limit preconnects. We need to do something similar for non-preconnects.
,
May 17 2017
The design doc is at https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMSOo/edit I have sent out to relevant folks and net-dev@ for review. I am planning to send a CL out next week.
,
May 18 2017
,
May 22 2017
M60 branch cut is this week, so target this to M61. Also change this to Type=Feature.
,
Jun 7 2017
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/253a62d89689b5a61846a38d48037d0ea719f905 commit 253a62d89689b5a61846a38d48037d0ea719f905 Author: xunjieli <xunjieli@chromium.org> Date: Wed Jun 07 16:25:20 2017 Make SpdySessionKey a const member of HttpStreamFactoryImpl::Job SpdySessionKey creation depends on proxy resolution result. Since proxy resolution is now done before Job is created, this allows us to make SpdySessionKey a const member of HttpStreamFactoryImpl::Job. BUG=475060, 706974 , 718576 Review-Url: https://codereview.chromium.org/2925093002 Cr-Commit-Position: refs/heads/master@{#477674} [modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job.h [modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job_controller_unittest.cc
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87258b768e4cf14f19dd7db4eb3ead725592e473 commit 87258b768e4cf14f19dd7db4eb3ead725592e473 Author: xunjieli <xunjieli@chromium.org> Date: Thu Jun 08 14:19:52 2017 Move SpdySessionRequestMap to SpdySessionPool This CL moves SpdySessonRequestMap from HttpStreamFactoryImpl to SpdySessionPool, because SpdySessionRequestMap is only relevant when HTTTP/2 is used. BUG=475060, 706974 , 718576 Review-Url: https://codereview.chromium.org/2928763002 Cr-Commit-Position: refs/heads/master@{#477958} [modify] https://crrev.com/87258b768e4cf14f19dd7db4eb3ead725592e473/net/http/http_stream_factory_impl.cc [modify] https://crrev.com/87258b768e4cf14f19dd7db4eb3ead725592e473/net/http/http_stream_factory_impl.h [modify] https://crrev.com/87258b768e4cf14f19dd7db4eb3ead725592e473/net/http/http_stream_factory_impl_job_controller.cc [modify] https://crrev.com/87258b768e4cf14f19dd7db4eb3ead725592e473/net/spdy/chromium/spdy_session_pool.cc [modify] https://crrev.com/87258b768e4cf14f19dd7db4eb3ead725592e473/net/spdy/chromium/spdy_session_pool.h
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a99720924b64953e3ea601cee5086b71f1a33950 commit a99720924b64953e3ea601cee5086b71f1a33950 Author: xunjieli <xunjieli@chromium.org> Date: Thu Jun 15 15:57:05 2017 Throttle HttpStreamFactoryImpl::Job to HTTP/2-supported servers If there are multiple HttpStreamFactoryImpl::Jobs to the same HTTP/2- supported server, throttle the socket connection establishments to allow the first one to go through and then the rest. This is to reduce the number of unnecessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMSOo/edit?usp=sharing BUG= 718576 Review-Url: https://codereview.chromium.org/2932513004 Cr-Commit-Position: refs/heads/master@{#479721} [modify] https://crrev.com/a99720924b64953e3ea601cee5086b71f1a33950/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/a99720924b64953e3ea601cee5086b71f1a33950/net/http/http_stream_factory_impl_job.h [modify] https://crrev.com/a99720924b64953e3ea601cee5086b71f1a33950/net/http/http_stream_factory_impl_job_controller_unittest.cc [modify] https://crrev.com/a99720924b64953e3ea601cee5086b71f1a33950/net/spdy/chromium/spdy_session_pool.cc [modify] https://crrev.com/a99720924b64953e3ea601cee5086b71f1a33950/net/spdy/chromium/spdy_session_pool.h
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16dc7c64578e96b27565f3a66f6134c817998274 commit 16dc7c64578e96b27565f3a66f6134c817998274 Author: xunjieli <xunjieli@chromium.org> Date: Thu Jun 15 20:47:38 2017 Add NetLog events for HttpStreamFactoryImpl::Job throttle logic This CL adds two NetLog events for HttpStreamFactoryImpl::Job throttle logic. (1) HTTP_STREAM_JOB_THROTTLED: emitted when Job is throttled. (2) HTTP_STREAM_JOB_RESUME_INIT_CONNECTION: emitted when Job is unblocked from being previously throttled. BUG= 718576 Review-Url: https://codereview.chromium.org/2944443002 Cr-Commit-Position: refs/heads/master@{#479820} [modify] https://crrev.com/16dc7c64578e96b27565f3a66f6134c817998274/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/16dc7c64578e96b27565f3a66f6134c817998274/net/log/net_log_event_type_list.h
,
Jun 16 2017
,
Jun 16 2017
Re-opening. Need one more patch to restrict the throttling logic to non-QUIC jobs.
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb70779863bf9dfe419601a6de536c336feaf591 commit cb70779863bf9dfe419601a6de536c336feaf591 Author: xunjieli <xunjieli@chromium.org> Date: Wed Jun 21 19:12:07 2017 Add NetLog events for HttpServerPropertiesManager This CL adds NetLog events for HttpServerPropertiesManager. (1) HTTP_SERVER_PROPERTIES_INITIALIZATION This is emitted when initialization starts and finishes. This is useful to know if server information is used in making network requests. (2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE This is emitted when we are populating the in-memory HttpServerProperties from the on-disk version. (2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS This is emitted when we are persisting the in-memory HttpServerProperties to disk. A sample NetLog: https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/view?usp=sharing BUG= 718576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2944043002 Cr-Commit-Position: refs/heads/master@{#481267} [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/chrome/browser/net/http_server_properties_manager_factory.h [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/chrome/browser/profiles/profile_impl_io_data.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/components/cronet/android/cronet_url_request_context_adapter.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/ios/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/ios/chrome/browser/net/http_server_properties_manager_factory.h [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/net/http/http_server_properties_manager.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/net/http/http_server_properties_manager.h [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/net/http/http_server_properties_manager_unittest.cc [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/net/log/net_log_event_type_list.h [modify] https://crrev.com/cb70779863bf9dfe419601a6de536c336feaf591/net/log/net_log_source_type_list.h
,
Jun 21 2017
r479721 used 300ms timeout for the throttling logic. It should work relatively well for good networks. tbansal@ is working on tuning proxy connect timeout ( Issue 704339 ) based on Network Quality Estimator's RTT estimates. He is planning to do it for non-proxy TCP connect timeout as well. We can revisit this hardcoded timeout value once tbansal@'s work is done.
,
Jun 21 2017
,
Aug 16 2017
Reminder: There are still TODOs associated with this bug.
,
Aug 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9eee0e533dbc2e7ebd47158a86e0584dbdfe63d5 commit 9eee0e533dbc2e7ebd47158a86e0584dbdfe63d5 Author: Helen Li <xunjieli@chromium.org> Date: Wed Aug 16 15:45:51 2017 Convert CHECKs to DCHECKs in http_stream_factory The CHECKs are no longer needed. This CL converts them to DCHECKs Bug: 718576 Change-Id: I61b1705100b77c5329b78100eba70197686de9d7 Reviewed-on: https://chromium-review.googlesource.com/616707 Reviewed-by: Bence Béky <bnc@chromium.org> Commit-Queue: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#494798} [modify] https://crrev.com/9eee0e533dbc2e7ebd47158a86e0584dbdfe63d5/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/9eee0e533dbc2e7ebd47158a86e0584dbdfe63d5/net/http/http_stream_factory_impl_job_controller.cc
,
Aug 25 2017
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by xunji...@chromium.org
, May 4 2017