SimpleCache: open with fewer seeks.

net/disk_cache/simple/simple_synchronous_entry.h

Index: net/disk_cache/simple/simple_synchronous_entry.h
diff --git a/net/disk_cache/simple/simple_synchronous_entry.h b/net/disk_cache/simple/simple_synchronous_entry.h
index 629be76e1cfaf0ea79cf30eb7c76a7fabcb8ad54..613acbec4ff51dc916bcd4c3edd4fa50f5378a54 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.h
+++ b/net/disk_cache/simple/simple_synchronous_entry.h
@@ -41,12 +41,10 @@ class NET_EXPORT_PRIVATE SimpleEntryStat {
                   const int32_t data_size[],
                   const int32_t sparse_data_size);
 
-  int GetOffsetInFile(const std::string& key,
-                      int offset,
-                      int stream_index) const;
-  int GetEOFOffsetInFile(const std::string& key, int stream_index) const;
-  int GetLastEOFOffsetInFile(const std::string& key, int file_index) const;
-  int64_t GetFileSize(const std::string& key, int file_index) const;
+  int GetOffsetInFile(size_t key_length, int offset, int stream_index) const;
+  int GetEOFOffsetInFile(size_t key_length, int stream_index) const;
+  int GetLastEOFOffsetInFile(size_t key_length, int file_index) const;
+  int64_t GetFileSize(size_t key_length, int file_index) const;
 
   base::Time last_used() const { return last_used_; }
   base::Time last_modified() const { return last_modified_; }
@@ -114,8 +112,12 @@ class SimpleSynchronousEntry {
     bool doomed;
   };
 
+  // Opens a disk cache entry on disk. The |key| parameter is optional, if empty
+  // the operation may be slower. The |entry_hash| parameter is required.
+  // |had_index| is provided only for histograms.

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

I'm torn here, because for someone reading the code, knowing that the key
parameter will exist except for the iteration case is really really useful, but
it's not appropriate to document at this level.  Maybe document in the CL
description and/or the first location upstack from this call that has knowledge
of the calling API?

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

Done. Documented it at SetKey in SimpleEntryImpl.

   static void OpenEntry(net::CacheType cache_type,
                         const base::FilePath& path,
+                        const std::string& key,
                         uint64_t entry_hash,
                         bool had_index,
                         SimpleEntryCreationResults* out_results);
@@ -150,7 +152,7 @@ class SimpleSynchronousEntry {
                 net::IOBuffer* out_buf,
                 uint32_t* out_crc32,
                 SimpleEntryStat* entry_stat,
-                int* out_result) const;
+                int* out_result);
   void WriteData(const EntryOperationData& in_entry_op,
                  net::IOBuffer* in_buf,
                  SimpleEntryStat* out_entry_stat,
@@ -210,7 +212,8 @@ class SimpleSynchronousEntry {
   SimpleSynchronousEntry(net::CacheType cache_type,
                          const base::FilePath& path,
                          const std::string& key,
-                         uint64_t entry_hash);
+                         uint64_t entry_hash,
+                         bool had_index);
 
   // Like Entry, the SimpleSynchronousEntry self releases when Close() is
   // called.
@@ -227,18 +230,19 @@ class SimpleSynchronousEntry {
   bool MaybeCreateFile(int file_index,
                        FileRequired file_required,
                        base::File::Error* out_error);
-  bool OpenFiles(bool had_index,
-                 SimpleEntryStat* out_entry_stat);
-  bool CreateFiles(bool had_index,
-                   SimpleEntryStat* out_entry_stat);
+  bool OpenFiles(SimpleEntryStat* out_entry_stat);
+  bool CreateFiles(SimpleEntryStat* out_entry_stat);
   void CloseFile(int index);
   void CloseFiles();
 
-  // Returns a net error, i.e. net::OK on success. |had_index| is passed
-  // from the main entry for metrics purposes, and is true if the index was
-  // initialized when the open operation began.
-  int InitializeForOpen(bool had_index,
-                        SimpleEntryStat* out_entry_stat,
+  // Read the header and key at the beginning of the file, and validate that
+  // they are correct. If this entry was opened with a key, the key is checked
+  // for a match. If not, then the |key_| member is set based on the value in
+  // this header. Records histograms if any check is failed.
+  bool CheckHeaderAndKeyForOpen(int file_index);
+
+  // Returns a net error, i.e. net::OK on success.
+  int InitializeForOpen(SimpleEntryStat* out_entry_stat,
                         scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
                         uint32_t* out_stream_0_crc32);
 
@@ -248,10 +252,8 @@ class SimpleSynchronousEntry {
   bool InitializeCreatedFile(int index, CreateEntryResult* out_result);
 
   // Returns a net error, including net::OK on success and net::FILE_EXISTS
-  // when the entry already exists. |had_index| is passed from the main entry
-  // for metrics purposes, and is true if the index was initialized when the
-  // create operation began.
-  int InitializeForCreate(bool had_index, SimpleEntryStat* out_entry_stat);
+  // when the entry already exists.
+  int InitializeForCreate(SimpleEntryStat* out_entry_stat);
 
   // Allocates and fills a buffer with stream 0 data in |stream_0_data|, then
   // checks its crc32.
@@ -320,11 +322,17 @@ class SimpleSynchronousEntry {
   const net::CacheType cache_type_;
   const base::FilePath path_;
   const uint64_t entry_hash_;
+  const bool had_index_;
   std::string key_;
 
   bool have_open_files_;
   bool initialized_;
 
+  // Initially false, this is set to true when an entry is opened and the
+  // headers have not yet been checked. Any subsequent Read of data should
+  // validate them before completing.

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

Suggestion: I'd find this comment more understandable in the context of the code
if it said ".. when an entry's headers are not checked at time of open ...". 
The phrasing above implies to me that they might have been checked at some point
before open, which I don't think is true or makes sense :-}.

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

Done.

+  bool header_and_key_check_needed_;
+
   base::File files_[kSimpleEntryFileCount];
 
   // True if the corresponding stream is empty and therefore no on-disk file