Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625883002:
SSLClientSessionCache: Log number of times Lookup is called per Session. (Closed)
DescriptionSSLClientSessionCache: Log number of times Lookup is called per Session.
Session tickets in TLS 1.3 will be single-use, so to be able to resume
multiple sessions to the same host, we will need to store multiple
session tickets per host. By counting how many times Lookup is called
per session in an SSLClientSessionCache, we can tune how many session
tickets per host we need to store for TLS 1.3.
BUG=631988
Review-Url: https://codereview.chromium.org/2625883002
Cr-Commit-Position: refs/heads/master@{#444902}
Committed: https://chromium.googlesource.com/chromium/src/+/00d06ff78fc0705706dd0c7db1170072072e8225
Patch Set 1 #
Total comments: 8
Patch Set 2 : log number of concurrent handshakes with the same session, not total number of times a session was … #
Total comments: 3
Patch Set 3 : reset count on handshake completion; log count from lookup #
Total comments: 12
Patch Set 4 : rebase #Patch Set 5 : reply to comments #
Total comments: 8
Patch Set 6 : reply to comments #
Messages
Total messages: 24 (7 generated)
nharper@chromium.org changed reviewers: + davidben@chromium.org
https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.cc:64: iter->second->lookup_count++; To simulate TLS 1.3 renewal, I think we want this number to get reset whenever the handshake finishes. https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.cc:66: iter->second->lookup_count, 20); This will record for both HTTP/1.1 and HTTP/2 whereas we want to have the numbers separate. Perhaps have SSLClientSessionCache return the lookup count too and SSLClientSocket can record it based on whether the session got resumed, etc. https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.h:68: size_t lookup_count; If you just write size_t lookup_count = 0 here, I think you can avoid the out-of-line constructor and destructor? Or do the clang plugins complain? https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.h:90: base::HashingMRUCache<std::string, std::unique_ptr<Entry>> cache_; Does it work to do base::HashingMRUCache<std::string, Entry> cache_? Saves a bunch of needless mallocs. I think the compiler will just generate a move constructor if you don't explicitly define Entry and ~Entry.
https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.cc:64: iter->second->lookup_count++; On 2017/01/11 16:47:49, davidben wrote: > To simulate TLS 1.3 renewal, I think we want this number to get reset whenever > the handshake finishes. You're right that I need to consider when the handshake finishes, but I'm not convinced that resetting it on handshake completion is the correct behavior. I think the metric we want to look at is "how many concurrent handshakes are using the same session?". If I change lookup_count to be something like concurrent_handshake_count, where it gets incremented on lookup and decremented on handshake completion (with some carefulness to make sure increments and decrements are always paired), I think that would give us the right number. That still leaves open the question of when to log this number to UMA. https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.cc:66: iter->second->lookup_count, 20); On 2017/01/11 16:47:50, davidben wrote: > This will record for both HTTP/1.1 and HTTP/2 whereas we want to have the > numbers separate. Perhaps have SSLClientSessionCache return the lookup count too > and SSLClientSocket can record it based on whether the session got resumed, etc. It sounds like returning an Entry and having SSLClientSocketImpl handle recording the value (and possibly the bookkeeping of the count) would be reasonable. https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.h:68: size_t lookup_count; On 2017/01/11 16:47:50, davidben wrote: > If you just write size_t lookup_count = 0 here, I think you can avoid the > out-of-line constructor and destructor? Or do the clang plugins complain? The chromium-style clang plugin complains that this struct with just two members is complex, thus it needs an explicit out-of-line c'tor/d'tor. https://codereview.chromium.org/2625883002/diff/1/net/ssl/ssl_client_session_... net/ssl/ssl_client_session_cache.h:90: base::HashingMRUCache<std::string, std::unique_ptr<Entry>> cache_; On 2017/01/11 16:47:50, davidben wrote: > Does it work to do > > base::HashingMRUCache<std::string, Entry> cache_? > > Saves a bunch of needless mallocs. I think the compiler will just generate a > move constructor if you don't explicitly define Entry and ~Entry. I'd prefer to it to be base::HashingMRUCache<std::string, Entry>, but clang was complaining about it when it was like that. For what it's worth, this used to be a base::HashingMRUCache<std::string, std::unique_ptr<CacheEntry>> (where CacheEntry had a bssl::UniquePtr<SSL_SESSION> member), so it's no more indirection/mallocs than that. I'll try poking at it again, since I don't see a reason why putting Entry in directly shouldn't work.
This should have the metric recording the correct value now. I also fixed putting Entry directly in the MRUCache instead of a unique_ptr.
https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2001: GetSessionCacheKey(), negotiated_protocol_ == kProtoHTTP2); Doing it in LogConnectEndEvent seems wrong. If Init() fails, you never did an SSL_SESSION lookup. This gets called on failed connects too. I would think we only need to do anything on successful connects (on failed connect, that would still count as "using" the session since we've leaked it). On successful connect, we would expect the TLS 1.3 server to mint us a new ticket, so we should be able to reset the counter to zero, right?
https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2001: GetSessionCacheKey(), negotiated_protocol_ == kProtoHTTP2); On 2017/01/17 20:16:42, davidben wrote: > Doing it in LogConnectEndEvent seems wrong. If Init() fails, you never did an > SSL_SESSION lookup. This gets called on failed connects too. I would think we > only need to do anything on successful connects (on failed connect, that would > still count as "using" the session since we've leaked it). The lookup happens in Init(), and it can return ERR_UNEXPECTED after that lookup (all of the failures of Init() appear to be ERR_UNEXPECTED), so I need to decrement the counter after Lookup. How about adding a member to SSLClientSocketImpl bool log_session_lookup_ which gets set to true after Lookup and false after DecrementLookupCount. I can also add to the d'tor a check of that bool and a call to DecrementLookupCount in case it's possible to start the connection and then destroy the socket before this function gets called. Other suggestions for where to put the DecrementLookupCount call to ensure it gets called exactly once per Lookup are welcome. > > On successful connect, we would expect the TLS 1.3 server to mint us a new > ticket, so we should be able to reset the counter to zero, right? We expect the server to mint a single ticket (although it could mint more), so we decrement by one - the new ticket replaces the one we just used for this connection.
https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2001: GetSessionCacheKey(), negotiated_protocol_ == kProtoHTTP2); On 2017/01/17 20:33:16, nharper wrote: > On 2017/01/17 20:16:42, davidben wrote: > > Doing it in LogConnectEndEvent seems wrong. If Init() fails, you never did an > > SSL_SESSION lookup. This gets called on failed connects too. I would think we > > only need to do anything on successful connects (on failed connect, that would > > still count as "using" the session since we've leaked it). > > The lookup happens in Init(), and it can return ERR_UNEXPECTED after that lookup > (all of the failures of Init() appear to be ERR_UNEXPECTED), so I need to > decrement the counter after Lookup. > > How about adding a member to SSLClientSocketImpl bool log_session_lookup_ which > gets set to true after Lookup and false after DecrementLookupCount. I can also > add to the d'tor a check of that bool and a call to DecrementLookupCount in case > it's possible to start the connection and then destroy the socket before this > function gets called. > > Other suggestions for where to put the DecrementLookupCount call to ensure it > gets called exactly once per Lookup are welcome. Oh, I see. You're tracking active lookups. But if Init() fails after an active lookup, we don't actually get a fresh session or anything, so if we're simulating what would happen with single use tickets, that should simulate a lookup we never get back. > > On successful connect, we would expect the TLS 1.3 server to mint us a new > > ticket, so we should be able to reset the counter to zero, right? > > We expect the server to mint a single ticket (although it could mint more), so > we decrement by one - the new ticket replaces the one we just used for this > connection. Don't we mint two tickets? Though there was talk of whether we should just be minting one. My thought was, just so we're only controlling one parameter at a time, we'd act as if the server gave us an infinite number of tickets, record at that point, and otherwise just increment the counter on lookup. Then we don't need to hook too many things. That would tell us how many times a session gets used in parallel before it is used successfully.
I updated the code based on our discussion yesterday.
Last iteration, hopefully! :-) https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1151: GetSessionCacheKey(), SSL_get_session(ssl_.get())); Doesn't this want to be reset regardless of whether ssl_session_cache_lookup_count_ is set and regardless of whether the sessions are the same? We expect to get a fresh session from the server in all cases. (SSL_get_session is also really Weird(TM). It doesn't return the offered session in TLS 1.3 because 1.3 resumption.) https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1154: ssl_session_cache_lookup_count_, 20); This should probably have a comment for what's going on here. https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.cc:22: SSLClientSessionCache::Entry::~Entry() = default; Nit: Order these matching the header. https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:81: size_t lookups; size_t is a little odd here. This is a count of things, so it doesn't actually measure the size of a buffer. In BoringSSL, I'd probably have used unsigned or uint64_t, but I think Chromium may prescribe differently. My read of these two: https://google.github.io/styleguide/cppguide.html#Integer_Types https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... is that int is what to use? (I don't know why I made lookups_since_flush a size_t. I think that was just a mistake.) https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:83: }; [Does it work to just do: struct Entry { size_t lookups = 0; bssl::UniquePtr<SSL_SESSION> session; }; I dunno if that trips up our inline vs out-of-line ctor/dtor check.] https://codereview.chromium.org/2625883002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2625883002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35606: + (including this one) are in the process of resuming the same session. This probably wants to be rephrased and maybe cite the bug because of just how specific it is to what we're doing. Maybe: For each SSL connection where we resume a session and negotiate HTTP/2, the simulated minimum number of sessions retained per host it would have required with TLS 1.3 single-use sessions. See https://crbug.com/whatever.
https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1151: GetSessionCacheKey(), SSL_get_session(ssl_.get())); On 2017/01/19 20:29:34, davidben wrote: > Doesn't this want to be reset regardless of whether > ssl_session_cache_lookup_count_ is set and regardless of whether the sessions > are the same? We expect to get a fresh session from the server in all cases. > > (SSL_get_session is also really Weird(TM). It doesn't return the offered session > in TLS 1.3 because 1.3 resumption.) Yes, that sounds right. We're modelling the server sending infinite NewSessionTickets, so it doesn't matter whether this session was resumed, or with which session. https://codereview.chromium.org/2625883002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1154: ssl_session_cache_lookup_count_, 20); On 2017/01/19 20:29:34, davidben wrote: > This should probably have a comment for what's going on here. Done. https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.cc:22: SSLClientSessionCache::Entry::~Entry() = default; On 2017/01/19 20:29:34, davidben wrote: > Nit: Order these matching the header. They're both in the same order (c'tor, move c'tor, d'tor), which I think is the order in the style guide (c'tors then d'tor). https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:81: size_t lookups; On 2017/01/19 20:29:34, davidben wrote: > size_t is a little odd here. This is a count of things, so it doesn't actually > measure the size of a buffer. In BoringSSL, I'd probably have used unsigned or > uint64_t, but I think Chromium may prescribe differently. > > My read of these two: > https://google.github.io/styleguide/cppguide.html#Integer_Types > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > is that int is what to use? > > (I don't know why I made lookups_since_flush a size_t. I think that was just a > mistake.) I'm fine with int. I'm only logging up to 20, and I doubt this count will get anywhere close to 2^31 (although maybe something in a tight loop trying to make a lot of connections with no network connectivity might?) https://codereview.chromium.org/2625883002/diff/40001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:83: }; On 2017/01/19 20:29:34, davidben wrote: > [Does it work to just do: > > struct Entry { > size_t lookups = 0; > bssl::UniquePtr<SSL_SESSION> session; > }; > > I dunno if that trips up our inline vs out-of-line ctor/dtor check.] That (with int instead of size_t) still trips up the chromium-style check. https://codereview.chromium.org/2625883002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2625883002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35606: + (including this one) are in the process of resuming the same session. On 2017/01/19 20:29:34, davidben wrote: > This probably wants to be rephrased and maybe cite the bug because of just how > specific it is to what we're doing. Maybe: > > For each SSL connection where we resume a session and negotiate HTTP/2, the > simulated minimum number of sessions retained per host it would have required > with TLS 1.3 single-use sessions. See https://crbug.com/whatever. Done.
lgtm with comments. https://codereview.chromium.org/2625883002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1156: // be speaking h2). Nit: Probably link to the bug here too, so it's clear this is about tuning numbers for single-use tickets. https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.cc:22: SSLClientSessionCache::Entry::~Entry() = default; Oh, I meant order relative to the other methods. I think we do that? I dunno. C++. https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:53: size_t* count); size_t => int to match https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache_unittest.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache_unittest.cc:121: EXPECT_EQ(0u, count); For completeness, I'd probably add a test that ResetLookupCount("not present") doesn't explode.
https://codereview.chromium.org/2625883002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1156: // be speaking h2). On 2017/01/19 21:56:12, davidben wrote: > Nit: Probably link to the bug here too, so it's clear this is about tuning > numbers for single-use tickets. Done. https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.cc:22: SSLClientSessionCache::Entry::~Entry() = default; On 2017/01/19 21:56:12, davidben wrote: > Oh, I meant order relative to the other methods. I think we do that? I dunno. > C++. I moved them so the ordering in the .cc is closer to the ordering in the .h, but there's some mixing of public and private in the .cc, and within the private methods, some are out of order. https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache.h (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache.h:53: size_t* count); On 2017/01/19 21:56:12, davidben wrote: > size_t => int to match Done. https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... File net/ssl/ssl_client_session_cache_unittest.cc (right): https://codereview.chromium.org/2625883002/diff/80001/net/ssl/ssl_client_sess... net/ssl/ssl_client_session_cache_unittest.cc:121: EXPECT_EQ(0u, count); On 2017/01/19 21:56:12, davidben wrote: > For completeness, I'd probably add a test that ResetLookupCount("not present") > doesn't explode. Done.
The CQ bit was checked by nharper@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2625883002/#ps100001 (title: "reply to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nharper@chromium.org changed reviewers: + holte@chromium.org
Adding holte as a reviewer for histograms.xml change.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484863755528280,
"parent_rev": "0b662a9cf9d75b3223e9fad83228e8f6d515cfe4", "commit_rev":
"00d06ff78fc0705706dd0c7db1170072072e8225"}
Message was sent while issue was closed.
Description was changed from ========== SSLClientSessionCache: Log number of times Lookup is called per Session. Session tickets in TLS 1.3 will be single-use, so to be able to resume multiple sessions to the same host, we will need to store multiple session tickets per host. By counting how many times Lookup is called per session in an SSLClientSessionCache, we can tune how many session tickets per host we need to store for TLS 1.3. BUG=631988 ========== to ========== SSLClientSessionCache: Log number of times Lookup is called per Session. Session tickets in TLS 1.3 will be single-use, so to be able to resume multiple sessions to the same host, we will need to store multiple session tickets per host. By counting how many times Lookup is called per session in an SSLClientSessionCache, we can tune how many session tickets per host we need to store for TLS 1.3. BUG=631988 Review-Url: https://codereview.chromium.org/2625883002 Cr-Commit-Position: refs/heads/master@{#444902} Committed: https://chromium.googlesource.com/chromium/src/+/00d06ff78fc0705706dd0c7db117... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/00d06ff78fc0705706dd0c7db117...
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + mpearson@chromium.org
Message was sent while issue was closed.
Hallo mpearson@chromium.org! Due to a depot_tools patch which mistakenly removed the OWNERS check for non-source files (see crbug.com/684270), the following files landed in this CL and need a retrospective review from you: tools/metrics/histograms/histograms.xml Thanks, Wez
Message was sent while issue was closed.
post-fact histograms.xml lgtm On 2017/02/07 22:33:57, Wez wrote: > Hallo mailto:mpearson@chromium.org! > Due to a depot_tools patch which mistakenly removed the OWNERS check for > non-source files (see crbug.com/684270), the following files landed in this CL > and need a retrospective review from you: > tools/metrics/histograms/histograms.xml > Thanks, > Wez |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
