SimpleCache: read small files all at once.

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/compiler_specific.h"
 #include "base/files/file_util.h"
 #include "base/hash.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/sha1.h"
+#include "base/strings/string_piece.h"
 #include "base/timer/elapsed_timer.h"
 #include "crypto/secure_hash.h"
 #include "net/base/hash_value.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_backend_version.h"
+#include "net/disk_cache/simple/simple_experiment.h"
 #include "net/disk_cache/simple/simple_histogram_macros.h"
 #include "net/disk_cache/simple/simple_util.h"
 #include "third_party/zlib/zlib.h"
 
 using base::File;
 using base::FilePath;
 using base::Time;
 
 namespace {
 
@@ skipping 76 matching lines @@
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncCloseResult", cache_type, result, CLOSE_RESULT_MAX);
 }
 
 void RecordKeySHA256Result(net::CacheType cache_type, KeySHA256Result result) {
   SIMPLE_CACHE_UMA(ENUMERATION, "SyncKeySHA256Result", cache_type,
                    static_cast<int>(result),
                    static_cast<int>(KeySHA256Result::MAX));
 }
 
+void RecordWhetherOpenDidPrefetch(net::CacheType cache_type, bool result) {
+  SIMPLE_CACHE_UMA(BOOLEAN, "SyncOpenDidPrefetch", cache_type, result);
+}
+
 bool CanOmitEmptyFile(int file_index) {
   DCHECK_GE(file_index, 0);
   DCHECK_LT(file_index, disk_cache::kSimpleEntryFileCount);
   return file_index == disk_cache::simple_util::GetFileIndexFromStreamIndex(2);
 }
 
 bool TruncatePath(const FilePath& filename_to_truncate) {
   File file_to_truncate;
   int flags = File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE |
               File::FLAG_SHARE_DELETE;
@@ skipping 18 matching lines @@
 namespace disk_cache {
 
 using simple_util::GetEntryHashKey;
 using simple_util::GetFilenameFromEntryHashAndFileIndex;
 using simple_util::GetSparseFilenameFromEntryHash;
 using simple_util::GetHeaderSize;
 using simple_util::GetDataSizeFromFileSize;
 using simple_util::GetFileSizeFromDataSize;
 using simple_util::GetFileIndexFromStreamIndex;
 
+const base::Feature kSimpleCachePrefetchExperiment = {
+    "GetSimpleCachePrefetchExperiment", base::FEATURE_DISABLED_BY_DEFAULT};
+const char kSimplePrefetchBytesParam[] = "Bytes";
+
+int GetSimpleCachePrefetchSize() {
+  return base::GetFieldTrialParamByFeatureAsInt(kSimpleCachePrefetchExperiment,
+                                                kSimplePrefetchBytesParam, 0);
+}
+
 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_));
 }
 
@@ skipping 32 matching lines @@
   int32_t total_data_size;
   if (file_index == 0) {
     total_data_size = data_size_[0] + data_size_[1] +
                       sizeof(net::SHA256HashValue) + sizeof(SimpleFileEOF);
   } else {
     total_data_size = data_size_[2];
   }
   return GetFileSizeFromDataSize(key_length, total_data_size);
 }
 
+SimpleStreamPrefetchData::SimpleStreamPrefetchData()
+    : stream_crc32(crc32(0, Z_NULL, 0)) {}
+
+SimpleStreamPrefetchData::~SimpleStreamPrefetchData() {}
+
 SimpleEntryCreationResults::SimpleEntryCreationResults(
     SimpleEntryStat entry_stat)
