Chromium Code Reviews| 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(); |