Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928763002:
Move SpdySessionRequestMap to SpdySessionPool (Closed)
DescriptionMove 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}
Committed: https://chromium.googlesource.com/chromium/src/+/87258b768e4cf14f19dd7db4eb3ead725592e473
Patch Set 1 : self #
Total comments: 25
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address comment #
Messages
Total messages: 23 (16 generated)
Description was changed from ========== Move SpdySessionRequestMap to SpdySessionPool BUG= ========== to ========== Move SpdySessionRequestMap to SpdySessionPool This CL moves SpdySessonRequestMap to SpdySessionPool, because SpdySessionRequestMap is only relevant when HTTTP/2 is used. BUG=475060, 706974, 718576 ==========
xunjieli@chromium.org changed reviewers: + bnc@chromium.org
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move SpdySessionRequestMap to SpdySessionPool This CL moves SpdySessonRequestMap to SpdySessionPool, because SpdySessionRequestMap is only relevant when HTTTP/2 is used. BUG=475060, 706974, 718576 ========== to ========== 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 ==========
Bence, PTAL. Thank you.
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...
Couple of nits, many optional. https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:451: SpdySessionPool* spdy_session_pool = session_->spdy_session_pool(); I think there is no need to do this here, because |spdy_session| should still be valid below. https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:486: // Notify other requests that can use the same SpdySession. |request_| and Do you mean SpdySessionKey instead of SpdySession? That's the key of SpdySessionRequestMap. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... File net/spdy/chromium/spdy_session_pool.cc (right): https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:410: while (true) { Optional: while(spdy_session) { https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:421: if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key)) ContainsKey and operator[] below are two lookups, you could cut down on one of them by: iterator it = find(); if (it == end) return; https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:422: break; Optional: return instead of break might reflect our intention better. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:443: if (request->HasSpdySessionKey()) { Optional: early return instead of wrapping entire function body in if. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:445: DCHECK(base::ContainsKey(spdy_session_request_map_, spdy_session_key)); Optional: do only one lookup instead of two in debug build with find(). Optional because debug build performance is not critical. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:447: DCHECK(base::ContainsKey(request_set, request)); Optional: do only one lookup instead of two in debug build with find(). Optional because debug build performance is not critical. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:458: if (!request->HasSpdySessionKey()) { Optional: early return instead of wrapping entire function body in if. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:460: DCHECK(!base::ContainsKey(request_set, request)); Optional: cut down on lookups by auto result = insert(); DCHECK(result.second); https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... File net/spdy/chromium/spdy_session_pool.h (right): https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.h:182: void RemoveRequestFromSpdySessionRequestMap( Please add comments for all public methods. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.h:182: void RemoveRequestFromSpdySessionRequestMap( Optional: You could list "Add" before "Remove", that order might be more natural (here and also definition in .cc file).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, PTAL. https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:451: SpdySessionPool* spdy_session_pool = session_->spdy_session_pool(); On 2017/06/07 17:41:42, Bence wrote: > I think there is no need to do this here, because |spdy_session| should still be > valid below. |session_| (a member variable) might not be valid if |delegate_->OnStreamReady| deletes |this|. I not sure if it's possible. I kept it for consistency with the original code. https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:486: // Notify other requests that can use the same SpdySession. |request_| and On 2017/06/07 17:41:42, Bence wrote: > Do you mean SpdySessionKey instead of SpdySession? That's the key of > SpdySessionRequestMap. Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... File net/spdy/chromium/spdy_session_pool.cc (right): https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:410: while (true) { On 2017/06/07 17:41:43, Bence wrote: > Optional: while(spdy_session) { Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:421: if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key)) On 2017/06/07 17:41:43, Bence wrote: > ContainsKey and operator[] below are two lookups, you could cut down on one of > them by: > iterator it = find(); > if (it == end) > return; Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:422: break; On 2017/06/07 17:41:43, Bence wrote: > Optional: return instead of break might reflect our intention better. Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:443: if (request->HasSpdySessionKey()) { On 2017/06/07 17:41:43, Bence wrote: > Optional: early return instead of wrapping entire function body in if. Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:445: DCHECK(base::ContainsKey(spdy_session_request_map_, spdy_session_key)); On 2017/06/07 17:41:43, Bence wrote: > Optional: do only one lookup instead of two in debug build with find(). > Optional because debug build performance is not critical. Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:447: DCHECK(base::ContainsKey(request_set, request)); On 2017/06/07 17:41:43, Bence wrote: > Optional: do only one lookup instead of two in debug build with find(). > Optional because debug build performance is not critical. Acknowledged. One lookup is in |request_set| and the other is in |spdy_session_request_map_|. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:458: if (!request->HasSpdySessionKey()) { On 2017/06/07 17:41:43, Bence wrote: > Optional: early return instead of wrapping entire function body in if. Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.cc:460: DCHECK(!base::ContainsKey(request_set, request)); On 2017/06/07 17:41:42, Bence wrote: > Optional: cut down on lookups by > auto result = insert(); > DCHECK(result.second); Acknowledged. One lookup is in |request_set| and the other is in |spdy_session_request_map_|. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... File net/spdy/chromium/spdy_session_pool.h (right): https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.h:182: void RemoveRequestFromSpdySessionRequestMap( On 2017/06/07 17:41:43, Bence wrote: > Optional: You could list "Add" before "Remove", that order might be more natural > (here and also definition in .cc file). Done. https://codereview.chromium.org/2928763002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session_pool.h:182: void RemoveRequestFromSpdySessionRequestMap( On 2017/06/07 17:41:43, Bence wrote: > Please add comments for all public methods. Done.
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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Thank you. LGTM with one nit. https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2928763002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:451: SpdySessionPool* spdy_session_pool = session_->spdy_session_pool(); On 2017/06/07 21:55:55, xunjieli wrote: > On 2017/06/07 17:41:42, Bence wrote: > > I think there is no need to do this here, because |spdy_session| should still > be > > valid below. > > |session_| (a member variable) might not be valid if |delegate_->OnStreamReady| > deletes |this|. I not sure if it's possible. I kept it for consistency with the > original code. Oh sorry, I overlooked that. Thank you for clarifying. https://codereview.chromium.org/2928763002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2928763002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:486: // Notify other requests that has the same SpdySessionKey. |request_| and s/has/have/ (since "requests" is plural)
https://codereview.chromium.org/2928763002/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2928763002/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:486: // Notify other requests that has the same SpdySessionKey. |request_| and On 2017/06/07 23:09:53, Bence wrote: > s/has/have/ (since "requests" is plural) Done.
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/2928763002/#ps60001 (title: "address comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496925326708820,
"parent_rev": "678f9a59f4f2794ac47ec48a781e65cffb87ef09", "commit_rev":
"87258b768e4cf14f19dd7db4eb3ead725592e473"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/87258b768e4cf14f19dd7db4eb3e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/87258b768e4cf14f19dd7db4eb3e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
