SimpleCache: merge the first and second stream in one file

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 a1ff1ac6adaab1664d6a61656ed1a1af93a0d308..d7c13718c3d109d4145135d303a73e51aaa59d71 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.cc
+++ b/net/disk_cache/simple/simple_synchronous_entry.cc
@@ -146,7 +146,9 @@ using simple_util::GetEntryHashKey;
 using simple_util::GetFilenameFromEntryHashAndIndex;
 using simple_util::GetDataSizeFromKeyAndFileSize;
 using simple_util::GetFileSizeFromKeyAndDataSize;
-using simple_util::GetFileOffsetFromKeyAndDataOffset;
+using simple_util::GetFileIndexFromStreamIndex;
+using simple_util::GetMaximumDataOffset;
+using simple_util::GetFileOffsetFromDataOffset;
 
 SimpleEntryStat::SimpleEntryStat() {}
 
@@ -206,11 +208,12 @@ void SimpleSynchronousEntry::OpenEntry(
   SimpleSynchronousEntry* sync_entry =
       new SimpleSynchronousEntry(cache_type, path, "", entry_hash);
   out_results->result = sync_entry->InitializeForOpen(
-      had_index, &out_results->entry_stat);
+      had_index, &out_results->entry_stat, &out_results->stream_0_data);
   if (out_results->result != net::OK) {
     sync_entry->Doom();
     delete sync_entry;
     out_results->sync_entry = NULL;
+    out_results->stream_0_data = NULL;
     return;
   }
   out_results->sync_entry = sync_entry;
@@ -279,17 +282,19 @@ int SimpleSynchronousEntry::DoomEntrySet(
 void SimpleSynchronousEntry::ReadData(const EntryOperationData& in_entry_op,
                                       net::IOBuffer* out_buf,
                                       uint32* out_crc32,
-                                      base::Time* out_last_used,
+                                      SimpleEntryStat* out_entry_stat,
                                       int* out_result) const {
   DCHECK(initialized_);
-  int64 file_offset =
-      GetFileOffsetFromKeyAndDataOffset(key_, in_entry_op.offset);
-  int bytes_read = ReadPlatformFile(files_[in_entry_op.index],
-                                    file_offset,
-                                    out_buf->data(),
-                                    in_entry_op.buf_len);
+  int64 file_offset = GetFileOffsetFromDataOffset(
+      key_,
+      in_entry_op.offset,
+      in_entry_op.index,
+      out_entry_stat->data_size[1]);
+  int file_index = GetFileIndexFromStreamIndex(in_entry_op.index);
+  int bytes_read = ReadPlatformFile(
+      files_[file_index], file_offset, out_buf->data(), in_entry_op.buf_len);
   if (bytes_read > 0) {
-    *out_last_used = Time::Now();
+    out_entry_stat->last_used = Time::Now();
     *out_crc32 = crc32(crc32(0L, Z_NULL, 0),
                        reinterpret_cast<const Bytef*>(out_buf->data()),
                        bytes_read);
@@ -308,26 +313,29 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
                                        int* out_result) const {
   DCHECK(initialized_);
   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;
   int truncate = in_entry_op.truncate;
-
+  const int64 file_offset = GetFileOffsetFromDataOffset(
+      key_, offset, index, out_entry_stat->data_size[1]);
   bool extending_by_write = offset + buf_len > out_entry_stat->data_size[index];
   if (extending_by_write) {
-    // We are extending the file, and need to insure the EOF record is zeroed.
-    const int64 file_eof_offset = GetFileOffsetFromKeyAndDataOffset(
-        key_, out_entry_stat->data_size[index]);
-    if (!TruncatePlatformFile(files_[index], file_eof_offset)) {
+    // The EOF record and the eventual stream afterward need to be zeroed out.
+    const int64 file_eof_offset = GetFileOffsetFromDataOffset(
+        key_, out_entry_stat->data_size[index],
+        index, out_entry_stat->data_size[1]);
+    if (!TruncatePlatformFile(files_[file_index], file_eof_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_PRETRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
   }
-  const int64 file_offset = GetFileOffsetFromKeyAndDataOffset(key_, offset);
   if (buf_len > 0) {
     if (WritePlatformFile(
-            files_[index], file_offset, in_buf->data(), buf_len) != buf_len) {
+            files_[file_index], file_offset, in_buf->data(), buf_len) !=
+        buf_len) {
       RecordWriteResult(cache_type_, WRITE_RESULT_WRITE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
@@ -338,13 +346,18 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
     out_entry_stat->data_size[index] =
         std::max(out_entry_stat->data_size[index], offset + buf_len);
   } else {
-    if (!TruncatePlatformFile(files_[index], file_offset + buf_len)) {
+    out_entry_stat->data_size[index] = offset + buf_len;
+    int file_max_offset = GetFileOffsetFromDataOffset(
+        key_,
+        GetMaximumDataOffset(file_index, out_entry_stat->data_size),
+        index,
+        out_entry_stat->data_size[1]);
+    if (!TruncatePlatformFile(files_[file_index], file_max_offset)) {
       RecordWriteResult(cache_type_, WRITE_RESULT_TRUNCATE_FAILURE);
       Doom();
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
-    out_entry_stat->data_size[index] = offset + buf_len;
   }
 
   RecordWriteResult(cache_type_, WRITE_RESULT_SUCCESS);
@@ -353,60 +366,56 @@ void SimpleSynchronousEntry::WriteData(const EntryOperationData& in_entry_op,
 }
 
 void SimpleSynchronousEntry::CheckEOFRecord(int index,
-                                            int32 data_size,
+                                            const int32 data_size[],
                                             uint32 expected_crc32,
                                             int* out_result) const {
   DCHECK(initialized_);
-
-  SimpleFileEOF eof_record;
-  int64 file_offset = GetFileOffsetFromKeyAndDataOffset(key_, data_size);
-  if (ReadPlatformFile(files_[index],
-                       file_offset,
-                       reinterpret_cast<char*>(&eof_record),
-                       sizeof(eof_record)) != sizeof(eof_record)) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
+  *out_result = CheckEOFRecordInternal(index, data_size, expected_crc32);
+  if (*out_result != net::OK)
     Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_READ_FAILURE;
-    return;
-  }
+}
 
-  if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
-    DLOG(INFO) << "eof record had bad magic number.";
-    Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_READ_FAILURE;
-    return;
+void SimpleSynchronousEntry::Close(
+    const SimpleEntryStat& entry_stat,
+    scoped_ptr<std::vector<CRCRecord> > crc32s_to_write,
+    net::GrowableIOBuffer* stream_0_data) {
+  if (!stream_0_data) {
+    if (entry_stat.data_size[0])
+      Doom();
+  } else {
+    // Write stream 0 data.
+    int stream_0_offset =
+        GetFileOffsetFromDataOffset(key_, 0, 0, entry_stat.data_size[1]);
+    if (WritePlatformFile(files_[0],
+                          stream_0_offset,
+                          stream_0_data->data(),
+                          entry_stat.data_size[0]) != entry_stat.data_size[0]) {
+      RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
+      DLOG(INFO) << "Could not write stream 0 data.";
+      Doom();
+    }
   }
-
-  const bool has_crc = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
-                       SimpleFileEOF::FLAG_HAS_CRC32;
-  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, has_crc);
-  if (has_crc && eof_record.data_crc32 != expected_crc32) {
-    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
-    DLOG(INFO) << "eof record had bad crc.";
+  // Write updated header for stream 0 and stream 1 file.
+  if (WriteHeader(0, entry_stat.data_size[0], entry_stat.data_size[1]) !=
+      net::OK) {
+    RecordCloseResult(cache_type_, CLOSE_RESULT_WRITE_FAILURE);
+    DLOG(INFO) << "Could not write updated header.";
     Doom();
-    *out_result = net::ERR_CACHE_CHECKSUM_MISMATCH;
-    return;
   }
 
-  RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
-  *out_result = net::OK;
-}
-
-void SimpleSynchronousEntry::Close(
-    const SimpleEntryStat& entry_stat,
-    scoped_ptr<std::vector<CRCRecord> > crc32s_to_write) {
   for (std::vector<CRCRecord>::const_iterator it = crc32s_to_write->begin();
        it != crc32s_to_write->end(); ++it) {
     SimpleFileEOF eof_record;
+    int index = it->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;
-    int64 file_offset = GetFileOffsetFromKeyAndDataOffset(
-        key_, entry_stat.data_size[it->index]);
-    if (WritePlatformFile(files_[it->index],
+    int file_index = GetFileIndexFromStreamIndex(index);
+    int64 file_offset = GetFileOffsetFromDataOffset(
+        key_, entry_stat.data_size[index], index, entry_stat.data_size[1]);
+    if (WritePlatformFile(files_[file_index],
                           file_offset,
                           reinterpret_cast<const char*>(&eof_record),
                           sizeof(eof_record)) != sizeof(eof_record)) {
@@ -415,7 +424,12 @@ void SimpleSynchronousEntry::Close(
       Doom();
       break;
     }
-    const int64 file_size = file_offset + sizeof(eof_record);
+  }
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+    bool did_close_file = ClosePlatformFile(files_[i]);
+    CHECK(did_close_file);
+    const int64 file_size = GetFileSizeFromKeyAndDataSize(
+        key_, GetMaximumDataOffset(i, entry_stat.data_size));
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "LastClusterSize", cache_type_,
                      file_size % 4096, 0, 4097, 50);
@@ -424,11 +438,6 @@ void SimpleSynchronousEntry::Close(
                      "LastClusterLossPercent", cache_type_,
                      cluster_loss * 100 / (cluster_loss + file_size));
   }
-
-  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    bool did_close_file = ClosePlatformFile(files_[i]);
-    CHECK(did_close_file);
-  }
   RecordCloseResult(cache_type_, CLOSE_RESULT_SUCCESS);
   have_open_files_ = false;
   delete this;
@@ -517,7 +526,7 @@ bool SimpleSynchronousEntry::OpenOrCreateFiles(
   have_open_files_ = true;
   if (create) {
     out_entry_stat->last_modified = out_entry_stat->last_used = Time::Now();
-    for (int i = 0; i < kSimpleEntryFileCount; ++i)
+    for (int i = 0; i < kSimpleEntryStreamCount; ++i)
       out_entry_stat->data_size[i] = 0;
   } else {
     base::TimeDelta entry_age = base::Time::Now() - base::Time::UnixEpoch();
@@ -540,9 +549,9 @@ bool SimpleSynchronousEntry::OpenOrCreateFiles(
       if (stream_age < entry_age)
         entry_age = stream_age;
 
-      // Keep the file size in |data size_| briefly until the key is initialized
-      // properly.
-      out_entry_stat->data_size[i] = file_info.size;
+      // Keep the file size in |data size_| temporarily until the data sizes are
+      // initialized correctly.
+      out_entry_stat->data_size[i + 1] = file_info.size;
     }
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "SyncOpenEntryAge", cache_type_,
@@ -560,8 +569,10 @@ void SimpleSynchronousEntry::CloseFiles() {
   }
 }
 
-int SimpleSynchronousEntry::InitializeForOpen(bool had_index,
-                                              SimpleEntryStat* out_entry_stat) {
+int SimpleSynchronousEntry::InitializeForOpen(
+    bool had_index,
+    SimpleEntryStat* out_entry_stat,
+    scoped_refptr<net::GrowableIOBuffer>* stream_0_data) {
   DCHECK(!initialized_);
   if (!OpenOrCreateFiles(false, had_index, out_entry_stat))
     return net::ERR_FAILED;
@@ -602,13 +613,26 @@ int SimpleSynchronousEntry::InitializeForOpen(bool had_index,
     }
 
     key_ = std::string(key.get(), header.key_length);
-    out_entry_stat->data_size[i] =
-        GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size[i]);
-    if (out_entry_stat->data_size[i] < 0) {
-      // This entry can't possibly be valid, as it does not have enough space to
-      // store a valid SimpleFileEOF record.
-      return net::ERR_FAILED;
+    // The data size needs to be updated. For stream 0 and 1 it is included in
+    // the header, for stream 2 it is computed from the file size temporarily
+    // stored in |out_entry_stat|.
+    int data_size_current_file = 0;
+    int min_data_size = 0;
+    if (i == 0) {
+      // File size for stream 0 has been stored temporarily in data_size[1].
+      data_size_current_file =
+          GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size[1]);
+      min_data_size =
+          header.stream_0_size + header.stream_1_size + sizeof(SimpleFileEOF);
+      out_entry_stat->data_size[0] = header.stream_0_size;

[pasko, 2013/09/13 19:09:21]

Keeping in mind what index is for what would be hard for an unfamiliar person.

How about encapsulating all operations on data_size_ in SimpleEntryStat?

I can see these operations regarding streams:
GetDataSize(stream_index)
GetFileName(entry_hash, stream_index)
GetOffsetInFile(stream_index, offset)
GetEOFOffsetInFile(stream_index)

and we need some operations to work on files:
iteration: 0..kSimpleEntryFileCount
GetClusterLoss(file_index)
ApplyDataSizesFromFileSize(file_index, file_size)

and maybe something else I forgot :)

Some functions may be used only once beyond tests, but the code responsible for
file naming on the disk and indexing in the file array would be local in one
place making it easier to follow than magic juggling with data_size[0] and
data_size[1].

[clamy, 2013/09/16 15:01:17]

- I am not sure about the GetDataSize(stream_index)? Is it so much clearer than
data_size[stream_index]?

- GetFileName(entry_hash, stream_index) does not touch data_size, and would be a
duplicate of functions in simple_util if they are split in two versions. If not,
well it would be better in simple_util anyway.

I implemented the two others.

+      out_entry_stat->data_size[1] = header.stream_1_size;
+    } else {
+      out_entry_stat->data_size[2] =
+          GetDataSizeFromKeyAndFileSize(key_, out_entry_stat->data_size[2]);
+      data_size_current_file = out_entry_stat->data_size[2];
     }
+    if (data_size_current_file < min_data_size)
+      return net::ERR_FAILED;
 
     if (base::Hash(key.get(), header.key_length) != header.key_hash) {
       DLOG(WARNING) << "Hash mismatch on key.";
@@ -616,6 +640,17 @@ int SimpleSynchronousEntry::InitializeForOpen(bool had_index,
           cache_type_, OPEN_ENTRY_KEY_HASH_MISMATCH, had_index);
       return net::ERR_FAILED;
     }
+
+    // Stream 0 needs to be read and put in memory
+    if (i == 0) {
+      *stream_0_data = NULL;
+      if (header.stream_0_size == 0)
+        break;
+      int ret_value_stream_0 =
+          ReadAndValidateStream0(out_entry_stat->data_size, stream_0_data);
+      if (ret_value_stream_0 != net::OK)
+        return ret_value_stream_0;
+    }
   }
   RecordSyncOpenResult(cache_type_, OPEN_ENTRY_SUCCESS, had_index);
   initialized_ = true;
@@ -631,23 +666,15 @@ int SimpleSynchronousEntry::InitializeForCreate(
     return net::ERR_FILE_EXISTS;
   }
   for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    SimpleFileHeader header;
-    header.initial_magic_number = kSimpleInitialMagicNumber;
-    header.version = kSimpleVersion;
-
-    header.key_length = key_.size();
-    header.key_hash = base::Hash(key_);
-
-    if (WritePlatformFile(files_[i], 0, reinterpret_cast<char*>(&header),
-                          sizeof(header)) != sizeof(header)) {
-      DLOG(WARNING) << "Could not write headers to new cache entry.";
+    if (WriteHeader(i, 0, 0) != net::OK) {
       RecordSyncCreateResult(
           cache_type_, CREATE_ENTRY_CANT_WRITE_HEADER, had_index);
       return net::ERR_FAILED;
     }
 
-    if (WritePlatformFile(files_[i], sizeof(header), key_.data(),
-                          key_.size()) != implicit_cast<int>(key_.size())) {
+    if (WritePlatformFile(
+            files_[i], sizeof(SimpleFileHeader), key_.data(), key_.size()) !=
+        implicit_cast<int>(key_.size())) {
       DLOG(WARNING) << "Could not write keys to new cache entry.";
       RecordSyncCreateResult(
           cache_type_, CREATE_ENTRY_CANT_WRITE_KEY, had_index);
@@ -659,6 +686,81 @@ int SimpleSynchronousEntry::InitializeForCreate(
   return net::OK;
 }
 
+int SimpleSynchronousEntry::ReadAndValidateStream0(
+    int data_size[],
+    scoped_refptr<net::GrowableIOBuffer>* stream_0_data) const {
+  *stream_0_data = new net::GrowableIOBuffer();
+  (*stream_0_data)->SetCapacity(data_size[0]);
+  int64 file_offset = GetFileOffsetFromDataOffset(key_, 0, 0, data_size[1]);
+  int bytes_read = ReadPlatformFile(
+      files_[0], file_offset, (*stream_0_data)->data(), data_size[0]);
+  if (bytes_read != data_size[0])
+    return net::ERR_FAILED;
+  // Check the CRC32.
+  uint32 expected_crc32 =
+      crc32(crc32(0L, Z_NULL, 0),
+            reinterpret_cast<const Bytef*>((*stream_0_data)->data()),
+            data_size[0]);
+  return CheckEOFRecordInternal(0, data_size, expected_crc32);
+}
+
+int SimpleSynchronousEntry::WriteHeader(int file_index,
+                                        int stream_0_size,
+                                        int stream_1_size) const {
+  SimpleFileHeader header;
+  header.initial_magic_number = kSimpleInitialMagicNumber;
+  header.version = kSimpleVersion;
+
+  header.key_length = key_.size();
+  header.key_hash = base::Hash(key_);
+  header.stream_0_size = stream_0_size;
+  header.stream_1_size = stream_1_size;
+
+  if (WritePlatformFile(files_[file_index],
+                        0,
+                        reinterpret_cast<char*>(&header),
+                        sizeof(header)) != sizeof(header)) {
+    DLOG(WARNING) << "Could not write cache file header to cache entry.";
+    return net::ERR_FAILED;
+  }
+  return net::OK;
+}
+
+int SimpleSynchronousEntry::CheckEOFRecordInternal(
+    int index,
+    const int data_size[],
+    uint32 expected_crc32) const {
+  SimpleFileEOF eof_record;
+  int64 file_offset =
+      GetFileOffsetFromDataOffset(key_, data_size[index], index, data_size[1]);
+  int file_index = GetFileIndexFromStreamIndex(index);
+  if (ReadPlatformFile(files_[file_index],
+                       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) {
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
+    DLOG(INFO) << "Eof record had bad magic number.";
+    return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
+  }
+
+  const bool has_crc = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
+                       SimpleFileEOF::FLAG_HAS_CRC32;
+  SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, has_crc);
+  if (has_crc && eof_record.data_crc32 != expected_crc32) {
+    RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
+    DLOG(INFO) << "Eof record had bad crc.";
+    return net::ERR_CACHE_CHECKSUM_MISMATCH;
+  }
+
+  RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_SUCCESS);
+  return net::OK;
+}
+
 void SimpleSynchronousEntry::Doom() const {
   // TODO(gavinp): Consider if we should guard against redundant Doom() calls.
   DeleteFilesForEntryHash(path_, entry_hash_);