Disk cache: Keep a map of all open entries....

net/disk_cache/backend_impl.cc

Index: net/disk_cache/backend_impl.cc
===================================================================
--- net/disk_cache/backend_impl.cc	(revision 19788)
+++ net/disk_cache/backend_impl.cc	(working copy)
@@ -426,6 +426,9 @@
     return false;
   }
 
+  // We are not failing the operation; let's add this to the map.
+  open_entries_[entry_address.value()] = cache_entry;
+
   if (parent.get())
     parent->SetNextAddress(entry_address);
 
@@ -720,11 +723,26 @@
   DecreaseNumEntries();
 }
 
-void BackendImpl::CacheEntryDestroyed() {
+void BackendImpl::CacheEntryDestroyed(Addr address) {
+  EntriesMap::iterator it = open_entries_.find(address.value());

[Nicolas Sylvain, 2009/07/07 17:47:14]

why would it not be in the map? dirty maybe? should we dcheck when it's not?

[rvargas (doing something else), 2009/07/07 18:13:24]

Yes and no. We don't store dirty entries in the map, but it should be a frequent
thing to open dirty entries so we cannot dcheck here (the dirty flag is cleared
before the destruction of the entry)

+  if (it != open_entries_.end())
+    open_entries_.erase(it);
   DecreaseNumRefs();
 }
 
-int32 BackendImpl::GetCurrentEntryId() {
+bool BackendImpl::IsOpen(CacheRankingsBlock* rankings) const {
+  DCHECK(rankings->HasData());
+  EntriesMap::const_iterator it =
+      open_entries_.find(rankings->Data()->contents);
+  if (it != open_entries_.end()) {
+    // We have this entry in memory.
+    return rankings->Data()->pointer == it->second;
+  }
+
+  return false;
+}
+
+int32 BackendImpl::GetCurrentEntryId() const {
   return data_->header.this_id;
 }
 
@@ -1035,6 +1053,16 @@
 }
 
 int BackendImpl::NewEntry(Addr address, EntryImpl** entry, bool* dirty) {
+  EntriesMap::iterator it = open_entries_.find(address.value());
+  if (it != open_entries_.end()) {
+    // Easy job. This entry is already in memory.
+    EntryImpl* this_entry = it->second;
+    this_entry->AddRef();
+    *entry = this_entry;
+    *dirty = false;
+    return 0;
+  }
+
   scoped_refptr<EntryImpl> cache_entry(new EntryImpl(this, address));
   IncreaseNumRefs();
   *entry = NULL;
@@ -1064,6 +1092,10 @@
   if (!rankings_.SanityCheck(cache_entry->rankings(), false))
     return ERR_INVALID_LINKS;
 
+  // We only add clean entries to the map.
+  if (!*dirty)
+    open_entries_[address.value()] = cache_entry;
+
   cache_entry.swap(entry);
   return 0;
 }
@@ -1119,11 +1151,17 @@
     }
 
     if (cache_entry->IsSameEntry(key, hash)) {
-      cache_entry = EntryImpl::Update(cache_entry);
+      if (!cache_entry->Update()) {
+        cache_entry->Release();
+        cache_entry = NULL;
+      }
       found = true;
       break;
     }
-    cache_entry = EntryImpl::Update(cache_entry);
+    if (!cache_entry->Update()) {
+      cache_entry->Release();
+      cache_entry = NULL;

[Nicolas Sylvain, 2009/07/07 17:47:14]

do you really need this here? since you do it below too. Well, i guess it makes
it clearer.

[rvargas (doing something else), 2009/07/07 18:13:24]

if I don't set it to null, we'll copy an invalid pointer into parent_entry. I
guess that now we have more calls to Release() than assignments to plain
pointers so I could see how this function looks after replacing cache_entry and
parent_entry with scoped pointers, but I'll leave that for another cl.

+    }
     if (parent_entry)
       parent_entry->Release();
     parent_entry = cache_entry;
@@ -1279,9 +1317,11 @@
 
     return NULL;
   }
+  if (!entry->Update())
+    return NULL;
 
   entry.swap(&temp);
-  return EntryImpl::Update(temp);  // Update returns an adref'd entry.
+  return temp;
 }
 
 bool BackendImpl::ResurrectEntry(EntryImpl* deleted_entry, Entry** entry) {