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