HttpCache::Transaction layer allowing parallel validation

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index ede3541aa4010750256d9acb89f3140bf6938def..bd80ac269c55582dc020fbb91b7382f6739d5b83 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -169,6 +169,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       new_entry_(NULL),
       new_response_(NULL),
       mode_(NONE),
+      original_mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
@@ -181,6 +182,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
+      validating_cannot_proceed_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
@@ -577,6 +579,11 @@ void HttpCache::Transaction::GetConnectionAttempts(
               old_connection_attempts_.end());
 }
 
+void HttpCache::Transaction::SetValidatingCannotProceed() {
+  validating_cannot_proceed_ = true;
+  entry_ = nullptr;
+}
+
 size_t HttpCache::Transaction::EstimateMemoryUsage() const {
   // TODO(xunjieli): Consider improving the coverage. crbug.com/669108.
   return 0;
@@ -600,7 +607,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   Start():
 //   GetBackend* -> InitEntry -> OpenEntry* -> AddToEntry* -> CacheReadResponse*
 //   -> CacheDispatchValidation -> BeginPartialCacheValidation() ->
-//   BeginCacheValidation() -> SetupEntryForRead()
+//   BeginCacheValidation() -> SetupEntryForRead() -> WaitBeforeRead*
 //
 //   Read():
 //   CacheReadData*
@@ -612,7 +619,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
 //   UpdateCachedResponse -> CacheWriteUpdatedResponse* ->
 //   UpdateCachedResponseComplete -> OverwriteCachedResponse ->
-//   PartialHeadersReceived
+//   PartialHeadersReceived -> WaitBeforeRead*
 //
 //   Read():
 //   CacheReadData*
@@ -712,6 +719,38 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   Like examples 2-4, only CacheToggleUnusedSincePrefetch* is inserted between
 //   CacheReadResponse* and CacheDispatchValidation.
 int HttpCache::Transaction::DoLoop(int result) {
+  // If its the start state machine and it cannot proceed due to failure by
+  // another transaction in writing the response, restart this transaction.
+  if (!reading_ && validating_cannot_proceed_) {
+    validating_cannot_proceed_ = false;
+    RestartAfterValidationStarted();
+  }
+
+  int rv = DoLoopImpl(result);
+
+  // If its the end of the Start() state machine, the transaction might have to
+  // wait before it can read from the cache if there is another transaction
+  // writing to the cache currently. This is being done to enable parallelizing
+  // the validation phase of a transaction while another transaction is still
+  // writing the response to the cache.
+  // Checking reading_ will make sure that the control is still in the start
+  // state machine and also that we do not enter the wait state for
+  // partial requests that have already started reading and re-entered the
+  // start() state machine.
+  if (!reading_ && rv == OK) {
+    next_state_ = STATE_WAIT_BEFORE_READ;
+    rv = DoLoopImpl(rv);
+  }
+
+  if (rv != ERR_IO_PENDING && !callback_.is_null()) {
+    read_buf_ = nullptr;  // Release the buffer before invoking the callback.
+    base::ResetAndReturn(&callback_).Run(rv);
+  }
+
+  return rv;
+}
+
+int HttpCache::Transaction::DoLoopImpl(int result) {
   DCHECK(next_state_ != STATE_NONE);
 
   int rv = result;
@@ -851,6 +890,13 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_CACHE_READ_METADATA_COMPLETE:
         rv = DoCacheReadMetadataComplete(rv);
         break;
+      case STATE_WAIT_BEFORE_READ:
+        DCHECK_EQ(OK, rv);
+        rv = DoWaitBeforeRead();
+        break;
+      case STATE_WAIT_BEFORE_READ_COMPLETE:
+        rv = DoWaitBeforeReadComplete(rv);
+        break;
       case STATE_NETWORK_READ:
         DCHECK_EQ(OK, rv);
         rv = DoNetworkRead();
@@ -885,11 +931,6 @@ int HttpCache::Transaction::DoLoop(int result) {
     }
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
-  if (rv != ERR_IO_PENDING && !callback_.is_null()) {
-    read_buf_ = NULL;  // Release the buffer before invoking the callback.
-    base::ResetAndReturn(&callback_).Run(rv);
-  }
-
   return rv;
 }
 
@@ -965,7 +1006,7 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
 
   // This is only set if we have something to do with the response.
   range_requested_ = (partial_.get() != NULL);
-
+  original_mode_ = mode_;
   return OK;
 }
 
@@ -1497,7 +1538,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       int ret = cache_->DoomEntry(cache_key_, NULL);
       DCHECK_EQ(OK, ret);
     }
-    cache_->DoneWritingToEntry(entry_, true);
+    cache_->DoneWritingToEntry(entry_, true, this);
     entry_ = NULL;
     mode_ = NONE;
   }
@@ -1530,6 +1571,21 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
   }
 
   next_state_ = STATE_OVERWRITE_CACHED_RESPONSE;
