SimpleCache: open with fewer seeks.

There's usually no reason to read the file header before reading the
stream 0 (headers) data when opening a simple cache entry. By not doing
so, seeks in the critical path of the loading headers are reduced, which
should speed up http caching performance.

To be careful, the headers are still checked when the first read of data
(if any) is performed. This delays detecting hash collisions in the
cache, because the header data is required to compare keys (instead of
just hashes).

To compensate for the later key comparison, a SHA256 of the key is
stored as an optional prefix to the end of file record. This is used to
validate the key is correct before returning headers to the client.

Because the key checking logic is now totally migrated back into the
SimpleSynchronousEntry, we can return to reporting on key matching in
the SyncOpenResult histograms.

[email protected],[email protected],[email protected]
BUG=611732

Review-Url: https://codereview.chromium.org/1977863003
Cr-Commit-Position: refs/heads/master@{#395083}
(cherry picked from commit 9f15f22)

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

Cr-Commit-Position: refs/branch-heads/2743@{#4}
Cr-Branched-From: 2b3ae3b-refs/heads/master@{#394939}

Author: Gavin Peters

net/disk_cache/backend_unittest.cc

@@ -33,6 +33,7 @@
 #include "net/disk_cache/simple/simple_backend_impl.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
 #include "net/disk_cache/simple/simple_index.h"
+#include "net/disk_cache/simple/simple_synchronous_entry.h"
 #include "net/disk_cache/simple/simple_test_util.h"
 #include "net/disk_cache/simple/simple_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -253,16 +254,19 @@ void DiskCacheBackendTest::InitSparseCache(base::Time* doomed_start,
 bool DiskCacheBackendTest::CreateSetOfRandomEntries(
     std::set<std::string>* key_pool) {
   const int kNumEntries = 10;
+  const int initial_entry_count = cache_->GetEntryCount();
 
   for (int i = 0; i < kNumEntries; ++i) {
     std::string key = GenerateKey(true);
     disk_cache::Entry* entry;
-    if (CreateEntry(key, &entry) != net::OK)
+    if (CreateEntry(key, &entry) != net::OK) {
       return false;
+    }
     key_pool->insert(key);
     entry->Close();
   }
-  return key_pool->size() == static_cast<size_t>(cache_->GetEntryCount());
+  return key_pool->size() ==
+         static_cast<size_t>(cache_->GetEntryCount() - initial_entry_count);
 }
 
 // Performs iteration over the backend and checks that the keys of entries
@@ -3688,9 +3692,6 @@ TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationWhileDoomed) {
 TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationCorruption) {
   SetSimpleCacheMode();
   InitCache();
-  std::set<std::string> key_pool;
-  ASSERT_TRUE(CreateSetOfRandomEntries(&key_pool));
-
   // Create a corrupt entry. The write/read sequence ensures that the entry will
   // have been created before corrupting the platform files, in the case of
   // optimistic operations.
@@ -3707,6 +3708,9 @@ TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationCorruption) {
   ASSERT_EQ(kSize, ReadData(corrupted_entry, 0, 0, buffer.get(), kSize));
   corrupted_entry->Close();
 
+  std::set<std::string> key_pool;
+  ASSERT_TRUE(CreateSetOfRandomEntries(&key_pool));
+
   EXPECT_TRUE(disk_cache::simple_util::CreateCorruptFileForTests(
       key, cache_path_));
   EXPECT_EQ(key_pool.size() + 1, static_cast<size_t>(cache_->GetEntryCount()));
@@ -3740,6 +3744,27 @@ TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationDestruction) {
   // This test passes if we don't leak memory.
 }
 
+// Tests that enumerations include entries with long keys.
+TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationLongKeys) {
+  SetSimpleCacheMode();
+  InitCache();
+  std::set<std::string> key_pool;
+  ASSERT_TRUE(CreateSetOfRandomEntries(&key_pool));
+
+  const size_t long_key_length =
+      disk_cache::SimpleSynchronousEntry::kInitialHeaderRead + 10;
+  std::string long_key(long_key_length, 'X');
+  key_pool.insert(long_key);
+  disk_cache::Entry* entry = NULL;
+  ASSERT_EQ(net::OK, CreateEntry(long_key.c_str(), &entry));
+  entry->Close();
+
+  std::unique_ptr<TestIterator> iter = CreateIterator();
+  size_t count = 0;
+  EXPECT_TRUE(EnumerateAndMatchKeys(-1, iter.get(), &key_pool, &count));
+  EXPECT_TRUE(key_pool.empty());
+}
+
 // Tests that a SimpleCache doesn't crash when files are deleted very quickly
 // after closing.
 // NOTE: IF THIS TEST IS FLAKY THEN IT IS FAILING. See https://crbug.com/416940

net/disk_cache/entry_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/threading/platform_thread.h"
@@ -21,6 +22,7 @@
 #include "net/disk_cache/disk_cache_test_base.h"
 #include "net/disk_cache/disk_cache_test_util.h"
 #include "net/disk_cache/memory/mem_entry_impl.h"
+#include "net/disk_cache/simple/simple_backend_impl.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
 #include "net/disk_cache/simple/simple_entry_impl.h"
 #include "net/disk_cache/simple/simple_synchronous_entry.h"
@@ -2708,7 +2710,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheNoEOF) {
   SetSimpleCacheMode();
   InitCache();
 
-  const char key[] = "the first key";
+  const std::string key("the first key");
 
   disk_cache::Entry* entry = NULL;
   ASSERT_EQ(net::OK, CreateEntry(key, &entry));
@@ -2728,9 +2730,8 @@ TEST_F(DiskCacheEntryTest, SimpleCacheNoEOF) {
   int kTruncationBytes = -static_cast<int>(sizeof(disk_cache::SimpleFileEOF));
   const base::FilePath entry_path = cache_path_.AppendASCII(
       disk_cache::simple_util::GetFilenameFromKeyAndFileIndex(key, 0));
-  const int64_t invalid_size =
-      disk_cache::simple_util::GetFileSizeFromKeyAndDataSize(key,
-                                                             kTruncationBytes);
+  const int64_t invalid_size = disk_cache::simple_util::GetFileSizeFromDataSize(
+      key.size(), kTruncationBytes);
   EXPECT_TRUE(TruncatePath(entry_path, invalid_size));
   EXPECT_EQ(net::ERR_FAILED, OpenEntry(key, &entry));
   DisableIntegrityCheck();
@@ -3688,7 +3689,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheStream1SizeChanges) {
   SetSimpleCacheMode();
   InitCache();
   disk_cache::Entry* entry = NULL;
-  const char key[] = "the key";
+  const std::string key("the key");
   const int kSize = 100;
   scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kSize));
   scoped_refptr<net::IOBuffer> buffer_read(new net::IOBuffer(kSize));
