SimpleCache: read small files all at once.

net/disk_cache/simple/simple_synchronous_entry.cc

Index: net/disk_cache/simple/simple_synchronous_entry.cc
diff --git a/net/disk_cache/simple/simple_synchronous_entry.cc b/net/disk_cache/simple/simple_synchronous_entry.cc
index dc27812c71382e95de0430d79a699a080174899d..4457312246fd19d7af79359d136d7d85ab5dabd7 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.cc
+++ b/net/disk_cache/simple/simple_synchronous_entry.cc
@@ -13,6 +13,7 @@
 #include "base/files/file_util.h"
 #include "base/hash.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/sha1.h"
@@ -22,6 +23,7 @@
 #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"
@@ -118,6 +120,10 @@ void RecordKeySHA256Result(net::CacheType cache_type, KeySHA256Result result) {
                    static_cast<int>(KeySHA256Result::MAX));
 }
 
+void RecordSyncOpenPrefetchStatus(net::CacheType cache_type, bool result) {

[pasko, 2017/07/18 13:46:31]

my favorite: nit on naming, because it is always possible to nit on names :)

"Sync" is implied and unnecessary in this context. Are we planning to extend the
status from boolean? If not, then better be explicit:

RecordWhetherDidPrefetchedOnOpen(...)

[Maks Orlovich, 2017/07/21 18:37:33]

Done'ish

+  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);
@@ -269,7 +275,8 @@ void SimpleSynchronousEntry::OpenEntry(
       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->stream_0_crc32, &out_results->stream_1_data,
+      &out_results->stream_1_crc32);

[pasko, 2017/07/18 13:46:31]

too many output arguments some set under non-trivial conditions? Maybe consider
pre-filling a SimpleEntryCreationResults in InitializeForOpen() or make another
struct?

[Maks Orlovich, 2017/07/28 17:27:35]

Ended up grouping into array of structs, also simplifying the CRC code.

   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
