SimpleCache: open with fewer seeks.

net/disk_cache/simple/simple_synchronous_entry.cc

 // Copyright (c) 2013 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/disk_cache/simple/simple_synchronous_entry.h"
 
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <limits>
 
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_util.h"
 #include "base/hash.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
 #include "base/timer/elapsed_timer.h"
 #include "net/base/io_buffer.h"
@@ skipping 10 matching lines @@
 namespace {
 
 // Used in histograms, please only add entries at the end.
 enum OpenEntryResult {
   OPEN_ENTRY_SUCCESS = 0,
   OPEN_ENTRY_PLATFORM_FILE_ERROR = 1,
   OPEN_ENTRY_CANT_READ_HEADER = 2,
   OPEN_ENTRY_BAD_MAGIC_NUMBER = 3,
   OPEN_ENTRY_BAD_VERSION = 4,
   OPEN_ENTRY_CANT_READ_KEY = 5,
-  // OPEN_ENTRY_KEY_MISMATCH = 6, Deprecated.
+  OPEN_ENTRY_KEY_MISMATCH = 6,
   OPEN_ENTRY_KEY_HASH_MISMATCH = 7,
   OPEN_ENTRY_SPARSE_OPEN_FAILED = 8,
   OPEN_ENTRY_MAX = 9,
 };
 
 // Used in histograms, please only add entries at the end.
 enum WriteResult {
   WRITE_RESULT_SUCCESS = 0,
   WRITE_RESULT_PRETRUNCATE_FAILURE,
   WRITE_RESULT_WRITE_FAILURE,
@@ skipping 12 matching lines @@
   CHECK_EOF_RESULT_CRC_MISMATCH,
   CHECK_EOF_RESULT_MAX,
 };
 
 // Used in histograms, please only add entries at the end.
 enum CloseResult {
   CLOSE_RESULT_SUCCESS,
   CLOSE_RESULT_WRITE_FAILURE,
 };
 
+// When opening an entry without knowing the key, the header must be read
+// without knowing the size of the key. This is how much to read initially, to
+// make it likely the entire key is read.
+const size_t kInitialHeaderRead = 64 * 1024;
+
 void RecordSyncOpenResult(net::CacheType cache_type,
                           OpenEntryResult result,
                           bool had_index) {
   DCHECK_LT(result, OPEN_ENTRY_MAX);
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncOpenResult", cache_type, result, OPEN_ENTRY_MAX);
   if (had_index) {
     SIMPLE_CACHE_UMA(ENUMERATION,
                      "SyncOpenResult_WithIndex", cache_type,
                      result, OPEN_ENTRY_MAX);
@@ skipping 38 matching lines @@
   return true;
 }
 
 }  // namespace
 
 namespace disk_cache {
 
 using simple_util::GetEntryHashKey;
 using simple_util::GetFilenameFromEntryHashAndFileIndex;
 using simple_util::GetSparseFilenameFromEntryHash;
-using simple_util::GetDataSizeFromKeyAndFileSize;
-using simple_util::GetFileSizeFromKeyAndDataSize;
+using simple_util::GetHeaderSize;
+using simple_util::GetDataSizeFromFileSize;
+using simple_util::GetFileSizeFromDataSize;
 using simple_util::GetFileIndexFromStreamIndex;
 
 SimpleEntryStat::SimpleEntryStat(base::Time last_used,
                                  base::Time last_modified,
                                  const int32_t data_size[],
                                  const int32_t sparse_data_size)
     : last_used_(last_used),
       last_modified_(last_modified),
       sparse_data_size_(sparse_data_size) {
   memcpy(data_size_, data_size, sizeof(data_size_));
 }
 
-int SimpleEntryStat::GetOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetOffsetInFile(size_t key_length,
                                      int offset,
                                      int stream_index) const {
-  const size_t headers_size = sizeof(SimpleFileHeader) + key.size();
+  const size_t headers_size = sizeof(SimpleFileHeader) + key_length;
   const size_t additional_offset =
       stream_index == 0 ? data_size_[1] + sizeof(SimpleFileEOF) : 0;
   return headers_size + offset + additional_offset;
 }
 
-int SimpleEntryStat::GetEOFOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetEOFOffsetInFile(size_t key_length,
                                         int stream_index) const {
-  return GetOffsetInFile(key, data_size_[stream_index], stream_index);
+  return GetOffsetInFile(key_length, data_size_[stream_index], stream_index);
 }
 
-int SimpleEntryStat::GetLastEOFOffsetInFile(const std::string& key,
+int SimpleEntryStat::GetLastEOFOffsetInFile(size_t key_length,
                                             int stream_index) const {
   const int file_index = GetFileIndexFromStreamIndex(stream_index);
   const int eof_data_offset =
       file_index == 0 ? data_size_[0] + data_size_[1] + sizeof(SimpleFileEOF)
                       : data_size_[2];
-  return GetOffsetInFile(key, eof_data_offset, stream_index);
+  return GetOffsetInFile(key_length, eof_data_offset, stream_index);
 }
 
-int64_t SimpleEntryStat::GetFileSize(const std::string& key,
-                                     int file_index) const {
+int64_t SimpleEntryStat::GetFileSize(size_t key_length, int file_index) const {
   const int32_t total_data_size =
       file_index == 0 ? data_size_[0] + data_size_[1] + sizeof(SimpleFileEOF)
                       : data_size_[2];
-  return GetFileSizeFromKeyAndDataSize(key, total_data_size);
+  return GetFileSizeFromDataSize(key_length, total_data_size);
 }
 
 SimpleEntryCreationResults::SimpleEntryCreationResults(
     SimpleEntryStat entry_stat)
     : sync_entry(NULL),
       entry_stat(entry_stat),
       stream_0_crc32(crc32(0, Z_NULL, 0)),
       result(net::OK) {
 }
 
@@ skipping 30 matching lines @@
 
 SimpleSynchronousEntry::EntryOperationData::EntryOperationData(
     int64_t sparse_offset_p,
     int buf_len_p)
     : sparse_offset(sparse_offset_p), buf_len(buf_len_p) {}
 
 // static
 void SimpleSynchronousEntry::OpenEntry(
     net::CacheType cache_type,
     const FilePath& path,
+    const std::string& key,
     const uint64_t entry_hash,
-    bool had_index,
+    const bool had_index,
     SimpleEntryCreationResults* out_results) {
   base::ElapsedTimer open_time;
   SimpleSynchronousEntry* sync_entry =
-      new SimpleSynchronousEntry(cache_type, path, "", entry_hash);
-  out_results->result =
-      sync_entry->InitializeForOpen(had_index,
-                                    &out_results->entry_stat,
-                                    &out_results->stream_0_data,
-                                    &out_results->stream_0_crc32);
+      new SimpleSynchronousEntry(cache_type, path, key, entry_hash, had_index);
+  out_results->result = sync_entry->InitializeForOpen(
+      &out_results->entry_stat, &out_results->stream_0_data,
+      &out_results->stream_0_crc32);
   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
     out_results->sync_entry = NULL;
     out_results->stream_0_data = NULL;
     return;
   }
   UMA_HISTOGRAM_TIMES("SimpleCache.DiskOpenLatency", open_time.Elapsed());
   out_results->sync_entry = sync_entry;
 }
 
 // static
 void SimpleSynchronousEntry::CreateEntry(
     net::CacheType cache_type,
     const FilePath& path,
     const std::string& key,
     const uint64_t entry_hash,
-    bool had_index,
+    const bool had_index,
     SimpleEntryCreationResults* out_results) {
   DCHECK_EQ(entry_hash, GetEntryHashKey(key));
   SimpleSynchronousEntry* sync_entry =
-      new SimpleSynchronousEntry(cache_type, path, key, entry_hash);
-  out_results->result = sync_entry->InitializeForCreate(
-      had_index, &out_results->entry_stat);
+      new SimpleSynchronousEntry(cache_type, path, key, entry_hash, had_index);
+  out_results->result =
+      sync_entry->InitializeForCreate(&out_results->entry_stat);
   if (out_results->result != net::OK) {
     if (out_results->result != net::ERR_FILE_EXISTS)
       sync_entry->Doom();
     delete sync_entry;
     out_results->sync_entry = NULL;
     return;
   }
   out_results->sync_entry = sync_entry;
 }
 
@@ skipping 20 matching lines @@
       [&path](const uint64_t& key_hash) {
         return SimpleSynchronousEntry::DeleteFilesForEntryHash(path, key_hash);
       });
   return (did_delete_count == key_hashes->size()) ? net::OK : net::ERR_FAILED;
 }
 
 void SimpleSynchronousEntry::ReadData(const EntryOperationData& in_entry_op,
                                       net::IOBuffer* out_buf,
                                       uint32_t* out_crc32,
                                       SimpleEntryStat* entry_stat,
-                                      int* out_result) const {
+                                      int* out_result) {
   DCHECK(initialized_);
   DCHECK_NE(0, in_entry_op.index);
-  const int64_t file_offset =
-      entry_stat->GetOffsetInFile(key_, in_entry_op.offset, in_entry_op.index);
   int file_index = GetFileIndexFromStreamIndex(in_entry_op.index);
+  if (header_and_key_check_needed_ && !CheckHeaderAndKeyForOpen(file_index)) {
+    *out_result = net::ERR_FAILED;
+    Doom();
+    return;
+  }
+  const int64_t file_offset = entry_stat->GetOffsetInFile(
+      key_.size(), in_entry_op.offset, in_entry_op.index);
   // Zero-length reads and reads to the empty streams of omitted files should
   // be handled in the SimpleEntryImpl.
   DCHECK_GT(in_entry_op.buf_len, 0);
   DCHECK(!empty_file_omitted_[file_index]);
-  File* file = const_cast<File*>(&files_[file_index]);
-  int bytes_read =
-      file->Read(file_offset, out_buf->data(), in_entry_op.buf_len);
+  int bytes_read = files_[file_index].Read(file_offset, out_buf->data(),
+                                           in_entry_op.buf_len);
   if (bytes_read > 0) {
     entry_stat->set_last_used(Time::Now());
     *out_crc32 = crc32(crc32(0L, Z_NULL, 0),
                        reinterpret_cast<const Bytef*>(out_buf->data()),
                        bytes_read);
   }
   if (bytes_read >= 0) {
     *out_result = bytes_read;
   } else {
     *out_result = net::ERR_CACHE_READ_FAILURE;
     Doom();
   }
 }
 
 void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
                                        net::IOBuffer* in_buf,
                                        SimpleEntryStat* out_entry_stat,
                                        int* out_result) {
   DCHECK(initialized_);
   DCHECK_NE(0, in_entry_op.index);
   int index = in_entry_op.index;
   int file_index = GetFileIndexFromStreamIndex(index);
   int offset = in_entry_op.offset;
   int buf_len = in_entry_op.buf_len;
   bool truncate = in_entry_op.truncate;
   bool doomed = in_entry_op.doomed;
   const int64_t file_offset = out_entry_stat->GetOffsetInFile(
-      key_, in_entry_op.offset, in_entry_op.index);
+      key_.size(), in_entry_op.offset, in_entry_op.index);
   bool extending_by_write = offset + buf_len > out_entry_stat->data_size(index);
 
   if (empty_file_omitted_[file_index]) {
     // Don't create a new file if the entry has been doomed, to avoid it being
     // mixed up with a newly-created entry with the same key.
     if (doomed) {
       DLOG(WARNING) << "Rejecting write to lazily omitted stream "
                     << in_entry_op.index << " of doomed cache entry.";
       RecordWriteResult(cache_type_, WRITE_RESULT_LAZY_STREAM_ENTRY_DOOMED);
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
@@ skipping 12 matching lines @@
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
   DCHECK(!empty_file_omitted_[file_index]);
 
   if (extending_by_write) {
     // The EOF record and the eventual stream afterward need to be zeroed out.
     const int64_t file_eof_offset =
-        out_entry_stat->GetEOFOffsetInFile(key_, index);
+        out_entry_stat->GetEOFOffsetInFile(key_.size(), index);
     if (!files_[file_index].SetLength(file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_PRETRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
   if (buf_len > 0) {
     if (files_[file_index].Write(file_offset, in_buf->data(), buf_len) !=
         buf_len) {
       RecordWriteResult(cache_type_, WRITE_RESULT_WRITE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
   if (!truncate && (buf_len > 0 || !extending_by_write)) {
     out_entry_stat->set_data_size(
         index, std::max(out_entry_stat->data_size(index), offset + buf_len));
   } else {
     out_entry_stat->set_data_size(index, offset + buf_len);
-    int file_eof_offset = out_entry_stat->GetLastEOFOffsetInFile(key_, index);
+    int file_eof_offset =
+        out_entry_stat->GetLastEOFOffsetInFile(key_.size(), index);
     if (!files_[file_index].SetLength(file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_TRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
 
   RecordWriteResult(cache_type_, WRITE_RESULT_SUCCESS);
   base::Time modification_time = Time::Now();
@@ skipping 230 matching lines @@
   }
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
 }
 
 void SimpleSynchronousEntry::Close(
     const SimpleEntryStat& entry_stat,
     std::unique_ptr<std::vector<CRCRecord>> crc32s_to_write,
     net::GrowableIOBuffer* stream_0_data) {
   DCHECK(stream_0_data);
   // Write stream 0 data.
-  int stream_0_offset = entry_stat.GetOffsetInFile(key_, 0, 0);
+  int stream_0_offset = entry_stat.GetOffsetInFile(key_.size(), 0, 0);
   if (files_[0].Write(stream_0_offset, stream_0_data->data(),
                       entry_stat.data_size(0)) !=
       entry_stat.data_size(0)) {
     RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
     DVLOG(1) << "Could not write stream 0 data.";
     Doom();
   }
 
   for (std::vector<CRCRecord>::const_iterator it = crc32s_to_write->begin();
        it != crc32s_to_write->end(); ++it) {
     const int stream_index = it->index;
     const int file_index = GetFileIndexFromStreamIndex(stream_index);
     if (empty_file_omitted_[file_index])
       continue;
 
     SimpleFileEOF eof_record;
     eof_record.stream_size = entry_stat.data_size(stream_index);
     eof_record.final_magic_number = kSimpleFinalMagicNumber;
     eof_record.flags = 0;
     if (it->has_crc32)
       eof_record.flags |= SimpleFileEOF::FLAG_HAS_CRC32;
     eof_record.data_crc32 = it->data_crc32;
-    int eof_offset = entry_stat.GetEOFOffsetInFile(key_, stream_index);
+    int eof_offset = entry_stat.GetEOFOffsetInFile(key_.size(), stream_index);
     // If stream 0 changed size, the file needs to be resized, otherwise the
     // next open will yield wrong stream sizes. On stream 1 and stream 2 proper
     // resizing of the file is handled in SimpleSynchronousEntry::WriteData().
     if (stream_index == 0 &&
         !files_[file_index].SetLength(eof_offset)) {
       RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
       DVLOG(1) << "Could not truncate stream 0 file.";
       Doom();
       break;
     }
     if (files_[file_index].Write(eof_offset,
                                  reinterpret_cast<const char*>(&eof_record),
                                  sizeof(eof_record)) !=
         sizeof(eof_record)) {
       RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
       DVLOG(1) << "Could not write eof record.";
       Doom();
       break;
     }
   }
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     if (empty_file_omitted_[i])
       continue;
 
     files_[i].Close();
-    const int64_t file_size = entry_stat.GetFileSize(key_, i);
+    const int64_t file_size = entry_stat.GetFileSize(key_.size(), i);
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "LastClusterSize", cache_type_,
                      file_size % 4096, 0, 4097, 50);
     const int64_t cluster_loss = file_size % 4096 ? 4096 - file_size % 4096 : 0;
     SIMPLE_CACHE_UMA(PERCENTAGE,
                      "LastClusterLossPercent", cache_type_,
                      static_cast<base::HistogramBase::Sample>(
                          cluster_loss * 100 / (cluster_loss + file_size)));
   }
 
   if (sparse_file_open())
     sparse_file_.Close();
 
   if (files_created_) {
     const int stream2_file_index = GetFileIndexFromStreamIndex(2);
     SIMPLE_CACHE_UMA(BOOLEAN, "EntryCreatedAndStream2Omitted", cache_type_,
                      empty_file_omitted_[stream2_file_index]);
   }
   RecordCloseResult(cache_type_, CLOSE_RESULT_SUCCESS);
   have_open_files_ = false;
   delete this;
 }
 
 SimpleSynchronousEntry::SimpleSynchronousEntry(net::CacheType cache_type,
                                                const FilePath& path,
                                                const std::string& key,
-                                               const uint64_t entry_hash)
+                                               const uint64_t entry_hash,
+                                               const bool had_index)
     : cache_type_(cache_type),
       path_(path),
       entry_hash_(entry_hash),
+      had_index_(had_index),
       key_(key),
       have_open_files_(false),
-      initialized_(false) {
+      initialized_(false),
+      header_and_key_check_needed_(false) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i)
     empty_file_omitted_[i] = false;
 }
 
 SimpleSynchronousEntry::~SimpleSynchronousEntry() {
   DCHECK(!(have_open_files_ && initialized_));
   if (have_open_files_)
     CloseFiles();
 }
 
@@ skipping 32 matching lines @@
   int flags = File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE |
               File::FLAG_SHARE_DELETE;
   files_[file_index].Initialize(filename, flags);
   *out_error = files_[file_index].error_details();
 
   empty_file_omitted_[file_index] = false;
 
   return files_[file_index].IsValid();
 }
 
-bool SimpleSynchronousEntry::OpenFiles(
-    bool had_index,
-    SimpleEntryStat* out_entry_stat) {
+bool SimpleSynchronousEntry::OpenFiles(SimpleEntryStat* out_entry_stat) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     File::Error error;
     if (!MaybeOpenFile(i, &error)) {
       // TODO(juliatuttle,gavinp): Remove one each of these triplets of
       // histograms. We can calculate the third as the sum or difference of the
       // other two.
-      RecordSyncOpenResult(
-          cache_type_, OPEN_ENTRY_PLATFORM_FILE_ERROR, had_index);
+      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_PLATFORM_FILE_ERROR,
+                           had_index_);
       SIMPLE_CACHE_UMA(ENUMERATION,
                        "SyncOpenPlatformFileError", cache_type_,
                        -error, -base::File::FILE_ERROR_MAX);
-      if (had_index) {
+      if (had_index_) {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncOpenPlatformFileError_WithIndex", cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
       } else {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncOpenPlatformFileError_WithoutIndex",
                          cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
       }
       while (--i >= 0)
@@ skipping 46 matching lines @@
   }
   SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                    "SyncOpenEntryAge", cache_type_,
                    entry_age.InHours(), 1, 1000, 50);
 
   files_created_ = false;
 
   return true;
 }
 
-bool SimpleSynchronousEntry::CreateFiles(
-    bool had_index,
-    SimpleEntryStat* out_entry_stat) {
+bool SimpleSynchronousEntry::CreateFiles(SimpleEntryStat* out_entry_stat) {
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     File::Error error;
     if (!MaybeCreateFile(i, FILE_NOT_REQUIRED, &error)) {
       // TODO(juliatuttle,gavinp): Remove one each of these triplets of
       // histograms. We can calculate the third as the sum or difference of the
       // other two.
-      RecordSyncCreateResult(CREATE_ENTRY_PLATFORM_FILE_ERROR, had_index);
+      RecordSyncCreateResult(CREATE_ENTRY_PLATFORM_FILE_ERROR, had_index_);
       SIMPLE_CACHE_UMA(ENUMERATION,
                        "SyncCreatePlatformFileError", cache_type_,
                        -error, -base::File::FILE_ERROR_MAX);
-      if (had_index) {
+      if (had_index_) {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncCreatePlatformFileError_WithIndex", cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
       } else {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "SyncCreatePlatformFileError_WithoutIndex",
                          cache_type_,
                          -error, -base::File::FILE_ERROR_MAX);
       }
       while (--i >= 0)
@@ skipping 25 matching lines @@
 
   if (sparse_file_open())
     CloseSparseFile();
 }
 
 void SimpleSynchronousEntry::CloseFiles() {
   for (int i = 0; i < kSimpleEntryFileCount; ++i)
     CloseFile(i);
 }
 
+bool SimpleSynchronousEntry::CheckHeaderAndKeyForOpen(int file_index) {

[Randy Smith (Not in Mondays), 2016/05/18 16:41:30]

Given that this function is no longer being called during open, maybe a
different name?  Maybe just deleting "For Open"?

[gavinp, 2016/05/18 19:05:02]

Well, it's still only for open... It's just that open completes later sometimes.
But yes, it doesn't contribute meaning, so I'm removing it.

+  // TODO(gavinp): Frequently we are doing this at the same time as we read from
+  // the beginning of an entry. It might improve performance to make a single
+  // read(2) call rather than two separate reads. On the other hand, it would
+  // mean an extra memory to memory copy.

[Randy Smith (Not in Mondays), 2016/05/17 20:04:07]

Hmmm.  The other issue I'd add to this comment (presuming you agree) is the
waste if kInitialHeaderRead actually reads into the entry; then it's not just
the two separate reads, but the two reads doing the same thing.  I presume that
won't happen often because you'll usually have the key_ and GetHeaderSize() is
mostly reliable?

[gavinp, 2016/05/18 15:46:00]

Yeah, the key is almost always present. And in iteration, it's unlikely that the
entry will subsequently be read. More typically, it's doomed. 

Called it out.

[Randy Smith (Not in Mondays), 2016/05/18 16:41:29]

Ah, good point; I hadn't kept in mind no key -> iteration -> probably just
deleting based on time.  You're welcome to go back to the old comment or not as
you choose.

+  std::vector<char> header_data(key_.empty() ? kInitialHeaderRead
+                                             : GetHeaderSize(key_.size()));
+  int bytes_read =
+      files_[file_index].Read(0, header_data.data(), header_data.size());
+  const SimpleFileHeader* header;
+
+  if (bytes_read == -1 || static_cast<size_t>(bytes_read) < sizeof(*header)) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_HEADER, had_index_);
+    return false;
+  }
+  header_data.resize(bytes_read);
+  header = reinterpret_cast<const SimpleFileHeader*>(header_data.data());

[Randy Smith (Not in Mondays), 2016/05/17 20:04:07]

Maybe comment here about dodging the resize() repositioning chainsaw?

[gavinp, 2016/05/18 15:46:00]

Actually, I checked, and resizes down in size (which this is) does not adjust
capacity(), so there's no iterator invalidation.

+
+  if (header->initial_magic_number != kSimpleInitialMagicNumber) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_MAGIC_NUMBER, had_index_);
+    return false;
+  }
+
+  if (header->version != kSimpleEntryVersionOnDisk) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_VERSION, had_index_);
+    return false;
+  }
+
+  size_t expected_header_size = GetHeaderSize(header->key_length);