-    : sync_entry(NULL),
-      entry_stat(entry_stat),
-      stream_0_crc32(crc32(0, Z_NULL, 0)),
-      result(net::OK) {
-}
+    : sync_entry(NULL), entry_stat(entry_stat), result(net::OK) {}
 
 SimpleEntryCreationResults::~SimpleEntryCreationResults() {
 }
 
 SimpleSynchronousEntry::CRCRecord::CRCRecord() : index(-1),
                                                  has_crc32(false),
                                                  data_crc32(0) {
 }
 
 SimpleSynchronousEntry::CRCRecord::CRCRecord(int index_p,
@@ skipping 33 matching lines @@
     const bool had_index,
     const base::TimeTicks& time_enqueued,
     SimpleEntryCreationResults* out_results) {
   base::TimeTicks start_sync_open_entry = base::TimeTicks::Now();
   SIMPLE_CACHE_UMA(TIMES, "QueueLatency.OpenEntry", cache_type,
                    (start_sync_open_entry - time_enqueued));
 
   SimpleSynchronousEntry* sync_entry =
       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);
+      &out_results->entry_stat, out_results->stream_prefetch_data);
   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
     out_results->sync_entry = NULL;
-    out_results->stream_0_data = NULL;
+    out_results->stream_prefetch_data[0].data = nullptr;
+    out_results->stream_prefetch_data[1].data = nullptr;
     return;
   }
   SIMPLE_CACHE_UMA(TIMES, "DiskOpenLatency", cache_type,
                    base::TimeTicks::Now() - start_sync_open_entry);
   out_results->sync_entry = sync_entry;
 }
 
 // static
 void SimpleSynchronousEntry::CreateEntry(
     net::CacheType cache_type,
@@ skipping 394 matching lines @@
          it->second.offset == start + avail_so_far) {
     avail_so_far += it->second.length;
     ++it;
   }
 
   int64_t len_from_start = len - (start - offset);
   *out_start = start;
   *out_result = static_cast<int>(std::min(avail_so_far, len_from_start));
 }
 
