Log states of JobController in Net MemoryDumpProvider

Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
Review-Url: https://codereview.chromium.org/2623383005
Cr-Commit-Position: refs/heads/master@{#445165}
Committed: https://chromium.googlesource.com/chromium/src/+/f5267debb22eacffc2287fba55832299ef640bbb

[xunjieli, 2017-01-13 20:39:31]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many JobControllers exist because of pending Alt Jobs?
(3) How many JobControllers exist because of preconnect?

Note that (2) and (3) are not disjoint.

BUG=669108
==========

[xunjieli, 2017-01-13 20:42:05]

xunjieli@chromium.org changed reviewers:
+ ssid@chromium.org, zhongyi@chromium.org

[xunjieli, 2017-01-13 20:42:06]

Sid and Cherie: PTAL.
Something like this will allow us to spot an issue like 678768.

[Zhongyi Shi, 2017-01-13 21:57:50]

That's a great idea to log the details of the JobController's state. Thanks
Helen!

I'm not the expert on how MemoryDumpProvider works here, delegate to ssid@ to
review that part. I will be mainly looking at how we log the JobController's
state.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
I thought we are trying to logging how many JobControllers are there and why
they are live. Not sure why we only care the job controller having the pending
alt job?

I would prefer to breakdown into following category:

1. JobController with preconnect
2. JobController with two jobs running
3. JobController with only one job running: 
   you might also want to break this down to which job is running
 3.1. JobController with only one job running, job is orphaned.
 3.2. JobController with only one job running, job is bind to request

How does that sound?

[ssid, 2017-01-13 22:23:59]

Why are we logging only the number of jobs. Why are we not dumping the size of
the controllers?
I think we should also add the memory usage of these jobs in dump provider,
which is more important for a common user who does not understand job states.

Is it hard / takes long to compute the size of the jobs?
Also, you could use the EstimateMemoryUsage methods to get the size of std::set.

https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/memo...

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:433: if (args.level_of_detail ==
I think your should call this always. Yes it adds an additional row. But this is
going to account for memory usage better.
Else we will miss tracking the size of stream factory in background mode.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
net/http/http_network_session.h:300: const base::trace_event::MemoryDumpArgs&
args,
You don't need to add an argument here. You can use pmd->dump_args().
 I know OnMemoryDump sends this. But we are doing it redundantly because we need
to change a lot of dump providers to fix this.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:313:
base::trace_event::MemoryAllocatorDump* factory =
factory_dump? Not to confuse with the Factory object.

[xunjieli, 2017-01-17 17:51:24]

> Is it hard / takes long to compute the size of the jobs?
> Also, you could use the EstimateMemoryUsage methods to get the size of
std::set.

I changed the CL to EstimateMemoryUsage.  The size of the jobs isn't
straightforward. According to your instrumentation, I think a Job is ~100bytes
(?) except the case where it is a regular Http1.1 Job and the connection has
certs and socket buffers. I tried to take the second part into account. 

I could also add a 100bytes to every Job, or does EstimateMemoryUsage(std::set)
already take that into account? 

PTAL.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:433: if (args.level_of_detail ==
On 2017/01/13 22:23:58, ssid wrote:
> I think your should call this always. Yes it adds an additional row. But this
is
> going to account for memory usage better.
> Else we will miss tracking the size of stream factory in background mode.

Done.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_network_s...
net/http/http_network_session.h:300: const base::trace_event::MemoryDumpArgs&
args,
On 2017/01/13 22:23:58, ssid wrote:
> You don't need to add an argument here. You can use pmd->dump_args().
>  I know OnMemoryDump sends this. But we are doing it redundantly because we
need
> to change a lot of dump providers to fix this.

Done.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:313:
base::trace_event::MemoryAllocatorDump* factory =
On 2017/01/13 22:23:59, ssid wrote:
> factory_dump? Not to confuse with the Factory object.

Done.

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
On 2017/01/13 21:57:50, Zhongyi Shi wrote:
> I thought we are trying to logging how many JobControllers are there and why
> they are live. 

Great question. I think this was the goal while investigating the "leak." In the
longer term, we don't have the capacity to do things in that detail. Here we can
only try log the aggregated data across all controllers. If we want
per-controller data, we need a MemoryAllocatorDump per controller, which is
expensive and will prohibit the net/ dump provider from being turned on in
SlowReports.

I think here the main goal is to make sure that there aren't too many
controllers and the total number will *decrease* over time.
If somehow, the traces we obtained from the wild (through SlowReports) show us
that the number of controllers only increase and not decrease, we can dive in
and find out more.

We can try logging the extra information by creating one dump per controller in
heavy mode. But I am not sure if we gain much by doing that. The dump provider
serves to tell us if a subcomponent is misbehaving in terms of memory
consumption. We can get a rough idea from the aggregate data.
 
> Not sure why we only care the job controller having the pending
> alt job?
> 
> I would prefer to breakdown into following category:
> 
> 1. JobController with preconnect
> 2. JobController with two jobs running
> 3. JobController with only one job running: 
>    you might also want to break this down to which job is running
>  3.1. JobController with only one job running, job is orphaned.
>  3.2. JobController with only one job running, job is bind to request
> 
> How does that sound?

[Zhongyi Shi, 2017-01-18 04:56:21]

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
On 2017/01/17 17:51:24, xunjieli wrote:
> On 2017/01/13 21:57:50, Zhongyi Shi wrote:
> > I thought we are trying to logging how many JobControllers are there and why
> > they are live. 
> 
> Great question. I think this was the goal while investigating the "leak." In
the
> longer term, we don't have the capacity to do things in that detail. Here we
can
> only try log the aggregated data across all controllers. If we want
> per-controller data, we need a MemoryAllocatorDump per controller, which is
> expensive and will prohibit the net/ dump provider from being turned on in
> SlowReports.
> 
> I think here the main goal is to make sure that there aren't too many
> controllers and the total number will *decrease* over time.
> If somehow, the traces we obtained from the wild (through SlowReports) show us
> that the number of controllers only increase and not decrease, we can dive in
> and find out more.
> 
> We can try logging the extra information by creating one dump per controller
in
> heavy mode. But I am not sure if we gain much by doing that. The dump provider
> serves to tell us if a subcomponent is misbehaving in terms of memory
> consumption. We can get a rough idea from the aggregate data.
>  
> > Not sure why we only care the job controller having the pending
> > alt job?
> > 
> > I would prefer to breakdown into following category:
> > 
> > 1. JobController with preconnect
> > 2. JobController with two jobs running
> > 3. JobController with only one job running: 
> >    you might also want to break this down to which job is running
> >  3.1. JobController with only one job running, job is orphaned.
> >  3.2. JobController with only one job running, job is bind to request
> > 
> > How does that sound?

The rationale makes sense to me, thanks for explaining. The preconnect case
looks good, but why we only count for those job controllers which has pending
ALT job? I thought what you're saying is jobcontroller that has pending jobs. I
don't understand why we cares about the specific case that we have pending ALT
job.

[xunjieli, 2017-01-18 19:49:31]

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
On 2017/01/18 04:56:21, Zhongyi Shi wrote:
> On 2017/01/17 17:51:24, xunjieli wrote:
> > On 2017/01/13 21:57:50, Zhongyi Shi wrote:
> > > I thought we are trying to logging how many JobControllers are there and
why
> > > they are live. 
> > 
> > Great question. I think this was the goal while investigating the "leak." In
> the
> > longer term, we don't have the capacity to do things in that detail. Here we
> can
> > only try log the aggregated data across all controllers. If we want
> > per-controller data, we need a MemoryAllocatorDump per controller, which is
> > expensive and will prohibit the net/ dump provider from being turned on in
> > SlowReports.
> > 
> > I think here the main goal is to make sure that there aren't too many
> > controllers and the total number will *decrease* over time.
> > If somehow, the traces we obtained from the wild (through SlowReports) show
us
> > that the number of controllers only increase and not decrease, we can dive
in
> > and find out more.
> > 
> > We can try logging the extra information by creating one dump per controller
> in
> > heavy mode. But I am not sure if we gain much by doing that. The dump
provider
> > serves to tell us if a subcomponent is misbehaving in terms of memory
> > consumption. We can get a rough idea from the aggregate data.
> >  
> > > Not sure why we only care the job controller having the pending
> > > alt job?
> > > 
> > > I would prefer to breakdown into following category:
> > > 
> > > 1. JobController with preconnect
> > > 2. JobController with two jobs running
> > > 3. JobController with only one job running: 
> > >    you might also want to break this down to which job is running
> > >  3.1. JobController with only one job running, job is orphaned.
> > >  3.2. JobController with only one job running, job is bind to request
> > > 
> > > How does that sound?
> 
> The rationale makes sense to me, thanks for explaining. The preconnect case
> looks good, but why we only count for those job controllers which has pending
> ALT job? I thought what you're saying is jobcontroller that has pending jobs.
I
> don't understand why we cares about the specific case that we have pending ALT
> job.

I didn't do a "pending main job count" is because we have (a) the total # of
controllers, and (b) total # of controllers with pending ALT job. The # of
controllers with pending Main job should be slightly larger than a - b. This
assumes that the number of controllers with both a main job and an alt job is
small.

Do you think we should do a pending main job count as well?

[Zhongyi Shi, 2017-01-18 20:17:45]

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
On 2017/01/18 19:49:31, xunjieli wrote:
> On 2017/01/18 04:56:21, Zhongyi Shi wrote:
> > On 2017/01/17 17:51:24, xunjieli wrote:
> > > On 2017/01/13 21:57:50, Zhongyi Shi wrote:
> > > > I thought we are trying to logging how many JobControllers are there and
> why
> > > > they are live. 
> > > 
> > > Great question. I think this was the goal while investigating the "leak."
In
> > the
> > > longer term, we don't have the capacity to do things in that detail. Here
we
> > can
> > > only try log the aggregated data across all controllers. If we want
> > > per-controller data, we need a MemoryAllocatorDump per controller, which
is
> > > expensive and will prohibit the net/ dump provider from being turned on in
> > > SlowReports.
> > > 
> > > I think here the main goal is to make sure that there aren't too many
> > > controllers and the total number will *decrease* over time.
> > > If somehow, the traces we obtained from the wild (through SlowReports)
show
> us
> > > that the number of controllers only increase and not decrease, we can dive
> in
> > > and find out more.
> > > 
> > > We can try logging the extra information by creating one dump per
controller
> > in
> > > heavy mode. But I am not sure if we gain much by doing that. The dump
> provider
> > > serves to tell us if a subcomponent is misbehaving in terms of memory
> > > consumption. We can get a rough idea from the aggregate data.
> > >  
> > > > Not sure why we only care the job controller having the pending
> > > > alt job?
> > > > 
> > > > I would prefer to breakdown into following category:
> > > > 
> > > > 1. JobController with preconnect
> > > > 2. JobController with two jobs running
> > > > 3. JobController with only one job running: 
> > > >    you might also want to break this down to which job is running
> > > >  3.1. JobController with only one job running, job is orphaned.
> > > >  3.2. JobController with only one job running, job is bind to request
> > > > 
> > > > How does that sound?
> > 
> > The rationale makes sense to me, thanks for explaining. The preconnect case
> > looks good, but why we only count for those job controllers which has
pending
> > ALT job? I thought what you're saying is jobcontroller that has pending
jobs.
> I
> > don't understand why we cares about the specific case that we have pending
ALT
> > job.
> 
> I didn't do a "pending main job count" is because we have (a) the total # of
> controllers, and (b) total # of controllers with pending ALT job. The # of
> controllers with pending Main job should be slightly larger than a - b. This
> assumes that the number of controllers with both a main job and an alt job is
> small.
> 
> Do you think we should do a pending main job count as well? 
It will be a good idea if you also do a pending main job so that we could
calculate the number of job controllers in each type. Does that sound plausible
or do we have the capacity?

[xunjieli, 2017-01-18 20:25:04]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many JobControllers exist because of pending Alt Jobs?
(3) How many JobControllers exist because of preconnect?

Note that (2) and (3) are not disjoint.

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many JobControllers exist because of a pending Alt Job?
(3) How many JobControllers exist because of a pending Main Job?
(4) How many JobControllers exist because of preconnect?

Union of (2) and (3) should give (1). 

BUG=669108
==========

[xunjieli, 2017-01-18 20:25:20]

Thanks! PTAL?

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:322: alt_job_count++;
On 2017/01/18 20:17:45, Zhongyi Shi wrote:
> On 2017/01/18 19:49:31, xunjieli wrote:
> > On 2017/01/18 04:56:21, Zhongyi Shi wrote:
> > > On 2017/01/17 17:51:24, xunjieli wrote:
> > > > On 2017/01/13 21:57:50, Zhongyi Shi wrote:
> > > > > I thought we are trying to logging how many JobControllers are there
and
> > why
> > > > > they are live. 
> > > > 
> > > > Great question. I think this was the goal while investigating the
"leak."
> In
> > > the
> > > > longer term, we don't have the capacity to do things in that detail.
Here
> we
> > > can
> > > > only try log the aggregated data across all controllers. If we want
> > > > per-controller data, we need a MemoryAllocatorDump per controller, which
> is
> > > > expensive and will prohibit the net/ dump provider from being turned on
in
> > > > SlowReports.
> > > > 
> > > > I think here the main goal is to make sure that there aren't too many
> > > > controllers and the total number will *decrease* over time.
> > > > If somehow, the traces we obtained from the wild (through SlowReports)
> show
> > us
> > > > that the number of controllers only increase and not decrease, we can
dive
> > in
> > > > and find out more.
> > > > 
> > > > We can try logging the extra information by creating one dump per
> controller
> > > in
> > > > heavy mode. But I am not sure if we gain much by doing that. The dump
> > provider
> > > > serves to tell us if a subcomponent is misbehaving in terms of memory
> > > > consumption. We can get a rough idea from the aggregate data.
> > > >  
> > > > > Not sure why we only care the job controller having the pending
> > > > > alt job?
> > > > > 
> > > > > I would prefer to breakdown into following category:
> > > > > 
> > > > > 1. JobController with preconnect
> > > > > 2. JobController with two jobs running
> > > > > 3. JobController with only one job running: 
> > > > >    you might also want to break this down to which job is running
> > > > >  3.1. JobController with only one job running, job is orphaned.
> > > > >  3.2. JobController with only one job running, job is bind to request
> > > > > 
> > > > > How does that sound?
> > > 
> > > The rationale makes sense to me, thanks for explaining. The preconnect
case
> > > looks good, but why we only count for those job controllers which has
> pending
> > > ALT job? I thought what you're saying is jobcontroller that has pending
> jobs.
> > I
> > > don't understand why we cares about the specific case that we have pending
> ALT
> > > job.
> > 
> > I didn't do a "pending main job count" is because we have (a) the total # of
> > controllers, and (b) total # of controllers with pending ALT job. The # of
> > controllers with pending Main job should be slightly larger than a - b. This
> > assumes that the number of controllers with both a main job and an alt job
is
> > small.
> > 
> > Do you think we should do a pending main job count as well? 
> It will be a good idea if you also do a pending main job so that we could
> calculate the number of job controllers in each type. Does that sound
plausible
> or do we have the capacity?

Done. SGTM. PTAL?

[Zhongyi Shi, 2017-01-18 20:39:32]

net LGTM! Delegate MemoryDumpProvider review to ssid@. Thanks :)

https://codereview.chromium.org/2623383005/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:323: main_job_count++;
nit: preconnect jobcontrollers will only have main job, which means preconnect
controllers will be double counted. The current counting will work, it might be
better to determine whether the job controller is preconnect, and return early
so that when we count for pending main jobs, we don't double counting the
preconnect controllers. Both will work, I'll leave it to you to make the call.

[xunjieli, 2017-01-18 20:55:58]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many JobControllers exist because of a pending Alt Job?
(3) How many JobControllers exist because of a pending Main Job?
(4) How many JobControllers exist because of preconnect?

Union of (2) and (3) should give (1). 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers?
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

[xunjieli, 2017-01-18 20:56:13]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers?
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

[xunjieli, 2017-01-18 20:56:58]

Done. PTAL?

https://codereview.chromium.org/2623383005/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:323: main_job_count++;
On 2017/01/18 20:39:32, Zhongyi Shi wrote:
> nit: preconnect jobcontrollers will only have main job, which means preconnect
> controllers will be double counted. The current counting will work, it might
be
> better to determine whether the job controller is preconnect, and return early
> so that when we count for pending main jobs, we don't double counting the
> preconnect controllers. Both will work, I'll leave it to you to make the call.

Done.

[xunjieli, 2017-01-18 20:57:32]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP under detailed dumps.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

[Zhongyi Shi, 2017-01-18 21:00:10]

lgtm

[ssid, 2017-01-18 23:51:52]

lgtm, with a few questions.

On 2017/01/17 17:51:24, xunjieli wrote:
> > Is it hard / takes long to compute the size of the jobs?
> > Also, you could use the EstimateMemoryUsage methods to get the size of
> std::set.
> 
> I changed the CL to EstimateMemoryUsage.  The size of the jobs isn't
> straightforward. According to your instrumentation, I think a Job is ~100bytes
> (?) except the case where it is a regular Http1.1 Job and the connection has
> certs and socket buffers. I tried to take the second part into account. 
> 
> I could also add a 100bytes to every Job, or does
EstimateMemoryUsage(std::set)
> already take that into account? 
> 

The current instrumentation makes sense. I do not understand what you mean by
adding 100bytes to every job?
The EstimateMemoryUsage does add sizeof(JobController) for each object in the
set,
(https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/memo...)
The certs and socket buffers should be accounted in
JobController::EstimateMemoryUsage() like you did, since they are just pointers
owned by object.

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory.h:247: const std::string& parent_absolute_name)
const {}
Can we make it pure virtual, so that some other implementations in future does
not miss this? Just an extra definition in the unittest I guess?

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:329: main_job_count++;
I think this was already discussed. Just making sure that we do not miss the
jobs here which does not have both main or alternate job pending.
In case we keep around controllers without any job pending.

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:342: return stats.total_size;
So, this does not include
stream_, websocket_stream_, bidirectional_stream_impl_.
Are these not important?

[xunjieli, 2017-01-19 14:50:42]

>The current instrumentation makes sense. I do not understand what you mean by
> adding 100bytes to every job?
> The EstimateMemoryUsage does add sizeof(JobController) for each object in the
> set,
(https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/memo...)
> The certs and socket buffers should be accounted in
JobController::EstimateMemoryUsage() like you did, since they are just pointers
> owned by object.

Got it. 
I also changed the implementation to skip adding a MemoryAllocatorDump if
job_controller_set_ is empty.

Sid, PTAL.Thanks!

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory.h:247: const std::string& parent_absolute_name)
const {}
On 2017/01/18 23:51:52, ssid wrote:
> Can we make it pure virtual, so that some other implementations in future does
> not miss this? Just an extra definition in the unittest I guess?

