Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932513004:
Throttle HttpStreamFactoryImpl::Job to HTTP/2-supported servers (Closed)
DescriptionThrottle 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}
Committed: https://chromium.googlesource.com/chromium/src/+/a99720924b64953e3ea601cee5086b71f1a33950
Patch Set 1 : self #
Total comments: 56
Patch Set 2 : address comments #
Total comments: 1
Patch Set 3 : self #
Total comments: 2
Patch Set 4 : fix tests #Patch Set 5 : fix tests #Patch Set 6 : add comment #Patch Set 7 : fix flaky test #Patch Set 8 : fix compile #Patch Set 9 : fix test on windows for real #Patch Set 10 : rebased #
Total comments: 2
Patch Set 11 : Add a comment #Patch Set 12 : add a pair of braces #
Dependent Patchsets: Messages
Total messages: 52 (40 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== temp not for review BUG= ========== to ========== Throttle HttpStreamFactoryImpl::Job to H2-supported servers If there are multiple HttpStreamFactoryImpl::Jobs to the same HTTP/2- supported server, throttle the socket connection establishment to allow one at a time. This is to reduce the number of unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ==========
Description was changed from ========== Throttle HttpStreamFactoryImpl::Job to H2-supported servers If there are multiple HttpStreamFactoryImpl::Jobs to the same HTTP/2- supported server, throttle the socket connection establishment to allow one at a time. This is to reduce the number of unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ========== to ========== Throttle HttpStreamFactoryImpl::Job to H2-supported servers If there are multiple HttpStreamFactoryImpl::Jobs to the same HTTP/2- supported server, throttle the socket connection establishments to allow one at a time. This is to reduce the number of unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xunjieli@chromium.org changed reviewers: + bnc@chromium.org
Patchset #2 (id:160001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Looking really good! https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:789: if (!session_->http_server_properties()->RequiresHTTP11( Please invert this condition and do an early return. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:795: if (session_->http_server_properties()->GetSupportsSpdy(scheme_host_port)) { Please invert this condition and do an early return. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:802: FROM_HERE, callback, base::TimeDelta::FromMilliseconds(300)); Please define a file-scope const int at the top of this file instead of hardcoding 300 here. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:811: if (resumed_init_connection_) Under what circumstances can this be called multiple times? In other words, why is this member necessary at all? https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:487: // Whether the Job should continue to DoInitConnection(). Isn't it rather whether the Job has already continued to DoInitConnection() so that it should not try to do it again? https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:488: bool resumed_init_connection_; Optional: init_connection_already_resumed_. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:165: random_generator_(0), Please assign default value at member declaration below as with all the other members. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:230: if (!skip_create_job_controller_) { This feels like a double negation. It might be less confusing to invert value and rename to |create_job_controller_|, true by default. (I prefer to avoid |disable|, |do_not|, and similar negations in variable names for this reason.) Then the setter could be called SkipCreatingJobController() or DoNotCreateJobController(). https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1590: void SetUp() override { SetSkipCreateJobController(); } Why not set |skip_create_job_controller_| directly, since it's protected anyway? Or if going down this route, maybe make |skip_create_job_controller_| private. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1594: // Make sure there are only one socket connect. s/are/is/ https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1613: std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates; Could you use std::vector<MockHttpStreamRequestDelegate> here? emplace_back() with no arguments below. Same in other tests below. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1614: std::vector<std::unique_ptr<HttpStreamRequest>> requests; I understand that this needs to use unique_ptr because that's what JobController::Start returns. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1616: request_delegates.emplace_back( I think a simple push_back() would be enough here. MakeUnique already constructs the unique_ptr instance, so a move is required even when using emplace_back() (it will be the move constructor). I don't think there is a performance benefit. See also comments at https://chromium-cpp.appspot.com/. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1648: std::vector<std::unique_ptr<SequencedSocketData>> socket_data; Hm, could you not just use std::vector<SequencedSocketData>, emplace_back() below, and access it through vector::back()? Not sure if that would hurt readability, but I usually try to avoid extra allocations and indirections. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1649: std::vector<std::unique_ptr<SSLSocketDataProvider>> ssl_socket_data; Same here. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1670: SetSkipCreateJobController(); Please remove this call, as SetUp() already takes care of it. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1699: // Delete |request[i] when request completes is needed because Nit: unmatched '|'. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1699: // Delete |request[i] when request completes is needed because s/request/requests/ https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1707: test_task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(300)); Do you not need to use a larger time here to make sure callbacks are actually completed? I am worried about off-by-one errors due to rounding of internal representations, or due to inconsistent use of < vs. <= in internal implementation. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1735: SetSkipCreateJobController(); Please remove this call, as SetUp() already takes care of it. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1776: // Make sure there are only one socket connect. s/are/is. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1788: pool_peer.SetEnableSendingInitialData(false); Turns out these two lines are not necessary for this test (but it is for the other ones), please remove them. https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... File net/spdy/chromium/spdy_session_pool.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.cc:471: iter->second.pop_front(); Do not dereference |iter| after it's erased. How about: base::Closure callback = iter->second.front(); iter->second.pop_front(); if (iter->second.empty()) spdy_session_pending_request_map_.erase(iter); https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... File net/spdy/chromium/spdy_session_pool.h (right): https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.h:185: bool StartRequest(const SpdySessionKey& spdy_session_key, Optional: return OK instead of true and discard |callback|, or ERR_IO_PENDING instead of false and call |callback| later, just like it's done elsewhere in the network stack. https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.h:191: void ResumeNextPendingRequest(const SpdySessionKey& spdy_session_key); Do you think it is worth making this method private, or inlining it, since there is only one call site?
BTW what is happening with HttpNetworkTransactionTest.AlternateProtocolWithSpdyLateBinding? I would expect this change to make it fail, but it does not, do you know why? https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1587: class LimitMultipleH2Requests : public HttpStreamFactoryImplJobControllerTest { Optional: add a test case where the server is thought to support HTTP/2 but in fact negotiates HTTP/1.1, so the second request does not bind to the first socket when resumed.
rch@chromium.org changed reviewers: + rch@chromium.org
nice! one small drive-by https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:272: STATE_BEFORE_INIT_CONNECTION, nit: Instead of "before init connection" there there a term that might be more self-explanatory? Maybe something about "throttling" or "evaluate_throttle"?
Description was changed from ========== Throttle HttpStreamFactoryImpl::Job to H2-supported servers If there are multiple HttpStreamFactoryImpl::Jobs to the same HTTP/2- supported server, throttle the socket connection establishments to allow one at a time. This is to reduce the number of unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ========== to ========== Throttle HttpStreamFactoryImpl::Job to H2-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 unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ==========
Thanks! PTAL. > BTW what is happening with HttpNetworkTransactionTest.AlternateProtocolWithSpdyLateBinding? I would expect this change to make it fail, but it does not, do you know why? This change doesn't affect AlternateProtocolWithSpdyLateBinding because: When the alternative job completes in the second transaction, SpdySessionPool::OnNewSpdySessionReady() will mark all pending requests as complete. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:789: if (!session_->http_server_properties()->RequiresHTTP11( On 2017/06/09 19:17:38, Bence wrote: > Please invert this condition and do an early return. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:795: if (session_->http_server_properties()->GetSupportsSpdy(scheme_host_port)) { On 2017/06/09 19:17:38, Bence wrote: > Please invert this condition and do an early return. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:802: FROM_HERE, callback, base::TimeDelta::FromMilliseconds(300)); On 2017/06/09 19:17:38, Bence wrote: > Please define a file-scope const int at the top of this file instead of > hardcoding 300 here. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:811: if (resumed_init_connection_) On 2017/06/09 19:17:38, Bence wrote: > Under what circumstances can this be called multiple times? In other words, why > is this member necessary at all? Assuming there are 2 Jobs (A and B) issued at the same time to the same H2-supported server. Job B is throttled because A went first, so B posts a delayed task to resume itself. When the Job A completes, Job A posts a task to resume B (through SpdySessionPool). The boolean makes sure that when the delayed task run, it doesn't make the Job B run DoLoop() again. One the other hand, if B already has resumed itself (and is happily doing connection establishment), when A completes, SpdySession doesn't try to make B do DoLoop() because B might be in the middle of DoInitConnection() or is in STATE_DONE. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:272: STATE_BEFORE_INIT_CONNECTION, On 2017/06/12 13:30:49, Ryan Hamilton wrote: > nit: Instead of "before init connection" there there a term that might be more > self-explanatory? Maybe something about "throttling" or "evaluate_throttle"? Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:487: // Whether the Job should continue to DoInitConnection(). On 2017/06/09 19:17:38, Bence wrote: > Isn't it rather whether the Job has already continued to DoInitConnection() so > that it should not try to do it again? Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:488: bool resumed_init_connection_; On 2017/06/09 19:17:38, Bence wrote: > Optional: init_connection_already_resumed_. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:165: random_generator_(0), On 2017/06/09 19:17:39, Bence wrote: > Please assign default value at member declaration below as with all the other > members. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:230: if (!skip_create_job_controller_) { On 2017/06/09 19:17:39, Bence wrote: > This feels like a double negation. It might be less confusing to invert value > and rename to |create_job_controller_|, true by default. (I prefer to avoid > |disable|, |do_not|, and similar negations in variable names for this reason.) > Then the setter could be called SkipCreatingJobController() or > DoNotCreateJobController(). Done. Good idea. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1587: class LimitMultipleH2Requests : public HttpStreamFactoryImplJobControllerTest { On 2017/06/09 23:14:02, Bence wrote: > Optional: add a test case where the server is thought to support HTTP/2 but in > fact negotiates HTTP/1.1, so the second request does not bind to the first > socket when resumed. Good idea. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1590: void SetUp() override { SetSkipCreateJobController(); } On 2017/06/09 19:17:39, Bence wrote: > Why not set |skip_create_job_controller_| directly, since it's protected anyway? > Or if going down this route, maybe make |skip_create_job_controller_| private. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1594: // Make sure there are only one socket connect. On 2017/06/09 19:17:39, Bence wrote: > s/are/is/ Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1613: std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates; On 2017/06/09 19:17:39, Bence wrote: > Could you use std::vector<MockHttpStreamRequestDelegate> here? emplace_back() > with no arguments below. > > Same in other tests below. Acknowledged. MockHttpStreamRequestDelegate has no copy constructor. std::vector requires type T to have a copy constructor. https://stackoverflow.com/questions/40457302/c-vector-emplace-back-calls-copy... https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1614: std::vector<std::unique_ptr<HttpStreamRequest>> requests; On 2017/06/09 19:17:39, Bence wrote: > I understand that this needs to use unique_ptr because that's what > JobController::Start returns. Acknowledged. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1616: request_delegates.emplace_back( On 2017/06/09 19:17:39, Bence wrote: > I think a simple push_back() would be enough here. MakeUnique already > constructs the unique_ptr instance, so a move is required even when using > emplace_back() (it will be the move constructor). I don't think there is a > performance benefit. See also comments at https://chromium-cpp.appspot.com/. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1648: std::vector<std::unique_ptr<SequencedSocketData>> socket_data; On 2017/06/09 19:17:39, Bence wrote: > Hm, could you not just use std::vector<SequencedSocketData>, emplace_back() > below, and access it through vector::back()? Not sure if that would hurt > readability, but I usually try to avoid extra allocations and indirections. Same here. SequencedSocketData has DISALLOW_COPY_AND_ASSIGN so the copy constructor is deleted. std::vector<> requires the type T to have a copy constructor. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1649: std::vector<std::unique_ptr<SSLSocketDataProvider>> ssl_socket_data; On 2017/06/09 19:17:39, Bence wrote: > Same here. Acknowledged. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1670: SetSkipCreateJobController(); On 2017/06/09 19:17:39, Bence wrote: > Please remove this call, as SetUp() already takes care of it. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1699: // Delete |request[i] when request completes is needed because On 2017/06/09 19:17:39, Bence wrote: > Nit: unmatched '|'. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1699: // Delete |request[i] when request completes is needed because On 2017/06/09 19:17:39, Bence wrote: > s/request/requests/ Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1707: test_task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(300)); On 2017/06/09 19:17:39, Bence wrote: > Do you not need to use a larger time here to make sure callbacks are actually > completed? I am worried about off-by-one errors due to rounding of internal > representations, or due to inconsistent use of < vs. <= in internal > implementation. Probably not. The delayed task is posted when |job_controller| is started, so we are sure that the delayed task is run if we fast forward the test task runner by 300ms. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1735: SetSkipCreateJobController(); On 2017/06/09 19:17:39, Bence wrote: > Please remove this call, as SetUp() already takes care of it. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1776: // Make sure there are only one socket connect. On 2017/06/09 19:17:38, Bence wrote: > s/are/is. Done. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1788: pool_peer.SetEnableSendingInitialData(false); On 2017/06/09 19:17:39, Bence wrote: > Turns out these two lines are not necessary for this test (but it is for the > other ones), please remove them. Done. Good catch. https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... File net/spdy/chromium/spdy_session_pool.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.cc:471: iter->second.pop_front(); On 2017/06/09 19:17:39, Bence wrote: > Do not dereference |iter| after it's erased. How about: > > base::Closure callback = iter->second.front(); > iter->second.pop_front(); > if (iter->second.empty()) > spdy_session_pending_request_map_.erase(iter); Done. Thanks! https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... File net/spdy/chromium/spdy_session_pool.h (right): https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.h:185: bool StartRequest(const SpdySessionKey& spdy_session_key, On 2017/06/09 19:17:40, Bence wrote: > Optional: return OK instead of true and discard |callback|, or ERR_IO_PENDING > instead of false and call |callback| later, just like it's done elsewhere in the > network stack. Acknowledged. But the callback is not a net::CompletionCallback. I will keep it as it is so it's easier to think about the return value. Change it to return a net_error code, the caller might add error handling code for non-OK and non-ERR_IO_PENDING. https://codereview.chromium.org/2932513004/diff/170001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.h:191: void ResumeNextPendingRequest(const SpdySessionKey& spdy_session_key); On 2017/06/09 19:17:40, Bence wrote: > Do you think it is worth making this method private, or inlining it, since there > is only one call site? Done. https://codereview.chromium.org/2932513004/diff/190001/net/spdy/chromium/spdy... File net/spdy/chromium/spdy_session_pool.cc (right): https://codereview.chromium.org/2932513004/diff/190001/net/spdy/chromium/spdy... net/spdy/chromium/spdy_session_pool.cc:491: ResumeAllPendingRequests(spdy_session_key); I remove ResumeNextPendingRequest() to make the logic simpler and easier to refactor later. For any failed or canceled Request, RemoveRequestFromSpdySessionRequestMap() will be called. When that is called, we just open the floodgate to allow throttled Jobs to go through (rather than the first one in the previous patch). For preconnects, since they do not have a Request object, we open the floodgate when Job::RunLoop() completes.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, the new patch affected a couple SpdyNetworkTransactionTest. Let me figure out what's wrong and I will update the thread when the CL is ready for another round of review.
One more nit. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:811: if (resumed_init_connection_) On 2017/06/13 15:23:10, xunjieli wrote: > On 2017/06/09 19:17:38, Bence wrote: > > Under what circumstances can this be called multiple times? In other words, > why > > is this member necessary at all? > > Assuming there are 2 Jobs (A and B) issued at the same time to the same > H2-supported server. Job B is throttled because A went first, so B posts a > delayed task to resume itself. When the Job A completes, Job A posts a task to > resume B (through SpdySessionPool). > The boolean makes sure that when the delayed task run, it doesn't make the Job B > run DoLoop() again. > > One the other hand, if B already has resumed itself (and is happily doing > connection establishment), when A completes, SpdySession doesn't try to make B > do DoLoop() because B might be in the middle of DoInitConnection() or is in > STATE_DONE. Thanks for the explanation. https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/170001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1707: test_task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(300)); On 2017/06/13 15:23:11, xunjieli wrote: > On 2017/06/09 19:17:39, Bence wrote: > > Do you not need to use a larger time here to make sure callbacks are actually > > completed? I am worried about off-by-one errors due to rounding of internal > > representations, or due to inconsistent use of < vs. <= in internal > > implementation. > > Probably not. The delayed task is posted when |job_controller| is started, so we > are sure that the delayed task is run if we fast forward the test task runner by > 300ms. Acknowledged. https://codereview.chromium.org/2932513004/diff/210001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/210001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1781: SpdySessionPoolPeer pool_peer(session_->spdy_session_pool()); |pool_peer| is unused.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Throttle HttpStreamFactoryImpl::Job to H2-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 unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ========== to ========== 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 unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The tests are all passing now. PTAL. Thank you! https://codereview.chromium.org/2932513004/diff/210001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2932513004/diff/210001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:1781: SpdySessionPoolPeer pool_peer(session_->spdy_session_pool()); On 2017/06/13 16:04:51, Bence wrote: > |pool_peer| is unused. Done.
LGTM, great! https://codereview.chromium.org/2932513004/diff/350001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2932513004/diff/350001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:812: CHECK_EQ(next_state_, STATE_INIT_CONNECTION); Very optional: a DCHECK should be enough here.
Thank you. I will land this now and work on adding NetLog events for this throttling logic so if users run into issues we can see if this is the cause. https://codereview.chromium.org/2932513004/diff/350001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2932513004/diff/350001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:812: CHECK_EQ(next_state_, STATE_INIT_CONNECTION); On 2017/06/15 13:37:46, Bence wrote: > Very optional: a DCHECK should be enough here. Thanks. I add a TODO to change it back to DCHECK. It seems that Canary channel doesn't have DCHECKs enabled. In case we have a bug in this CL, it might be easier to spot.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org Link to the patchset: https://codereview.chromium.org/2932513004/#ps390001 (title: "add a pair of braces")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 unncessary HTTP/2 socket connections. Design doc: https://docs.google.com/document/d/1XeAOPduGVv95_U371bmlTgg8Parp_D3QK9aEeDoMS... BUG=718576 ========== to ========== 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_D3QK9aEeDoMS... BUG=718576 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 390001, "attempt_start_ts": 1497537685971470,
"parent_rev": "2acdcced51403c0b6d0ed515f0e86d432a901839", "commit_rev":
"a99720924b64953e3ea601cee5086b71f1a33950"}
Message was sent while issue was closed.
Description was changed from ========== 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_D3QK9aEeDoMS... BUG=718576 ========== to ========== 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_D3QK9aEeDoMS... BUG=718576 Review-Url: https://codereview.chromium.org/2932513004 Cr-Commit-Position: refs/heads/master@{#479721} Committed: https://chromium.googlesource.com/chromium/src/+/a99720924b64953e3ea601cee508... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:390001) as https://chromium.googlesource.com/chromium/src/+/a99720924b64953e3ea601cee508... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
