Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621983004:
Improve HttpStreamFactory NetLog events (Closed)
DescriptionImprove HttpStreamFactory NetLog events
This CL introduces a SourceType for JobController to better account
for orphaned and preconnect jobs:
(1) For orphaned job, it will be correctly closed when the job
completes.
(2) For preconnect job, it will now appear in net-internals because
we take NetLog from HttpNetworkSession and log events there. Before
this CL, because preconnects do not have an associated stream request,
their events are not logged and it is hard to see whether a socket is
created due to preconnects.
BUG=679968
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2621983004
Cr-Commit-Position: refs/heads/master@{#446985}
Committed: https://chromium.googlesource.com/chromium/src/+/9255195315efa671012f0bd0da1afe685d70b2f6
Patch Set 1 #
Total comments: 2
Patch Set 2 : Get rid of long-running Source #
Total comments: 23
Patch Set 3 : Rebased #Patch Set 4 : address Matt's and Eric's comments #Patch Set 5 : Fix tests #Patch Set 6 : self review #Patch Set 7 : self review 2 #
Total comments: 8
Patch Set 8 : Fix lifetime issue #Patch Set 9 : Address comments on UI and fixed tests #
Total comments: 9
Patch Set 10 : Address Eric and Cherie comments #Patch Set 11 : Revert inline to Fix compile #Patch Set 12 : self #Patch Set 13 : fix test #Patch Set 14 : Fix SpdyNetworkTransactionTest #Patch Set 15 : Rebased #Patch Set 16 : fix compile #
Total comments: 7
Patch Set 17 : Address Bence comments and change back to auto #Messages
Total messages: 78 (44 generated)
Description was changed from ========== Improve HttpStreamFactory netlog events This CL changes HttpStreamFactory netlog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ========== to ========== Improve HttpStreamFactory netlog events This CL changes HttpStreamFactory netlog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ==========
Description was changed from ========== Improve HttpStreamFactory netlog events This CL changes HttpStreamFactory netlog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ========== to ========== Improve HttpStreamFactory NetLog events This CL changes HttpStreamFactory NetLog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org, zhongyi@chromium.org
Cherie and Matt: PTAL. This will solve my use cases (differentiate preconnects vs non-preconnects, identify the origins of the JobControllers, and know the lifetime of JobController and Jobs). If this is okay, I can start adding some tests (which I assume will be sprinkling checks on NetLog entries in existing tests) Thanks!
Description was changed from ========== Improve HttpStreamFactory NetLog events This CL changes HttpStreamFactory NetLog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ========== to ========== Improve HttpStreamFactory NetLog events This CL changes HttpStreamFactory NetLog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly start and close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ==========
On 2017/01/11 23:10:01, xunjieli wrote: > Cherie and Matt: PTAL. This will solve my use cases (differentiate preconnects > vs non-preconnects, identify the origins of the JobControllers, and know the > lifetime of JobController and Jobs). > > If this is okay, I can start adding some tests (which I assume will be > sprinkling checks on NetLog entries in existing tests) > > Thanks! We have some rather ugly infrastructure to help test net logs (TestNetLog/TestBoundNetLog are fine, but test_net_log_util.h is rather unwieldy). We're pretty inconsistent about NetLog tests. Some log lines have pretty extensive test coverage, but others have none. I generally don't bother to write them, myself, but then it's not unusual to notice breakages only when investigating bugs where you need the broken log entries.
mmenke@chromium.org changed reviewers: + eroman@chromium.org
https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:100: NetLogSourceType::HTTP_STREAM_FACTORY)) {} This isn't really how sources are currently used - currently, they're all short lived objects. With this, we'd basically always have objects 0, 1, 2, etc, at the top (As long as they're used sometime while logging) which are all HttpStreamFactories. [+eroman] for his thoughts on this.
https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:100: NetLogSourceType::HTTP_STREAM_FACTORY)) {} On 2017/01/12 16:35:08, mmenke wrote: > This isn't really how sources are currently used - currently, they're all short > lived objects. With this, we'd basically always have objects 0, 1, 2, etc, at > the top (As long as they're used sometime while logging) which are all > HttpStreamFactories. > > [+eroman] for his thoughts on this. My main motivation for creating a long-lived event is that I would like to identify HttpStream Jobs that are created for preconnects. If we don't pass a NetLog to PreconnectStreams(), preconnects Jobs won't show up since there isn't a NetLog attached to the HttpStreamFactoryImpl::Request. The JobController for these preconnects Jobs won't show up as well. Non-preconnects Jobs are fine, because they have NetLogWithSource from the URLRequest. An alternative approach to see/identify preconnects Jobs will be fine for me too.
On 2017/01/12 18:09:43, xunjieli wrote: > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > File net/http/http_stream_factory_impl.cc (right): > > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > net/http/http_stream_factory_impl.cc:100: > NetLogSourceType::HTTP_STREAM_FACTORY)) {} > On 2017/01/12 16:35:08, mmenke wrote: > > This isn't really how sources are currently used - currently, they're all > short > > lived objects. With this, we'd basically always have objects 0, 1, 2, etc, at > > the top (As long as they're used sometime while logging) which are all > > HttpStreamFactories. > > > > [+eroman] for his thoughts on this. Eric, a friendly ping! Thanks!
On 2017/01/17 15:25:46, xunjieli wrote: > On 2017/01/12 18:09:43, xunjieli wrote: > > > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > > File net/http/http_stream_factory_impl.cc (right): > > > > > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > > net/http/http_stream_factory_impl.cc:100: > > NetLogSourceType::HTTP_STREAM_FACTORY)) {} > > On 2017/01/12 16:35:08, mmenke wrote: > > > This isn't really how sources are currently used - currently, they're all > > short > > > lived objects. With this, we'd basically always have objects 0, 1, 2, etc, > at > > > the top (As long as they're used sometime while logging) which are all > > > HttpStreamFactories. > > > > > > [+eroman] for his thoughts on this. > > Eric, a friendly ping! Thanks! Note that this same issue has come up in https://codereview.chromium.org/2593243003/.
On 2017/01/18 19:43:42, mmenke wrote: > On 2017/01/17 15:25:46, xunjieli wrote: > > On 2017/01/12 18:09:43, xunjieli wrote: > > > > > > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > > > File net/http/http_stream_factory_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2621983004/diff/60001/net/http/http_stream_fa... > > > net/http/http_stream_factory_impl.cc:100: > > > NetLogSourceType::HTTP_STREAM_FACTORY)) {} > > > On 2017/01/12 16:35:08, mmenke wrote: > > > > This isn't really how sources are currently used - currently, they're all > > > short > > > > lived objects. With this, we'd basically always have objects 0, 1, 2, > etc, > > at > > > > the top (As long as they're used sometime while logging) which are all > > > > HttpStreamFactories. > > > > > > > > [+eroman] for his thoughts on this. > > > > Eric, a friendly ping! Thanks! > > Note that this same issue has come up in > https://codereview.chromium.org/2593243003/. (And I want to come up with a decision here, and then apply it as well to that CL)
The general approach looks fine to me. Conceptually I see Source primarily as a way to associate a sequence of events with a |this| pointer. This use-case fits that model. The expected life time of that object doesn't bother me. All that matters is whether having an affinity between events and that object is useful. If there is only one such object (i.e. a singleton), that benefit is mostly gone, but I don't find the pattern to be harmful either. In general when to split things into separate Sources or not is better understood from the resulting log file. It tends to be a tradeoff between accuracy (in representing the C++ object graph, when distinctions between entities is useful) and readability. Too many separate Sources and links between them can be difficult to navigate in our net log viewer UI. Rather than set forth hard rules, I would defer to the usefulness of the abstraction. If the resulting logs are more useful to you then go for it. Experimenting and iterating on the log types is inexpensive. So my perspective is looks good, feel free to try it out ;)
On 2017/01/19 22:31:42, eroman (slow) wrote: > The general approach looks fine to me. > > Conceptually I see Source primarily as a way to associate a sequence of events > with a |this| pointer. > > This use-case fits that model. The expected life time of that object doesn't > bother me. All that matters is whether having an affinity between events and > that object is useful. > > If there is only one such object (i.e. a singleton), that benefit is mostly > gone, but I don't find the pattern to be harmful either. > > In general when to split things into separate Sources or not is better > understood from the resulting log file. It tends to be a tradeoff between > accuracy (in representing the C++ object graph, when distinctions between > entities is useful) and readability. > > Too many separate Sources and links between them can be difficult to navigate in > our net log viewer UI. > > Rather than set forth hard rules, I would defer to the usefulness of the > abstraction. If the resulting logs are more useful to you then go for it. > Experimenting and iterating on the log types is inexpensive. So my perspective > is looks good, feel free to try it out ;) Thanks, Eric! I just realized that having a long-lived Source is probably not a good idea, because it will appear weird when it's displayed in chrome://tracing's UI (as the long lived source event is usually not closed). Let me rework this. I will update this thread when I come up with a better approach.
Description was changed from ========== Improve HttpStreamFactory NetLog events This CL changes HttpStreamFactory NetLog events in the following way: (1) Introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly start and close the corresponding HTTP_STREAM_JOB. (2) Introduces a SourceType for HttpStreamFactory so that preconnect JobController can log events even though it is not tied to a URLRequest. BUG=679968 ========== to ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly start and close the corresponding HTTP_STREAM_JOB. BUG=679968 ==========
Thanks everyone. I reworked the CL to avoid the usage of a long-running source. PTAL. Thanks!
Description was changed from ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController so even when Job is orphaned or created by preconnects, we will correctly start and close the corresponding HTTP_STREAM_JOB. BUG=679968 ========== to ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController to better account for orphaned and preconnect jobs: (1) For orphaned job, it will be correctly closed when the job completes. (2) For preconnect job, it will now appear in net-internals because we take NetLog from HttpNetworkSession and log events there. Before this CL, because preconnects do not have an associated stream request, their events are not logged and it is hard to see whether a socket is created due to preconnects. BUG=679968 ==========
Thanks for cook up the CL and improve the NetLog :) https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:76: net_log_.source().ToEventParametersCallback()); I am unfamiliar with NetLog, why do we need |net_log| to be passed in? Is that for building dependency?
https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:76: net_log_.source().ToEventParametersCallback()); On 2017/01/20 22:42:10, Zhongyi Shi wrote: > I am unfamiliar with NetLog, why do we need |net_log| to be passed in? Is that > for building dependency? Yes, this is for linking the caller of JobController (in this case it's the URLRequest) with the JobController event. Here's an example netlog https://drive.google.com/a/google.com/file/d/0B_9WseIqRoaFTEZSLUw4aFUyc2M/vie...
Matt: a friendly ping. Cherie's on triage duty. Could you do a first pass? Thanks!
On 2017/01/24 14:40:13, xunjieli wrote: > Matt: a friendly ping. Cherie's on triage duty. Could you do a first pass? > Thanks! For some reason I was thinking Eric signed off on this, so it dropped off my radar. I'll plan to get to this later today or tomorrow. Low on sleep today, and lifecycles of these classes make my head spin.
https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:181: NetLogWithSource dummy_netlog_with_source; Can just inline NetLogWithSource() in the line below. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:57: const NetLogWithSource& net_log) Calling this net_log and having it be different from net_log_ is very confusing. source_net_log? https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:74: if (net_log.source().IsValid()) { Not needed - AddEvent checks if the NetLogWithSource has an nullptr for the NetLog and if so, does nothing. And if the NetLog is non-NULL, then the source must be valid. Or do mean to check net_log_ instead of net_log? If so, should probably check net_log_.net_log(). https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:108: stream_type); Should the request really be using the Controller's log? It seems like the request is scoped to the HttpNetworkTransaction, and it uses the controller, so it should look like: <URL_REQUEST's events> +STREAM_REQUEST source_dependency: <link to controller> BOUND_TO_JOB source_dependency: <link to job (Shortcut, to avoid having to look at controller, similar to the event in the controller)> -STREAM_REQUEST https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:122: DCHECK(!alternative_job_); DCHECK(is_preconnect_);?
https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:182: JobController* job_controller = side-comment: can you update this to be std::unique_ptr<JobController> ? https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (left): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:689: if (net_log) { It isn't clear why this removal is safe when reading this line. From an interface perspective, GetNetLog() can return nullptr. It may be that in practice based on how the pieces are connected (does this delegate come from JobController, which now returns non-null?) this is never null, but that coupling between implementations is not obvious. I suggest keeping the null-check, even if in practice the concrete delegate implementation won't hit it. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:57: const NetLogWithSource& net_log) On 2017/01/25 19:32:17, mmenke wrote: > Calling this net_log and having it be different from net_log_ is very confusing. > source_net_log? Strongly agreed -- I found this quite confusing when reading. There are a variety of checks around "is valid" in this file, and similarly passing a "dummy" netlog in HttpStreamFactoryImpl. I think this can be solved by removing the parameter from the constructor alltogether, and making the association be a separate method. For instance in HttpStreamFactoryImpl you might call some new method AssociateWithNetLogSource() in the non-preconnect case: HttpStreamFactoryImpl::RequestStreamInternal(...) { auto job_controller = base::MakeUnique<JobController>(XXX); // Call an extra function to associate the two net log Sources job_controller->AssociateWithNetSource(net_log.source()); ... } HttpStreamFactoryImpl::PreconnectStreams(...) { // Note, no use of "dummy_net_log_with_source" anywhere auto job_controller = base::MakeUnique<JobController>(XXX); ... } HttpStreamFactoryImpl::JobController::AssociateWithNetLogSource(source) { < Emit two separate "bound" events. One that associates source with net_log_.source(), and another for the opposite direction. This does mean introducing a separate net log event type rather than overloading the possible parameters for HTTP_STREAM_JOB_CONTROLLER> } https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:122: DCHECK(!alternative_job_); On 2017/01/25 19:32:17, mmenke wrote: > DCHECK(is_preconnect_);? +1, I had same comment. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:641: return &net_log_; does this still need to check for |bound_job_ != job| ? https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_request.h:139: const NetLogWithSource* net_log_; NetLogWithSource supports copy-construction do this doesn't need to be a pointer (can have a copy of NetLogWithSource instead -- all it is is a pointer to NetLog and a Source identifier).
I believe I have address all comments. Here's a netlog with the suggested edits: https://drive.google.com/open?id=0B_9WseIqRoaFOUhDcDRPdEhQZzA PTAL, thanks! https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:181: NetLogWithSource dummy_netlog_with_source; On 2017/01/25 19:32:17, mmenke wrote: > Can just inline NetLogWithSource() in the line below. Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:182: JobController* job_controller = On 2017/01/25 19:55:26, eroman (slow) wrote: > side-comment: can you update this to be std::unique_ptr<JobController> ? Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (left): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:689: if (net_log) { On 2017/01/25 19:55:26, eroman (slow) wrote: > It isn't clear why this removal is safe when reading this line. From an > interface perspective, GetNetLog() can return nullptr. > > It may be that in practice based on how the pieces are connected (does this > delegate come from JobController, which now returns non-null?) this is never > null, but that coupling between implementations is not obvious. > > I suggest keeping the null-check, even if in practice the concrete delegate > implementation won't hit it. Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:57: const NetLogWithSource& net_log) On 2017/01/25 19:32:17, mmenke wrote: > Calling this net_log and having it be different from net_log_ is very confusing. > source_net_log? Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:57: const NetLogWithSource& net_log) On 2017/01/25 19:55:27, eroman (slow) wrote: > On 2017/01/25 19:32:17, mmenke wrote: > > Calling this net_log and having it be different from net_log_ is very > confusing. > > source_net_log? > > Strongly agreed -- I found this quite confusing when reading. There are a > variety of checks around "is valid" in this file, and similarly passing a > "dummy" netlog in HttpStreamFactoryImpl. > > I think this can be solved by removing the parameter from the constructor > alltogether, and making the association be a separate method. For instance in > HttpStreamFactoryImpl you might call some new method AssociateWithNetLogSource() > in the non-preconnect case: > > > HttpStreamFactoryImpl::RequestStreamInternal(...) { > auto job_controller = base::MakeUnique<JobController>(XXX); > > // Call an extra function to associate the two net log Sources > job_controller->AssociateWithNetSource(net_log.source()); > ... > } > > HttpStreamFactoryImpl::PreconnectStreams(...) { > // Note, no use of "dummy_net_log_with_source" anywhere > auto job_controller = base::MakeUnique<JobController>(XXX); > ... > } > > > HttpStreamFactoryImpl::JobController::AssociateWithNetLogSource(source) { > < Emit two separate "bound" events. One that associates source with > net_log_.source(), and another for the opposite direction. This does mean > introducing a separate net log event type rather than overloading the possible > parameters for HTTP_STREAM_JOB_CONTROLLER> > } Done. Great idea. That's neat! https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:74: if (net_log.source().IsValid()) { On 2017/01/25 19:32:17, mmenke wrote: > Not needed - AddEvent checks if the NetLogWithSource has an nullptr for the > NetLog and if so, does nothing. And if the NetLog is non-NULL, then the source > must be valid. > > Or do mean to check net_log_ instead of net_log? If so, should probably check > net_log_.net_log(). Done. Thanks! I didn't realized that AddEvent() checks if the NetLogWithSource has a non-null NetLog. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:108: stream_type); On 2017/01/25 19:32:17, mmenke wrote: > Should the request really be using the Controller's log? It seems like the > request is scoped to the HttpNetworkTransaction, and it uses the controller, so > it should look like: > > <URL_REQUEST's events> > +STREAM_REQUEST > source_dependency: <link to controller> > BOUND_TO_JOB > source_dependency: <link to job (Shortcut, to avoid having to look at > controller, similar to the event in the controller)> > -STREAM_REQUEST > Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:122: DCHECK(!alternative_job_); On 2017/01/25 19:55:27, eroman (slow) wrote: > On 2017/01/25 19:32:17, mmenke wrote: > > DCHECK(is_preconnect_);? > > +1, I had same comment. Done. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:641: return &net_log_; On 2017/01/25 19:55:27, eroman (slow) wrote: > does this still need to check for |bound_job_ != job| ? No. The check is not necessary. Before the change, net_log is null when request_ is gone. In the new change, JobController has a always valid net_log_ to make preconnect & orphaned job events show up. https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/2621983004/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_request.h:139: const NetLogWithSource* net_log_; On 2017/01/25 19:55:27, eroman (slow) wrote: > NetLogWithSource supports copy-construction do this doesn't need to be a pointer > (can have a copy of NetLogWithSource instead -- all it is is a pointer to NetLog > and a Source identifier). Done.
Looks good! Just one concern about lifetimes. https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:162: auto job_controller = base::MakeUnique<JobController>( Am I the only one who can't stand this pattern? :( I find not putting types before variables obfuscates types of variables (Yes, even when the type and variable name are the same). Would you mind doing: std::unique_ptr<JobController> job_controller = base::MakeUnique... https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:188: /*is_preconnect=*/true); Same as above https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:191: job_controller_set_.insert(std::move(job_controller)); Are you sure Preconnect can't complete synchronously? I guess the fact that HttpStreamFactoryImpl::RequestStreamInternal can't indicates perhaps not, though Preconnect doesn't actually create a stream, so it misses the posttask for the final return.
Two other things, looking at the log: Can we give a description to the HTTP_STREAM_JOB_CONTROLLERS to net-internals? Seems simplest just add the URL to the first event, and update net-internals' source to find it. Also, for the HTTP_STREAM_JOB...I don't suppose we can somehow log the REQUEST event before the CONTROLLER event? Seems a little weird wither the CONTROLLER event first. (Haven't thought about how difficult this would be to do, architecturally - may just not be worth the effort)
Description was changed from ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController to better account for orphaned and preconnect jobs: (1) For orphaned job, it will be correctly closed when the job completes. (2) For preconnect job, it will now appear in net-internals because we take NetLog from HttpNetworkSession and log events there. Before this CL, because preconnects do not have an associated stream request, their events are not logged and it is hard to see whether a socket is created due to preconnects. BUG=679968 ========== to ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController to better account for orphaned and preconnect jobs: (1) For orphaned job, it will be correctly closed when the job completes. (2) For preconnect job, it will now appear in net-internals because we take NetLog from HttpNetworkSession and log events there. Before this CL, because preconnects do not have an associated stream request, their events are not logged and it is hard to see whether a socket is created due to preconnects. BUG=679968 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
> Two other things, looking at the log: > Can we give a description to the > HTTP_STREAM_JOB_CONTROLLERS to net-internals? > Seems simplest just add the URL to the first event, and > update net-internals' > source to find it. Done. I added the source type to the js file. > Also, for the HTTP_STREAM_JOB...I don't suppose we can somehow log the REQUEST > event before the CONTROLLER event? Seems a little weird wither the CONTROLLER > event first. (Haven't thought about how difficult this would be to do, > architecturally - may just not be worth the effort) Done. That works great since it will remove the AssociateNetLogSource() method all together. Here's the netlog after applying the change: https://drive.google.com/open?id=0B_9WseIqRoaFOC1FOUNlRW56MjQ Thanks! PTAL https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:162: auto job_controller = base::MakeUnique<JobController>( On 2017/01/26 20:47:43, mmenke wrote: > Am I the only one who can't stand this pattern? :( I find not putting types > before variables obfuscates types of variables (Yes, even when the type and > variable name are the same). Would you mind doing: > > std::unique_ptr<JobController> job_controller = base::MakeUnique... Done. https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:188: /*is_preconnect=*/true); On 2017/01/26 20:47:43, mmenke wrote: > Same as above Done. https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:191: job_controller_set_.insert(std::move(job_controller)); On 2017/01/26 20:47:43, mmenke wrote: > Are you sure Preconnect can't complete synchronously? I guess the fact that > HttpStreamFactoryImpl::RequestStreamInternal can't indicates perhaps not, though > Preconnect doesn't actually create a stream, so it misses the posttask for the > final return. Done. Sorry about that. I think we shouldn't make the assumption. Changed this and the above one back.
Looks good. I was looking at the net logs you provided in #31(controller5.json) and notices that the there's always only 1 HTTP_STREAM_JOB tracked in JOB_CONTROLLER. Looking at the logs, I guess QUIC is not forced on and we have no racing, Helen, could you grab a new net log with QUIC turning on?
On 2017/01/26 22:57:43, Zhongyi Shi wrote: > Looks good. > > I was looking at the net logs you provided in #31(controller5.json) and notices > that the there's always only 1 HTTP_STREAM_JOB tracked in JOB_CONTROLLER. > Looking at the logs, I guess QUIC is not forced on and we have no racing, Helen, > could you grab a new net log with QUIC turning on? Sure thing. That is probably because I used a fresh profile. Here's one with some quic jobs. https://drive.google.com/open?id=0B_9WseIqRoaFUVBERnVLcVBKd00
lgtm https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:162: auto job_controller = base::MakeUnique<JobController>( On 2017/01/26 20:47:43, mmenke wrote: > Am I the only one who can't stand this pattern? :( I find not putting types > before variables obfuscates types of variables (Yes, even when the type and > variable name are the same). Would you mind doing: > > std::unique_ptr<JobController> job_controller = base::MakeUnique... Most chrome code (84%) uses "auto": verbose way: 149 auto: 804 I personally find repeating the type to be unnecessarily verbose. Based on the number I would say use of auto is more idiomatic, unless there was an explicit style discussion somewhere. https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:43: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue()); [optional] style: MakeUnique https://codereview.chromium.org/2621983004/diff/220001/net/log/net_log_source... File net/log/net_log_source_type_list.h (right): https://codereview.chromium.org/2621983004/diff/220001/net/log/net_log_source... net/log/net_log_source_type_list.h:25: SOURCE_TYPE(HTTP_STREAM_JOB_CONTROLLER) Can you put this at the bottom (as this list isn't alphabetical) ? The order shouldn't matter... however this does end up changing IDs in some log files; may as well keep the existing source IDs the same by appending.
Almost there. Just one extra question on the creation of the JOB_CONTROLLER net log. https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:75: &request_info.url, /*is_preconnect=*/false)); Why this is always set to false? https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:647: return &net_log_; nit: you might want o move the implementation of this method to the .h file.
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...
Thanks! https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:43: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue()); On 2017/01/26 23:37:00, eroman (slow) wrote: > [optional] style: MakeUnique Done. https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:75: &request_info.url, /*is_preconnect=*/false)); On 2017/01/27 00:50:32, Zhongyi Shi wrote: > Why this is always set to false? Ah, sorry about that. Done. https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:647: return &net_log_; On 2017/01/27 00:50:32, Zhongyi Shi wrote: > nit: you might want o move the implementation of this method to the .h file. Done. https://codereview.chromium.org/2621983004/diff/220001/net/log/net_log_source... File net/log/net_log_source_type_list.h (right): https://codereview.chromium.org/2621983004/diff/220001/net/log/net_log_source... net/log/net_log_source_type_list.h:25: SOURCE_TYPE(HTTP_STREAM_JOB_CONTROLLER) On 2017/01/26 23:37:00, eroman (slow) wrote: > Can you put this at the bottom (as this list isn't alphabetical) ? > > The order shouldn't matter... however this does end up changing IDs in some log > files; may as well keep the existing source IDs the same by appending. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:647: return &net_log_; On 2017/01/27 14:35:30, xunjieli wrote: > On 2017/01/27 00:50:32, Zhongyi Shi wrote: > > nit: you might want o move the implementation of this method to the .h file. > > Done. Hmm.. actually this is a virtual function so I can't inline it. [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2621983004/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:162: auto job_controller = base::MakeUnique<JobController>( On 2017/01/26 23:36:59, eroman (slow) wrote: > On 2017/01/26 20:47:43, mmenke wrote: > > Am I the only one who can't stand this pattern? :( I find not putting types > > before variables obfuscates types of variables (Yes, even when the type and > > variable name are the same). Would you mind doing: > > > > std::unique_ptr<JobController> job_controller = base::MakeUnique... > > Most chrome code (84%) uses "auto": > > verbose way: 149 > auto: 804 > > I personally find repeating the type to be unnecessarily verbose. Based on the > number I would say use of auto is more idiomatic, unless there was an explicit > style discussion somewhere. When I see a variable and want to know its type, I find skimming up and finding auto and then looking over to MakeUnique<blah> and then mapping MakeUnique<blah> to std::unique_ptr<blah> involves significantly more cognitive load than just skimming up and seeing std::unique_ptr<blah>, and then going back to the code I was looking at.
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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
bnc@: PTAL at net/spdy/spdy_network_transaction_unittest.cc Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM. https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:44: std::unique_ptr<base::DictionaryValue> dict = Optional: feel free to use auto here (as suggested by comments in base/memory/ptr_util.h). https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller_unittest.cc:930: Initialize(request_info, false, false); The original Initialize() call has true argument, do you not need true here? https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request_unittest.cc (left): https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_request_unittest.cc:38: factory->job_controller_set_.insert(base::WrapUnique(job_controller)); I really like your change in http_stream_factory_impl.cc with MakeUnique(), std::move(), and _raw_ptr. Could you do the same here please? https://codereview.chromium.org/2621983004/diff/360001/net/log/net_log_event_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2621983004/diff/360001/net/log/net_log_event_... net/log/net_log_event_type_list.h:1100: // Links a JobController with its user(a URL_REQUEST). Add space between "user" and "(".
LGTM.
Thanks for the review! Matt and Eric: Bence pointed out that "base/memory/ptr_util.h" suggests to use auto = base::MakeUnique<>(). I have changed the CL to use "auto". I guess we do have a codestyle guideline on this :) https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:44: std::unique_ptr<base::DictionaryValue> dict = On 2017/01/28 02:08:25, Bence wrote: > Optional: feel free to use auto here (as suggested by comments in > base/memory/ptr_util.h). Done. https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request_unittest.cc (left): https://codereview.chromium.org/2621983004/diff/360001/net/http/http_stream_f... net/http/http_stream_factory_impl_request_unittest.cc:38: factory->job_controller_set_.insert(base::WrapUnique(job_controller)); On 2017/01/28 02:08:25, Bence wrote: > I really like your change in http_stream_factory_impl.cc with MakeUnique(), > std::move(), and _raw_ptr. Could you do the same here please? Done. https://codereview.chromium.org/2621983004/diff/360001/net/log/net_log_event_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2621983004/diff/360001/net/log/net_log_event_... net/log/net_log_event_type_list.h:1100: // Links a JobController with its user(a URL_REQUEST). On 2017/01/28 02:08:25, Bence wrote: > Add space between "user" and "(". Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, mmenke@chromium.org, zhongyi@chromium.org, bnc@chromium.org Link to the patchset: https://codereview.chromium.org/2621983004/#ps380001 (title: "Address Bence comments and change back to auto")
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": 380001, "attempt_start_ts": 1485785327699050,
"parent_rev": "da00ce3b58c2116e88defdf99339ac9b503ceecc", "commit_rev":
"9255195315efa671012f0bd0da1afe685d70b2f6"}
Message was sent while issue was closed.
Description was changed from ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController to better account for orphaned and preconnect jobs: (1) For orphaned job, it will be correctly closed when the job completes. (2) For preconnect job, it will now appear in net-internals because we take NetLog from HttpNetworkSession and log events there. Before this CL, because preconnects do not have an associated stream request, their events are not logged and it is hard to see whether a socket is created due to preconnects. BUG=679968 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improve HttpStreamFactory NetLog events This CL introduces a SourceType for JobController to better account for orphaned and preconnect jobs: (1) For orphaned job, it will be correctly closed when the job completes. (2) For preconnect job, it will now appear in net-internals because we take NetLog from HttpNetworkSession and log events there. Before this CL, because preconnects do not have an associated stream request, their events are not logged and it is hard to see whether a socket is created due to preconnects. BUG=679968 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2621983004 Cr-Commit-Position: refs/heads/master@{#446985} Committed: https://chromium.googlesource.com/chromium/src/+/9255195315efa671012f0bd0da1a... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:380001) as https://chromium.googlesource.com/chromium/src/+/9255195315efa671012f0bd0da1a... |