+
+  if (!entry_)
+    return OK;
+
+  // Invalidate any current entry with a successful response if this transaction
+  // cannot write to this entry.
+  if (new_response->headers->response_code() != 304 &&
+      (entry_->writer || !entry_->readers.empty())) {
+    DCHECK_EQ(entry_->headers_transaction, this);
+    cache_->DoneResponseHeaders(entry_, this, false);
+    entry_ = nullptr;
+    mode_ = NONE;
+    return OK;
+  }
+
   return OK;
 }
 
@@ -1599,7 +1655,6 @@ int HttpCache::Transaction::DoUpdateCachedResponseComplete(int result) {
   } else if (entry_ && !handling_206_) {
     DCHECK_EQ(READ_WRITE, mode_);
     if (!partial_ || partial_->IsLastRange()) {
-      cache_->ConvertWriterToReader(entry_);
       mode_ = READ;
     }
     // We no longer need the network transaction, so destroy it.
@@ -1736,6 +1791,53 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
   return OK;
 }
 
+int HttpCache::Transaction::DoWaitBeforeRead() {
+  if (!entry_)
+    return OK;
+
+  next_state_ = STATE_WAIT_BEFORE_READ_COMPLETE;
+
+  // If |this| is the writer or there is no response body to be written or read,
+  // it does not need to wait.
+  if (entry_->writer == this || request_->method == "HEAD")

[Randy Smith (Not in Mondays), 2017/03/17 18:13:06]

I'm a bit concerned about races here.  The transition to writer from
validating_transaction completes after a PostTask.  Can we be in a situation
where that happens after this test, and if so, what's the result?

[shivanisha, 2017/03/20 18:14:49]

Actually, after the state simplification since a transaction only becomes writer
after this function, this condition can be removed.
A related change:
(We just have to make sure that after completing DoWaitBeforeReadComplete is
executed, this function does not get executed again due to the DoLoop code. For
that, I am adding a boolean is_wait_before_read_complete_ and checking it in
DoLoop in addition to !reading_ before setting the state to
STATE_WAIT_BEFORE_READ)

+    return OK;
+
+  // If |this| is in readers, then no need to wait.

[Randy Smith (Not in Mondays), 2017/03/17 18:13:06]

This comment doesn't seem to fit the following lines?

[shivanisha, 2017/03/20 18:14:49]

Expanded the comment to be more clear. The if condition basically wants to
exclude transactions that are already in readers, which can happen to
transactions with read-only permissions.

+  if (entry_->headers_transaction == this)

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

When does this happen?  Naively, I think of WAIT_BEFORE_READ as being the state
reflection in HttpCache::Transaction between the completion of being a
headers_transaction and the start of being a reader/writer, so we should always
be in the done_headers_queue at this point.  What am I missing?

[shivanisha, 2017/03/20 18:14:49]

At this point a transaction should always be a headers_transaction and will move
to done_headers_queue in DoneResponseHeaders(). The only exception is the
transactions that are read-only, they would be in readers at this point, thus
this condition.

+    return cache_->DoneResponseHeaders(entry_, this, true);
+
+  return OK;
+}
+
+int HttpCache::Transaction::DoWaitBeforeReadComplete(int rv) {
+  if (rv == ERR_CACHE_RACE) {
+    RestartAfterValidationStarted();
+    return OK;
+  }
+
+  if (rv != OK)
+    return rv;
+
+  // If successful then this must be either be the writer or a reader as the
+  // response must have completely been written to the cache.
+  if (entry_->writer == this)
+    return OK;
+
+  mode_ = READ;
+  if (network_trans_)
+    ResetNetworkTransaction();
+  return OK;
+}
+
+void HttpCache::Transaction::RestartAfterValidationStarted() {

[shivanisha, 2017/03/16 15:43:26]

DCHECK(!reading_);

[shivanisha, 2017/03/20 18:14:49]

done

+  next_state_ = STATE_INIT_ENTRY;
+  cache_entry_status_ = CacheEntryStatus::ENTRY_UNDEFINED;
+  entry_ = nullptr;
+  mode_ = original_mode_;
+  if (network_trans_)
+    network_trans_.reset();
+}
+
 int HttpCache::Transaction::DoCacheReadMetadata() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadMetadata");
   DCHECK(entry_);
@@ -2565,7 +2667,10 @@ int HttpCache::Transaction::SetupEntryForRead() {
       partial_.reset();
     }
   }
-  cache_->ConvertWriterToReader(entry_);
+
+  // This might or might not be able to add it as an active reader based on
+  // whether this is the active writer or not. If not, it will be added to a
+  // wait queue in DoWaitBeforeRead.
   mode_ = READ;
 
   if (request_->method == "HEAD")
@@ -2651,7 +2756,7 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
 
   RecordHistograms();
 
-  cache_->DoneWritingToEntry(entry_, success);
+  cache_->DoneWritingToEntry(entry_, success, this);
   entry_ = NULL;
   mode_ = NONE;  // switch to 'pass through' mode
 }