Add a multiplier in tracking certificate memory allocation size

Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, these metrics ("cert_count", "cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108, 671420
Review-Url: https://codereview.chromium.org/2696403007
Cr-Commit-Position: refs/heads/master@{#452555}
Committed: https://chromium.googlesource.com/chromium/src/+/ffe62df6448a031aa6449908076d057dd110d562

[xunjieli, 2017-02-16 22:52:35]

Description was changed from

==========
Add a multiplier in tracking certificate memory allocation size

This CL adds a multiplier (x4) in tracking certificate memory allocation size
in net/ MemoryDumpProvider.

This is to account for the difference between the size of serialized form and
the real allocation size.

This CL additionally adds an undeduped_cert_size in memory dump provider to note
the size of certs before deduplication.

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

to

==========
Add a multiplier in tracking certificate memory allocation size

This CL adds a multiplier (x4) in tracking certificate memory 
allocation size in net/ MemoryDumpProvider.

This is to account for the difference between the size of serialized 
form and the real allocation size.

This CL additionally adds an "undeduped_cert_size" in memory dump 
provider to denote the size of certs before deduplication.

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

[xunjieli, 2017-02-16 22:54:24]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, dskiba@chromium.org

[xunjieli, 2017-02-16 22:54:24]

PTAL. Thanks!
This is the only blocking item for net MemoryDumpProvider to be enabled in
SlowReports. 

Here's a sample trace with this change applied:
https://drive.google.com/open?id=0Bw0C8-TgzJfsYlYwV3hwTTJUcVE

https://codereview.chromium.org/2696403007/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2696403007/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_impl.cc:851: stats->cert_size += 4 * i2d_X509(cert,
nullptr);
I didn't find a way to track distinct X509*. This function is many layers down
from the actual iteration (client_socket_pool_base.cc and spdy_session_pool.cc).
If I pass in a pointer to std::set as a function argument and do operation on
it, there seems to be some optimization that prevents the set<> from seeing
insertions from other loop iterations.


I used a std::unordered_set in ssl_client_session_cache.cc where calculation is
all at one place.

[davidben, 2017-02-16 23:57:44]

lgtm.

So the commit message is clearer, probably worth linking to crbug.com/671420 and
with something like:

"""
net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is being
worked on to resolve this. Until that is done, estimate the cost of the
in-memory representation with a 4x multiplier.
"""

(I want to make sure the state of things is accurately communicated to memory
folks. Otherwise we'll keep having to repeat meetings where context is explained
and delay the actual fixes.)

Also note that ignoring deduplication and ref-counting will grossly overcount
the memory usage. This is fine, but memory people looking at metrics in the
future must be aware of this. In particular, these metrics should not be used
for decisions relating to socket pools; that's where the deduplication matters
most. In HTTP/1.1, we need to open multiple sockets (limit of 6 I believe) to a
single host. Each of those sockets will have the same certificate attached and
only be stored once. Not taking that into account means a 6x overcount.

[xunjieli, 2017-02-17 14:36:25]

Description was changed from

==========
Add a multiplier in tracking certificate memory allocation size

This CL adds a multiplier (x4) in tracking certificate memory 
allocation size in net/ MemoryDumpProvider.

This is to account for the difference between the size of serialized 
form and the real allocation size.

This CL additionally adds an "undeduped_cert_size" in memory dump 
provider to denote the size of certs before deduplication.

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

to

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is being
worked on to resolve this. Until that is done, estimate the cost of the
in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, scheme
triplet, we can have a fair amount of sharing of certs among sockets. Therefore,
this metrics ("cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

[xunjieli, 2017-02-17 14:37:16]

Description was changed from

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is being
worked on to resolve this. Until that is done, estimate the cost of the
in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, scheme
triplet, we can have a fair amount of sharing of certs among sockets. Therefore,
this metrics ("cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

to

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, this metrics ("cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

[xunjieli, 2017-02-17 14:41:03]

On 2017/02/16 23:57:44, davidben wrote:
> lgtm.
> 
> So the commit message is clearer, probably worth linking to crbug.com/671420
and
> with something like:
> 
> """
> net/ tracks the serialized sizes of a certificates, but the in-memory
> representation is much less efficient than that. crbug.com/671420 is being
> worked on to resolve this. Until that is done, estimate the cost of the
> in-memory representation with a 4x multiplier.
> """
> 
> (I want to make sure the state of things is accurately communicated to memory
> folks. Otherwise we'll keep having to repeat meetings where context is
explained
> and delay the actual fixes.)
> 
> Also note that ignoring deduplication and ref-counting will grossly overcount
> the memory usage. This is fine, but memory people looking at metrics in the
> future must be aware of this. In particular, these metrics should not be used
> for decisions relating to socket pools; that's where the deduplication matters
> most. In HTTP/1.1, we need to open multiple sockets (limit of 6 I believe) to
a
> single host. Each of those sockets will have the same certificate attached and
> only be stored once. Not taking that into account means a 6x overcount.

Done. You made it very clear. 
Dmitry: PTAL.

[xunjieli, 2017-02-17 14:41:50]

Description was changed from

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, this metrics ("cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

to

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, these metrics ("cert_count", "cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

[xunjieli, 2017-02-17 14:42:19]

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

[commit-bot: I haz the power, 2017-02-17 14:42:36]

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-02-17 16:22:30]

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

[commit-bot: I haz the power, 2017-02-17 16:22:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DmitrySkiba, 2017-02-17 22:02:58]

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
CRYPTO_BUFFER*> crypto_buffer_set;
I'm worried that this will slow things down. We've seen malloc() (which is
called in the end by operator new) taking very long, and what is worse, it's
unpredictable.

One way to speed things up is to count total number of certs and reserve() that
many elements in the set. That should preallocate buckets, but nodes will still
be allocated on demand.

Another alternative might be base::flat_set, which is backed by std::vector. So
reserve() guarantees that we allocate only once, but at the same time insertion
is n*logn.

BTW, here you are doing find / insert, while you could just always do insert()
and examine resulting pair.

To decide we need some stats on total number of certs / unique number of certs -
can you get that? I would just browse ~10 sites and see (btw,
undeduped_cert_count is not reported).

[davidben, 2017-02-17 22:09:46]

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
CRYPTO_BUFFER*> crypto_buffer_set;
On 2017/02/17 22:02:58, DmitrySkiba wrote:
> I'm worried that this will slow things down. We've seen malloc() (which is
> called in the end by operator new) taking very long, and what is worse, it's
> unpredictable.
> 
> One way to speed things up is to count total number of certs and reserve()
that
> many elements in the set. That should preallocate buckets, but nodes will
still
> be allocated on demand.
> 
> Another alternative might be base::flat_set, which is backed by std::vector.
So
> reserve() guarantees that we allocate only once, but at the same time
insertion
> is n*logn.
> 
> BTW, here you are doing find / insert, while you could just always do insert()
> and examine resulting pair.
> 
> To decide we need some stats on total number of certs / unique number of certs
-
> can you get that? I would just browse ~10 sites and see (btw,
> undeduped_cert_count is not reported).

In the session cache, I would expect it to not do much *for now*. (Though it may
still do something for intermediates... depends a LOT on which 10 sites.) In the
future, with TLS 1.3's single-use sessions, it will be important.

In the socket pools, it would depend heavily on which 10 sites you picked.
HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.

As always, this is the sort of thing where the kind of instrumentation you want
to do is very dependent on how the net stack happens to work right now (with how
it happens to work being a constantly changing thing as we improve it) and also
very dependent on what question you're trying to answer.

[DmitrySkiba, 2017-02-17 22:16:17]

On 2017/02/17 22:09:46, davidben wrote:
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> File net/ssl/ssl_client_session_cache.cc (right):
> 
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
> CRYPTO_BUFFER*> crypto_buffer_set;
> On 2017/02/17 22:02:58, DmitrySkiba wrote:
> > I'm worried that this will slow things down. We've seen malloc() (which is
> > called in the end by operator new) taking very long, and what is worse, it's
> > unpredictable.
> > 
> > One way to speed things up is to count total number of certs and reserve()
> that
> > many elements in the set. That should preallocate buckets, but nodes will
> still
> > be allocated on demand.
> > 
> > Another alternative might be base::flat_set, which is backed by std::vector.
> So
> > reserve() guarantees that we allocate only once, but at the same time
> insertion
> > is n*logn.
> > 
> > BTW, here you are doing find / insert, while you could just always do
insert()
> > and examine resulting pair.
> > 
> > To decide we need some stats on total number of certs / unique number of
certs
> -
> > can you get that? I would just browse ~10 sites and see (btw,
> > undeduped_cert_count is not reported).
> 
> In the session cache, I would expect it to not do much *for now*. (Though it
may
> still do something for intermediates... depends a LOT on which 10 sites.) In
the
> future, with TLS 1.3's single-use sessions, it will be important.
> 
> In the socket pools, it would depend heavily on which 10 sites you picked.
> HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.
> 
> As always, this is the sort of thing where the kind of instrumentation you
want
> to do is very dependent on how the net stack happens to work right now (with
how
> it happens to work being a constantly changing thing as we improve it) and
also
> very dependent on what question you're trying to answer.

Interesting, thanks for the explanation.

How would you estimate number of deduped certs? 100 - 1000?

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

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
CRYPTO_BUFFER*> crypto_buffer_set;
On 2017/02/17 22:02:58, DmitrySkiba wrote:
> I'm worried that this will slow things down. We've seen malloc() (which is
> called in the end by operator new) taking very long, and what is worse, it's
> unpredictable.
> 
> One way to speed things up is to count total number of certs and reserve()
that
> many elements in the set. That should preallocate buckets, but nodes will
still
> be allocated on demand.
> 
> Another alternative might be base::flat_set, which is backed by std::vector.
So
> reserve() guarantees that we allocate only once, but at the same time
insertion
> is n*logn.
> 
> BTW, here you are doing find / insert, while you could just always do insert()
> and examine resulting pair.
> 
> To decide we need some stats on total number of certs / unique number of certs
-
> can you get that? I would just browse ~10 sites and see (btw,
> undeduped_cert_count is not reported).

I don't think we can know the factor of total number of undeduped certs/unique
number of certs. Every browsing session will be different. 
This "cert_size" is a best effort. In the trace linked 
https://drive.google.com/file/d/0Bw0C8-TgzJfsYlYwV3hwTTJUcVE/view?pli=1,  you
can see that the "undeduped_cert_size" is about 1.5 more than "cert_size" in
session cache. Again, this is one run. 
If allocating a std::unordered_set<> is slow, how about I get rid of
"undeduped_cert_size" and reduce the multiplier from 4 to 2? This is sort of
arbitrary anyway.

I don't know a way to estimate the number of deduped certs.

[davidben, 2017-02-17 22:28:21]

On 2017/02/17 22:16:17, DmitrySkiba wrote:
> On 2017/02/17 22:09:46, davidben wrote:
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > File net/ssl/ssl_client_session_cache.cc (right):
> > 
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
> > CRYPTO_BUFFER*> crypto_buffer_set;
> > On 2017/02/17 22:02:58, DmitrySkiba wrote:
> > > I'm worried that this will slow things down. We've seen malloc() (which is
> > > called in the end by operator new) taking very long, and what is worse,
it's
> > > unpredictable.
> > > 
> > > One way to speed things up is to count total number of certs and reserve()
> > that
> > > many elements in the set. That should preallocate buckets, but nodes will
> > still
> > > be allocated on demand.
> > > 
> > > Another alternative might be base::flat_set, which is backed by
std::vector.
> > So
> > > reserve() guarantees that we allocate only once, but at the same time
> > insertion
> > > is n*logn.
> > > 
> > > BTW, here you are doing find / insert, while you could just always do
> insert()
> > > and examine resulting pair.
> > > 
> > > To decide we need some stats on total number of certs / unique number of
> certs
> > -
> > > can you get that? I would just browse ~10 sites and see (btw,
> > > undeduped_cert_count is not reported).
> > 
> > In the session cache, I would expect it to not do much *for now*. (Though it
> may
> > still do something for intermediates... depends a LOT on which 10 sites.) In
> the
> > future, with TLS 1.3's single-use sessions, it will be important.
> > 
> > In the socket pools, it would depend heavily on which 10 sites you picked.
> > HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.
> > 
> > As always, this is the sort of thing where the kind of instrumentation you
> want
> > to do is very dependent on how the net stack happens to work right now (with
> how
> > it happens to work being a constantly changing thing as we improve it) and
> also
> > very dependent on what question you're trying to answer.
> 
> Interesting, thanks for the explanation.
> 
> How would you estimate number of deduped certs? 100 - 1000?

In what context? As always, the answer is "measure it". But this kind of thing
where you play whackamole with all the ways where the numbers are inaccurate
aren't going to do much good.

I think the more important question is: what questions are you trying to make
with this data? What decisions are you trying to make? That will guide whether
any individual simplifications are appropriate or not. I am very worried this
route will lead to more bad decisions like crbug.com/642082, rather than the
better decision paths which led to reprioritizing the CRYPTO_BUFFER work or the
net::IOBuffer work that Helen's been doing (which I still owe her a review
on...).

Are you trying to identify problem areas? Are you trying to evaluate whether
changes in progress helped? Are you trying to evaluate a particular ongoing
experiment like tuning the session cache size?

[davidben, 2017-02-17 22:42:13]

On 2017/02/17 22:28:21, davidben wrote:
> On 2017/02/17 22:16:17, DmitrySkiba wrote:
> > On 2017/02/17 22:09:46, davidben wrote:
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > File net/ssl/ssl_client_session_cache.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
> > > CRYPTO_BUFFER*> crypto_buffer_set;
> > > On 2017/02/17 22:02:58, DmitrySkiba wrote:
> > > > I'm worried that this will slow things down. We've seen malloc() (which
is
> > > > called in the end by operator new) taking very long, and what is worse,
> it's
> > > > unpredictable.
> > > > 
> > > > One way to speed things up is to count total number of certs and
reserve()
> > > that
> > > > many elements in the set. That should preallocate buckets, but nodes
will
> > > still
> > > > be allocated on demand.
> > > > 
> > > > Another alternative might be base::flat_set, which is backed by
> std::vector.
> > > So
> > > > reserve() guarantees that we allocate only once, but at the same time
> > > insertion
> > > > is n*logn.
> > > > 
> > > > BTW, here you are doing find / insert, while you could just always do
> > insert()
> > > > and examine resulting pair.
> > > > 
> > > > To decide we need some stats on total number of certs / unique number of
> > certs
> > > -
> > > > can you get that? I would just browse ~10 sites and see (btw,
> > > > undeduped_cert_count is not reported).
> > > 
> > > In the session cache, I would expect it to not do much *for now*. (Though
it
> > may
> > > still do something for intermediates... depends a LOT on which 10 sites.)
In
> > the
> > > future, with TLS 1.3's single-use sessions, it will be important.
> > > 
> > > In the socket pools, it would depend heavily on which 10 sites you picked.
> > > HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.
> > > 
> > > As always, this is the sort of thing where the kind of instrumentation you
> > want
> > > to do is very dependent on how the net stack happens to work right now
(with
> > how
> > > it happens to work being a constantly changing thing as we improve it) and
> > also
> > > very dependent on what question you're trying to answer.
> > 
> > Interesting, thanks for the explanation.
> > 
> > How would you estimate number of deduped certs? 100 - 1000?
> 
> In what context? As always, the answer is "measure it". But this kind of thing
> where you play whackamole with all the ways where the numbers are inaccurate
> aren't going to do much good.
> 
> I think the more important question is: what questions are you trying to make
> with this data? What decisions are you trying to make? That will guide whether
> any individual simplifications are appropriate or not. I am very worried this
> route will lead to more bad decisions like crbug.com/642082, rather than the
> better decision paths which led to reprioritizing the CRYPTO_BUFFER work or
the
> net::IOBuffer work that Helen's been doing (which I still owe her a review
> on...).
> 
> Are you trying to identify problem areas? Are you trying to evaluate whether
> changes in progress helped? Are you trying to evaluate a particular ongoing
> experiment like tuning the session cache size?

I should probably add: ref-counting and deduplication are *memory
optimizations*. It should set off immediate red flags that one is trying to
answer a memory-related question while ignoring a memory optimization. :-P

I can imagine questions where omitting individual optimizations are a suitable
simplification, but those would be very domain-specific questions to evaluate a
domain-specific decision. From what you're asking, it doesn't sound like there
is such a question being asked right now.

[DmitrySkiba, 2017-02-17 23:07:55]

On 2017/02/17 22:28:21, davidben wrote:
> On 2017/02/17 22:16:17, DmitrySkiba wrote:
> > On 2017/02/17 22:09:46, davidben wrote:
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > File net/ssl/ssl_client_session_cache.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
> > > CRYPTO_BUFFER*> crypto_buffer_set;
> > > On 2017/02/17 22:02:58, DmitrySkiba wrote:
> > > > I'm worried that this will slow things down. We've seen malloc() (which
is
> > > > called in the end by operator new) taking very long, and what is worse,
> it's
> > > > unpredictable.
> > > > 
> > > > One way to speed things up is to count total number of certs and
reserve()
> > > that
> > > > many elements in the set. That should preallocate buckets, but nodes
will
> > > still
> > > > be allocated on demand.
> > > > 
> > > > Another alternative might be base::flat_set, which is backed by
> std::vector.
> > > So
> > > > reserve() guarantees that we allocate only once, but at the same time
> > > insertion
> > > > is n*logn.
> > > > 
> > > > BTW, here you are doing find / insert, while you could just always do
> > insert()
> > > > and examine resulting pair.
> > > > 
> > > > To decide we need some stats on total number of certs / unique number of
> > certs
> > > -
> > > > can you get that? I would just browse ~10 sites and see (btw,
> > > > undeduped_cert_count is not reported).
> > > 
> > > In the session cache, I would expect it to not do much *for now*. (Though
it
> > may
> > > still do something for intermediates... depends a LOT on which 10 sites.)
In
> > the
> > > future, with TLS 1.3's single-use sessions, it will be important.
> > > 
> > > In the socket pools, it would depend heavily on which 10 sites you picked.
> > > HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.
> > > 
> > > As always, this is the sort of thing where the kind of instrumentation you
> > want
> > > to do is very dependent on how the net stack happens to work right now
(with
> > how
> > > it happens to work being a constantly changing thing as we improve it) and
> > also
> > > very dependent on what question you're trying to answer.
> > 
> > Interesting, thanks for the explanation.
> > 
> > How would you estimate number of deduped certs? 100 - 1000?
> 
> In what context? As always, the answer is "measure it". But this kind of thing
> where you play whackamole with all the ways where the numbers are inaccurate
> aren't going to do much good.
> 
> I think the more important question is: what questions are you trying to make
> with this data? What decisions are you trying to make? That will guide whether
> any individual simplifications are appropriate or not. I am very worried this
> route will lead to more bad decisions like crbug.com/642082, rather than the
> better decision paths which led to reprioritizing the CRYPTO_BUFFER work or
the
> net::IOBuffer work that Helen's been doing (which I still owe her a review
> on...).

:/

I don't know why you keep bitting that horse, but fine:

1. My original solution (https://codereview.chromium.org/2289933002) didn't have
the side effect of creating extra x509 certs.

2. Memory effectiveness of the solution we ended up with (implemented by
rsleevi@) depends on ratio of all / unique certs. The measurements in the bug
give two values: 244/84 (<4) and 1027/209 (>4). If the magic number is 4, then
we lost in the first case and won in the second.


> Are you trying to identify problem areas? Are you trying to evaluate whether
> changes in progress helped? Are you trying to evaluate a particular ongoing
> experiment like tuning the session cache size?

I'm trying to decide between flat_map and unordered_map. We can make flat_map
allocate only once, but then each search is logn + possible insertion. On the
other hand, unordered_map will call malloc() for each element.

So the question is: what is faster, N insertions into flat_map or N malloc()
calls?

Hmm, having laid it all down I think we should just go with flat_map.

[xunjieli, 2017-02-21 20:35:22]

On 2017/02/17 23:07:55, DmitrySkiba wrote:
> On 2017/02/17 22:28:21, davidben wrote:
> > On 2017/02/17 22:16:17, DmitrySkiba wrote:
> > > On 2017/02/17 22:09:46, davidben wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > > File net/ssl/ssl_client_session_cache.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_...
> > > > net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const
> > > > CRYPTO_BUFFER*> crypto_buffer_set;
> > > > On 2017/02/17 22:02:58, DmitrySkiba wrote:
> > > > > I'm worried that this will slow things down. We've seen malloc()
(which
> is
> > > > > called in the end by operator new) taking very long, and what is
worse,
> > it's
> > > > > unpredictable.
> > > > > 
> > > > > One way to speed things up is to count total number of certs and
> reserve()
> > > > that
> > > > > many elements in the set. That should preallocate buckets, but nodes
> will
> > > > still
> > > > > be allocated on demand.
> > > > > 
> > > > > Another alternative might be base::flat_set, which is backed by
> > std::vector.
> > > > So
> > > > > reserve() guarantees that we allocate only once, but at the same time
> > > > insertion
> > > > > is n*logn.
> > > > > 
> > > > > BTW, here you are doing find / insert, while you could just always do
> > > insert()
> > > > > and examine resulting pair.
> > > > > 
> > > > > To decide we need some stats on total number of certs / unique number
of
> > > certs
> > > > -
> > > > > can you get that? I would just browse ~10 sites and see (btw,
> > > > > undeduped_cert_count is not reported).
> > > > 
> > > > In the session cache, I would expect it to not do much *for now*.
(Though
> it
> > > may
> > > > still do something for intermediates... depends a LOT on which 10
sites.)
> In
> > > the
> > > > future, with TLS 1.3's single-use sessions, it will be important.
> > > > 
> > > > In the socket pools, it would depend heavily on which 10 sites you
picked.
> > > > HTTP/1.1 you can expect it to be valuable. HTTP/2 probably less so.
> > > > 
> > > > As always, this is the sort of thing where the kind of instrumentation
you
> > > want
> > > > to do is very dependent on how the net stack happens to work right now
> (with
> > > how
> > > > it happens to work being a constantly changing thing as we improve it)
and
> > > also
> > > > very dependent on what question you're trying to answer.
> > > 
> > > Interesting, thanks for the explanation.
> > > 
> > > How would you estimate number of deduped certs? 100 - 1000?
> > 
> > In what context? As always, the answer is "measure it". But this kind of
thing
> > where you play whackamole with all the ways where the numbers are inaccurate
> > aren't going to do much good.
> > 
> > I think the more important question is: what questions are you trying to
make
> > with this data? What decisions are you trying to make? That will guide
whether
> > any individual simplifications are appropriate or not. I am very worried
this
> > route will lead to more bad decisions like crbug.com/642082, rather than the
> > better decision paths which led to reprioritizing the CRYPTO_BUFFER work or
> the
> > net::IOBuffer work that Helen's been doing (which I still owe her a review
> > on...).
> 
> :/
> 
> I don't know why you keep bitting that horse, but fine:
> 
> 1. My original solution (https://codereview.chromium.org/2289933002) didn't
have
> the side effect of creating extra x509 certs.
> 
> 2. Memory effectiveness of the solution we ended up with (implemented by
> rsleevi@) depends on ratio of all / unique certs. The measurements in the bug
> give two values: 244/84 (<4) and 1027/209 (>4). If the magic number is 4, then
> we lost in the first case and won in the second.
> 
> 
> > Are you trying to identify problem areas? Are you trying to evaluate whether
> > changes in progress helped? Are you trying to evaluate a particular ongoing
> > experiment like tuning the session cache size?
> 
> I'm trying to decide between flat_map and unordered_map. We can make flat_map
> allocate only once, but then each search is logn + possible insertion. On the
> other hand, unordered_map will call malloc() for each element.
> 
> So the question is: what is faster, N insertions into flat_map or N malloc()
> calls?
> 
> Hmm, having laid it all down I think we should just go with flat_map.

Done. I used flat_set instead of unordered_set. Dmitry: PTAL. Thanks!

[xunjieli, 2017-02-21 20:35:28]

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

[commit-bot: I haz the power, 2017-02-21 20:36:30]

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

[DmitrySkiba, 2017-02-21 22:08:26]

https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache.cc:130: base::flat_set<const CRYPTO_BUFFER*>
crypto_buffer_set;
Two things:

1. We need to resize crypto_buffer_set before using it, to make sure it doesn't
allocate. The only way to do this (I think) is to count total_cert_count in a
separate loop.

2. Instead of find/insert you can just always call insert() and analyze its
return value. That'll avoid double lookups when we insert new cert.

[xunjieli, 2017-02-21 22:29:56]

On 2017/02/21 22:08:26, DmitrySkiba wrote:
>
https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
> File net/ssl/ssl_client_session_cache.cc (right):
> 
>
https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
> net/ssl/ssl_client_session_cache.cc:130: base::flat_set<const CRYPTO_BUFFER*>
> crypto_buffer_set;
> Two things:
> 
> 1. We need to resize crypto_buffer_set before using it, to make sure it
doesn't
> allocate. The only way to do this (I think) is to count total_cert_count in a
> separate loop.
> 
> 2. Instead of find/insert you can just always call insert() and analyze its
> return value. That'll avoid double lookups when we insert new cert.

Done. PTAL. Thanks.

[DmitrySkiba, 2017-02-21 22:47:39]

On 2017/02/21 22:29:56, xunjieli wrote:
> On 2017/02/21 22:08:26, DmitrySkiba wrote:
> >
>
https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
> > File net/ssl/ssl_client_session_cache.cc (right):
> > 
> >
>
https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_sess...
> > net/ssl/ssl_client_session_cache.cc:130: base::flat_set<const
CRYPTO_BUFFER*>
> > crypto_buffer_set;
> > Two things:
> > 
> > 1. We need to resize crypto_buffer_set before using it, to make sure it
> doesn't
> > allocate. The only way to do this (I think) is to count total_cert_count in
a
> > separate loop.
> > 
> > 2. Instead of find/insert you can just always call insert() and analyze its
> > return value. That'll avoid double lookups when we insert new cert.
> 
> Done. PTAL. Thanks.

LGTM, but please wait until I check the performance of this.

[xunjieli, 2017-02-23 15:35:40]

Thanks Dmitry. Since perf measurements showed that this is fine, I am checking
in this now.

[xunjieli, 2017-02-23 15:35:41]

Thanks Dmitry. Since perf measurements showed that this is fine, I am checking
in this now.

[xunjieli, 2017-02-23 15:35:47]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-23 15:35:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, dskiba@chromium.org
Link to the patchset: https://codereview.chromium.org/2696403007/#ps100001
(title: "self")

[commit-bot: I haz the power, 2017-02-23 15:36:16]

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-02-23 16:37:17]

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

[commit-bot: I haz the power, 2017-02-23 16:37:19]

Failed to apply patch for net/spdy/spdy_session_pool.cc:
While running git apply --index -p1;
  error: patch failed: net/spdy/spdy_session_pool.cc:380
  error: net/spdy/spdy_session_pool.cc: patch does not apply

Patch:       net/spdy/spdy_session_pool.cc
Index: net/spdy/spdy_session_pool.cc
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index
b014e51657538121c30bd341cad51b9016f99f4b..fd254f1f5927967019963cc50452510b7516c3ab
100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -380,7 +380,7 @@ void SpdySessionPool::DumpMemoryStats(
   size_t total_size = 0;
   size_t buffer_size = 0;
   size_t cert_count = 0;
-  size_t serialized_cert_size = 0;
+  size_t cert_size = 0;
   size_t num_active_sessions = 0;
   for (const auto& session : sessions_) {
     StreamSocket::SocketMemoryStats stats;
@@ -388,7 +388,7 @@ void SpdySessionPool::DumpMemoryStats(
     total_size += session->DumpMemoryStats(&stats, &is_session_active);
     buffer_size += stats.buffer_size;
     cert_count += stats.cert_count;
-    serialized_cert_size += stats.serialized_cert_size;
+    cert_size += stats.cert_size;
     if (is_session_active)
       num_active_sessions++;
   }
@@ -412,9 +412,9 @@ void SpdySessionPool::DumpMemoryStats(
   dump->AddScalar("cert_count",
                   base::trace_event::MemoryAllocatorDump::kUnitsObjects,
                   cert_count);
-  dump->AddScalar("serialized_cert_size",
+  dump->AddScalar("cert_size",
                   base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                  serialized_cert_size);
+                  cert_size);
 }
 
 bool SpdySessionPool::IsSessionAvailable(

[xunjieli, 2017-02-23 17:22:09]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-23 17:22:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, dskiba@chromium.org
Link to the patchset: https://codereview.chromium.org/2696403007/#ps120001
(title: "self")

[commit-bot: I haz the power, 2017-02-23 17:22:45]

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-02-23 18:22:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487870529955400,
"parent_rev": "5aa7b9c3dd6963f4afa93c23053634d36ff1a872", "commit_rev":
"ffe62df6448a031aa6449908076d057dd110d562"}

[commit-bot: I haz the power, 2017-02-23 18:23:56]

Description was changed from

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, these metrics ("cert_count", "cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420
==========

to

==========
Add a multiplier in tracking certificate memory allocation size

net/ tracks the serialized sizes of a certificates, but the in-memory
representation is much less efficient than that. crbug.com/671420 is 
being worked on to resolve this. Until that is done, estimate the cost 
of the in-memory representation with a 4x multiplier.

Note that this CL ignores deduplication/ref-countedness of X509* in 
ssl_client_socket_impl.cc. This will overcount memory usage if sockets
share the same certs. Since we allow 6 sockets to the same host, port, 
scheme triplet, we can have a fair amount of sharing of certs among 
sockets. Therefore, these metrics ("cert_count", "cert_size" and 
"undeduped_cert_size") should not be used to make decisions on changing 
socket pool behavior. 

BUG=669108,671420

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

[commit-bot: I haz the power, 2017-02-23 18:23:57]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ffe62df6448a031aa6449908076d...