Done.

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:329: main_job_count++;
On 2017/01/18 23:51:52, ssid wrote:
> I think this was already discussed. Just making sure that we do not miss the
> jobs here which does not have both main or alternate job pending.
> In case we keep around controllers without any job pending.

I also logged the total size of |job_controller_set_|

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:342: return stats.total_size;
On 2017/01/18 23:51:52, ssid wrote:
> So, this does not include
> stream_, websocket_stream_, bidirectional_stream_impl_.
> Are these not important?

The lifetime of a Job is very short. Once connection is established, stream_ and
websocket_stream_, bidirectional_stream_impl_ are also released to the caller
(usually HttpNetworkTransaction), and Job is destroyed.
Is there an easy way to estimate their size?

[xunjieli, 2017-01-19 14:51:22]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 14:51:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 15:01:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:01:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xunjieli, 2017-01-19 15:11:35]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 15:11:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 16:42:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 16:42:22]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-01-20 20:20:24]

Sorry missed this email.
still lg.
Please update the description of the cl to something like Report memory used by
JobController along with states.
I think the major change is that we report the usage.

Thanks.

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:329: main_job_count++;
On 2017/01/19 14:50:42, xunjieli wrote:
> On 2017/01/18 23:51:52, ssid wrote:
> > I think this was already discussed. Just making sure that we do not miss the
> > jobs here which does not have both main or alternate job pending.
> > In case we keep around controllers without any job pending.
> 
> I also logged the total size of |job_controller_set_|

