SimpleCache: merge the first and second stream in one file

net/disk_cache/simple/simple_entry_impl.cc

Index: net/disk_cache/simple/simple_entry_impl.cc
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index d270098c34d7841633f1444e5a07fd47830b6237..87ad6de082baa115364da5b2269c31089245bc3d 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -332,7 +332,7 @@ int SimpleEntryImpl::ReadData(int stream_index,
                                           false));
   }
 
-  if (stream_index < 0 || stream_index >= kSimpleEntryFileCount ||
+  if (stream_index < 0 || stream_index >= kSimpleEntryStreamCount ||
       buf_len < 0) {
     if (net_log_.IsLoggingAllEvents()) {
       net_log_.AddEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_READ_END,
@@ -378,8 +378,8 @@ int SimpleEntryImpl::WriteData(int stream_index,
                                           truncate));
   }
 
-  if (stream_index < 0 || stream_index >= kSimpleEntryFileCount || offset < 0 ||
-      buf_len < 0) {
+  if (stream_index < 0 || stream_index >= kSimpleEntryStreamCount ||
+      offset < 0 || buf_len < 0) {
     if (net_log_.IsLoggingAllEvents()) {
       net_log_.AddEvent(
           net::NetLog::TYPE_SIMPLE_CACHE_ENTRY_WRITE_END,
@@ -399,17 +399,6 @@ int SimpleEntryImpl::WriteData(int stream_index,
   }
   ScopedOperationRunner operation_runner(this);
 
-  // Currently, Simple Cache is only used for HTTP, which stores the headers in
-  // stream 0 and always writes them with a single, truncating write.  Detect
-  // these writes and record the size and size changes of the headers.  Also,
-  // note writes to stream 0 that violate those assumptions.
-  if (stream_index == 0) {
-    if (offset == 0 && truncate)
-      RecordHeaderSizeChange(cache_type_, data_size_[0], buf_len);
-    else
-      RecordUnexpectedStream0Write(cache_type_);
-  }
-
   // We can only do optimistic Write if there is no pending operations, so
   // that we are sure that the next call to RunNextOperationIfNeeded will
   // actually run the write operation that sets the stream size. It also
@@ -677,7 +666,7 @@ void SimpleEntryImpl::CreateEntryInternal(bool have_index,
   last_used_ = last_modified_ = base::Time::Now();
 
   // If creation succeeds, we should mark all streams to be saved on close.
-  for (int i = 0; i < kSimpleEntryFileCount; ++i)
+  for (int i = 0; i < kSimpleEntryStreamCount; ++i)
     have_written_[i] = true;
 
   const base::TimeTicks start_time = base::TimeTicks::Now();
@@ -712,7 +701,7 @@ void SimpleEntryImpl::CloseInternal() {
   if (state_ == STATE_READY) {
     DCHECK(synchronous_entry_);
     state_ = STATE_IO_PENDING;
-    for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+    for (int i = 0; i < kSimpleEntryStreamCount; ++i) {
       if (have_written_[i]) {
         if (GetDataSize(i) == crc32s_end_offset_[i]) {
           int32 crc = GetDataSize(i) == 0 ? crc32(0, Z_NULL, 0) : crc32s_[i];
@@ -731,12 +720,13 @@ void SimpleEntryImpl::CloseInternal() {
         base::Bind(&SimpleSynchronousEntry::Close,
                    base::Unretained(synchronous_entry_),
                    SimpleEntryStat(last_used_, last_modified_, data_size_),
-                   base::Passed(&crc32s_to_write));
+                   base::Passed(&crc32s_to_write),
+                   stream_0_data_);
     Closure reply = base::Bind(&SimpleEntryImpl::CloseOperationComplete, this);
     synchronous_entry_ = NULL;
     worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 
-    for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+    for (int i = 0; i < kSimpleEntryStreamCount; ++i) {
       if (!have_written_[i]) {
         SIMPLE_CACHE_UMA(ENUMERATION,
                          "CheckCRCResult", cache_type_,
@@ -787,6 +777,16 @@ void SimpleEntryImpl::ReadDataInternal(int stream_index,
     return;
   }
 
+  // Since stream 0 data is kept in memory, it is read immediately.
+  if (stream_index == 0) {
+    int ret_value = ReadStream0Data(buf, offset, buf_len);
+    if (!callback.is_null()) {
+      MessageLoopProxy::current()
+          ->PostTask(FROM_HERE, base::Bind(callback, ret_value));
+    }
+    return;
+  }
+
   buf_len = std::min(buf_len, GetDataSize(stream_index) - offset);
 
   state_ = STATE_IO_PENDING;
@@ -795,14 +795,15 @@ void SimpleEntryImpl::ReadDataInternal(int stream_index,
 
   scoped_ptr<uint32> read_crc32(new uint32());
   scoped_ptr<int> result(new int());
-  scoped_ptr<base::Time> last_used(new base::Time());
+  scoped_ptr<SimpleEntryStat> entry_stat(
+      new SimpleEntryStat(last_used_, last_modified_, data_size_));
   Closure task = base::Bind(
       &SimpleSynchronousEntry::ReadData,
       base::Unretained(synchronous_entry_),
       SimpleSynchronousEntry::EntryOperationData(stream_index, offset, buf_len),
       make_scoped_refptr(buf),
       read_crc32.get(),
-      last_used.get(),
+      entry_stat.get(),
       result.get());
   Closure reply = base::Bind(&SimpleEntryImpl::ReadOperationComplete,
                              this,
@@ -810,7 +811,7 @@ void SimpleEntryImpl::ReadDataInternal(int stream_index,
                              offset,
                              callback,
                              base::Passed(&read_crc32),
-                             base::Passed(&last_used),
+                             base::Passed(&entry_stat),
                              base::Passed(&result));
   worker_pool_->PostTaskAndReply(FROM_HERE, task, reply);
 }
@@ -849,6 +850,17 @@ void SimpleEntryImpl::WriteDataInternal(int stream_index,
   }
 
   DCHECK_EQ(STATE_READY, state_);
+  // Since stream 0 data is kept in memory, it will be written immediatly.
+  if (stream_index == 0) {
+    int ret_value = CopyStream0Data(buf, offset, buf_len, truncate);
+    if (!callback.is_null()) {
+      // Postask prevents creating a loop when calling the callback directly.

[Deprecated (see juliatuttle), 2013/09/12 22:05:56]

nit: PostTask

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

Done.

+      MessageLoopProxy::current()
+          ->PostTask(FROM_HERE, base::Bind(callback, ret_value));
+    }
+    return;
+  }
+
   state_ = STATE_IO_PENDING;
   if (backend_.get())
     backend_->index()->UseIfExists(entry_hash_);
@@ -883,6 +895,9 @@ void SimpleEntryImpl::WriteDataInternal(int stream_index,
   last_used_ = last_modified_ = base::Time::Now();
 
   have_written_[stream_index] = true;
+  // Writing on stream 1 affects the placement of stream 0 in the file.
+  if (stream_index == 1)
+    have_written_[0] = true;
 
   scoped_ptr<int> result(new int());
   Closure task = base::Bind(&SimpleSynchronousEntry::WriteData,
@@ -943,6 +958,9 @@ void SimpleEntryImpl::CreationOperationComplete(
 
   state_ = STATE_READY;
   synchronous_entry_ = in_results->sync_entry;
+  stream_0_data_ = in_results->stream_0_data;
+  // The crc was read in SimpleSynchronousEntry.
+  crc_check_state_[0] = CRC_CHECK_DONE;
   if (key_.empty()) {
     SetKey(synchronous_entry_->key());
   } else {
@@ -993,7 +1011,7 @@ void SimpleEntryImpl::ReadOperationComplete(
     int offset,
     const CompletionCallback& completion_callback,
     scoped_ptr<uint32> read_crc32,
-    scoped_ptr<base::Time> last_used,
+    scoped_ptr<SimpleEntryStat> entry_stat,
     scoped_ptr<int> result) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(synchronous_entry_);
@@ -1029,7 +1047,7 @@ void SimpleEntryImpl::ReadOperationComplete(
       Closure task = base::Bind(&SimpleSynchronousEntry::CheckEOFRecord,
                                 base::Unretained(synchronous_entry_),
                                 stream_index,
-                                data_size_[stream_index],
+                                data_size_,
                                 crc32s_[stream_index],
                                 new_result.get());
       Closure reply = base::Bind(&SimpleEntryImpl::ChecksumOperationComplete,
@@ -1058,10 +1076,7 @@ void SimpleEntryImpl::ReadOperationComplete(
   }
 
   EntryOperationComplete(
-      stream_index,
-      completion_callback,
-      SimpleEntryStat(*last_used, last_modified_, data_size_),
-      result.Pass());
+      stream_index, completion_callback, *entry_stat, result.Pass());
 }
 
 void SimpleEntryImpl::WriteOperationComplete(
@@ -1147,7 +1162,7 @@ void SimpleEntryImpl::UpdateDataFromEntryStat(
 
   last_used_ = entry_stat.last_used;
   last_modified_ = entry_stat.last_modified;
-  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+  for (int i = 0; i < kSimpleEntryStreamCount; ++i) {
     data_size_[i] = entry_stat.data_size[i];
   }
   if (backend_.get())
@@ -1156,7 +1171,7 @@ void SimpleEntryImpl::UpdateDataFromEntryStat(
 
 int64 SimpleEntryImpl::GetDiskUsage() const {
   int64 file_size = 0;
-  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+  for (int i = 0; i < kSimpleEntryStreamCount; ++i) {
     file_size +=
         simple_util::GetFileSizeFromKeyAndDataSize(key_, data_size_[i]);
   }
@@ -1234,4 +1249,62 @@ void SimpleEntryImpl::RecordWriteDependencyType(
                    type, WRITE_DEPENDENCY_TYPE_MAX);
 }
 
+int SimpleEntryImpl::ReadStream0Data(net::IOBuffer* buf,
+                                     int offset,
+                                     int buf_len) {
+  int read_size = std::min(data_size_[0] - offset, buf_len);
+  if (read_size < 0) {
+    RecordReadResult(cache_type_, READ_RESULT_SUCCESS);

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

READ_RESULT_SYNC_READ_FAILURE

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

Done.

+    return 0;
+  }
+  memcpy(buf->data(), stream_0_data_->data() + offset, read_size);
+  UpdateDataFromEntryStat(
+      SimpleEntryStat(base::Time::Now(), last_modified_, data_size_));
+  RecordReadResult(cache_type_, READ_RESULT_SUCCESS);
+  return read_size;
+}
+
+int SimpleEntryImpl::CopyStream0Data(net::IOBuffer* buf,

[gavinp, 2013/09/10 22:20:49]

Does stream 0 need to support anything than a truncating write of length 0 at
offset 0, and a write of length X at offset 0 to a stream already of length 0?

Other than for unit tests, I mean.

[clamy, 2013/09/11 12:46:21]

Well on HTTP cache, according to UMA other cases never happened in two weeks.
But I am not sure we can drop support of the other types of write (wouldn't we
be "breaking the contract of the API"?).

+                                     int offset,
+                                     int buf_len,
+                                     bool truncate) {
+  // Currently, Simple Cache is only used for HTTP, which stores the headers in

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

"only used for HTTP" will have to be changed soon. Better rephrase to say that
stream 0 is used only for HTTP headers, all other clients are encouraged to use
stream 1.

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

Done.

+  // stream 0 and always writes them with a single, truncating write.  Detect
+  // these writes and record the size and size changes of the headers.  Also,
+  // supports writes to stream 0 that violate those assumptions.
+  if (!stream_0_data_)
+    stream_0_data_ = new net::GrowableIOBuffer();
+  have_written_[0] = true;

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

I am somewhat annoyed to observe the zero index in data_size_[0] along this
function, can we add a temporary and initialize it here:
data_size = data_size_[0]
?

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

Done.

+  if (offset == 0 && truncate) {
+    RecordHeaderSizeChange(cache_type_, data_size_[0], buf_len);
+    stream_0_data_->SetCapacity(buf_len);
+    memcpy(stream_0_data_->data(), buf->data(), buf_len);
+    data_size_[0] = buf_len;
+  } else {
+    RecordUnexpectedStream0Write(cache_type_);
+    const int buffer_size =
+        truncate ? offset + buf_len : std::max(offset + buf_len, data_size_[0]);
+    stream_0_data_->SetCapacity(buffer_size);
+    // If |stream_0_data_| was extended. the extension need to be zeroed.
+    const int fill_size = buffer_size - data_size_[0];

[gavinp, 2013/09/10 22:20:49]

const int fill_size = (truncate || offset <= data_size_[0]) ? 0 : offset -
data_size_[0];

Or is that too complicated?

[clamy, 2013/09/11 12:46:21]

With a comment such as 
// The extension until offset needs to be zeroed 
it should be ok (otherwise I needed a drawing to see what this was about). And I
think the formula should be:
const int fill_size = offset <= data_size_[0] ? 0 : offset -data_size_[0];

+    if (fill_size > 0)
+      memset(stream_0_data_->data() + data_size_[0], 0, fill_size);
+    if (buf)
+      memcpy(stream_0_data_->data() + offset, buf->data(), buf_len);
+    data_size_[0] = buffer_size;
+  }
+  base::Time modification_time = base::Time::Now();
+  UpdateDataFromEntryStat(
+      SimpleEntryStat(modification_time, modification_time, data_size_));
+  if (stream_0_data_) {
+    crc32s_[0] = crc32(crc32(0L, Z_NULL, 0),
+                       reinterpret_cast<const Bytef*>(stream_0_data_->data()),
+                       data_size_[0]);
+  } else {
+    crc32s_[0] = crc32(0L, Z_NULL, 0);
+  }
+  RecordWriteResult(cache_type_, WRITE_RESULT_SUCCESS);
+  return buf_len;
+}
+
 }  // namespace disk_cache