@@ -3726,7 +3727,7 @@ TEST_F(DiskCacheEntryTest, SimpleCacheStream1SizeChanges) {
   int sparse_data_size = 0;
   disk_cache::SimpleEntryStat entry_stat(
       base::Time::Now(), base::Time::Now(), data_size, sparse_data_size);
-  int eof_offset = entry_stat.GetEOFOffsetInFile(key, 0);
+  int eof_offset = entry_stat.GetEOFOffsetInFile(key.size(), 0);
   disk_cache::SimpleFileEOF eof_record;
   ASSERT_EQ(static_cast<int>(sizeof(eof_record)),
             entry_file0.Read(eof_offset, reinterpret_cast<char*>(&eof_record),
@@ -4165,3 +4166,75 @@ TEST_F(DiskCacheEntryTest, SimpleCacheTruncateLargeSparseFile) {
 
   entry->Close();
 }
+
+TEST_F(DiskCacheEntryTest, SimpleCacheReadWithoutKeySHA256) {
+  // This test runs as APP_CACHE to make operations more synchronous.
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  InitCache();
+  disk_cache::Entry* entry;
+  std::string key("a key");
+  ASSERT_EQ(net::OK, CreateEntry(key, &entry));
+
+  const std::string stream_0_data = "data for stream zero";
+  scoped_refptr<net::IOBuffer> stream_0_iobuffer(
+      new net::StringIOBuffer(stream_0_data));
+  EXPECT_EQ(static_cast<int>(stream_0_data.size()),
+            WriteData(entry, 0, 0, stream_0_iobuffer.get(),
+                      stream_0_data.size(), false));
+  const std::string stream_1_data = "FOR STREAM ONE, QUITE DIFFERENT THINGS";
+  scoped_refptr<net::IOBuffer> stream_1_iobuffer(
+      new net::StringIOBuffer(stream_1_data));
+  EXPECT_EQ(static_cast<int>(stream_1_data.size()),
+            WriteData(entry, 1, 0, stream_1_iobuffer.get(),
+                      stream_1_data.size(), false));
+  entry->Close();
+
+  base::RunLoop().RunUntilIdle();
+  disk_cache::SimpleBackendImpl::FlushWorkerPoolForTesting();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(
+      disk_cache::simple_util::RemoveKeySHA256FromEntry(key, cache_path_));
+  ASSERT_EQ(net::OK, OpenEntry(key, &entry));
+  ScopedEntryPtr entry_closer(entry);
+
+  EXPECT_EQ(static_cast<int>(stream_0_data.size()), entry->GetDataSize(0));
+  scoped_refptr<net::IOBuffer> check_stream_0_data(
+      new net::IOBuffer(stream_0_data.size()));
+  EXPECT_EQ(
+      static_cast<int>(stream_0_data.size()),
+      ReadData(entry, 0, 0, check_stream_0_data.get(), stream_0_data.size()));
+  EXPECT_EQ(0, stream_0_data.compare(0, std::string::npos,
+                                     check_stream_0_data->data(),
+                                     stream_0_data.size()));
+
+  EXPECT_EQ(static_cast<int>(stream_1_data.size()), entry->GetDataSize(1));
+  scoped_refptr<net::IOBuffer> check_stream_1_data(
+      new net::IOBuffer(stream_1_data.size()));
+  EXPECT_EQ(
+      static_cast<int>(stream_1_data.size()),
+      ReadData(entry, 1, 0, check_stream_1_data.get(), stream_1_data.size()));
+  EXPECT_EQ(0, stream_1_data.compare(0, std::string::npos,
+                                     check_stream_1_data->data(),
+                                     stream_1_data.size()));
+}
+
+TEST_F(DiskCacheEntryTest, SimpleCacheReadCorruptKeySHA256) {
+  // This test runs as APP_CACHE to make operations more synchronous.
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  InitCache();
+  disk_cache::Entry* entry;
+  std::string key("a key");
+  ASSERT_EQ(net::OK, CreateEntry(key, &entry));
+  entry->Close();
+
+  base::RunLoop().RunUntilIdle();
+  disk_cache::SimpleBackendImpl::FlushWorkerPoolForTesting();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(
+      disk_cache::simple_util::CorruptKeySHA256FromEntry(key, cache_path_));
+  EXPECT_NE(net::OK, OpenEntry(key, &entry));
+}

net/disk_cache/simple/simple_backend_impl.cc

@@ -390,14 +390,7 @@ int SimpleBackendImpl::OpenEntry(const std::string& key,
   }
   scoped_refptr<SimpleEntryImpl> simple_entry =
       CreateOrFindActiveEntry(entry_hash, key);
-  CompletionCallback backend_callback =
-      base::Bind(&SimpleBackendImpl::OnEntryOpenedFromKey,
-                 AsWeakPtr(),
-                 key,
-                 entry,
-                 simple_entry,
-                 callback);
-  return simple_entry->OpenEntry(entry, backend_callback);
+  return simple_entry->OpenEntry(entry, callback);
 }
 
 int SimpleBackendImpl::CreateEntry(const std::string& key,
@@ -713,29 +706,6 @@ void SimpleBackendImpl::OnEntryOpenedFromHash(
   }
 }
 
-void SimpleBackendImpl::OnEntryOpenedFromKey(
-    const std::string key,
-    Entry** entry,
-    const scoped_refptr<SimpleEntryImpl>& simple_entry,
-    const CompletionCallback& callback,
-    int error_code) {
-  int final_code = error_code;
-  if (final_code == net::OK) {
-    bool key_matches = key.compare(simple_entry->key()) == 0;
-    if (!key_matches) {
-      // TODO(clamy): Add a unit test to check this code path.
-      DLOG(WARNING) << "Key mismatch on open.";
-      simple_entry->Doom();
-      simple_entry->Close();
-      final_code = net::ERR_FAILED;
-    } else {
-      DCHECK_EQ(simple_entry->entry_hash(), simple_util::GetEntryHashKey(key));
-    }
-    SIMPLE_CACHE_UMA(BOOLEAN, "KeyMatchedOnOpen", cache_type_, key_matches);
-  }
-  callback.Run(final_code);
-}
-
 void SimpleBackendImpl::DoomEntriesComplete(
     std::unique_ptr<std::vector<uint64_t>> entry_hashes,
     const net::CompletionCallback& callback,

net/disk_cache/simple/simple_entry_format.h

@@ -25,7 +25,15 @@ const uint64_t kSimpleSparseRangeMagicNumber = UINT64_C(0xeb97bf016553676b);
 //   - the data from stream 1.
 //   - a SimpleFileEOF record for stream 1.
 //   - the data from stream 0.
+//   - (optionally) the SHA256 of the key.
 //   - a SimpleFileEOF record for stream 0.
+//
+// Because stream 0 data (typically HTTP headers) is on the critical path of
+// requests, on open, the cache reads the end of the record and does not
+// read the SimpleFileHeader. If the key can be validated with a SHA256, then
+// the stream 0 data can be returned to the caller without reading the
+// SimpleFileHeader. If the key SHA256 is not present, then the cache must
+// read the SimpleFileHeader to confirm key equality.
 
 // A file containing stream 2 in the Simple cache consists of:
 //   - a SimpleFileHeader.
@@ -47,6 +55,7 @@ struct NET_EXPORT_PRIVATE SimpleFileHeader {
 struct NET_EXPORT_PRIVATE SimpleFileEOF {
   enum Flags {
     FLAG_HAS_CRC32 = (1U << 0),
+    FLAG_HAS_KEY_SHA256 = (1U << 1),  // Preceding the record if present.
   };
 
   SimpleFileEOF();

net/disk_cache/simple/simple_entry_impl.cc

@@ -696,19 +696,13 @@ void SimpleEntryImpl::OpenEntryInternal(bool have_index,
   std::unique_ptr<SimpleEntryCreationResults> results(
       new SimpleEntryCreationResults(SimpleEntryStat(
           last_used_, last_modified_, data_size_, sparse_data_size_)));
-  Closure task = base::Bind(&SimpleSynchronousEntry::OpenEntry,
-                            cache_type_,
-                            path_,
-                            entry_hash_,
-                            have_index,
-                            results.get());
-  Closure reply = base::Bind(&SimpleEntryImpl::CreationOperationComplete,
-                             this,
-                             callback,
-                             start_time,
-                             base::Passed(&results),
-                             out_entry,
-                             net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_OPEN_END);
+  Closure task =
+      base::Bind(&SimpleSynchronousEntry::OpenEntry, cache_type_, path_, key_,
+                 entry_hash_, have_index, results.get());
+  Closure reply =
+      base::Bind(&SimpleEntryImpl::CreationOperationComplete, this, callback,
+                 start_time, base::Passed(&results), out_entry,
+                 net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_OPEN_END);
   worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 }
 
@@ -1147,11 +1141,14 @@ void SimpleEntryImpl::CreationOperationComplete(
     crc32s_[0] = in_results->stream_0_crc32;
     crc32s_end_offset_[0] = in_results->entry_stat.data_size(0);
   }
+  // If this entry was opened by hash, key_ could still be empty. If so, update
+  // it with the key read from the synchronous entry.
   if (key_.empty()) {
     SetKey(synchronous_entry_->key());
   } else {
-    // This should only be triggered when creating an entry. The key check in
-    // the open case is handled in SimpleBackendImpl.
+    // This should only be triggered when creating an entry. In the open case
+    // the key is either copied from the arguments to open, or checked
+    // in the synchronous entry.
     DCHECK_EQ(key_, synchronous_entry_->key());
   }
   UpdateDataFromEntryStat(in_results->entry_stat);
@@ -1403,7 +1400,7 @@ int64_t SimpleEntryImpl::GetDiskUsage() const {
   int64_t file_size = 0;
   for (int i = 0; i < kSimpleEntryStreamCount; ++i) {
     file_size +=
-        simple_util::GetFileSizeFromKeyAndDataSize(key_, data_size_[i]);
+        simple_util::GetFileSizeFromDataSize(key_.size(), data_size_[i]);
   }
   file_size += sparse_data_size_;
   return file_size;

net/disk_cache/simple/simple_entry_impl.h

@@ -79,6 +79,11 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
 
   const std::string& key() const { return key_; }
   uint64_t entry_hash() const { return entry_hash_; }
+
+  // The key is not a constructor parameter to the SimpleEntryImpl, because
+  // during cache iteration, it's necessary to open entries by their hash
+  // alone. In that case, the SimpleSynchronousEntry will read the key from disk
+  // and it will be set.
   void SetKey(const std::string& key);
 
   // From Entry: