Chromium Code Reviews
Help | Chromium Project | Sign in
(7)

Issue 2696403007: Add a multiplier in tracking certificate memory allocation size

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 days, 3 hours ago by xunjieli
Modified:
2 days, 2 hours ago
Reviewers:
davidben, DmitrySkiba
CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, net-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -25 lines) Patch
M net/socket/client_socket_pool_base.cc View 3 chunks +4 lines, -5 lines 0 comments Download
M net/socket/ssl_client_socket_impl.cc View 1 chunk +6 lines, -5 lines 1 comment Download
M net/socket/ssl_client_socket_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M net/socket/stream_socket.h View 1 chunk +1 line, -1 line 0 comments Download
M net/socket/stream_socket.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/spdy/spdy_session_pool.cc View 3 chunks +4 lines, -4 lines 0 comments Download
M net/ssl/ssl_client_session_cache.cc View 2 chunks +19 lines, -5 lines 3 comments Download
M net/ssl/ssl_client_session_cache_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 19 (9 generated)
xunjieli
PTAL. Thanks! This is the only blocking item for net MemoryDumpProvider to be enabled in ...
3 days, 2 hours ago (2017-02-16 22:54:24 UTC) #3
davidben
lgtm. So the commit message is clearer, probably worth linking to crbug.com/671420 and with something ...
3 days, 1 hour ago (2017-02-16 23:57:44 UTC) #4
xunjieli
On 2017/02/16 23:57:44, davidben wrote: > lgtm. > > So the commit message is clearer, ...
2 days, 11 hours ago (2017-02-17 14:41:03 UTC) #7
DmitrySkiba
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc#newcode129 net/ssl/ssl_client_session_cache.cc:129: std::unordered_set<const CRYPTO_BUFFER*> crypto_buffer_set; I'm worried that this will slow ...
2 days, 3 hours ago (2017-02-17 22:02:58 UTC) #13
davidben
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc#newcode129 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: > ...
2 days, 3 hours ago (2017-02-17 22:09:46 UTC) #14
DmitrySkiba
On 2017/02/17 22:09:46, davidben wrote: > https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc > File net/ssl/ssl_client_session_cache.cc (right): > > https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc#newcode129 > ...
2 days, 3 hours ago (2017-02-17 22:16:17 UTC) #15
xunjieli
https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2696403007/diff/1/net/ssl/ssl_client_session_cache.cc#newcode129 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: > ...
2 days, 3 hours ago (2017-02-17 22:22:01 UTC) #16
davidben
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_cache.cc ...
2 days, 3 hours ago (2017-02-17 22:28:21 UTC) #17
davidben
On 2017/02/17 22:28:21, davidben wrote: > On 2017/02/17 22:16:17, DmitrySkiba wrote: > > On 2017/02/17 ...
2 days, 3 hours ago (2017-02-17 22:42:13 UTC) #18
DmitrySkiba
2 days, 2 hours ago (2017-02-17 23:07:55 UTC) #19
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd