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

net/http/disk_based_cert_cache.cc

 // 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.
 
 #include "net/http/disk_based_cert_cache.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/disk_cache.h"
 
 namespace net {
 
 namespace {
 
+// TODO(brandonsalmon): change this number to improve performance.
+const int kMemoryCacheMaxSize = 30;

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

size_t

+
 // Used to obtain a unique cache key for a certificate in the form of
 // "cert:<hash>".
 std::string GetCacheKeyToCert(const X509Certificate::OSCertHandle cert_handle) {
   SHA1HashValue fingerprint =
       X509Certificate::CalculateFingerprint(cert_handle);
 
   return "cert:" +
          base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
 }
 
@@ skipping 69 matching lines @@
   std::vector<SetCallback> user_callbacks_;
   CompletionCallback io_callback_;
 };
 
 DiskBasedCertCache::WriteWorker::WriteWorker(
     disk_cache::Backend* backend,
     const std::string& key,
     X509Certificate::OSCertHandle cert_handle,
     const base::Closure& cleanup_callback)
     : backend_(backend),
       cert_handle_(cert_handle),

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

I wonder if we should dup the cert_handle here, and free it in the destructor.

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

Oof. Missed this. Yes, we should.

       key_(key),
       canceled_(false),
       entry_(NULL),
       state_(STATE_NONE),
       io_buf_len_(0),
       cleanup_callback_(cleanup_callback),
       io_callback_(
           base::Bind(&WriteWorker::OnIOComplete, base::Unretained(this))) {
 }
 
@@ skipping 77 matching lines @@
   state_ = STATE_WRITE;
   return OK;
 }
 
 int DiskBasedCertCache::WriteWorker::DoOpen() {
   state_ = STATE_OPEN_COMPLETE;
   return backend_->OpenEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::WriteWorker::DoOpenComplete(int rv) {
-  if (rv < 0) {
-    state_ = STATE_NONE;
+  if (rv < 0)
     return rv;
-  }
+
   state_ = STATE_WRITE;
   return OK;
 }
 
 int DiskBasedCertCache::WriteWorker::DoWrite() {
   std::string write_data;
   bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
 
-  if (!encoded) {
-    state_ = STATE_NONE;
+  if (!encoded)
     return ERR_FAILED;
-  }
 
   buffer_ = new IOBuffer(write_data.size());
   io_buf_len_ = write_data.size();
   memcpy(buffer_->data(), write_data.data(), io_buf_len_);
 
   state_ = STATE_WRITE_COMPLETE;
 
   return entry_->WriteData(0 /* index */,
                            0 /* offset */,
                            buffer_,
                            write_data.size(),
                            io_callback_,
                            true /* truncate */);
 }
 
 int DiskBasedCertCache::WriteWorker::DoWriteComplete(int rv) {
-  state_ = STATE_NONE;
   if (rv < io_buf_len_)
     return ERR_FAILED;
 
   return OK;
 }
 
 void DiskBasedCertCache::WriteWorker::RunCallbacks(int rv) {
   std::string key;
   if (rv >= 0)
     key = key_;
@@ skipping 10 matching lines @@
   cleanup_callback_.Run();
   cleanup_callback_.Reset();
   RunCallbacks(rv);
   delete this;
 }
 
 void DiskBasedCertCache::WriteWorker::Cancel() {
   canceled_ = true;
 }
 
 DiskBasedCertCache::WriteWorker::~WriteWorker() {

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

Define the destructor right after the constructor.

   if (entry_)
     entry_->Close();
 }
 
 // ReadWorkers represent pending Get jobs in the DiskBasedCertCache. Each
 // certificate requested to be retrieved from the cache is assigned a ReadWorker
 // on a one-to-one basis. The same |key| should not have multiple ReadWorkers
 // at the same time; instead, call AddCallback to add a user_callback_ to
 // the the existing ReadWorker.
 class DiskBasedCertCache::ReadWorker {
  public:
   // |backend| is the backend to read |certificate| from, using
   // |key| as the key for the disk_cache::Entry.
   // |cleanup_callback| is called to clean up this ReadWorker,
   // regardless of success or failure.
   ReadWorker(disk_cache::Backend* backend,
              const std::string& key,
-             const base::Closure& cleanup_callback);
+             const GetCallback& cleanup_callback);
 
   ~ReadWorker();
 
   // Reads the given certificate from the cache. On completion, will invoke all
   // user callbacks.
   void Start();
 
   // Adds a callback to the set of callbacks to be run when this
   // ReadWorker finishes processing.
   void AddCallback(const GetCallback& user_callback);
@@ skipping 27 matching lines @@
   X509Certificate::OSCertHandle cert_handle_;
   std::string key_;
   bool canceled_;
 
   disk_cache::Entry* entry_;
 
   State state_;
   scoped_refptr<IOBuffer> buffer_;
   int io_buf_len_;
 
-  base::Closure cleanup_callback_;
+  GetCallback cleanup_callback_;
   std::vector<GetCallback> user_callbacks_;
   CompletionCallback io_callback_;
 };
 