@@ -692,19 +699,17 @@ void SimpleSynchronousEntry::GetAvailableRange(
 
 int SimpleSynchronousEntry::CheckEOFRecord(int 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(nullptr, 0, index, entry_stat, &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();
@@ -714,6 +719,41 @@ int SimpleSynchronousEntry::CheckEOFRecord(int index,
   return net::OK;
 }
 
+int SimpleSynchronousEntry::PreReadStreamPayload(

[pasko, 2017/07/18 13:46:31]

Why Pre? It just reads, no?

[morlovich, 2017/07/18 14:32:31]

Because it's only used in the prefetch path?

[pasko, 2017/07/19 16:28:26]

ah, ok

[Maks Orlovich, 2017/07/21 18:37:34]

Acknowledged.

+    char* prefetch_buf,
+    int file_size,
+    int stream_index,
+    int extra_size,
+    const SimpleEntryStat& entry_stat,
+    const SimpleFileEOF& eof_record,
+    scoped_refptr<net::GrowableIOBuffer>* stream_data,
+    uint32_t* out_crc32) {
+  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(prefetch_buf, file_size, file_offset, read_size,
+                                (*stream_data)->data()))
+    return net::ERR_FAILED;
+
+  // Check the CRC32.
+  uint32_t expected_crc32 =
+      stream_size == 0
+          ? crc32(0, Z_NULL, 0)
+          : crc32(crc32(0, Z_NULL, 0),
+                  reinterpret_cast<const Bytef*>((*stream_data)->data()),

[pasko, 2017/07/19 16:28:26]

In case you have similar allergies to reinterpret_cast repetitions, perhaps
worth a function in simple_util to get a crc32 of a void* and size?

[Maks Orlovich, 2017/07/25 16:06:29]

Split out into https://chromium-review.googlesource.com/581803

+                  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);

[pasko, 2017/07/19 16:28:26]

just realized that this might slightly increase the crc mismatch rate because we
would newly-fail for prefetched stream1 that we would have otherwise discarded.

Sounds not very important (because mismatches are rare), but wanted to highlight
to be less worried later if we get alerts about that.

[Maks Orlovich, 2017/07/21 18:37:34]

Acknowledged.

+    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,
@@ -1096,7 +1136,9 @@ bool SimpleSynchronousEntry::CheckHeaderAndKey(int file_index) {
 int SimpleSynchronousEntry::InitializeForOpen(
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
-    uint32_t* out_stream_0_crc32) {
+    uint32_t* out_stream_0_crc32,
+    scoped_refptr<net::GrowableIOBuffer>* stream_1_data,
+    uint32_t* out_stream_1_crc32) {
   DCHECK(!initialized_);
   if (!OpenFiles(out_entry_stat)) {
     DLOG(WARNING) << "Could not open platform files for entry.";
@@ -1124,9 +1166,9 @@ int SimpleSynchronousEntry::InitializeForOpen(
 
     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 = ReadAndValidateStream0(
+          out_entry_stat->data_size(1), out_entry_stat, stream_0_data,
+          out_stream_0_crc32, stream_1_data, out_stream_1_crc32);
       if (ret_value_stream_0 != net::OK)
         return ret_value_stream_0;
     } else {
@@ -1221,7 +1263,23 @@ int SimpleSynchronousEntry::ReadAndValidateStream0(
     int file_size,
     SimpleEntryStat* out_entry_stat,
     scoped_refptr<net::GrowableIOBuffer>* stream_0_data,
-    uint32_t* out_stream_0_crc32) {
+    uint32_t* out_stream_0_crc32,
+    scoped_refptr<net::GrowableIOBuffer>* stream_1_data,
+    uint32_t* out_stream_1_crc32) {
+  // 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;
+
+  if (file_size <= GetSimpleCachePrefetchSize()) {
+    RecordSyncOpenPrefetchStatus(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;
+  } else {
+    RecordSyncOpenPrefetchStatus(cache_type_, false);

[pasko, 2017/07/18 13:46:31]

nit: it is usually easier to read if the short branch comes first

[Maks Orlovich, 2017/07/21 18:37:33]

[Citation needed], but done.

[pasko, 2017/07/24 12:22:45]

unfortunately, could not find a good one, and now hesitating as a result, on
such a basic thing, wow. I could argue that this looks analogous to the "early
return" argument [1], where I generally prefer early returns, but there is a
point that "more common things" should go first. Certainly this helps
understanding in some cases. I just read this code as all edge cases everywhere
disregarding what is common on purpose, so probably it's just me. Sorry for the
noise.

[1]
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NO2CUzFVmUA

+  }
+
   // 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.
@@ -1230,49 +1288,56 @@ int SimpleSynchronousEntry::ReadAndValidateStream0(
   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(prefetch_buf.get(), file_size, 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(prefetch_buf.get(), file_size, 0,
+                            extra_stream_0_read, *out_entry_stat, stream_0_eof,
+                            stream_0_data, out_stream_0_crc32);
+  if (rv != net::OK)
+    return rv;
 
-  // 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;
+  // 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(prefetch_buf.get(), file_size, 1, *out_entry_stat,
+                          &stream_1_eof);
+    if (rv != net::OK)
+      return rv;
+
+    rv = PreReadStreamPayload(prefetch_buf.get(), file_size, 1, 0,
+                              *out_entry_stat, stream_1_eof, stream_1_data,
+                              out_stream_1_crc32);
+    if (rv != net::OK)
+      return rv;
+  } else {
+    *stream_1_data = nullptr;
   }
-  *out_stream_0_crc32 = expected_crc32;
 
   // If present, check the key SHA256.
   if (has_key_sha256) {
@@ -1300,40 +1365,66 @@ int SimpleSynchronousEntry::ReadAndValidateStream0(
   return net::OK;
 }
 
-int SimpleSynchronousEntry::GetEOFRecordData(int index,
+bool SimpleSynchronousEntry::ReadFromFileOrPrefetched(char* prefetch_buf,
+                                                      int file_size,
+                                                      int offset,
+                                                      int size,
+                                                      char* dest) {
+  if (!prefetch_buf) {
+    return files_[0].Read(offset, dest, size) == size;
+  } else {
+    if (offset < 0 || size < 0)
+      return false;
+    base::CheckedNumeric<int> end =
+        base::CheckedNumeric<int>(offset) + size - 1;
+    if (!end.IsValid())
+      return false;
+    if (offset >= file_size || end.ValueOrDie() >= file_size)

[pasko, 2017/07/18 13:46:31]

feel free to insert a DCHECK for the overflow, but not a CHECK!

If reading some corrupt data can lead to an overflow here, then we must handle
it. An explanation for why it can/cannot happen would be handy because this
function can later be used for something else.

[morlovich, 2017/07/18 14:32:31]

See the conditional in line 1380, this can't actually fail at this point. I
guess it would be better to use AssignIfValid, though?

[pasko, 2017/07/19 16:28:26]

Ah, missed that. AssignIfValid() looks lengthy, I'd prefer ValueOrDefault(), but
ValueOrDie() would also work for me.

[Maks Orlovich, 2017/07/21 18:37:33]

Ended up with AssignIfValid anyway, but I think it works better with how the
code evolved..

+      return false;
+
+    memcpy(dest, prefetch_buf + offset, size);
+    return true;
+  }
+}
+
+int SimpleSynchronousEntry::GetEOFRecordData(char* prefetch_buf,
+                                             int file_size,
+                                             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 {

[pasko, 2017/07/18 13:46:31]

Following how the state migrates from disk to the buffer and across EOF checks
is becoming difficult to understand. It was non-trivial before the change, but
now it hits near my thresholds :)

For example: ReadAndValidateStream0 sets the buffer and then sits on top  of
PreRead to get it back from the buffer. Another example: GetEOFRecordData hides
reading something else under the scenes. These things make it non-trivial to
guess how the functionality is layered, and the file has 1750 lines.

This makes me wonder whether we can redesign it a little. How about wrapping
base::File and hiding from the sync entry the fact that it has a buffer cached?
This would make the sync entry virtually unchanged (modulo clearing the buffer
sometimes, copying from it into the SimpleEntryImpl, histograms will need to
know cache type), all the EOF checking stays the same.

Will this lead too over-copying? Any other problems?

[morlovich, 2017/07/18 14:32:31]

So my idea was that ReadFromFileOrPrefetched would abstract whether reading from
file or prefetch, though not fully since you still need to  drag the two extra
arguments around (might makes sense to bundle them up, maybe even as a
StringPiece). GetEOFRecordData is kinda where it shows extra roughness since
that's also used outside initial read and for files_[1].  Maybe a smaller change
would be to make ReadFromFileOrPrefetch take file number, so that handles of the
wrinkle here?

I need to sleep on the wrapping-File idea, but what bother me about it a little
is that files_ is permanent state, while the intent with the prefetch buffer was
to just to have it for the duration of initial read. 

Also you one probably still want to do some of the method additions I did, like
creating PreReadStreamPayload, since otherwise a whole bunch of stuff gets
dupllicated.

[pasko, 2017/07/19 16:28:26]

Yeah, I understand your way to put ReadFromFileOrPrefetched under everything. It
is just not easy to figure from sync_entry.h, and the name is not very
memorable, so in a month from now I'd need to rediscover it again. Tossing file
numbers vs streams are actually also confusing.

Thinking about those files: could make a something like a CacheStreams object
that abstracts away file manipulation as much as possible. The sync entry
operates on top of streams 0, 1 and 2. What CacheStreams would do:

* mapping of stream to file
* opening/closing streams and associated laziness
* pull/push the key
* it does _not_ know about EOF records and crc32/sha256, those are just part of
streams
* has this readahead optimization that allows to memcpy later instead of a fair
read
* this is where the abstraction leaks: allows the client to 'drop caches' to
save memory (the streams object drops it under the scenes when encountering a
write/truncate)

this makes it sort of a logical lower layer, which will allow not carrying the
prefetch_buf around and removes the usual file/offset calculation/passing
clutter in all codepaths. We still have the problem of verifying all CRC32 and
dealing with variants of SHA256 in the SSE.

Would it help?

-  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);
+
+  // Should only have prefetch_buf when reading things in file 0.
+  DCHECK(file_index == 0 || !prefetch_buf);
+  bool ok;
+  if (prefetch_buf)
+    ok = ReadFromFileOrPrefetched(prefetch_buf, file_size, file_offset,
+                                  sizeof(SimpleFileEOF),
+                                  reinterpret_cast<char*>(eof_record));
+  else
+    // Need to support files_[1], so can't use ReadFromFileOrPrefetched
+    // in this case.
+    ok = files_[file_index].Read(
+             file_offset, reinterpret_cast<char*>(eof_record),
+             sizeof(SimpleFileEOF)) == sizeof(SimpleFileEOF);
+
+  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_,
+                   (eof_record->flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
+                       SimpleFileEOF::FLAG_HAS_CRC32);
   return net::OK;
 }