Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811573003:
Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job (Closed)
|
Created:
3 years, 8 months ago by Zhongyi Shi Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLink NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job
When HttpStreamFactoryImpl::Job init connection to QUIC server, in some
cases, it might not result in QuicStreamFactory creating a new
QuicStreamFactory::Job to serve the QuicRequest but use an exisitng
QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and
QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection
is traceable.
BUG=702411
Review-Url: https://codereview.chromium.org/2811573003
Cr-Commit-Position: refs/heads/master@{#463536}
Committed: https://chromium.googlesource.com/chromium/src/+/011ef8d97d0b06877ca4cfc621d2e56f19aeef8c
Patch Set 1 #
Total comments: 4
Patch Set 2 : reduce two map lookups to 1 #Patch Set 3 : reduce one more lookup #Patch Set 4 : add a todo #
Total comments: 2
Patch Set 5 : fix a typo #
Total comments: 4
Patch Set 6 : address #14 #Messages
Total messages: 23 (8 generated)
Description was changed from ========== Link HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not results in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traciable. BUG=702411 ========== to ========== Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not results in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traciable. BUG=702411 ==========
zhongyi@chromium.org changed reviewers: + eroman@chromium.org, xunjieli@chromium.org
https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... net/quic/chromium/quic_stream_factory.cc:1012: if (active_jobs_[server_id].size() == 1) { nit: There are 3 instances of active_jobs_[server_id], and technically HasActiveJob() is also doing a lookup by key. Should we reduce the number of map lookups for efficiency?
Thanks Eric, ptal! https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... net/quic/chromium/quic_stream_factory.cc:1012: if (active_jobs_[server_id].size() == 1) { On 2017/04/10 21:55:07, eroman wrote: > nit: There are 3 instances of active_jobs_[server_id], and technically > HasActiveJob() is also doing a lookup by key. Should we reduce the number of map > lookups for efficiency? Yeah, we could reduce the 2 lookups to 1. However, as c++ implementation for [] access in map would introduce an entry creation, we use ContainsKey before accessing the map.
https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... net/quic/chromium/quic_stream_factory.cc:1012: if (active_jobs_[server_id].size() == 1) { On 2017/04/10 23:50:49, Zhongyi Shi wrote: > On 2017/04/10 21:55:07, eroman wrote: > > nit: There are 3 instances of active_jobs_[server_id], and technically > > HasActiveJob() is also doing a lookup by key. Should we reduce the number of > map > > lookups for efficiency? > > Yeah, we could reduce the 2 lookups to 1. However, as c++ implementation for [] > access in map would introduce an entry creation, we use ContainsKey before > accessing the map. I agreed with Eric. I landed a CL last week to reduce map lookups in this class (https://codereview.chromium.org/2797633003/). Let's not add more :) Can we use find and check the iterator? I will try out the new logging code first thing tomorrow morning.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/1/net/quic/chromium/quic_stre... net/quic/chromium/quic_stream_factory.cc:1012: if (active_jobs_[server_id].size() == 1) { On 2017/04/10 23:56:24, xunjieli wrote: > On 2017/04/10 23:50:49, Zhongyi Shi wrote: > > On 2017/04/10 21:55:07, eroman wrote: > > > nit: There are 3 instances of active_jobs_[server_id], and technically > > > HasActiveJob() is also doing a lookup by key. Should we reduce the number of > > map > > > lookups for efficiency? > > > > Yeah, we could reduce the 2 lookups to 1. However, as c++ implementation for > [] > > access in map would introduce an entry creation, we use ContainsKey before > > accessing the map. > > I agreed with Eric. I landed a CL last week to reduce map lookups in this class > (https://codereview.chromium.org/2797633003/). Let's not add more :) > > Can we use find and check the iterator? Done. > I will try out the new logging code first thing tomorrow morning. Sorry, which logging code you mean? I guess you're referring to the other CL(https://codereview.chromium.org/2809453002/) which is not related to this one?
I wasn't on the last CL, so I just want to build Chrome and see how the logs look like :) s/it might not results/it might not result s/traciable/traceable. https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1015: // one jobs serving the same server id, i.e., auxiliary job is also nit: s/one jobs/one job. Remove comma after "i.e."
Description was changed from ========== Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not results in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traciable. BUG=702411 ========== to ========== Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not result in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traceable. BUG=702411 ==========
A sample net log is available at https://goo.gl/TkbmrG. PTAL :) https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... net/quic/chromium/quic_stream_factory.cc:1015: // one jobs serving the same server id, i.e., auxiliary job is also On 2017/04/11 00:32:02, xunjieli wrote: > nit: s/one jobs/one job. Done. > Remove comma after "i.e." Acknowledged.
On 2017/04/11 00:54:33, Zhongyi Shi wrote: > A sample net log is available at https://goo.gl/TkbmrG. PTAL :) QUIC_STREAM_FACTORY_JOB with ID = 659. > https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... > File net/quic/chromium/quic_stream_factory.cc (right): > > https://codereview.chromium.org/2811573003/diff/80001/net/quic/chromium/quic_... > net/quic/chromium/quic_stream_factory.cc:1015: // one jobs serving the same > server id, i.e., auxiliary job is also > On 2017/04/11 00:32:02, xunjieli wrote: > > nit: s/one jobs/one job. > Done. > > Remove comma after "i.e." > > Acknowledged.
On 2017/04/11 00:57:33, Zhongyi Shi wrote: > On 2017/04/11 00:54:33, Zhongyi Shi wrote: > > A sample net log is available at https://goo.gl/TkbmrG. PTAL :) > > QUIC_STREAM_FACTORY_JOB with ID = 659. It's slightly confusing that the BOUND_TO_HTTP_STREAM_JOB can be nested inside a LOAD_SERVER_INFO. It seems to imply that LOAD_SERVER_INFO gives rise to the BOUND_TO_HTTP_STREAM_JOB event, which isn't the case. I don't have a solution for that, so this LGTM. t=2623 [st= 2] +QUIC_STREAM_FACTORY_JOB_LOAD_SERVER_INFO [dt=17] t=2628 [st= 7] QUIC_STREAM_FACTORY_JOB_BOUND_TO_HTTP_STREAM_JOB --> source_dependency = 656 (HTTP_STREAM_JOB) t=2640 [st=19] -QUIC_STREAM_FACTORY_JOB_LOAD_SERVER_INFO
lgtm https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory.cc:1011: auto itr = active_jobs_.find(server_id); Can you use a more descriptive name than |itr| ? I think either of these would be more consistent with the rest of this file: * job_it * active_jobs_it * it https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory.cc:1013: const JobSet* job_set = &itr->second; Could this be a |const JobSet&| instead?
Thanks Eric and Helen for helping the reviews, landing now. https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... File net/quic/chromium/quic_stream_factory.cc (right): https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory.cc:1011: auto itr = active_jobs_.find(server_id); On 2017/04/11 01:29:44, eroman wrote: > Can you use a more descriptive name than |itr| ? I think either of these would > be more consistent with the rest of this file: > > * job_it > * active_jobs_it > * it Done. https://codereview.chromium.org/2811573003/diff/100001/net/quic/chromium/quic... net/quic/chromium/quic_stream_factory.cc:1013: const JobSet* job_set = &itr->second; On 2017/04/11 01:29:44, eroman wrote: > Could this be a |const JobSet&| instead? Done.
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2811573003/#ps120001 (title: "address #14")
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": 120001, "attempt_start_ts": 1491882580190040,
"parent_rev": "d46617148d9a542238218ebafd39620731fb85c8", "commit_rev":
"011ef8d97d0b06877ca4cfc621d2e56f19aeef8c"}
Message was sent while issue was closed.
Description was changed from ========== Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not result in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traceable. BUG=702411 ========== to ========== Link NetLogs for HttpStreamFactoryImpl::Job and existing QuicStreamFactory::Job When HttpStreamFactoryImpl::Job init connection to QUIC server, in some cases, it might not result in QuicStreamFactory creating a new QuicStreamFactory::Job to serve the QuicRequest but use an exisitng QuicStreamFactory::Job. Link the HttpStreamFactoryImpl::Job and QuicStreamFactory::Job so that the HttpStreamFactoryImpl::Job::DoInitConnection is traceable. BUG=702411 Review-Url: https://codereview.chromium.org/2811573003 Cr-Commit-Position: refs/heads/master@{#463536} Committed: https://chromium.googlesource.com/chromium/src/+/011ef8d97d0b06877ca4cfc621d2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/011ef8d97d0b06877ca4cfc621d2...
Message was sent while issue was closed.
On 2017/04/11 05:02:41, commit-bot: I haz the power wrote: > Committed patchset #6 (id:120001) as > https://chromium.googlesource.com/chromium/src/+/011ef8d97d0b06877ca4cfc621d2... You forgot to remove previous events on the same connection
Message was sent while issue was closed.
Findit identified this CL at revision 463536 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