-DiskBasedCertCache::ReadWorker::ReadWorker(
-    disk_cache::Backend* backend,
-    const std::string& key,
-    const base::Closure& cleanup_callback)
+DiskBasedCertCache::ReadWorker::ReadWorker(disk_cache::Backend* backend,
+                                           const std::string& key,
+                                           const GetCallback& cleanup_callback)
     : backend_(backend),
       cert_handle_(NULL),
       key_(key),
       canceled_(false),
       entry_(NULL),
       state_(STATE_NONE),
       io_buf_len_(0),
       cleanup_callback_(cleanup_callback),
       io_callback_(
           base::Bind(&ReadWorker::OnIOComplete, base::Unretained(this))) {
@@ skipping 54 matching lines @@
 
   return rv;
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpen() {
   state_ = STATE_OPEN_COMPLETE;
   return backend_->OpenEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpenComplete(int rv) {
-  if (rv < 0) {
-    state_ = STATE_NONE;
+  if (rv < 0)
     return rv;
-  }
+
   state_ = STATE_READ;
   return OK;
 }
 
 int DiskBasedCertCache::ReadWorker::DoRead() {
   state_ = STATE_READ_COMPLETE;
   io_buf_len_ = entry_->GetDataSize(0 /* index */);
   buffer_ = new IOBuffer(io_buf_len_);
   return entry_->ReadData(
       0 /* index */, 0 /* offset */, buffer_, io_buf_len_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoReadComplete(int rv) {
-  state_ = STATE_NONE;
   if (rv < io_buf_len_)
     return ERR_FAILED;
 
   cert_handle_ = X509Certificate::CreateOSCertHandleFromBytes(buffer_->data(),
                                                               io_buf_len_);
   if (!cert_handle_)
     return ERR_FAILED;
 
   return OK;
 }
 
 void DiskBasedCertCache::ReadWorker::RunCallbacks() {
   for (std::vector<GetCallback>::const_iterator it = user_callbacks_.begin();
        it != user_callbacks_.end();
        ++it) {
     it->Run(cert_handle_);
   }
   user_callbacks_.clear();
 }
 
 void DiskBasedCertCache::ReadWorker::Finish(int rv) {
-  cleanup_callback_.Run();
+  cleanup_callback_.Run(cert_handle_);
   cleanup_callback_.Reset();
   RunCallbacks();
   delete this;
 }
 
 void DiskBasedCertCache::ReadWorker::Cancel() {
   canceled_ = true;
 }
 
 DiskBasedCertCache::ReadWorker::~ReadWorker() {

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

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

   if (entry_)
     entry_->Close();
   if (cert_handle_)
     X509Certificate::FreeOSCertHandle(cert_handle_);
 }
 
 DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
-    : backend_(backend), weak_factory_(this) {
+    : backend_(backend),
+      mru_cert_cache_(kMemoryCacheMaxSize),
+      mem_cache_hits_(0),
+      mem_cache_misses_(0),
+      weak_factory_(this) {
   DCHECK(backend_);
 }
 
 DiskBasedCertCache::~DiskBasedCertCache() {
   for (WriteWorkerMap::iterator it = write_worker_map_.begin();
        it != write_worker_map_.end();
        ++it) {
     it->second->Cancel();
   }
   for (ReadWorkerMap::iterator it = read_worker_map_.begin();
        it != read_worker_map_.end();
        ++it) {
     it->second->Cancel();
   }
 }
 
 void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
   DCHECK(!key.empty());
 
+  // If the handle is already in the MRU cache, just return that (via callback).
+  // Note, this will also bring the cert_handle to the front

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

Nit: this line break is too short.

+  // of the recency list in the MRU cache.
+  MRUCertCache::iterator mru_it = mru_cert_cache_.Get(key);
+  if (mru_it != mru_cert_cache_.end()) {
+    ++mem_cache_hits_;
+    cb.Run(mru_it->second);
+    return;

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

You may want to tell the backend about this: backend_->OnExternalCacheHit()

+  }
+  ++mem_cache_misses_;
+
   ReadWorkerMap::iterator it = read_worker_map_.find(key);
 
   if (it == read_worker_map_.end()) {
     ReadWorker* worker =
         new ReadWorker(backend_,
                        key,
                        base::Bind(&DiskBasedCertCache::FinishedReadOperation,
                                   weak_factory_.GetWeakPtr(),
                                   key));
     read_worker_map_[key] = worker;
     worker->AddCallback(cb);
     worker->Start();
   } else {
     it->second->AddCallback(cb);
   }
 }
 
 void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
                              const SetCallback& cb) {
   DCHECK(!cb.is_null());
   DCHECK(cert_handle);
   std::string key = GetCacheKeyToCert(cert_handle);
 
+  // If |cert_handle| already exists in the MRU cache, there is no need to
+  // re-write it to the disk cache. This will also bring |cert_handle|
+  // to the front of the recency list in the MRU cache.
+  if (mru_cert_cache_.Get(key) != mru_cert_cache_.end()) {
+    ++mem_cache_hits_;
+    cb.Run(key);
+    return;
+  }
+  ++mem_cache_misses_;

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

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?

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

Yes.

I think we only want Get to be satisfied by the cache. Set should add to the
cache.

+
   WriteWorkerMap::iterator it = write_worker_map_.find(key);
 
   if (it == write_worker_map_.end()) {
     WriteWorker* worker =
         new WriteWorker(backend_,
                         key,
                         cert_handle,
                         base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
                                    weak_factory_.GetWeakPtr(),
-                                   key));
+                                   key,
+                                   cert_handle));
     write_worker_map_[key] = worker;
     worker->AddCallback(cb);
     worker->Start();
   } else {
     it->second->AddCallback(cb);
   }
 }
 
-void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
+void DiskBasedCertCache::FinishedWriteOperation(
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle) {
   write_worker_map_.erase(key);
+  if (key != std::string())

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

Test !key.empty().

+    mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));
 }
 
-void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+void DiskBasedCertCache::FinishedReadOperation(
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle) {
+  if (cert_handle)
+    mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));
   read_worker_map_.erase(key);
 }
 
 }  // namespace net