Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1995)

Unified Diff: net/http/http_cache.cc

Issue 2721933002: HttpCache::Transaction layer allowing parallel validation (Closed)
Patch Set: Initial patch Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: net/http/http_cache.cc
diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index 1dc390a474c3abcc21e95539ed56ef75fb9dda16..ce4065c08ada25bb477a88a5c5a3443e5681635b 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -96,16 +96,12 @@ int HttpCache::DefaultBackend::CreateBackend(
//-----------------------------------------------------------------------------
HttpCache::ActiveEntry::ActiveEntry(disk_cache::Entry* entry)
- : disk_entry(entry),
- writer(NULL),
- will_process_pending_queue(false),
- doomed(false) {
-}
+ : disk_entry(entry) {}
HttpCache::ActiveEntry::~ActiveEntry() {
if (disk_entry) {
disk_entry->Close();
- disk_entry = NULL;
+ disk_entry = nullptr;
}
}
@@ -117,7 +113,12 @@ size_t HttpCache::ActiveEntry::EstimateMemoryUsage() const {
}
bool HttpCache::ActiveEntry::HasNoTransactions() {
- return !writer && readers.empty() && pending_queue.empty();
+ return !writer && readers.empty() && pending_queue.empty() &&
+ validated_queue.empty() && !validating_transaction;
+}
+
+bool HttpCache::ActiveEntry::IsReader(Transaction* transaction) {
+ return readers.count(transaction);
}
//-----------------------------------------------------------------------------
@@ -360,15 +361,18 @@ HttpCache::~HttpCache() {
weak_factory_.InvalidateWeakPtrs();
// If we have any active entries remaining, then we need to deactivate them.
- // We may have some pending calls to OnProcessPendingQueue, but since those
- // won't run (due to our destruction), we can simply ignore the corresponding
- // will_process_pending_queue flag.
+ // We may have some pending tasks to process pending queue for reading or
+ // parallel validation, but since those won't run (due to our destruction),
+ // we can simply ignore the corresponding flags.
while (!active_entries_.empty()) {
ActiveEntry* entry = active_entries_.begin()->second.get();
entry->will_process_pending_queue = false;
+ entry->will_process_parallel_validation = false;
entry->pending_queue.clear();
entry->readers.clear();
- entry->writer = NULL;
+ entry->validated_queue.clear();
+ entry->validating_transaction = nullptr;
+ entry->writer = nullptr;
DeactivateEntry(entry);
}
@@ -628,7 +632,10 @@ int HttpCache::DoomEntry(const std::string& key, Transaction* trans) {
entry_ptr->doomed = true;
DCHECK(entry_ptr->writer || !entry_ptr->readers.empty() ||
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Do you know what the purpose of this DCHECK is? I
shivanisha 2017/03/08 21:42:12 DoomEntry's consumer is the HttpCache::Transaction
- entry_ptr->will_process_pending_queue);
+ !entry_ptr->validated_queue.empty() ||
+ entry_ptr->validating_transaction ||
+ entry_ptr->will_process_pending_queue ||
+ entry_ptr->will_process_parallel_validation);
return OK;
}
@@ -684,7 +691,15 @@ void HttpCache::FinalizeDoomedEntry(ActiveEntry* entry) {
HttpCache::ActiveEntry* HttpCache::FindActiveEntry(const std::string& key) {
auto it = active_entries_.find(key);
- return it != active_entries_.end() ? it->second.get() : NULL;
+ return it != active_entries_.end() ? it->second.get() : nullptr;
+}
+
+bool HttpCache::IsEntryActive(const std::string& key,
+ const ActiveEntry* entry) {
+ ActiveEntry* entry_alive = FindActiveEntry(key);
+ if (!entry_alive || entry != entry_alive)
jkarlin 2017/03/07 16:45:41 return entry_alive && entry_alive == entry;
shivanisha 2017/03/08 21:42:11 Done. Also renamed entry_alive to active_entry
+ return false;
+ return true;
}
HttpCache::ActiveEntry* HttpCache::ActivateEntry(
@@ -696,7 +711,6 @@ HttpCache::ActiveEntry* HttpCache::ActivateEntry(
}
void HttpCache::DeactivateEntry(ActiveEntry* entry) {
- DCHECK(!entry->will_process_pending_queue);
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Why?
shivanisha 2017/03/08 21:42:12 Now that we have 2 types of tasks posted (OnProces
DCHECK(!entry->doomed);
DCHECK(entry->disk_entry);
DCHECK(entry->HasNoTransactions());
@@ -826,97 +840,155 @@ void HttpCache::DestroyEntry(ActiveEntry* entry) {
}
}
-int HttpCache::AddTransactionToEntry(ActiveEntry* entry, Transaction* trans) {
+int HttpCache::AddTransactionToEntry(ActiveEntry* entry,
+ Transaction* transaction,
+ bool validated_queue,
jkarlin 2017/03/07 16:45:41 We discussed this F2F, but can you remove validate
+ bool new_transaction) {
jkarlin 2017/03/07 16:45:40 is new_transaction ever set to true? I don't see i
shivanisha 2017/03/08 21:42:11 Refactored this function into 4 functions: - Alwa
DCHECK(entry);
DCHECK(entry->disk_entry);
- // We implement a basic reader/writer lock for the disk cache entry. If
- // there is already a writer, then everyone has to wait for the writer to
- // finish before they can access the cache entry. There can be multiple
- // readers.
- //
- // NOTE: If the transaction can only write, then the entry should not be in
- // use (since any existing entry should have already been doomed).
-
- if (entry->writer || entry->will_process_pending_queue) {
- entry->pending_queue.push_back(trans);
+ // We implement a basic reader/writer lock for the disk cache entry. If there
+ // is a writer, then all read-only transactions must wait. Non read-only
+ // transactions can proceed to their validation phase. Validation is allowed
+ // for one transaction at a time so that we do not end up with wasted network
+ // requests.
+
+ // If a pending queue processing task is posted and this is invoked for a
+ // new transaction as opposed to a queued transaction, then this transaction
+ // should be added after the queue task is processed, to maintain FIFO
+ // ordering.
+ if (new_transaction && (entry->will_process_pending_queue ||
+ entry->will_process_parallel_validation)) {
+ entry->pending_queue.push_back(transaction);
return ERR_IO_PENDING;
}
- if (trans->mode() & Transaction::WRITE) {
- // transaction needs exclusive access to the entry
- if (entry->readers.empty()) {
- entry->writer = trans;
- } else {
- entry->pending_queue.push_back(trans);
+ // A transaction that has the write mode and has not already passed the
+ // validation phase can become the next validating transaction. It can also
+ // become the writer if there is no active writer at this time.
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Is there a reason for it to take up the writer slo
shivanisha 2017/03/08 21:42:12 The writer slot works as a lock (in the existing c
+ if ((transaction->mode() & Transaction::WRITE) && !validated_queue) {
+ if (entry->validating_transaction) {
+ entry->pending_queue.push_back(transaction);
return ERR_IO_PENDING;
}
- } else {
- // transaction needs read access to the entry
- entry->readers.insert(trans);
+ entry->validating_transaction = transaction;
+ if (!entry->writer && entry->readers.empty()) {
+ entry->writer = transaction;
+ }
+ if (!entry->writer && !entry->pending_queue.empty())
+ ProcessPendingQueue(entry);
+ return OK;
}
- // We do this before calling EntryAvailable to force any further calls to
- // AddTransactionToEntry to add their transaction to the pending queue, which
- // ensures FIFO ordering.
- if (!entry->writer && !entry->pending_queue.empty())
+ // Read-only transactions.
+ if (entry->writer) {
+ entry->pending_queue.push_back(transaction);
+ return ERR_IO_PENDING;
+ }
+ entry->readers.insert(transaction);
+ if (!entry->pending_queue.empty())
ProcessPendingQueue(entry);
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 nit, suggestion: It feels like this routine might
shivanisha 2017/03/08 21:42:12 I have refactored this function quite a bit in the
-
return OK;
}
-void HttpCache::DoneWithEntry(ActiveEntry* entry, Transaction* trans,
+int HttpCache::OnValidationMatch(ActiveEntry* entry,
+ Transaction* transaction,
+ bool write_this_response) {
+ int rv = OK;
+ entry->validating_transaction = nullptr;
+
+ if (entry->writer != transaction) {
+ entry->validated_queue.push_back(transaction);
+ rv = ERR_IO_PENDING;
+ } else if (!write_this_response) {
+ entry->writer = nullptr;
+ entry->readers.insert(transaction);
jkarlin 2017/03/07 16:45:40 We're adding it to readers, but I don't think the
shivanisha 2017/03/08 21:42:11 Actually this should never happen. ConvertWriterTo
+ }
+ ProcessParallelValidation(entry);
+ return rv;
+}
+
+void HttpCache::OnValidationNoMatch(ActiveEntry* entry,
+ Transaction* transaction) {
+ entry->validating_transaction = nullptr;
+ DoomEntryRestartPendingTransactions(entry);
+}
+
+void HttpCache::DoneWithEntry(ActiveEntry* entry,
+ Transaction* transaction,
bool cancel) {
// If we already posted a task to move on to the next transaction and this was
// the writer, there is nothing to cancel.
- if (entry->will_process_pending_queue && entry->readers.empty())
+ if (entry->will_process_pending_queue && entry->writer == transaction)
return;
- if (entry->writer) {
- DCHECK(trans == entry->writer);
-
- // Assume there was a failure.
- bool success = false;
- if (cancel) {
- DCHECK(entry->disk_entry);
- // This is a successful operation in the sense that we want to keep the
- // entry.
- success = trans->AddTruncatedFlag();
- // The previous operation may have deleted the entry.
- if (!trans->entry())
- return;
- }
- DoneWritingToEntry(entry, success);
- } else {
- DoneReadingFromEntry(entry, trans);
+ // Transaction is waiting in the validated_queue.
+ auto i = std::find(entry->validated_queue.begin(),
jkarlin 2017/03/07 16:45:40 There are a few linear searches in this new code.
+ entry->validated_queue.end(), transaction);
+ if (i != entry->validated_queue.end()) {
+ entry->validated_queue.erase(i);
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Can't it be in the validated queue and be the writ
shivanisha 2017/03/08 21:42:12 That should not be possible. validated_queue inser
+ return;
+ }
+
+ // Assume there was a failure.
+ bool success = false;
+ if (transaction == entry->writer && cancel) {
+ DCHECK(entry->disk_entry);
+ // This is a successful operation in the sense that we want to keep the
+ // entry.
+ success = transaction->AddTruncatedFlag();
+ // The previous operation may have deleted the entry.
+ if (!transaction->entry())
+ return;
+ }
+
+ if (transaction == entry->writer ||
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Suggestion: I'd value a comment here, parallel to
shivanisha 2017/03/08 21:42:11 Done.
+ transaction == entry->validating_transaction) {
+ DoneWritingToEntry(entry, success, transaction);
+ return;
}
+
+ DoneReadingFromEntry(entry, transaction);
}
-void HttpCache::DoneWritingToEntry(ActiveEntry* entry, bool success) {
- DCHECK(entry->readers.empty());
+void HttpCache::DoneWritingToEntry(ActiveEntry* entry,
+ bool success,
+ Transaction* transaction) {
+ // Transaction is done writing to entry in the validation phase.
+ if (transaction == entry->validating_transaction) {
+ entry->validating_transaction = nullptr;
- entry->writer = NULL;
+ if (transaction == entry->writer)
+ entry->writer = nullptr;
- if (success) {
- ProcessPendingQueue(entry);
- } else {
- DCHECK(!entry->will_process_pending_queue);
+ if (!entry->writer)
+ DoneWritingToEntryProcessOtherTransactions(entry, success);
+ return;
+ }
- // We failed to create this entry.
- TransactionList pending_queue;
- pending_queue.swap(entry->pending_queue);
+ // Transaction is done writing to entry in the response body phase.
+ DCHECK_EQ(transaction, entry->writer);
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Suggestion: Hoist this up tot he top of the routin
shivanisha 2017/03/08 21:42:11 Done.
+ entry->writer = nullptr;
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 Suggestion: If you take the above suggestion, you
shivanisha 2017/03/08 21:42:12 done
- entry->disk_entry->Doom();
- DestroyEntry(entry);
+ // Whether its a failure or success validating transaction can be the new
+ // writer.
+ if (entry->validating_transaction) {
+ entry->writer = entry->validating_transaction;
+ return;
+ }
- // We need to do something about these pending entries, which now need to
- // be added to a new entry.
- while (!pending_queue.empty()) {
- // ERR_CACHE_RACE causes the transaction to restart the whole process.
- pending_queue.front()->io_callback().Run(ERR_CACHE_RACE);
- pending_queue.pop_front();
- }
+ DoneWritingToEntryProcessOtherTransactions(entry, success);
+}
+
+void HttpCache::DoneWritingToEntryProcessOtherTransactions(ActiveEntry* entry,
+ bool success) {
+ if (success) {
+ DCHECK(entry->readers.empty());
+ ProcessPendingQueue(entry);
+ return;
}
+
+ DestroyEntryRestartPendingTransactions(entry);
}
void HttpCache::DoneReadingFromEntry(ActiveEntry* entry, Transaction* trans) {
@@ -930,17 +1002,30 @@ void HttpCache::DoneReadingFromEntry(ActiveEntry* entry, Transaction* trans) {
ProcessPendingQueue(entry);
}
-void HttpCache::ConvertWriterToReader(ActiveEntry* entry) {
- DCHECK(entry->writer);
- DCHECK(entry->writer->mode() == Transaction::READ_WRITE);
- DCHECK(entry->readers.empty());
+bool HttpCache::ConvertWriterToReader(ActiveEntry* entry,
+ Transaction* transaction) {
+ DCHECK(entry->writer == transaction ||
+ entry->validating_transaction == transaction);
+ DCHECK(transaction->mode() == Transaction::READ_WRITE);
- Transaction* trans = entry->writer;
+ if (transaction == entry->validating_transaction) {
+ entry->validating_transaction = nullptr;
+ if (transaction == entry->writer)
+ entry->writer = nullptr;
+ } else {
+ DCHECK(entry->readers.empty());
+ DCHECK_EQ(transaction, entry->writer);
+ entry->writer = nullptr;
+ }
- entry->writer = NULL;
- entry->readers.insert(trans);
+ // If a reader cannot exist at the moment, it will be added to validated queue
+ // in OnValidationMatch, so return.
+ if (entry->writer)
+ return false;
+ entry->readers.insert(transaction);
ProcessPendingQueue(entry);
+ return true;
}
LoadState HttpCache::GetLoadStateForPendingTransaction(
@@ -990,10 +1075,10 @@ void HttpCache::RemovePendingTransaction(Transaction* trans) {
}
bool HttpCache::RemovePendingTransactionFromEntry(ActiveEntry* entry,
- Transaction* trans) {
+ Transaction* transaction) {
TransactionList& pending_queue = entry->pending_queue;
- auto j = find(pending_queue.begin(), pending_queue.end(), trans);
+ auto j = find(pending_queue.begin(), pending_queue.end(), transaction);
if (j == pending_queue.end())
return false;
@@ -1025,16 +1110,77 @@ void HttpCache::ProcessPendingQueue(ActiveEntry* entry) {
// not delete the entry before OnProcessPendingQueue runs.
if (entry->will_process_pending_queue)
return;
+
entry->will_process_pending_queue = true;
base::ThreadTaskRunnerHandle::Get()->PostTask(
- FROM_HERE,
- base::Bind(&HttpCache::OnProcessPendingQueue, GetWeakPtr(), entry));
+ FROM_HERE, base::Bind(&HttpCache::OnProcessPendingQueue, GetWeakPtr(),
+ entry->disk_entry->GetKey(), entry));
}
-void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
+void HttpCache::OnProcessPendingQueue(const std::string& key,
+ ActiveEntry* entry) {
+ // Check if the entry is still active.
+ if (!IsEntryActive(key, entry))
+ return;
+
entry->will_process_pending_queue = false;
- DCHECK(!entry->writer);
+
+ // If no one is interested in this entry, then we can deactivate it.
+ if (entry->HasNoTransactions()) {
+ DestroyEntry(entry);
+ return;
+ }
+
+ if (entry->validated_queue.empty() && entry->pending_queue.empty())
+ return;
+
+ Transaction* next = nullptr;
+ bool validated_queue = false;
+
+ // To maintain FIFO order of transactions, validated_queue should be processed
+ // first and then pending_queue.
+ if (!entry->validated_queue.empty()) {
+ next = entry->validated_queue.front();
+ validated_queue = true;
+ entry->validated_queue.erase(entry->validated_queue.begin());
+ } else {
+ // Promote next transaction from the pending queue.
+ next = entry->pending_queue.front();
+ if ((next->mode() & Transaction::WRITE) && !entry->readers.empty())
+ return; // Have to wait.
+ entry->pending_queue.erase(entry->pending_queue.begin());
+ }
+
+ int rv = AddTransactionToEntry(entry, next, validated_queue);
+ if (rv != ERR_IO_PENDING) {
+ next->io_callback().Run(rv);
+ }
+}
+
+void HttpCache::ProcessParallelValidation(ActiveEntry* entry) {
+ if (entry->will_process_parallel_validation)
+ return;
+
+ entry->will_process_parallel_validation = true;
+
+ base::ThreadTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE, base::Bind(&HttpCache::OnProcessParallelValidation,
+ GetWeakPtr(), entry->disk_entry->GetKey(), entry));
+}
+
+void HttpCache::OnProcessParallelValidation(const std::string& key,
+ ActiveEntry* entry) {
+ // Check if the entry is still active.
+ if (!IsEntryActive(key, entry))
+ return;
+
+ entry->will_process_parallel_validation = false;
+
+ // A ProcessPendingQueue task before this may add a validating_transaction. In
+ // that case, do nothing.
+ if (entry->validating_transaction)
+ return;
// If no one is interested in this entry, then we can deactivate it.
if (entry->HasNoTransactions()) {
@@ -1045,9 +1191,10 @@ void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
if (entry->pending_queue.empty())
return;
- // Promote next transaction from the pending queue.
Transaction* next = entry->pending_queue.front();
- if ((next->mode() & Transaction::WRITE) && !entry->readers.empty())
+
+ // Read-only transactions cannot proceed to do validation.
+ if (!(next->mode() & Transaction::WRITE) && entry->writer)
return; // Have to wait.
entry->pending_queue.erase(entry->pending_queue.begin());
@@ -1058,6 +1205,39 @@ void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
}
}
+void HttpCache::DoomEntryRestartPendingTransactions(ActiveEntry* entry) {
Randy Smith (Not in Mondays) 2017/03/07 22:54:30 This function and the next one look simple and are
shivanisha 2017/03/08 21:42:11 In the new patch DestroyEntryRestartPendingTransac
+ DoomActiveEntry(entry->disk_entry->GetKey());
+ TransactionList list = GetAllPendingTransactions(entry);
+ RestartPendingTransactions(list);
+}
+
+void HttpCache::DestroyEntryRestartPendingTransactions(ActiveEntry* entry) {
+ entry->disk_entry->Doom();
+ TransactionList list = GetAllPendingTransactions(entry);
+ DestroyEntry(entry);
+ RestartPendingTransactions(list);
+}
+
+HttpCache::TransactionList HttpCache::GetAllPendingTransactions(
+ ActiveEntry* entry) {
+ TransactionList list;
+ for (auto* transaction : entry->validated_queue)
+ list.push_back(transaction);
+ entry->validated_queue.clear();
+
+ for (auto* transaction : entry->pending_queue)
+ list.push_back(transaction);
+ entry->pending_queue.clear();
+
+ return list;
+}
+
+void HttpCache::RestartPendingTransactions(const TransactionList& list) {
+ // ERR_CACHE_RACE causes the transaction to restart the whole process.
+ for (auto* transaction : list)
+ transaction->io_callback().Run(ERR_CACHE_RACE);
+}
+
void HttpCache::OnIOComplete(int result, PendingOp* pending_op) {
WorkItemOperation op = pending_op->writer->operation();

Powered by Google App Engine
This is Rietveld 408576698