-int SimpleSynchronousEntry::CheckEOFRecord(int index,
+int SimpleSynchronousEntry::CheckEOFRecord(int stream_index,
                                            const SimpleEntryStat& entry_stat,
-                                           uint32_t expected_crc32) const {
+                                           uint32_t expected_crc32) {
   DCHECK(initialized_);
-  uint32_t crc32;
-  bool has_crc32;
-  bool has_key_sha256;
-  int32_t stream_size;
-  int rv = GetEOFRecordData(index, entry_stat, &has_crc32, &has_key_sha256,
-                            &crc32, &stream_size);
+  SimpleFileEOF eof_record;
+  int rv = GetEOFRecordData(base::StringPiece(), stream_index, entry_stat,

[pasko, 2017/08/04 01:28:34]

base::StringPiece is one of those rare classes that does not require explicit
constructor, and StringPiece parameters are designed to accept both C++ and C
strings, so a nullptr would work here.

If you agree that nullptr is more readable, then I'd prefer also a comment:
GetEOFRecordData(nullptr /* file_0_prefetch */, ...)

[Maks Orlovich, 2017/08/04 18:35:44]

I prefer the explicit construction since it increases typechecking.
(As you can't pass a StringPiece to something that take a Object*, while you can
with nullptr)

[pasko, 2017/08/09 12:28:21]

OK, makes sense. I am not sure I fully understand the benefits of StringPiece
here compared to char[]. Not having to pass size as additional parameter?

[Maks Orlovich, 2017/08/09 15:25:01]

Yeah, bundles up the pointer and length together, so they are a single parameter
and it's clear they're associated.

+                            &eof_record);
+
   if (rv != net::OK) {
     Doom();
     return rv;
   }
-  if (has_crc32 && crc32 != expected_crc32) {
+  if ((eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) &&
+      eof_record.data_crc32 != expected_crc32) {
     DVLOG(1) << "EOF record had bad crc.";
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
     Doom();
     return net::ERR_CACHE_CHECKSUM_MISMATCH;
   }
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
   return net::OK;
 }
 
+int SimpleSynchronousEntry::PreReadStreamPayload(
+    base::StringPiece file_0_prefetch,
+    int stream_index,
+    int extra_size,
+    const SimpleEntryStat& entry_stat,
+    const SimpleFileEOF& eof_record,
+    scoped_refptr<net::GrowableIOBuffer>* stream_data,
+    uint32_t* out_crc32) {
+  DCHECK(stream_index == 0 || stream_index == 1);
+
+  int stream_size = entry_stat.data_size(stream_index);
+  int read_size = stream_size + extra_size;
+  *stream_data = new net::GrowableIOBuffer();
+  (*stream_data)->SetCapacity(read_size);
+  int file_offset = entry_stat.GetOffsetInFile(key_.size(), 0, stream_index);
+  if (!ReadFromFileOrPrefetched(file_0_prefetch, 0, file_offset, read_size,
+                                (*stream_data)->data()))
+    return net::ERR_FAILED;
+
+  // Check the CRC32.
+  uint32_t expected_crc32 =
+      simple_util::Crc32((*stream_data)->data(), stream_size);
+  if ((eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) &&
+      eof_record.data_crc32 != expected_crc32) {
+    DVLOG(1) << "EOF record had bad crc.";
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
+    return net::ERR_CACHE_CHECKSUM_MISMATCH;
+  }
+  *out_crc32 = expected_crc32;
+  return net::OK;
+}
+
 void SimpleSynchronousEntry::Close(
     const SimpleEntryStat& entry_stat,
     std::unique_ptr<std::vector<CRCRecord>> crc32s_to_write,
     net::GrowableIOBuffer* stream_0_data) {
   base::ElapsedTimer close_time;
   DCHECK(stream_0_data);
 
   for (std::vector<CRCRecord>::const_iterator it = crc32s_to_write->begin();
        it != crc32s_to_write->end(); ++it) {
     const int stream_index = it->index;
@@ skipping 217 matching lines @@
     // 2) Stream 0 and stream 1 are in the same file, and the exact size for
     // each will only be known when reading the EOF record for stream 0.
     //
     // The size for file 0 and 1 is temporarily kept in
     // |data_size(1)| and |data_size(2)| respectively. Reading the key in
     // InitializeForOpen yields the data size for each file. In the case of
     // file hash_1, this is the total size of stream 2, and is assigned to
     // data_size(2). In the case of file 0, it is the combined size of stream
     // 0, stream 1 and one EOF record. The exact distribution of sizes between
     // stream 1 and stream 0 is only determined after reading the EOF record
-    // for stream 0 in ReadAndValidateStream0.
+    // for stream 0 in ReadAndValidateStream0AndMaybe1.
     if (!base::IsValueInRangeForNumericType<int>(file_info.size)) {
       RecordSyncOpenResult(cache_type_, OPEN_ENTRY_INVALID_FILE_LENGTH,
                            had_index_);
       return false;
     }
     out_entry_stat->set_data_size(i + 1, static_cast<int>(file_info.size));
   }
   SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                    "SyncOpenEntryAge", cache_type_,
                    entry_age.InHours(), 1, 1000, 50);
@@ skipping 123 matching lines @@
       return false;
     }
   }
 
   header_and_key_check_needed_[file_index] = false;
   return true;
 }
 
 int SimpleSynchronousEntry::InitializeForOpen(
     SimpleEntryStat* out_entry_stat,
-    scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
-    uint32_t* out_stream_0_crc32) {
+    SimpleStreamPrefetchData* stream_prefetch_data) {
   DCHECK(!initialized_);
   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;
 
     if (key_.empty()) {
       // If |key_| is empty, we were opened via the iterator interface, without
       // knowing what our key is. We must therefore read the header immediately
       // to discover it, so SimpleEntryImpl can make it available to
       // disk_cache::Entry::GetKey().
       if (!CheckHeaderAndKey(i))
         return net::ERR_FAILED;
     } else {
       // If we do know which key were are looking for, we still need to
       // check that the file actually has it (rather than just being a hash
       // collision or some sort of file system accident), but that can be put
       // off until opportune time: either the read of the footer, or when we
       // start reading in the data, depending on stream # and format revision.
       header_and_key_check_needed_[i] = true;
     }
 
     if (i == 0) {
       // File size for stream 0 has been stored temporarily in data_size[1].
-      int ret_value_stream_0 =
-          ReadAndValidateStream0(out_entry_stat->data_size(1), out_entry_stat,
-                                 stream_0_data, out_stream_0_crc32);
+      int ret_value_stream_0 = ReadAndValidateStream0AndMaybe1(
+          out_entry_stat->data_size(1), out_entry_stat, stream_prefetch_data);
       if (ret_value_stream_0 != net::OK)
         return ret_value_stream_0;
     } else {
       out_entry_stat->set_data_size(
           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;
       }
@@ skipping 70 matching lines @@
     if (!InitializeCreatedFile(i, &result)) {
       RecordSyncCreateResult(result, had_index_);
       return net::ERR_FAILED;
     }
   }
   RecordSyncCreateResult(CREATE_ENTRY_SUCCESS, had_index_);
   initialized_ = true;
   return net::OK;
 }
 