Ah okay thanks.

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2623383005/diff/100001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:342: return stats.total_size;
On 2017/01/19 14:50:42, xunjieli wrote:
> On 2017/01/18 23:51:52, ssid wrote:
> > So, this does not include
> > stream_, websocket_stream_, bidirectional_stream_impl_.
> > Are these not important?
> 
> The lifetime of a Job is very short. Once connection is established, stream_
and
> websocket_stream_, bidirectional_stream_impl_ are also released to the caller
> (usually HttpNetworkTransaction), and Job is destroyed.
> Is there an easy way to estimate their size?

Okay this is good for now. If we see a lot of difference between the reported
memory and actual usage, lets come back to this issue.

[xunjieli, 2017-01-20 20:22:17]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

[xunjieli, 2017-01-20 20:22:34]

Updated CL description. Thanks!

[xunjieli, 2017-01-20 20:22:46]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-01-20 20:22:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org,
zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2623383005/#ps140001
(title: "self review")

[commit-bot: I haz the power, 2017-01-20 20:23:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-01-20 20:23:21]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider in detailed dumps.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

[commit-bot: I haz the power, 2017-01-20 21:19:02]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484943766310260,
"parent_rev": "2d6a760cbb0204f306e9312bc39036b75f80c5f2", "commit_rev":
"f5267debb22eacffc2287fba55832299ef640bbb"}

[commit-bot: I haz the power, 2017-01-20 21:19:37]

Description was changed from

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108
==========

to

==========
Log states of JobController in Net MemoryDumpProvider

This CL logs states of JobController in network stack's 
MemoryDumpProvider.
This will allow us to see how many pending Jobs there are at a given 
time, and whether there's anything out of ordinary.

The following information will be reported to MDP.

(0) Estimated size of all JobControllers in 
    HttpStreamFactoryImpl::job_controller_set_.
(1) How many JobControllers there are.
(2) How many preconnect JobControllers there are.
(3) How many non-preconnect JobControllers exist because of a pending Alt Job.
(4) How many non-preconnect JobControllers exist because of a pending Main Job.


[Union of 3 and 4] and [2] are disjoint. Together they sum up to [1]. 

BUG=669108

Review-Url: https://codereview.chromium.org/2623383005
Cr-Commit-Position: refs/heads/master@{#445165}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5267debb22eacffc2287fba5583...
==========

[commit-bot: I haz the power, 2017-01-20 21:19:38]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f5267debb22eacffc2287fba5583...