Remove unnecessary copies in CRLSet parsing.

This showed up in Chrome Traces covering the startup of Chrome for Android.
CRLSet parsing was taking 70ms of CPU on Nexus 4 (although this is happening on
the File thread).
That cost is now down to 18ms. There are certainly further ways to decrease
this even more though.

BUG=376793

Review URL: https://codereview.chromium.org/302643003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274607 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/net/crl_set_fetcher.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/net/crl_set_fetcher.h"
 
 #include "base/bind.h"
+#include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/path_service.h"
@@ -73,11 +74,16 @@ void CRLSetFetcher::DoInitialLoadFromDisk() {
 
 void CRLSetFetcher::LoadFromDisk(base::FilePath path,
                                  scoped_refptr<net::CRLSet>* out_crl_set) {
+  TRACE_EVENT0("CRLSetFetcher", "LoadFromDisk");
+
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   std::string crl_set_bytes;
-  if (!base::ReadFileToString(path, &crl_set_bytes))
-    return;
+  {
+    TRACE_EVENT0("CRLSetFetcher", "ReadFileToString");
+    if (!base::ReadFileToString(path, &crl_set_bytes))
+      return;
+  }
 
   if (!net::CRLSet::Parse(crl_set_bytes, out_crl_set)) {
     LOG(WARNING) << "Failed to parse CRL set from " << path.MaybeAsASCII();

net/cert/crl_set.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/base64.h"
+#include "base/debug/trace_event.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
@@ -151,27 +152,32 @@ static bool ReadCRL(base::StringPiece* data, std::string* out_parent_spki_hash,
                     std::vector<std::string>* out_serials) {
   if (data->size() < crypto::kSHA256Length)
     return false;
-  *out_parent_spki_hash = std::string(data->data(), crypto::kSHA256Length);
+  out_parent_spki_hash->assign(data->data(), crypto::kSHA256Length);
   data->remove_prefix(crypto::kSHA256Length);
 
   if (data->size() < sizeof(uint32))
     return false;
   uint32 num_serials;
   memcpy(&num_serials, data->data(), sizeof(uint32));  // assumes little endian
+  if (num_serials > 32 * 1024 * 1024)  // Sanity check.
+    return false;
+
+  out_serials->reserve(num_serials);
   data->remove_prefix(sizeof(uint32));
 
   for (uint32 i = 0; i < num_serials; ++i) {
-    uint8 serial_length;
     if (data->size() < sizeof(uint8))
       return false;
-    memcpy(&serial_length, data->data(), sizeof(uint8));
+
+    uint8 serial_length = data->data()[0];
     data->remove_prefix(sizeof(uint8));
 
     if (data->size() < serial_length)
       return false;
-    std::string serial(data->data(), serial_length);
+
+    out_serials->push_back(std::string());
+    out_serials->back().assign(data->data(), serial_length);
     data->remove_prefix(serial_length);
-    out_serials->push_back(serial);
   }
 
   return true;
@@ -185,14 +191,21 @@ bool CRLSet::CopyBlockedSPKIsFromHeader(base::DictionaryValue* header_dict) {
   }
 
   blocked_spkis_.clear();
+  blocked_spkis_.reserve(blocked_spkis_list->GetSize());
+
+  std::string spki_sha256_base64;
 
   for (size_t i = 0; i < blocked_spkis_list->GetSize(); ++i) {
-    std::string spki_sha256_base64, spki_sha256;
+    spki_sha256_base64.clear();
+
     if (!blocked_spkis_list->GetString(i, &spki_sha256_base64))
       return false;
-    if (!base::Base64Decode(spki_sha256_base64, &spki_sha256))
+
+    blocked_spkis_.push_back(std::string());
+    if (!base::Base64Decode(spki_sha256_base64, &blocked_spkis_.back())) {
+      blocked_spkis_.pop_back();
       return false;
-    blocked_spkis_.push_back(spki_sha256);
+    }
   }
 
   return true;
@@ -200,6 +213,7 @@ bool CRLSet::CopyBlockedSPKIsFromHeader(base::DictionaryValue* header_dict) {
 
 // static
 bool CRLSet::Parse(base::StringPiece data, scoped_refptr<CRLSet>* out_crl_set) {
+  TRACE_EVENT0("CRLSet", "Parse");
   // Other parts of Chrome assume that we're little endian, so we don't lose
   // anything by doing this.
 #if defined(__BYTE_ORDER)
@@ -238,18 +252,26 @@ bool CRLSet::Parse(base::StringPiece data, scoped_refptr<CRLSet>* out_crl_set) {
   if (not_after < 0)
     return false;
 
-  scoped_refptr<CRLSet> crl_set(new CRLSet);
+  scoped_refptr<CRLSet> crl_set(new CRLSet());
   crl_set->sequence_ = static_cast<uint32>(sequence);
   crl_set->not_after_ = static_cast<uint64>(not_after);
+  crl_set->crls_.reserve(64);  // Value observed experimentally.
 
   for (size_t crl_index = 0; !data.empty(); crl_index++) {
-    std::string parent_spki_sha256;
-    std::vector<std::string> serials;
-    if (!ReadCRL(&data, &parent_spki_sha256, &serials))
+    // Speculatively push back a pair and pass it to ReadCRL() to avoid
+    // unnecessary copies.
+    crl_set->crls_.push_back(
+        std::make_pair(std::string(), std::vector<std::string>()));
+    std::pair<std::string, std::vector<std::string> >* const back_pair =
+        &crl_set->crls_.back();
+
+    if (!ReadCRL(&data, &back_pair->first, &back_pair->second)) {
+      // Undo the speculative push_back() performed above.
+      crl_set->crls_.pop_back();
       return false;
+    }
 
-    crl_set->crls_.push_back(std::make_pair(parent_spki_sha256, serials));
-    crl_set->crls_index_by_issuer_[parent_spki_sha256] = crl_index;
+    crl_set->crls_index_by_issuer_[back_pair->first] = crl_index;
   }
 
   if (!crl_set->CopyBlockedSPKIsFromHeader(header_dict.get()))
@@ -548,7 +570,7 @@ CRLSet::Result CRLSet::CheckSerial(
   while (serial.size() > 1 && serial[0] == 0x00)
     serial.remove_prefix(1);
 
-  std::map<std::string, size_t>::const_iterator i =
+  base::hash_map<std::string, size_t>::const_iterator i =
       crls_index_by_issuer_.find(issuer_spki_hash.as_string());
   if (i == crls_index_by_issuer_.end())
     return UNKNOWN;

net/cert/crl_set.h

@@ -5,11 +5,11 @@
 #ifndef NET_CERT_CRL_SET_H_
 #define NET_CERT_CRL_SET_H_
 
-#include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/containers/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_piece.h"
 #include "net/base/net_export.h"
@@ -117,7 +117,7 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   // where the information for that issuer can be found. We have both |crls_|
   // and |crls_index_by_issuer_| because, when applying a delta update, we need
   // to identify a CRL by index.
-  std::map<std::string, size_t> crls_index_by_issuer_;
+  base::hash_map<std::string, size_t> crls_index_by_issuer_;
   // blocked_spkis_ contains the SHA256 hashes of SPKIs which are to be blocked
   // no matter where in a certificate chain they might appear.
   std::vector<std::string> blocked_spkis_;