[gavinp, 2016/05/16 23:19:28]

I think this needs a unit test; just something with a key size that's large
enough to trigger this read, and confirmation that it can be iterated.

[Randy Smith (Not in Mondays), 2016/05/17 20:04:07]

Agreed.

[gavinp, 2016/05/18 15:46:00]

Done.

+  if (header_data.size() < expected_header_size) {
+    size_t old_size = header_data.size();
+    header_data.resize(expected_header_size);
+    int bytes_read =
+        files_[file_index].Read(old_size, header_data.data() + old_size,
+                                expected_header_size - old_size);
+    if (bytes_read == -1)
+      header_data.resize(old_size);

[Randy Smith (Not in Mondays), 2016/05/17 20:04:07]

Call out that this is an error path?

[gavinp, 2016/05/18 15:46:00]

Done.

+    else
+      header_data.resize(old_size + bytes_read);
+    // The resize calls invalidated |*header|, so it must be reset.

[Randy Smith (Not in Mondays), 2016/05/18 16:41:30]

Why delete the comment?  This was an expansion, so it may invalidate iterators.

[gavinp, 2016/05/18 19:05:02]

This was not an expansion, because of the resize a few lines above. It's gone
now anyway, so moot.

+    header = reinterpret_cast<const SimpleFileHeader*>(header_data.data());
+  }
+
+  if (header_data.size() < sizeof(*header) + header->key_length) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_KEY, had_index_);
+    return false;
+  }
+  char* key_data = header_data.data() + sizeof(*header);
+  if (base::Hash(key_data, header->key_length) != header->key_hash) {
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_KEY_HASH_MISMATCH, had_index_);
+    return false;
+  }
+
+  std::string key_from_header(key_data, header->key_length);
+  if (key_.empty()) {
+    key_.swap(key_from_header);
+  } else {
+    if (key_ != key_from_header) {
+      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_KEY_MISMATCH, had_index_);
+      return false;
+    }
+  }
+
+  header_and_key_check_needed_ = false;
+  return true;
+}
+
 int SimpleSynchronousEntry::InitializeForOpen(
-    bool had_index,
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
     uint32_t* out_stream_0_crc32) {
   DCHECK(!initialized_);
-  if (!OpenFiles(had_index, out_entry_stat)) {
+  if (!OpenFiles(out_entry_stat)) {
     DLOG(WARNING) << "Could not open platform files for entry.";
     return net::ERR_FAILED;
   }
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     if (empty_file_omitted_[i])
       continue;
 
-    SimpleFileHeader header;
-    int header_read_result =
-        files_[i].Read(0, reinterpret_cast<char*>(&header), sizeof(header));
-    if (header_read_result != sizeof(header)) {
-      DLOG(WARNING) << "Cannot read header from entry.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_HEADER, had_index);
-      return net::ERR_FAILED;
+    if (!key_.empty() &&
+        base::CommandLine::ForCurrentProcess()->HasSwitch(
+            "simple-cache-dont-read-initial-header")) {

[gavinp, 2016/05/16 23:19:28]

Oh, yes, this must be removed for landing. I was using this for testing the
impact of this change using the disk cache perftool.

[Randy Smith (Not in Mondays), 2016/05/17 20:04:07]

Acknowledged.

[Randy Smith (Not in Mondays), 2016/05/18 16:41:29]

I'd feel better about it being gone in the PS I stamp?  What's blocking its
removal?

[gavinp, 2016/05/18 19:05:02]

Nothing, done. It was just convenient for me to use this CL to move it around
platform to platform. But we're far enough along now that removing it is right.

+      header_and_key_check_needed_ = true;
+    } else {
+      if (!CheckHeaderAndKeyForOpen(i))
+        return net::ERR_FAILED;
     }
 
-    if (header.initial_magic_number != kSimpleInitialMagicNumber) {
-      // TODO(gavinp): This seems very bad; for now we log at WARNING, but we
-      // should give consideration to not saturating the log with these if that
-      // becomes a problem.
-      DLOG(WARNING) << "Magic number did not match.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_MAGIC_NUMBER, had_index);
-      return net::ERR_FAILED;
-    }
-
-    if (header.version != kSimpleEntryVersionOnDisk) {
-      DLOG(WARNING) << "Unreadable version.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_BAD_VERSION, had_index);
-      return net::ERR_FAILED;
-    }
-
-    std::unique_ptr<char[]> key(new char[header.key_length]);
-    int key_read_result = files_[i].Read(sizeof(header), key.get(),
-                                         header.key_length);
-    if (key_read_result != base::checked_cast<int>(header.key_length)) {
-      DLOG(WARNING) << "Cannot read key from entry.";
-      RecordSyncOpenResult(cache_type_, OPEN_ENTRY_CANT_READ_KEY, had_index);
-      return net::ERR_FAILED;
-    }
-
-    key_ = std::string(key.get(), header.key_length);
     if (i == 0) {
       // File size for stream 0 has been stored temporarily in data_size[1].
       int total_data_size =
-          GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size(1));
+          GetDataSizeFromFileSize(key_.size(), out_entry_stat->data_size(1));
       int ret_value_stream_0 = ReadAndValidateStream0(
           total_data_size, out_entry_stat, stream_0_data, out_stream_0_crc32);
       if (ret_value_stream_0 != net::OK)
         return ret_value_stream_0;
     } else {
       out_entry_stat->set_data_size(
-          2, GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size(2)));
+          2,
+          GetDataSizeFromFileSize(key_.size(), out_entry_stat->data_size(2)));
       if (out_entry_stat->data_size(2) < 0) {
         DLOG(WARNING) << "Stream 2 file is too small.";
         return net::ERR_FAILED;
       }
     }
