Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by xunjieli
Modified:
3 months 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 Review-Url: https://codereview.chromium.org/2696403007 Cr-Commit-Position: refs/heads/master@{#452555} Committed: https://chromium.googlesource.com/chromium/src/+/ffe62df6448a031aa6449908076d057dd110d562

Patch Set 1 #

Total comments: 4

Patch Set 2 : use flat set #

Total comments: 1

Patch Set 3 : address comments #

Patch Set 4 : rename undeduped #

Patch Set 5 : fix rename #

Patch Set 6 : self #

Patch Set 7 : self #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -34 lines) Patch
M net/socket/client_socket_pool_base.cc View 1 2 3 4 3 chunks +4 lines, -5 lines 0 comments Download
M net/socket/ssl_client_socket_impl.cc View 1 2 3 4 5 6 1 chunk +6 lines, -5 lines 0 comments 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 1 2 3 4 5 6 3 chunks +4 lines, -4 lines 0 comments Download
M net/ssl/ssl_client_session_cache.cc View 1 2 3 4 5 2 chunks +35 lines, -14 lines 0 comments Download
M net/ssl/ssl_client_session_cache_unittest.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 38 (18 generated)
xunjieli
PTAL. Thanks! This is the only blocking item for net MemoryDumpProvider to be enabled in ...
3 months, 1 week 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 months, 1 week 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, ...
3 months, 1 week 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 ...
3 months, 1 week 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: > ...
3 months, 1 week 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 > ...
3 months, 1 week 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: > ...
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 months, 1 week ago (2017-02-17 22:42:13 UTC) #18
DmitrySkiba
On 2017/02/17 22:28:21, davidben wrote: > On 2017/02/17 22:16:17, DmitrySkiba wrote: > > On 2017/02/17 ...
3 months, 1 week ago (2017-02-17 23:07:55 UTC) #19
xunjieli
On 2017/02/17 23:07:55, DmitrySkiba wrote: > On 2017/02/17 22:28:21, davidben wrote: > > On 2017/02/17 ...
3 months ago (2017-02-21 20:35:22 UTC) #20
DmitrySkiba
https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_session_cache.cc File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_session_cache.cc#newcode130 net/ssl/ssl_client_session_cache.cc:130: base::flat_set<const CRYPTO_BUFFER*> crypto_buffer_set; Two things: 1. We need to ...
3 months ago (2017-02-21 22:08:26 UTC) #23
xunjieli
On 2017/02/21 22:08:26, DmitrySkiba wrote: > https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_session_cache.cc > File net/ssl/ssl_client_session_cache.cc (right): > > https://codereview.chromium.org/2696403007/diff/20001/net/ssl/ssl_client_session_cache.cc#newcode130 > ...
3 months ago (2017-02-21 22:29:56 UTC) #24
DmitrySkiba
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_session_cache.cc ...
3 months ago (2017-02-21 22:47:39 UTC) #25
xunjieli
Thanks Dmitry. Since perf measurements showed that this is fine, I am checking in this ...
3 months ago (2017-02-23 15:35:40 UTC) #26
xunjieli
Thanks Dmitry. Since perf measurements showed that this is fine, I am checking in this ...
3 months ago (2017-02-23 15:35:41 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696403007/100001
3 months ago (2017-02-23 15:36:16 UTC) #30
commit-bot: I haz the power
Failed to apply patch for net/spdy/spdy_session_pool.cc: While running git apply --index -p1; error: patch failed: ...
3 months ago (2017-02-23 16:37:19 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696403007/120001
3 months ago (2017-02-23 17:22:45 UTC) #35
commit-bot: I haz the power
3 months ago (2017-02-23 18:23:57 UTC) #38
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ffe62df6448a031aa6449908076d...
Sign in to reply to this message.

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