Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361513003:
Improving and adding an in-memory MRU cache to DiskBasedCertCache. (Closed)
|
Created:
6 years, 5 months ago by brandonsalmon Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@DBCC_Implement Project:
chromium Visibility:
Public. |
DescriptionImproving and adding an in-memory MRU cache to DiskBasedCertCache.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281191
Patch Set 1 #Patch Set 2 : Improved documentation. #
Total comments: 7
Patch Set 3 : Added tests, and fixed issues with last patch. #
Total comments: 22
Patch Set 4 : Revisions based on review of patch set 3. #
Total comments: 4
Patch Set 5 : Removed static cast. #
Total comments: 2
Messages
Total messages: 14 (0 generated)
I began adding an MRU cache to the DiskBasedCertCache and stopped setting state_ to NONE in all the do-loop functions.
Review comments on patch set 2: Please add tests. Other than that the CL is good. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:211: } Nit: omit the curly braces in these simple if statements. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:472: : backend_(backend), mru_cert_cache_(30), weak_factory_(this) { Define a constant with the value 30 near the top of this file. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:494: // of the regency list in the MRU cache. 1. Typo: regency => recency 2. Move some of the words to the previous line. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:526: // to the front of the regency list in the MRU cache. Typo: regency => recency https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:532: mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle)); A comment that explains why it is advantageous and correct to add the cert to the MRU cache now should be added. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:64: class MRUCertCache : public base::MRUCacheBase<std::string, Since MRUCertCache doesn't have any new method or constructor, I think it can be just a typedef: typedef base::MRUCacheBase<std::string, X509Certificate::OSCertHandle, CertFree> MRUCertCache; Does this work?
I added unittests to the MRU cache functionality and fixed the issues mentioned in the last patch. https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/20001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:532: mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle)); On 2014/07/01 02:11:31, wtc wrote: > > A comment that explains why it is advantageous and correct to add the cert to > the MRU cache now should be added. I no longer think this is that advantageous. (There are many write requests simultaneously, but in those cases the user callback will just be added to the WriteWorker). I moved this to the FinishedWriteOperation function.
Patch set 3 LGTM. Please wait for an approval from either Ricardo or Ryan. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:114: cert_handle_(cert_handle), I wonder if we should dup the cert_handle here, and free it in the destructor. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:271: DiskBasedCertCache::WriteWorker::~WriteWorker() { Define the destructor right after the constructor. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:466: DiskBasedCertCache::ReadWorker::~ReadWorker() { Define the destructor right after the constructor. Our Style Guide recommends declaring and defining methods in the same order. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:499: // Note, this will also bring the cert_handle to the front Nit: this line break is too short. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:540: ++mem_cache_misses_; IMPORTANT: We should think about whether Set() should be satisfied by the in-memory cache. Consider this case: for some reason, a certificate is evicted from the disk cache, but it is still in the in-memory cache. When the next Set() call comes in, it will be satisfied by the in-memory cache. But the caller of Set() may expect that the cert is persisted to the disk. Can this case happen? https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:565: if (key != std::string()) Test !key.empty(). https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:50: // These functions intended for testing purposes only. Nit: please document what they return. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:52: int MemCacheMisses() { return mem_cache_misses_; } 1. These methods should be declared const. 2. These getter methods should be named like the data members they return, as recommended by our Style Guide. So they should look like: int mem_cache_hits() const { return mem_cache_hits_; } int mem_cache_misses() const { return mem_cache_misses; } https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:89: MRUCertCache mru_cert_cache_; Ricardo: does the disk cache already have an internal in-memory cache? https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:476: // Issues two successive read requests to the DBCC for a certificate, Nit: the DBCC abbreviation is not common. Also, you fully spelled out DiskBasedCertCache in the next line. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:484: (ImportCert(&backend, kCert1, false /* not corrupted */))); The outer most parentheses on this line don't seem neessary. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:500: // recorded an in-memory cache hit for the second. Delete the second "recorded". https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache_unittest.cc:556: Nit: did you mean to add this blank line?
LGTM, although a few style issues to be fixed before landing. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:23: const int kMemoryCacheMaxSize = 30; size_t https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:114: cert_handle_(cert_handle), On 2014/07/02 20:51:49, wtc wrote: > > I wonder if we should dup the cert_handle here, and free it in the destructor. Oof. Missed this. Yes, we should. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:540: ++mem_cache_misses_; On 2014/07/02 20:51:50, wtc wrote: > > IMPORTANT: We should think about whether Set() should be satisfied by the > in-memory cache. > > Consider this case: for some reason, a certificate is evicted from the disk > cache, but it is still in the in-memory cache. > > When the next Set() call comes in, it will be satisfied by the in-memory cache. > But the caller of Set() may expect that the cert is persisted to the disk. > > Can this case happen? Yes. I think we only want Get to be satisfied by the cache. Set should add to the cache. https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:50: // These functions intended for testing purposes only. On 2014/07/02 20:51:50, wtc wrote: > > Nit: please document what they return. For testing, we prefer to use friend classes to a test fixture. See https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members For Chromium, we have a special macro (FRIEND_TEST_ALL_PREFIXES) to handle this. If it does need to be public, it should be named with "ForTesting" in the code, so that the presubmit code will yell if it's used outside of a _unittest file mem_cache_hits_for_testing() mem_cache_misses_for_testing() https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:52: int MemCacheMisses() { return mem_cache_misses_; } On 2014/07/02 20:51:50, wtc wrote: > > 1. These methods should be declared const. > > 2. These getter methods should be named like the data members they return, as > recommended by our Style Guide. > > So they should look like: > > int mem_cache_hits() const { return mem_cache_hits_; } > int mem_cache_misses() const { return mem_cache_misses; } http://www.chromium.org/developers/coding-style#TOC-Types "Use unsigned integer types (preferably size_t) for object and allocation sizes, object counts, array and pointer offsets, vector indices, and so on. " https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:64: }; You should be able to just declare this class, and leave the definition in the .cc "struct CertFreeHelper" If that doesn't work " struct CertFree { void operator()(X509Certificate::OSCertHandle handle); }; " and leave the definition in the .cc. Note also the preference for structs for unary functors.
lgtm https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.cc:505: return; You may want to tell the backend about this: backend_->OnExternalCacheHit() https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:50: // These functions intended for testing purposes only. nit: If it is important that the methods should be for testing only, that should be part of the name (FooForTest() or BlaForTesting()). If it is ok to expose general getters, remove the comment and use variable_name(). https://codereview.chromium.org/361513003/diff/60001/net/http/disk_based_cert... net/http/disk_based_cert_cache.h:89: MRUCertCache mru_cert_cache_; On 2014/07/02 20:51:50, wtc wrote: > > Ricardo: does the disk cache already have an internal in-memory cache? no, it doesn't
I revised things that were mentioned in review of patch set 3. Set no longer looks at the MRU cache.
LGTM, mod nits. You don't need to static_cast<> to get a size_t, you just have to make sure the constant is clearly marked as unsigned, by using the "U" suffix - eg: 0U. type promotion rules will then promote it to the larger type. https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... File net/http/disk_based_cert_cache_unittest.cc (right): https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:490: EXPECT_EQ(static_cast<size_t>(0), cache.mem_cache_hits_for_testing()); Use "0U" instead https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:493: EXPECT_EQ(static_cast<size_t>(1), cache.mem_cache_hits_for_testing()); Use "1U" instead https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:521: EXPECT_EQ(static_cast<size_t>(0), cache.mem_cache_hits_for_testing()); Use "0U" instead https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... net/http/disk_based_cert_cache_unittest.cc:527: EXPECT_EQ(static_cast<size_t>(1), cache.mem_cache_hits_for_testing()); Use "1U" instead
On 2014/07/02 22:49:23, Ryan Sleevi wrote: > LGTM, mod nits. You don't need to static_cast<> to get a size_t, you just have > to make sure the constant is clearly marked as unsigned, by using the "U" suffix > - eg: 0U. type promotion rules will then promote it to the larger type. > > https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... > File net/http/disk_based_cert_cache_unittest.cc (right): > > https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... > net/http/disk_based_cert_cache_unittest.cc:490: > EXPECT_EQ(static_cast<size_t>(0), cache.mem_cache_hits_for_testing()); > Use "0U" instead > > https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... > net/http/disk_based_cert_cache_unittest.cc:493: > EXPECT_EQ(static_cast<size_t>(1), cache.mem_cache_hits_for_testing()); > Use "1U" instead > > https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... > net/http/disk_based_cert_cache_unittest.cc:521: > EXPECT_EQ(static_cast<size_t>(0), cache.mem_cache_hits_for_testing()); > Use "0U" instead > > https://codereview.chromium.org/361513003/diff/120001/net/http/disk_based_cer... > net/http/disk_based_cert_cache_unittest.cc:527: > EXPECT_EQ(static_cast<size_t>(1), cache.mem_cache_hits_for_testing()); > Use "1U" instead Alright I replace them.
The CQ bit was checked by brandonsalmon@chromium.org
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/361513003/1...
Patch set 5 LGTM. You can fix the nits later. https://codereview.chromium.org/361513003/diff/140001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.cc (right): https://codereview.chromium.org/361513003/diff/140001/net/http/disk_based_cer... net/http/disk_based_cert_cache.cc:125: DiskBasedCertCache::WriteWorker::~WriteWorker() { Nit: make this and the ReadWorker destructor the same. Also, Ricardo told me there is a scoped type that closes the entry in its destructor. https://codereview.chromium.org/361513003/diff/140001/net/http/disk_based_cer... File net/http/disk_based_cert_cache.h (right): https://codereview.chromium.org/361513003/diff/140001/net/http/disk_based_cer... net/http/disk_based_cert_cache.h:50: // Returns the number of in-memory MRU cache hits that have occured Nit: occured => occurred Also on line 54.
Message was sent while issue was closed.
Change committed as 281191 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