-
-    if (base::Hash(key.get(), header.key_length) != header.key_hash) {
-      DLOG(WARNING) << "Hash mismatch on key.";
-      RecordSyncOpenResult(
-          cache_type_, OPEN_ENTRY_KEY_HASH_MISMATCH, had_index);
-      return net::ERR_FAILED;
-    }
   }
 
   int32_t sparse_data_size = 0;
   if (!OpenSparseFileIfExists(&sparse_data_size)) {
-    RecordSyncOpenResult(
-        cache_type_, OPEN_ENTRY_SPARSE_OPEN_FAILED, had_index);
+    RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SPARSE_OPEN_FAILED,
+                         had_index_);
     return net::ERR_FAILED;
   }
   out_entry_stat->set_sparse_data_size(sparse_data_size);
 
   bool removed_stream2 = false;
   const int stream2_file_index = GetFileIndexFromStreamIndex(2);
   DCHECK(CanOmitEmptyFile(stream2_file_index));
   if (!empty_file_omitted_[stream2_file_index] &&
       out_entry_stat->data_size(2) == 0) {
     DVLOG(1) << "Removing empty stream 2 file.";
     CloseFile(stream2_file_index);
     DeleteFileForEntryHash(path_, entry_hash_, stream2_file_index);
     empty_file_omitted_[stream2_file_index] = true;
     removed_stream2 = true;
   }
 
   SIMPLE_CACHE_UMA(BOOLEAN, "EntryOpenedAndStream2Removed", cache_type_,
                    removed_stream2);
 
-  RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index);
+  RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index_);
   initialized_ = true;
   return net::OK;
 }
 
 bool SimpleSynchronousEntry::InitializeCreatedFile(
     int file_index,
     CreateEntryResult* out_result) {
   SimpleFileHeader header;
   header.initial_magic_number = kSimpleInitialMagicNumber;
   header.version = kSimpleEntryVersionOnDisk;
@@ skipping 12 matching lines @@
                                            key_.size());
   if (bytes_written != base::checked_cast<int>(key_.size())) {
     *out_result = CREATE_ENTRY_CANT_WRITE_KEY;
     return false;
   }
 
   return true;
 }
 
 int SimpleSynchronousEntry::InitializeForCreate(
-    bool had_index,
     SimpleEntryStat* out_entry_stat) {
   DCHECK(!initialized_);
-  if (!CreateFiles(had_index, out_entry_stat)) {
+  if (!CreateFiles(out_entry_stat)) {
     DLOG(WARNING) << "Could not create platform files.";
     return net::ERR_FILE_EXISTS;
   }
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     if (empty_file_omitted_[i])
       continue;
 
     CreateEntryResult result;
     if (!InitializeCreatedFile(i, &result)) {
-      RecordSyncCreateResult(result, had_index);
+      RecordSyncCreateResult(result, had_index_);
       return net::ERR_FAILED;
     }
   }
-  RecordSyncCreateResult(CREATE_ENTRY_SUCCESS, had_index);
+  RecordSyncCreateResult(CREATE_ENTRY_SUCCESS, had_index_);
   initialized_ = true;
   return net::OK;
 }
 
 int SimpleSynchronousEntry::ReadAndValidateStream0(
     int total_data_size,
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
     uint32_t* out_stream_0_crc32) const {
   // Temporarily assign all the data size to stream 1 in order to read the
@@ skipping 13 matching lines @@
     return net::ERR_FAILED;
 
   // These are the real values of data size.
   out_entry_stat->set_data_size(0, stream_0_size);
   out_entry_stat->set_data_size(
       1, out_entry_stat->data_size(1) - stream_0_size);
 
   // Put stream 0 data in memory.
   *stream_0_data = new net::GrowableIOBuffer();
   (*stream_0_data)->SetCapacity(stream_0_size);
-  int file_offset = out_entry_stat->GetOffsetInFile(key_, 0, 0);
+  int file_offset = out_entry_stat->GetOffsetInFile(key_.size(), 0, 0);
   File* file = const_cast<File*>(&files_[0]);
   int bytes_read =
       file->Read(file_offset, (*stream_0_data)->data(), stream_0_size);
   if (bytes_read != stream_0_size)
     return net::ERR_FAILED;
 
   // Check the CRC32.
   uint32_t expected_crc32 =
       stream_0_size == 0
           ? crc32(0, Z_NULL, 0)
           : crc32(crc32(0, Z_NULL, 0),
                   reinterpret_cast<const Bytef*>((*stream_0_data)->data()),
                   stream_0_size);
   if (has_crc32 && read_crc32 != expected_crc32) {
     DVLOG(1) << "EOF record had bad crc.";
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
     return net::ERR_FAILED;
   }
   *out_stream_0_crc32 = expected_crc32;
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
   return net::OK;
 }
 
 int SimpleSynchronousEntry::GetEOFRecordData(int index,
                                              const SimpleEntryStat& entry_stat,
                                              bool* out_has_crc32,
                                              uint32_t* out_crc32,
                                              int* out_data_size) const {
   SimpleFileEOF eof_record;
-  int file_offset = entry_stat.GetEOFOffsetInFile(key_, index);
+  int file_offset = entry_stat.GetEOFOffsetInFile(key_.size(), index);
   int file_index = GetFileIndexFromStreamIndex(index);
   File* file = const_cast<File*>(&files_[file_index]);
   if (file->Read(file_offset, reinterpret_cast<char*>(&eof_record),
                  sizeof(eof_record)) !=
       sizeof(eof_record)) {
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
     return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
   }
 
   if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
@@ skipping 324 matching lines @@
   range.offset = offset;
   range.length = len;
   range.data_crc32 = data_crc32;
   range.file_offset = data_file_offset;
   sparse_ranges_.insert(std::make_pair(offset, range));
 
   return true;
 }
 
 }  // namespace disk_cache