Improving and adding an in-memory MRU cache to DiskBasedCertCache.

net/http/disk_based_cert_cache.h

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_HTTP_DISK_BASED_CERT_CACHE_H
 #define NET_HTTP_DISK_BASED_CERT_CACHE_H
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
+#include "base/containers/mru_cache.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/net_export.h"
 #include "net/cert/x509_certificate.h"
 
 namespace disk_cache {
 class Backend;
 }  // namespace disk_cache
 
 namespace net {
 
@@ skipping 17 matching lines @@
   // a reference to the certificate need to use X509Certificate::DupOSCertHandle
   // inside |cb|.
   void Get(const std::string& key, const GetCallback& cb);
 
   // Stores |cert_handle| in the cache. If |cert_handle| is successfully stored,
   // |cb| will be called with the key. If |cb| is called with an empty
   // string, then |cert_handle| was not stored.
   void Set(const X509Certificate::OSCertHandle cert_handle,
            const SetCallback& cb);
 
+  // These functions intended for testing purposes only.

[wtc, 2014/07/02 20:51:50]

Nit: please document what they return.

[Ryan Sleevi, 2014/07/02 21:49:58]

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()

[rvargas (doing something else), 2014/07/02 22:14:39]

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().

+  int MemCacheHits() { return mem_cache_hits_; }
+  int MemCacheMisses() { return mem_cache_misses_; }

[wtc, 2014/07/02 20:51:50]

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; }

[Ryan Sleevi, 2014/07/02 21:49:58]

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. "

+
  private:
   class ReadWorker;
   class WriteWorker;
 
+  // A functor used to free an OSCertHandle. Used by the MRUCertCache.
+  class CertFree {
+   public:
+    void operator()(X509Certificate::OSCertHandle cert_handle) {
+      X509Certificate::FreeOSCertHandle(cert_handle);
+    }
+  };

[Ryan Sleevi, 2014/07/02 21:49:58]

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.

+
+  // An in-memory cache that is used to prevent redundant reads and writes
+  // to and from the disk cache.
+  typedef base::MRUCacheBase<std::string,
+                             X509Certificate::OSCertHandle,
+                             CertFree> MRUCertCache;
+
   // ReadWorkerMap and WriteWorkerMap map cache keys to their
   // corresponding Workers.
   typedef base::hash_map<std::string, ReadWorker*> ReadWorkerMap;
   typedef base::hash_map<std::string, WriteWorker*> WriteWorkerMap;
 
   // FinishedReadOperation and FinishedWriteOperation are used by callbacks
   // given to the workers to signal the DiskBasedCertCache they have completed
   // their work.
-  void FinishedReadOperation(const std::string& key);
-  void FinishedWriteOperation(const std::string& key);
+  void FinishedReadOperation(const std::string& key,
+                             X509Certificate::OSCertHandle cert_handle);
+  void FinishedWriteOperation(const std::string& key,
+                              X509Certificate::OSCertHandle cert_handle);
+
+  disk_cache::Backend* backend_;
 
   ReadWorkerMap read_worker_map_;
   WriteWorkerMap write_worker_map_;
+  MRUCertCache mru_cert_cache_;

[wtc, 2014/07/02 20:51:50]

Ricardo: does the disk cache already have an internal in-memory cache?

[rvargas (doing something else), 2014/07/02 22:14:39]

no, it doesn't

 
-  disk_cache::Backend* backend_;
+  int mem_cache_hits_;
+  int mem_cache_misses_;
+
   base::WeakPtrFactory<DiskBasedCertCache> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(DiskBasedCertCache);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_DISK_BASED_CERT_CACHE_H