Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
356953003:
Adding DiskBasedCertCache to HttpCache (+UMA). (Closed)
|
Created:
6 years, 6 months ago by brandonsalmon Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@current Project:
chromium Visibility:
Public. |
DescriptionAdding DiskBasedCertCache to HttpCache (+UMA).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282429
Patch Set 1 #
Total comments: 12
Patch Set 2 : Callbacks no longer members, forward declaration added, style fixes. #Patch Set 3 : Added UMA histogram for timing DBCC certificate chain writing. #Patch Set 4 : Fixed incorrect UMA histogram name and improved documentation. #Patch Set 5 : Counting outwards from root instead of outwards from leaf (and added to histograms.xml) #Patch Set 6 : fixed mistake in last patch. (v2) #Patch Set 7 : Added field trial. #
Total comments: 39
Patch Set 8 : Fixed minor issues in patch 7. #
Total comments: 11
Patch Set 9 : Fixed nits. #
Total comments: 3
Patch Set 10 : Moved CertChain writing and reading to new functions. #
Total comments: 4
Patch Set 11 : Fixed nits. #
Total comments: 2
Patch Set 12 : Correctly renamed functions. #Patch Set 13 : Improved histogram descriptions/ used BooleanSuccess enum. #
Total comments: 5
Patch Set 14 : Updated histograms. #
Total comments: 3
Patch Set 15 : Fixed issues (remembered to compile). #
Total comments: 6
Messages
Total messages: 33 (0 generated)
This is a first attempt at adding the DBCC to the HttpCache.
So, I would suggest that you move the getting/setting of the cache bits into helper functions (in the unnamed namespace). There's a lot of information you can be looking at, and which would alter the design, so it'd be good to have that self-contained. For example: 1) Average size of the chain (length in bytes, not # of certs) 2) How does your success rate differ depending on where in the chain the cert is? - I would suspect that we'll want to create histograms for Leaf, Intermediate1, Intermediate2, IntermediateN, and Root (where intermediates 3-N get lumped into IntermediateN) and that count hits 3) How long does it take to read/write a full chain to the cache? Similarly, how long does it take the HttpCache to read/write the full chain (as it already does)? https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:27: "DiskBasedCertCache.WritePreviouslyExisted", already_existed); You will need to update histograms.xml to ad this https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:33: const X509Certificate::OSCertHandle cert_handle) { Seems like this fit before - why the change? https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:41: } // namespace This was correct the way it was before (two spaces before // ) https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:203: RecordHistogram(rv); Sanity check: What would you expect to use this information for? How would it influence the design? https://codereview.chromium.org/356953003/diff/1/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/356953003/diff/1/net/http/http_cache.h#newcode34 net/http/http_cache.h:34: #include "net/http/disk_based_cert_cache.h" Don't include this header in the .h. You can forward declare the class, and then include the .h in the .cc https://codereview.chromium.org/356953003/diff/1/net/http/http_cache.h#newcod... net/http/http_cache.h:205: DiskBasedCertCache* CertCache() { 1) This should be const 2) You should place it with the rest of the functions associated with this class, rather than after the inherited classes (we try to group 'this class' / 'other classes' separately) I suspect this should be around line 145 https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... net/http/http_cache_transaction.cc:50: // storing cache keys so that this doesn't need to be done. This comment can use some style guide love. Complete sentences, TODO on a new line https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... net/http/http_cache_transaction.cc:1521: cache_->CertCache()->Get(key, cert_read_io_callback_); BUG? You're not checking the intermediates (from the cache) https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... net/http/http_cache_transaction.cc:2358: cert_write_io_callback_); STYLE: Indent style BUG: Same as above, not checking intermediates. https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/356953003/diff/1/net/http/http_cache_transact... net/http/http_cache_transaction.h:436: DiskBasedCertCache::SetCallback cert_write_io_callback_; You don't need to store these callbacks as member variables. Just base::Bind() on the fly. It's generally recommended not to save callbacks, except for 'special' cases.
I uploaded patch 2 in response to the review of patch 1. https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:27: "DiskBasedCertCache.WritePreviouslyExisted", already_existed); On 2014/06/26 19:56:02, Ryan Sleevi wrote: > You will need to update histograms.xml to ad this What do I need to do to update this? I tried and git started acting weird. https://codereview.chromium.org/356953003/diff/1/net/http/disk_based_cert_cac... net/http/disk_based_cert_cache.cc:203: RecordHistogram(rv); On 2014/06/26 19:56:02, Ryan Sleevi wrote: > Sanity check: What would you expect to use this information for? How would it > influence the design? I added this because it seemed important to gather data on how often a certificate is overwritten on disk (it was my assumption that this would correspond to duplicate writes in the currently used method).
I added code that should create a UMA histogram of the time it takes to write a certificate chain to the cache using the DBCC. It was a little complicated because there is no guarantee that they will be written to the cache in the order of the chain.
I added the necessary code for this change to be used as a field trial.
Review comments on patch set 7: https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:49: bool UseCertCache() { IMPORTANT: Should we also use a command-line option to enable this code? https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:355: disk_cache_.reset(); Should we call cert_cache_.reset() before calling disk_cache_.reset()? cert_cache_ points to disk_cache_. So if disk_cache_ is gone, cert_cache_ can't be used. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.h#n... net/http/http_cache.h:46: class DiskBasedCertCache; List in alphabetical order. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.h#n... net/http/http_cache.h:146: DiskBasedCertCache* CertCache() const { return cert_cache_.get(); } This getter method should be named cert_cache. See the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:51: // Stores data relevant to the statistics of writing entire certificate Nit: you also use this struct for reading certificate chains. So it's not just "writing". https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:58: struct SharedCCData { Don't use the abbreviation "CC" in the name. See the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming... Without looking at the rest of the file, I didn't know whether "CC" stands for certificate chain or certificate cache. You can name this struct "SharedChainData". https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:59: SharedCCData(int num, TimeTicks time) These two parameters are poorly named. You can try: num => num_ops or pending_ops time => start Note: I remember it is actually legal to name these parameters num_pending_ops and start_time. You can also try that. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:65: typedef base::RefCountedData<SharedCCData> RefCountedSharedCCData; This isn't how a ref-counted type is defined. Use net/http/http_response_headers.h as an example: https://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_response_heade... https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:71: std::string GetCacheKeyToCert( Nit: GetCacheKeyToCert => GetCacheKeyForCert To guarantee that this file gets the exact cache key used by the CertBasedDiskCache, you should instead have CertBasedDiskCache expose its cache key function, with a similar TODO comment. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:72: const net::X509Certificate::OSCertHandle cert_handle) { This const is not necessary. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:81: // certificate chain, 0 indicating it is the leaf. |is_leaf| indicates Typo: 0 indicating it is the leaf => 0 indicating it is the root Also fix line 117. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:88: scoped_refptr<RefCountedSharedCCData> shared_chain_data, I think you can declare this as https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:91: // for this chain of certificates. The total time to write the chain write the chain => read the chain ? https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:93: (shared_chain_data->data).num_pending_ops--; Omit the parentheses. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:97: UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainReadTime", read_chain_wait); IMPORTANT: use a local histogram (HISTOGRAM_TIMES) instead of UMA_HISTOGRAM for now. Switch to UMA_HISTOGRAM when this code is ready. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:107: UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessRoot", success); We should not need to cache the root certificate because trusted root certificates are usually built in to the crypto library. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:111: UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessInt2", success); BUG: The Int1 and Int2 names imply these certificates are intermediate CA certs. But they can be a leaf certificate. To fix this, you need to return from the function if is_leaf is true, on line 104. (Remember to add curly braces to that if statement.) Make the same change to lines 139-140 in OnCertWriteIOComplete. An alternative is to use the dist_from_leaf count instead, and count the intermediate CA certificates from the leaf. This also allows us to omit is_leaf because it is simply dist_from_leaf == 0. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:136: bool success = (key != std::string()); key != std::string() => !key.empty() https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1604: X509Certificate::OSCertHandles intermediates = Declare this as const X509Certificate::OSCertHandles& (note the '&') to avoid the copying of X509Certificate::OSCertHandles. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1611: cache_->CertCache()->Get(key, IMPORTANT: Add new states to wait for the completion of Get() and Set(). Are you planning to do this in a separate CL? https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1615: shared_chain_data)); IMPORTANT: rather than using a heap-allocated SharedCCData struct, I would just consider adding a data member of the SharedCCData type to this class, and passing a WeakPtr of this object to base::Bind here. I haven't thought about this carefully, so this may not work.
wtc: I have a lot more comments for your comments than I do for Brandon's CL. I'm confused by a number of recommendations, but perhaps I'm missing context you shared offline? https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:49: bool UseCertCache() { On 2014/07/08 00:14:03, wtc wrote: > > IMPORTANT: Should we also use a command-line option to enable this code? wtc: What's the use case here? I discouraged Brandon from doing so, precisely because I cannot think of a reason. Please also note that field trials already have a means to force the group. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:355: disk_cache_.reset(); On 2014/07/08 00:14:03, wtc wrote: > > Should we call cert_cache_.reset() before calling disk_cache_.reset()? > > cert_cache_ points to disk_cache_. So if disk_cache_ is gone, cert_cache_ can't > be used. Yes https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:71: std::string GetCacheKeyToCert( On 2014/07/08 00:14:03, wtc wrote: > > Nit: GetCacheKeyToCert => GetCacheKeyForCert > > To guarantee that this file gets the exact cache key used by the > CertBasedDiskCache, you should instead have CertBasedDiskCache expose its cache > key function, with a similar TODO comment. I recommended Brandon not do this, precisely because it would have forced a more explicit coupling. The current method (recomputing by hand) more practically reflects the on-disk storage (in that if we do change the cache method, existing entries won't be updated) https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:88: scoped_refptr<RefCountedSharedCCData> shared_chain_data, On 2014/07/08 00:14:04, wtc wrote: > > I think you can declare this as Incomplete? passing "const scoped_refptr<RefCountedSharedCCData>& shared_chain_data" is the recommended way, though. non-const, non-ref for scoped_ptr const, ref for scoped_refptr https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:97: UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainReadTime", read_chain_wait); On 2014/07/08 00:14:03, wtc wrote: > > IMPORTANT: use a local histogram (HISTOGRAM_TIMES) instead of UMA_HISTOGRAM for > now. Switch to UMA_HISTOGRAM when this code is ready. wtc: Why do you suggest this? Part of the field trial is to gather performance metrics, and it's reason for field trial is to avoid regressing (due to induced additional I/O to read cache entries) https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:107: UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.ReadSuccessRoot", success); On 2014/07/08 00:14:03, wtc wrote: > > We should not need to cache the root certificate because trusted root > certificates are usually built in to the crypto library. We do cache the root certs today, precisely because root certificates change, but the UI status does not (it remains fixed in time). That is, if you validated a connection at time T0, to root R, and then at T5, removed the root R, you should still display a correct chain at T10. Considering the rate of NSS root changes as the timeline for deprecating 1024-bit roots consider, this is not out of the realm of possibility, especially for a large number of sites (Verisign/Symantec roots) https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:124: scoped_refptr<RefCountedSharedCCData> shared_chain_data, const-ref https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1604: X509Certificate::OSCertHandles intermediates = On 2014/07/08 00:14:03, wtc wrote: > > Declare this as const X509Certificate::OSCertHandles& (note the '&') to avoid > the copying of X509Certificate::OSCertHandles. Correct. So it's unambiguous: const-ref OSCertHandles (plural), by-value OSCertHandle (singular) https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1611: cache_->CertCache()->Get(key, On 2014/07/08 00:14:04, wtc wrote: > > IMPORTANT: Add new states to wait for the completion of Get() and Set(). Are you > planning to do this in a separate CL? wtc: I recommended this path to Brandon. Could you elaborate on your design concerns? https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1615: shared_chain_data)); On 2014/07/08 00:14:03, wtc wrote: > > IMPORTANT: rather than using a heap-allocated SharedCCData struct, I would just > consider adding a data member of the SharedCCData type to this class, and > passing a WeakPtr of this object to base::Bind here. I haven't thought about > this carefully, so this may not work. It's unclear to me what you're trying to solve. This path is preferred when binding a common structure to multiple callbacks.
Ryan: I answered your questions below. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:49: bool UseCertCache() { On 2014/07/08 00:34:18, Ryan Sleevi wrote: > > wtc: What's the use case here? A command-line option is a common way to allow us to check in code that is incomplete and should be disabled by default. We can certainly accomplish the same effect by not starting the field trial until this code is complete. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:88: scoped_refptr<RefCountedSharedCCData> shared_chain_data, On 2014/07/08 00:14:04, wtc wrote: > > I think you can declare this as Yes, this comment is incomplete. Please ignore it. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:97: UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainReadTime", read_chain_wait); On 2014/07/08 00:34:18, Ryan Sleevi wrote: > > wtc: Why do you suggest this? I didn't know you suggested the field trial. This CL looked incomplete to me, which is why I suggested using local histograms first to avoid sending statistics to UMA prematurely. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1611: cache_->CertCache()->Get(key, On 2014/07/08 00:34:18, Ryan Sleevi wrote: > > wtc: I recommended this path to Brandon. Could you elaborate on your design > concerns? Eventually we will need to wait for the completion of Get() and Set(). I was asking if Brandon plans to do that in a separate CL. https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1615: shared_chain_data)); On 2014/07/08 00:34:18, Ryan Sleevi wrote: > > It's unclear to me what you're trying to solve. > > This path is preferred when binding a common structure to multiple callbacks. Eventually this class will need to wait for the completion of Get() and Set(). So the GetCallback and SetCallback will need a pointer to this object at that time. In that case we might as well store num_pending_ops and start_time inside this class. Again, I haven't thought about this carefully, so what I suggested may not work.
https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:51: "ExperimentGroup"; IMPORTANT: Users in the field trial will have new cert-cache entries (with the key "cert:<sha1-fingerprint>") in their disk caches. Before we start the field trial, please resolve these two issues: 1. Find out if we need to provide UI (under chrome://settings > Privacy > Clear browsing data) to have the cert-cache entries removed. Perhaps the "Cached images and files" checkbox will take care of this automatically? 2. We may need to version the serialization format so that we can change the format in the future. Right now the serialization format is simply the DER-encoded certificate bytes, with no "metadata" such as the format version.
https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:51: "ExperimentGroup"; On 2014/07/08 18:24:21, wtc wrote: > > IMPORTANT: Users in the field trial will have new cert-cache entries (with the > key "cert:<sha1-fingerprint>") in their disk caches. Before we start the field > trial, please resolve these two issues: > > 1. Find out if we need to provide UI (under chrome://settings > Privacy > Clear > browsing data) to have the cert-cache entries removed. Perhaps the "Cached > images and files" checkbox will take care of this automatically? Yes. We're using the DiskCache. > > 2. We may need to version the serialization format so that we can change the > format in the future. Right now the serialization format is simply the > DER-encoded certificate bytes, with no "metadata" such as the format version. The metadata is the URI scheme, "cert:". We can change the URI scheme if we need to change versions.
https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... net/http/http_cache.cc:51: "ExperimentGroup"; On 2014/07/08 18:29:08, Ryan Sleevi wrote: > > The metadata is the URI scheme, "cert:". We can change the URI scheme if we need > to change versions. Suppose we change the URI scheme to "cert2:". Won't that make the entries with the old URI scheme "cert:" unreachable? It seems better to version the serialization format because the existing entries can be overwritten with data in the new format.
I fixed the minor issues mentioned in patch set 7.
On 2014/07/08 19:00:11, wtc wrote: > https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc > File net/http/http_cache.cc (right): > > https://codereview.chromium.org/356953003/diff/250001/net/http/http_cache.cc#... > net/http/http_cache.cc:51: "ExperimentGroup"; > > On 2014/07/08 18:29:08, Ryan Sleevi wrote: > > > > The metadata is the URI scheme, "cert:". We can change the URI scheme if we > need > > to change versions. > > Suppose we change the URI scheme to "cert2:". Won't that make the entries with > the old URI scheme "cert:" unreachable? It seems better to version the > serialization format because the existing entries can be overwritten with data > in the new format. No, it wouldn't. If we imagine a world where we're serializing the cache keys, rather than the certs, in the HttpResponseInfo, then the cache key would have the full key (eg: "cert:somehash"). So they'd still be accessible from old entries that were referring to them. If we updated the scheme to "cert2:somehash", new entries would be serialized with that scheme (and so too in the HttpResponseInfo), and old entries would be unaffected. So the version is handled by the scheme. If we think about what sort of changes we'd expect to make to the serialization of certs, the thing that comes to mind is book keeping data (such as reference counting), but the disk cache already has other ways to accomplish that (eg: the stream identifiers, which is how the Http cache handles it). We'd also potentially be exploring book keeping data in the HttpResponseInfo (which is versioned as well), rather than the disk entries themselves.
Patch set 8 LGTM. https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... net/http/http_cache_transaction.cc:61: : num_pending_ops(num_ops), start_time(start) {} Nit: add a blank line to separate the constructor from the members. https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... net/http/http_cache_transaction.cc:75: std::string GetCacheKeyToCert( Nit: GetCacheKeyToCert => GetCacheKeyForCert https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... net/http/http_cache_transaction.cc:76: const net::X509Certificate::OSCertHandle cert_handle) { This const is not necessary. https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... net/http/http_cache_transaction.cc:85: // certificate chain, 0 indicating it is the leaf. |is_leaf| indicates Typo: 0 indicating it is the leaf => 0 indicating it is the root Also fix line 120. https://codereview.chromium.org/356953003/diff/280005/net/http/http_cache_tra... net/http/http_cache_transaction.cc:95: // for this chain of certificates. The total time to write the chain write the chain => read the chain https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3505: + Measures the wall time from when the writing of a certificate chain to disk wall time => wall clock time https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3513: + Measures the wall time from when the reading of a certificate chain to disk wall time => wall clock time https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3514: + cache is issued until the last certificate has been retrieved. The summary fields of these two histograms should be reversed. https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3522: + reading the first intermediate x509 certificate from the disk cache. x509 => X.509 Fix all occurrences. I would omit "X.509" altogether. https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3526: +<histogram name="DiskBasedCertCache.ReadSuccessInt2"> Add DiskBasedCertCache.ReadSuccessIntN. https://codereview.chromium.org/356953003/diff/280005/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3566: +<histogram name="DiskBasedCertCache.WriteSuccessInt2"> Add DiskBasedCertCache.WriteSuccessIntN.
I fixed a few issues with the last patch set.
Consider my review a drive-by (I'm leaving the detailed review to wtc/ryan). https://codereview.chromium.org/356953003/diff/300001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/300001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:58: class SharedChainData : public base::RefCountedThreadSafe<SharedChainData> { ThreadSafe? Do we expect multiple threads to be involved? https://codereview.chromium.org/356953003/diff/300001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1604: std::string key = This block looks fairly self contained to me. Do you mind creating a dedicated method? https://codereview.chromium.org/356953003/diff/300001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2467: const X509Certificate::OSCertHandles& intermediates = same here
I moved the code for reading and writing the certificate chains to their own HttpCache::Transaction methods.
Thanks! https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1871: //----------------------------------------------------------------------------- nit: this line separates the state machine from random methods https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... net/http/http_cache_transaction.h:273: void DoCertChainRead(); nit: Do not use DoFoo here as that suggests an implementation of a state machine state. Simply Foo()
Patch set 11 LGTM. https://codereview.chromium.org/356953003/diff/360001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/356953003/diff/360001/net/http/http_cache_tra... net/http/http_cache_transaction.h:273: void CertChainWrite(); Please name these methods "ReadCertChain" and "WriteCertChain".
Fixed the last patch. https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1871: //----------------------------------------------------------------------------- On 2014/07/09 01:25:36, rvargas wrote: > nit: this line separates the state machine from random methods Moved the methods outside of the state machine. https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/356953003/diff/320001/net/http/http_cache_tra... net/http/http_cache_transaction.h:273: void DoCertChainRead(); On 2014/07/09 01:25:36, rvargas wrote: > nit: Do not use DoFoo here as that suggests an implementation of a state machine > state. Simply Foo() Changed this.
https://codereview.chromium.org/356953003/diff/360001/net/http/http_cache_tra... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/356953003/diff/360001/net/http/http_cache_tra... net/http/http_cache_transaction.h:273: void CertChainWrite(); On 2014/07/09 01:50:42, wtc wrote: > > Please name these methods "ReadCertChain" and "WriteCertChain". Done.
I improved my histogram descriptions in histograms.xml.
lgtm
Patch set 13 LGTM. https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3505: + Measures the wall clock time from when the reading of a certificate chain to Nit: to disk cache => from disk cache
https://codereview.chromium.org/356953003/diff/400001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/400001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:136: UMA_HISTOGRAM_TIMES("DiskBasedCertCache.ChainWriteTime", write_chain_wait); nit: suggest UMA_HISTOGRAM_CUSTOM_TIMES("...", sample, base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(10), 50) https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3541: +</histogram> nit: You may find it nicer to have two histograms, on for "successfully reading" and one for "failed reading". Probably use a UMA_CUSTOM_HISTOGRAM("names...", sample, 1, 10, 7); Also (perhaps), the clever way to naim these is to have *only* the final postfix vary, and then use the <field_trial> construct so that you don't have to repeat the prose. DiskBasedCertCache.ReadNthCertificateFailure DiskBasedCertCache.ReadNthCertificateSuccess You'd write the bulk of the prose for: DiskBasedCertCache.ReadNthCertificate end then have a <field_trial> that lists two alternative extensions (use the separator="" form). https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3551: +<histogram name="DiskBasedCertCache.ReadSuccessRoot" enum="BooleanSuccess"> This may nicely fall into the above histogram, using root==0 (as you suggested offline). https://codereview.chromium.org/356953003/diff/400001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3566: +<histogram name="DiskBasedCertCache.WriteSuccessInt1" enum="BooleanSuccess"> This will be a lot like the read histograms... sooooo... perhaps you could have a common prefix like: DiskBasedCertCache.ValidIoWriteSuccess DiskBasedCertCache.ValidIoWriteFailure DiskBasedCertCache.ValidIoReadSuccess DiskBasedCertCache.ValidIoReadFailure So have a nice bit of prose for: DiskBasedCertCache.ValidIo Then have a field trial to extend that for the four cases.
https://chromiumcodereview.appspot.com/356953003/diff/420001/net/http/http_ca... File net/http/http_cache_transaction.cc (right): https://chromiumcodereview.appspot.com/356953003/diff/420001/net/http/http_ca... net/http/http_cache_transaction.cc:117: "DiskBasedCertCache.CertIoReadFailure", 1, 10, 7); I think your'e missing an argument (sample). https://chromiumcodereview.appspot.com/356953003/diff/420001/net/http/http_ca... net/http/http_cache_transaction.cc:150: "DiskBasedCertCache.CertIoWriteSuccess", 1, 10, 7); Missing arg (sample) here too. https://chromiumcodereview.appspot.com/356953003/diff/420001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/356953003/diff/420001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:3533: + disk cache is issued until the last certificate is retrieved. Nit: Suggest at least commas, and probably use multiple or more detailed sentences, such as: Measures the wall clock time, starting when the request to read a certificate chain from disk cache is issued, and ending when the last certificate is retrieved.
I rewrote some of the histogram descriptions (and I remembered to compile this time).
lgtm
The CQ bit was checked by brandonsalmon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/356953003/4...
Patch set 15 LGTM. You don't need to abort the commit queue job for these nits. https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:101: UMA_HISTOGRAM_CUSTOM_TIMES("DiskBasedCertCache.ChainReadTime", Should this histogram name also use the "CertIo" prefix? https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:110: UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.CertIoReadSuccessLeaf", success); IMPORTANT: should we return here? Otherwise the results of leaf certificates will be recorded in histograms below. What you do is reasonable. I just wanted to make sure it was a deliberate decision rather than an oversight. https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:117: "DiskBasedCertCache.CertIoReadFailure", dist_from_root, 0, 10, 7); Nit: use curly braces because the bodies of the if and else branch are two lines (even though they are single statements). https://codereview.chromium.org/356953003/diff/440001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356953003/diff/440001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3534: + the certificates in the chain have been read into memory. Nit: two spaces between "the" ad "certificates". May not be worth fixing.
https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:101: UMA_HISTOGRAM_CUSTOM_TIMES("DiskBasedCertCache.ChainReadTime", On 2014/07/10 18:42:21, wtc wrote: > > Should this histogram name also use the "CertIo" prefix? It was my intention that "Chain" would become a different prefix here, to record statistics specific to certificate chains. Although I only have the histograms for timing right now. https://codereview.chromium.org/356953003/diff/440001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:110: UMA_HISTOGRAM_BOOLEAN("DiskBasedCertCache.CertIoReadSuccessLeaf", success); On 2014/07/10 18:42:21, wtc wrote: > > IMPORTANT: should we return here? Otherwise the results of leaf certificates > will be recorded in histograms below. > > What you do is reasonable. I just wanted to make sure it was a deliberate > decision rather than an oversight. I did this because it didn't make sense to me to record the root but not the leaf. I agree that it could be better to return here and not record the leaf, but I don't see the reason why. One other possibility would be to add a different histogram to measure the length of certificate chains (as this would tell us the position of the leaf).
Message was sent while issue was closed.
Change committed as 282429 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
