[OBSOLETE][memory-infra] Whitelist net dump providers in field trials

[memory-infra] Whitelist net dump providers in field trials

Remove details about url_request_context in background mode since it
does not add much information due to crbug.com/688128. Add the dump
provider and dump names to whitelist for field trials.

Alos fix the discardable provider names.

BUG=677346

[ssid, 2017-02-02 22:09:09]

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

[commit-bot: I haz the power, 2017-02-02 22:09:34]

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

[ssid, 2017-02-02 22:10:19]

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

[ssid, 2017-02-02 22:10:20]

ptal thanks.

[xunjieli, 2017-02-02 22:33:27]

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
net/url_request/url_request_context.cc:133: // The details about the url request
name is not added in BACKGROUND mode
nit: s/is/are
Think you meant to say "url request context" instead of "url request"

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
net/url_request/url_request_context.cc:138: dump_name =
base::StringPrintf("net/url_request_context/%s_%p",
I was about to instrument net::URLRequest and attribute the memory allocation to
net::URLRequestContext. If you get rid of the "/url_request_context" dump in
background mode, how do I attribute the memory of net::URLRequest?

CL is at https://codereview.chromium.org/2540233002/. URLRequestContext
(indirectly) "owns" a set of URLRequests.

[ssid, 2017-02-02 23:24:57]

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
net/url_request/url_request_context.cc:138: dump_name =
base::StringPrintf("net/url_request_context/%s_%p",
On 2017/02/02 22:33:27, xunjieli wrote:
> I was about to instrument net::URLRequest and attribute the memory allocation
to
> net::URLRequestContext. If you get rid of the "/url_request_context" dump in
> background mode, how do I attribute the memory of net::URLRequest?
> 
> CL is at https://codereview.chromium.org/2540233002/. URLRequestContext
> (indirectly) "owns" a set of URLRequests.

I see. I removed the url_requet_context dumps because they were just taking more
space in trace without any memory attributed to them. If you are going to add
child dumps then we should include these. Currently the only reason for these
dumps to exist is to add the name of the context in trace.

Okay wdyt should be done here? Should we enable the existing net provider in
slow reports and then continue to add more details or should we just wait till
you finish instrumenting all parts then turn on for slow reports? I thought the
instrumentation is mostly done and you mailed about asking to enable on slow
reports, so sent this CL.

The other issue is every time you add more details you'd have to update the
whitelist and also take care that the performance of the provider is not
regressing much since it will be directly enabled on user devices and the perf
tests would not catch this regression since telemetry does not support net very
well.

[commit-bot: I haz the power, 2017-02-02 23:33:49]

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

[commit-bot: I haz the power, 2017-02-02 23:33:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xunjieli, 2017-02-02 23:41:06]

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
net/url_request/url_request_context.cc:138: dump_name =
base::StringPrintf("net/url_request_context/%s_%p",
On 2017/02/02 23:24:57, ssid wrote:
> On 2017/02/02 22:33:27, xunjieli wrote:
> > I was about to instrument net::URLRequest and attribute the memory
allocation
> to
> > net::URLRequestContext. If you get rid of the "/url_request_context" dump in
> > background mode, how do I attribute the memory of net::URLRequest?
> > 
> > CL is at https://codereview.chromium.org/2540233002/. URLRequestContext
> > (indirectly) "owns" a set of URLRequests.
> 
> I see. I removed the url_requet_context dumps because they were just taking
more
> space in trace without any memory attributed to them. If you are going to add
> child dumps then we should include these. Currently the only reason for these
> dumps to exist is to add the name of the context in trace.
> 
> Okay wdyt should be done here? Should we enable the existing net provider in
> slow reports and then continue to add more details or should we just wait till
> you finish instrumenting all parts then turn on for slow reports? I thought
the
> instrumentation is mostly done and you mailed about asking to enable on slow
> reports, so sent this CL.
> 
> The other issue is every time you add more details you'd have to update the
> whitelist and also take care that the performance of the provider is not
> regressing much since it will be directly enabled on user devices and the perf
> tests would not catch this regression since telemetry does not support net
very
> well.

I see. I seriously wonder whether there is an end to the instrumentation. As I
try to instrument more of the codebase, I find even more to instrument. Just see
how many TODOs (!) I left behind :).
I think we should go ahead and enable this in SlowReports. It might be another
two quarters when net/ is properly instrumented. We might find interesting
behavior in the meanwhile.
I am fine with hiding "url_request_context" in background mode until they have
memory attributed to them.

[ssid, 2017-02-03 02:26:00]

Done. thanks

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/1/net/url_request/url_request...
net/url_request/url_request_context.cc:133: // The details about the url request
name is not added in BACKGROUND mode
On 2017/02/02 22:33:27, xunjieli wrote:
> nit: s/is/are
> Think you meant to say "url request context" instead of "url request"

Done.

[Primiano Tucci (use gerrit), 2017-02-03 03:18:50]

base/trave_event LGTM
take a look to the comment about the string though, I think it might screw up
background dumps passing an empty string.

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.cc (left):

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.cc:141:
SSLClientSocketImpl::DumpSSLClientSessionMemoryStats(pmd);
just checking, is this removal intended or just accidental?

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.cc:138: dump_name =
base::StringPrintf("net/url_request_context/%s_%p",
don't you have to do this outside of the if?
otherwise in background mode the DumpMemoryStats calls below will pass an empty
dump_name

[ssid, 2017-02-03 03:27:56]

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.cc (left):

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.cc:141:
SSLClientSocketImpl::DumpSSLClientSessionMemoryStats(pmd);
On 2017/02/03 03:18:50, Primiano Tucci wrote:
> just checking, is this removal intended or just accidental?

I moved it up since it is a static function call.
Line 131 in right side.

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2667293003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.cc:138: dump_name =
base::StringPrintf("net/url_request_context/%s_%p",
On 2017/02/03 03:18:50, Primiano Tucci wrote:
> don't you have to do this outside of the if?
> otherwise in background mode the DumpMemoryStats calls below will pass an
empty
> dump_name

I am intentionally passing empty dump name here so that the
session::DumpMemoryStats will not add a sub-allocation edge to the parent. Since
we do not attribute the memory to url_request_context anyway, there is no reason
to create these empty dumps in background mode. All we need to see in slow
reports is how much memory http_network_session is using. So, removing these
dumps for now.
I think Helen said she will add these back later in some other CL where she adds
children to these objects.

[xunjieli, 2017-02-03 14:33:57]

LGTM One last question: How do I make sure the performance of the provider
doesn't regress much when I add more EMU()s ?

[ssid, 2017-02-03 19:10:05]

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

[commit-bot: I haz the power, 2017-02-03 19:10:29]

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

[ssid, 2017-02-03 19:20:35]

On 2017/02/03 14:33:57, xunjieli wrote:
> LGTM One last question: How do I make sure the performance of the provider
> doesn't regress much when I add more EMU()s ?

That is a tricky question to answer in case of net. The existing plan was to
measure the performance of the dump provider using telemetry benchmark.
See
https://chromeperf.appspot.com/report?sid=e13de0de3e6ce2f211472737ab0e2237f9d...

This shows the overhead of the dump provider over time. So, that we don't
regress performance of dump provider in the field.
Now the issue here is for net we do not have a good way to keep track of this in
telemetry. So, we have to manually investigate before adding each detail if we
do not regress performance.

There are 2 way to solve this issue:
1. We can locally prepare test cases for the parts of code that is being changes
and test on our devices if the performance is good. This can be done using the
MemoryDumpManager::InvokeOnMemoryDump trace event on ChromeIOThread, which has
"URLRequestContext" as dump provider name. There would be multiple because of
multiple dump provider. A total of them all will give us the cpu overhead. Right
now I see around 1ms which seems okay. But, lets not regress a lot. Other dump
providers usually have 0.1ms CPU overheads.
2. We can check the overhead from the field on the trace from slow reports
dashboard. The tricky part is the traces form the field do not have the provider
name argument for privacy reasons. But, we know that the only dump provider
enabled on IO thread for slow reports is net providers. So, we can see the
overhead from the traces in field.

We can come up with better solution here like having a mapper that processes all
traces from the field and keeps track of the overhead of dump provider and we
can see impact on users. But that seems to be an overkill if we can keep this
under control with local testing.

[xunjieli, 2017-02-03 19:56:46]

On 2017/02/03 19:20:35, ssid wrote:
> On 2017/02/03 14:33:57, xunjieli wrote:
> > LGTM One last question: How do I make sure the performance of the provider
> > doesn't regress much when I add more EMU()s ?
> 
> That is a tricky question to answer in case of net. The existing plan was to
> measure the performance of the dump provider using telemetry benchmark.
> See
>
https://chromeperf.appspot.com/report?sid=e13de0de3e6ce2f211472737ab0e2237f9d...
> 
> This shows the overhead of the dump provider over time. So, that we don't
> regress performance of dump provider in the field.
> Now the issue here is for net we do not have a good way to keep track of this
in
> telemetry. So, we have to manually investigate before adding each detail if we
> do not regress performance.
> 
> There are 2 way to solve this issue:
> 1. We can locally prepare test cases for the parts of code that is being
changes
> and test on our devices if the performance is good. This can be done using the
> MemoryDumpManager::InvokeOnMemoryDump trace event on ChromeIOThread, which has
> "URLRequestContext" as dump provider name. There would be multiple because of
> multiple dump provider. A total of them all will give us the cpu overhead.
Right
> now I see around 1ms which seems okay. But, lets not regress a lot. Other dump
> providers usually have 0.1ms CPU overheads.

I think we need a better solution than this. The instrumentation is not one-off
thing nor a one-person project. If we have a new feature, we will require the
new classes to have EMU()s and hooked up with net/ MDP. So in the long run, I am
not the only one who will modify net/ MDP. We need a more robust way to know
that we won't affect users :(

> 2. We can check the overhead from the field on the trace from slow reports
> dashboard. The tricky part is the traces form the field do not have the
provider
> name argument for privacy reasons. But, we know that the only dump provider
> enabled on IO thread for slow reports is net providers. So, we can see the
> overhead from the traces in field.
> 
> We can come up with better solution here like having a mapper that processes
all
> traces from the field and keeps track of the overhead of dump provider and we
> can see impact on users. But that seems to be an overkill if we can keep this
> under control with local testing.

[Primiano Tucci (use gerrit), 2017-02-03 20:20:11]

On 2017/02/03 19:20:35, ssid wrote:
> On 2017/02/03 14:33:57, xunjieli wrote:
> > LGTM One last question: How do I make sure the performance of the provider
> > doesn't regress much when I add more EMU()s ?
> 
> That is a tricky question to answer in case of net. The existing plan was to
> measure the performance of the dump provider using telemetry benchmark.
> See
>
https://chromeperf.appspot.com/report?sid=e13de0de3e6ce2f211472737ab0e2237f9d...

> This shows the overhead of the dump provider over time. So, that we don't
> regress performance of dump provider in the field.
Yeah the way to go is to have coverage in telemetry similarly to the other
cases.

> Now the issue here is for net we do not have a good way to keep track of this
in
> telemetry. So, we have to manually investigate before adding each detail if we
> do not regress performance.
Let me se if I am getting this right. this is because we don't have harness that
exercises QUIC in telemetry?
If to I think this is the real problem to solve.
There was a bug somehere about this, what is the status of it?

[Primiano Tucci (use gerrit), 2017-02-03 20:22:07]

On 2017/02/03 19:20:35, ssid wrote:
> 2. We can check the overhead from the field on the trace from slow reports
> dashboard. The tricky part is the traces form the field do not have the
provider
> name argument for privacy reasons. But, we know that the only dump provider
> enabled on IO thread for slow reports is net providers. So, we can see the
> overhead from the traces in field.

Hmm I am not too happy to have multiple places to monitor, we'll lose track of
them.
Also, very likely data form the field will be super noisy.

[xunjieli, 2017-02-03 20:22:48]

> Let me se if I am getting this right. this is because we don't have harness
that
> exercises QUIC in telemetry?
> If to I think this is the real problem to solve.
> There was a bug somehere about this, what is the status of it?

Correct. The telemetry tests do not exercise disk cache, socket pool logic, QUIC
or HTTP2. There is currently no bug opened. Primiano@: do you want to open one
or should we schedule a meeting to talk about it? To improve telemetry w.r.t
networking, we need non-trivial investment.

[commit-bot: I haz the power, 2017-02-03 21:16:27]

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

[commit-bot: I haz the power, 2017-02-03 21:16:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ssid, 2017-02-03 22:20:29]

Patchset #3 (id:40001) has been deleted

[ssid, 2017-02-21 18:00:32]

Patchset #3 (id:60001) has been deleted

[ssid, 2017-02-23 20:36:28]

Description was changed from

==========
[memory-infra] Whitelist net dump providers in field trials

Remove details about url_request_context in background mode since it
does not add much information due to crbug.com/688128. Add the dump
provider and dump names to whitelist for field trials.

Alos fix the discardable provider names.

BUG=677346
==========

to

==========
[memory-infra] Whitelist net dump providers in field trials

Remove details about url_request_context in background mode since it
does not add much information due to crbug.com/688128. Add the dump
provider and dump names to whitelist for field trials.

Alos fix the discardable provider names.

BUG=677346
==========