-int SimpleSynchronousEntry::ReadAndValidateStream0(
+int SimpleSynchronousEntry::ReadAndValidateStream0AndMaybe1(
     int file_size,
     SimpleEntryStat* out_entry_stat,
-    scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
-    uint32_t* out_stream_0_crc32) {
+    SimpleStreamPrefetchData* stream_prefetch_data) {
+  // If the file is sufficiently small, we will prefetch everything --
+  // in which case |prefetch_buf| will be non-null, and we should look at it
+  // rather than call ::Read for the bits.
+  std::unique_ptr<char[]> prefetch_buf;
+  base::StringPiece file_0_prefetch;
+
+  if (file_size > GetSimpleCachePrefetchSize()) {
+    RecordWhetherOpenDidPrefetch(cache_type_, false);
+  } else {
+    RecordWhetherOpenDidPrefetch(cache_type_, true);
+    prefetch_buf = base::MakeUnique<char[]>(file_size);
+    if (files_[0].Read(0, prefetch_buf.get(), file_size) != file_size)
+      return net::ERR_FAILED;
+    file_0_prefetch.set(prefetch_buf.get(), file_size);
+  }
+
   // Pretend this file has a null stream zero, and contains the optional key
   // SHA256. This is good enough to read the EOF record on the file, which gives
   // the actual size of stream 0.
   int temp_data_size = GetDataSizeFromFileSize(key_.size(), file_size);
   out_entry_stat->set_data_size(0, 0);
   out_entry_stat->set_data_size(
       1, temp_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF));
 
-  bool has_crc32;
-  bool has_key_sha256;
-  uint32_t read_crc32;
-  int32_t stream_0_size;
-  int ret_value_crc32 =
-      GetEOFRecordData(0, *out_entry_stat, &has_crc32, &has_key_sha256,
-                       &read_crc32, &stream_0_size);
-  if (ret_value_crc32 != net::OK)
-    return ret_value_crc32;
+  SimpleFileEOF stream_0_eof;
+  int rv = GetEOFRecordData(file_0_prefetch, 0, *out_entry_stat, &stream_0_eof);
+  if (rv != net::OK)
+    return rv;
+
+  bool has_key_sha256 =
+      (stream_0_eof.flags & SimpleFileEOF::FLAG_HAS_KEY_SHA256) ==
+      SimpleFileEOF::FLAG_HAS_KEY_SHA256;
+  int32_t stream_0_size = stream_0_eof.stream_size;
 
   // Calculate and set the real values for the two streams.
   int32_t total_size = out_entry_stat->data_size(1);
   if (!has_key_sha256)
     total_size += sizeof(net::SHA256HashValue);
+
+  // Sanity-check: don't want to be allocating a gigantic buffer > file size
+  // just because of a header field.
   if (stream_0_size > total_size)
     return net::ERR_FAILED;
   out_entry_stat->set_data_size(0, stream_0_size);
   out_entry_stat->set_data_size(1, total_size - stream_0_size);
 
-  // Put stream 0 data in memory.
-  *stream_0_data = new net::GrowableIOBuffer();
-  (*stream_0_data)->SetCapacity(stream_0_size + sizeof(net::SHA256HashValue));
-  int file_offset = out_entry_stat->GetOffsetInFile(key_.size(), 0, 0);
-  int read_size = stream_0_size;
+  // Put stream 0 data in memory --- plus maybe the sha256(key) footer.
+  int extra_stream_0_read = 0;
   if (has_key_sha256)
-    read_size += sizeof(net::SHA256HashValue);
-  if (files_[0].Read(file_offset, (*stream_0_data)->data(), read_size) !=
-      read_size)
-    return net::ERR_FAILED;
+    extra_stream_0_read += sizeof(net::SHA256HashValue);
+  rv = PreReadStreamPayload(
+      file_0_prefetch, 0, extra_stream_0_read, *out_entry_stat, stream_0_eof,

[pasko, 2017/08/04 01:28:34]

maybe a few explanations how args correspond to params?

like:

s!extra_stream_0_read!extra_stream_0_size /* extra_size */!

[Maks Orlovich, 2017/08/04 18:35:44]

I went for extra_post_stream_0_read as name instead, to make it clear that it's
not really part of stream data. Commented stream_index, too.

+      &stream_prefetch_data[0].data, &stream_prefetch_data[0].stream_crc32);
+  if (rv != net::OK)
+    return rv;
 
-  // Check the CRC32.
-  uint32_t expected_crc32 =
-      simple_util::Crc32((*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;
+  // If prefetch buffer is available, and we have sha256(key) (so we don't need
+  // to look at the header), extract out stream 1 info as well.
+  if (prefetch_buf && has_key_sha256) {
+    SimpleFileEOF stream_1_eof;
+    rv = GetEOFRecordData(file_0_prefetch, 1, *out_entry_stat, &stream_1_eof);
+    if (rv != net::OK)
+      return rv;
+
+    rv = PreReadStreamPayload(file_0_prefetch, 1, 0, *out_entry_stat,

[pasko, 2017/08/04 01:28:34]

s!0,!0 /* extra_size */,!

[Maks Orlovich, 2017/08/04 18:35:44]

Did a variant of that.

[pasko, 2017/08/09 12:28:21]

Thanks for that, I forgot the style guide

+                              stream_1_eof, &stream_prefetch_data[1].data,
+                              &stream_prefetch_data[1].stream_crc32);
+    if (rv != net::OK)
+      return rv;
   }
-  *out_stream_0_crc32 = expected_crc32;
 
   // If present, check the key SHA256.
   if (has_key_sha256) {
     net::SHA256HashValue hash_value;
     CalculateSHA256OfKey(key_, &hash_value);
     bool matched =
-        std::memcmp(&hash_value, (*stream_0_data)->data() + stream_0_size,
+        std::memcmp(&hash_value,
+                    stream_prefetch_data[0].data->data() + stream_0_size,
                     sizeof(hash_value)) == 0;
     if (!matched) {
       RecordKeySHA256Result(cache_type_, KeySHA256Result::NO_MATCH);
       return net::ERR_FAILED;
     }
     // Elide header check if we verified sha256(key) via footer.
     header_and_key_check_needed_[0] = false;
     RecordKeySHA256Result(cache_type_, KeySHA256Result::MATCHED);
   } else {
     RecordKeySHA256Result(cache_type_, KeySHA256Result::NOT_PRESENT);
   }
 
   // Ensure the key is validated before completion.
   if (!has_key_sha256 && header_and_key_check_needed_[0])
     CheckHeaderAndKey(0);
 
   RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
   return net::OK;
 }
 
-int SimpleSynchronousEntry::GetEOFRecordData(int index,
+bool SimpleSynchronousEntry::ReadFromFileOrPrefetched(
+    base::StringPiece file_0_prefetch,
+    int file_index,
+    int offset,
+    int size,
+    char* dest) {
+  if (file_0_prefetch.empty() || file_index != 0) {
+    return files_[file_index].Read(offset, dest, size) == size;
+  } else {
+    if (offset < 0 || size < 0)
+      return false;
+    if (size == 0)
+      return true;
+
+    base::CheckedNumeric<size_t> start(offset);
+    size_t start_numeric;
+    if (!start.AssignIfValid(&start_numeric) ||
+        start_numeric >= file_0_prefetch.size())
+      return false;
+
+    base::CheckedNumeric<size_t> end = start + size - 1;
+    size_t end_numeric;
+    if (!end.AssignIfValid(&end_numeric) ||
+        end_numeric >= file_0_prefetch.size())
+      return false;
+
+    memcpy(dest, file_0_prefetch.data() + offset, size);
+    return true;
+  }
+}
+
+int SimpleSynchronousEntry::GetEOFRecordData(base::StringPiece file_0_prefetch,
+                                             int stream_index,
                                              const SimpleEntryStat& entry_stat,
-                                             bool* out_has_crc32,
-                                             bool* out_has_key_sha256,
-                                             uint32_t* out_crc32,
-                                             int32_t* out_data_size) const {
-  SimpleFileEOF eof_record;
-  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)) {
+                                             SimpleFileEOF* eof_record) {
+  int file_offset = entry_stat.GetEOFOffsetInFile(key_.size(), stream_index);
+  int file_index = GetFileIndexFromStreamIndex(stream_index);
+
+  bool ok = ReadFromFileOrPrefetched(file_0_prefetch, file_index, file_offset,
+                                     sizeof(SimpleFileEOF),

[pasko, 2017/08/04 01:28:34]

generally it is less error-prone to to use sizeof(variable) instead of
sizeof(type)

[Maks Orlovich, 2017/08/04 18:35:44]

It would have to be sizeof(*variable) in this case, though....?
I am more confident in my ability to know what I am reading --- when it's a
struct, rather than a buffer, anyway --- than to not forget a * in a way the
typechecker can't catch.

+                                     reinterpret_cast<char*>(eof_record));
+  if (!ok) {
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
     return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
   }
 
-  if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
+  if (eof_record->final_magic_number != kSimpleFinalMagicNumber) {
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
     DVLOG(1) << "EOF record had bad magic number.";
     return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
   }
 
-  if (!base::IsValueInRangeForNumericType<int32_t>(eof_record.stream_size))
+  if (!base::IsValueInRangeForNumericType<int32_t>(eof_record->stream_size))
     return net::ERR_FAILED;
-
-  *out_has_crc32 = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
-                   SimpleFileEOF::FLAG_HAS_CRC32;
-  *out_has_key_sha256 =
-      (eof_record.flags & SimpleFileEOF::FLAG_HAS_KEY_SHA256) ==
-      SimpleFileEOF::FLAG_HAS_KEY_SHA256;
-  *out_crc32 = eof_record.data_crc32;
-  *out_data_size = eof_record.stream_size;
-  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, *out_has_crc32);
+  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_,

[pasko, 2017/08/04 01:28:34]

Seems like both prefetch and last ReadData would record this histogram for
stream1... Am I right?

In this case we would have different counts in this histogram in control/
experiment group, which would be not ideal for comparisons.

Did not look at tests yet .. maybe worth some histogram tester work ...

[Maks Orlovich, 2017/08/04 18:35:44]

I think it can't actually happen. To hit sync-side ReadData on stream1, it would
need to have the prefetched stream1 data discarded due to a write, and we
don't/can't verify checksums if a stream has been written to.

[pasko, 2017/08/09 12:28:21]

Even though this might be currently true, I find it easy to overlook in later
changes. Please add a test to verify that SyncCheckEOFHasCrc is recorded only
once for both prefetched and non-prefetched stream1 cases when we read the entry
from the beginning to the end. Hopefully in two variants: when there is CRC and
when there is not.

[Maks Orlovich, 2017/08/09 17:52:29]

Good call on this since while SyncCheckEOFHasCrc was indeed recorded fine,
SyncCheckEOFResult was handled inconsistently.
(Will upload fix + new tests after the histogram thing is done with..)

+                   (eof_record->flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
+                       SimpleFileEOF::FLAG_HAS_CRC32);
   return net::OK;
 }
 
 void SimpleSynchronousEntry::Doom() const {
   DeleteFilesForEntryHash(path_, entry_hash_);
 }
 
 // static
 bool SimpleSynchronousEntry::DeleteFileForEntryHash(const FilePath& path,
                                                     const uint64_t entry_hash,
@@ skipping 299 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