Disk cache: Remove remaining uses of RankingsNode.pointer.

We now have a map of open entries so we don't need to
do a lookup through the rankings node anymore. This
simplifies the 64 bit version of the code.

BUG=17881
TEST=unittests

Review URL: http://codereview.chromium.org/159643

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22074 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 117 insertions, 46 deletions

diff --git a/net/disk_cache/backend_impl.cc b/net/disk_cache/backend_impl.cc
index 5993187..ea5caf3 100644
--- a/net/disk_cache/backend_impl.cc
+++ b/net/disk_cache/backend_impl.cc
@@ -762,16 +762,16 @@ void BackendImpl::CacheEntryDestroyed(Addr address) {
   DecreaseNumRefs();
 }
 
-bool BackendImpl::IsOpen(CacheRankingsBlock* rankings) const {
+EntryImpl* BackendImpl::GetOpenEntry(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 it->second;
   }
 
-  return false;
+  return NULL;
 }
 
 int32 BackendImpl::GetCurrentEntryId() const {
@@ -1641,7 +1641,7 @@ int BackendImpl::CheckAllEntries() {
 
 bool BackendImpl::CheckEntry(EntryImpl* cache_entry) {
   RankingsNode* rankings = cache_entry->rankings()->Data();
-  return !rankings->pointer;
+  return !rankings->dummy;
 }
 
 }  // namespace disk_cache
diff --git a/net/disk_cache/backend_impl.h b/net/disk_cache/backend_impl.h
index 2ab582d..61639e6 100644
--- a/net/disk_cache/backend_impl.h
+++ b/net/disk_cache/backend_impl.h
@@ -105,9 +105,10 @@ class BackendImpl : public Backend {
   // |address| is the cache address of the entry.
   void CacheEntryDestroyed(Addr address);
 
-  // Returns true if the data stored by the provided |rankings| points to an
-  // open entry, false otherwise.
-  bool IsOpen(CacheRankingsBlock* rankings) const;
+  // If the data stored by the provided |rankings| points to an open entry,
+  // returns a pointer to that entry, otherwise returns NULL. Note that this
+  // method does NOT increase the ref counter for the entry.
+  EntryImpl* GetOpenEntry(CacheRankingsBlock* rankings) const;
 
   // Returns the id being used on this run of the cache.
   int32 GetCurrentEntryId() const;
diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index 31df938..0747d04 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -50,6 +50,7 @@ class DiskCacheBackendTest : public DiskCacheTestWithCache {
   void BackendTrimInvalidEntry();
   void BackendTrimInvalidEntry2();
   void BackendEnumerations();
+  void BackendEnumerations2();
   void BackendInvalidEntryEnumeration();
   void BackendFixEnumerators();
   void BackendDoomRecent();
@@ -656,6 +657,50 @@ TEST_F(DiskCacheBackendTest, MemoryOnlyEnumerations) {
   BackendEnumerations();
 }
 
+// Verifies enumerations while entries are open.
+void DiskCacheBackendTest::BackendEnumerations2() {
+  InitCache();
+  const std::string first("first");
+  const std::string second("second");
+  disk_cache::Entry *entry1, *entry2;
+  ASSERT_TRUE(cache_->CreateEntry(first, &entry1));
+  entry1->Close();
+  ASSERT_TRUE(cache_->CreateEntry(second, &entry2));
+  entry2->Close();
+
+  // Make sure that the timestamp is not the same.
+  PlatformThread::Sleep(20);
+  ASSERT_TRUE(cache_->OpenEntry(second, &entry1));
+  void* iter = NULL;
+  ASSERT_TRUE(cache_->OpenNextEntry(&iter, &entry2));
+  ASSERT_EQ(entry2->GetKey(), second);
+
+  // Two entries and the iterator pointing at "first".
+  entry1->Close();
+  entry2->Close();
+
+  // The iterator should still be valid, se we should not crash.
+  ASSERT_TRUE(cache_->OpenNextEntry(&iter, &entry2));
+  ASSERT_EQ(entry2->GetKey(), first);
+  entry2->Close();
+  cache_->EndEnumeration(&iter);
+}
+
+TEST_F(DiskCacheBackendTest, Enumerations2) {
+  BackendEnumerations2();
+}
+
+TEST_F(DiskCacheBackendTest, NewEvictionEnumerations2) {
+  SetNewEviction();
+  BackendEnumerations2();
+}
+
+TEST_F(DiskCacheBackendTest, MemoryOnlyEnumerations2) {
+  SetMemoryOnlyMode();
+  BackendEnumerations2();
+}
+
+
 // Verify handling of invalid entries while doing enumerations.
 // We'll be leaking memory from this test.
 void DiskCacheBackendTest::BackendInvalidEntryEnumeration() {
diff --git a/net/disk_cache/disk_format.h b/net/disk_cache/disk_format.h
index 8f0843d..619bd89 100644
--- a/net/disk_cache/disk_format.h
+++ b/net/disk_cache/disk_format.h
@@ -154,7 +154,7 @@ struct RankingsNode {
   CacheAddr   prev;             // LRU list.
   CacheAddr   contents;         // Address of the EntryStore.
   int32       dirty;            // The entry is being modifyied.
-  void*       pointer;          // Pointer to the in-memory entry.
+  int32       dummy;            // Old files may have a pointer here.
 };
 #pragma pack(pop)
 
diff --git a/net/disk_cache/entry_impl.cc b/net/disk_cache/entry_impl.cc
index 334eb76..3493152 100644
--- a/net/disk_cache/entry_impl.cc
+++ b/net/disk_cache/entry_impl.cc
@@ -110,12 +110,6 @@ EntryImpl::~EntryImpl() {
             entry_.Data()->data_size[index]);
       }
     }
-    if (node_.HasData() && this == node_.Data()->pointer) {
-      // We have to do this after Flush because we may trigger a cache trim from
-      // there, and technically this entry should be "in use".
-      node_.Data()->pointer = NULL;
-      node_.set_modified();
-    }
 
     if (!ret) {
       // There was a failure writing the actual data. Mark the entry as dirty.
@@ -398,7 +392,6 @@ bool EntryImpl::CreateEntry(Addr node_address, const std::string& key,
 
   entry_store->rankings_node = node_address.value();
   node->contents = entry_.address().value();
-  node->pointer = this;
 
   entry_store->hash = hash;
   entry_store->creation_time = Time::Now().ToInternalValue();
@@ -515,12 +508,8 @@ bool EntryImpl::Update() {
   DCHECK(node_.HasData());
 
   RankingsNode* rankings = node_.Data();
-  if (rankings->pointer) {
-    // Nothing to do here, the entry was in memory.
-    DCHECK(rankings->pointer == this);
-  } else {
+  if (!rankings->dirty) {
     rankings->dirty = backend_->GetCurrentEntryId();
-    rankings->pointer = this;
     if (!node_.Store())
       return false;
   }
@@ -531,7 +520,7 @@ bool EntryImpl::IsDirty(int32 current_id) {
   DCHECK(node_.HasData());
   // We are checking if the entry is valid or not. If there is a pointer here,
   // we should not be checking the entry.
-  if (node_.Data()->pointer)
+  if (node_.Data()->dummy)
     return true;
 
   return node_.Data()->dirty && current_id != node_.Data()->dirty;
@@ -543,7 +532,7 @@ void EntryImpl::ClearDirtyFlag() {
 
 void EntryImpl::SetPointerForInvalidEntry(int32 new_id) {
   node_.Data()->dirty = new_id;
-  node_.Data()->pointer = this;
+  node_.Data()->dummy = 0;
   node_.Store();
 }
 
@@ -892,10 +881,8 @@ void EntryImpl::ReportIOTime(Operation op, const base::Time& start) {
 }
 
 void EntryImpl::Log(const char* msg) {
-  void* pointer = NULL;
   int dirty = 0;
   if (node_.HasData()) {
-    pointer = node_.Data()->pointer;
     dirty = node_.Data()->dirty;
   }
 
@@ -905,7 +892,7 @@ void EntryImpl::Log(const char* msg) {
   Trace("  data: 0x%x 0x%x 0x%x", entry_.Data()->data_addr[0],
         entry_.Data()->data_addr[1], entry_.Data()->long_key);
 
-  Trace("  doomed: %d 0x%p 0x%x", doomed_, pointer, dirty);
+  Trace("  doomed: %d 0x%x", doomed_, dirty);
 }
 
 }  // namespace disk_cache
diff --git a/net/disk_cache/eviction.cc b/net/disk_cache/eviction.cc
index b6d4be4..dfbdc25 100644
--- a/net/disk_cache/eviction.cc
+++ b/net/disk_cache/eviction.cc
@@ -92,7 +92,7 @@ void Eviction::TrimCache(bool empty) {
       break;
     node.reset(next.release());
     next.reset(rankings_->GetPrev(node.get(), Rankings::NO_USE));
-    if (!node->Data()->pointer || empty) {
+    if (node->Data()->dirty != backend_->GetCurrentEntryId() || empty) {
       // This entry is not being used by anybody.
       // Do NOT use node as an iterator after this point.
       rankings_->TrackRankingsBlock(node.get(), false);
@@ -275,7 +275,7 @@ void Eviction::TrimCacheV2(bool empty) {
       node.reset(next[list].release());
       next[list].reset(rankings_->GetPrev(node.get(),
                                           static_cast<Rankings::List>(list)));
-      if (!node->Data()->pointer || empty) {
+      if (node->Data()->dirty != backend_->GetCurrentEntryId() || empty) {
         // This entry is not being used by anybody.
         // Do NOT use node as an iterator after this point.
         rankings_->TrackRankingsBlock(node.get(), false);
@@ -425,7 +425,7 @@ bool Eviction::RemoveDeletedNode(CacheRankingsBlock* node) {
 
   // TODO(rvargas): figure out how to deal with corruption at this point (dirty
   // entries that live in this list).
-  if (node->Data()->pointer) {
+  if (node->Data()->dirty) {
     // We ignore the failure; we're removing the entry anyway.
     entry->Update();
   }
diff --git a/net/disk_cache/rankings.cc b/net/disk_cache/rankings.cc
index b36ee2e..d2250df 100644
--- a/net/disk_cache/rankings.cc
+++ b/net/disk_cache/rankings.cc
@@ -204,10 +204,10 @@ void Rankings::Reset() {
 }
 
 bool Rankings::GetRanking(CacheRankingsBlock* rankings) {
-  Time start = Time::Now();
   if (!rankings->address().is_initialized())
     return false;
 
+  Time start = Time::Now();
   if (!rankings->Load())
     return false;
 
@@ -216,33 +216,45 @@ bool Rankings::GetRanking(CacheRankingsBlock* rankings) {
     return false;
   }
 
-  if (!rankings->Data()->pointer) {
-    backend_->OnEvent(Stats::GET_RANKINGS);
-    return true;
-  }
-
   backend_->OnEvent(Stats::OPEN_RANKINGS);
 
-  if (backend_->GetCurrentEntryId() != rankings->Data()->dirty ||
-      !backend_->IsOpen(rankings)) {
+  // "dummy" is the old "pointer" value, so it has to be 0.
+  if (!rankings->Data()->dirty && !rankings->Data()->dummy)
+    return true;
+
+  EntryImpl* entry = backend_->GetOpenEntry(rankings);
+  if (backend_->GetCurrentEntryId() != rankings->Data()->dirty || !entry) {
     // We cannot trust this entry, but we cannot initiate a cleanup from this
     // point (we may be in the middle of a cleanup already). Just get rid of
     // the invalid pointer and continue; the entry will be deleted when detected
     // from a regular open/create path.
-    rankings->Data()->pointer = NULL;
+    rankings->Data()->dummy = 0;
     rankings->Data()->dirty = backend_->GetCurrentEntryId() - 1;
     if (!rankings->Data()->dirty)
       rankings->Data()->dirty--;
     return true;
   }
 
-  EntryImpl* cache_entry =
-      reinterpret_cast<EntryImpl*>(rankings->Data()->pointer);
-  rankings->SetData(cache_entry->rankings()->Data());
+  // Note that we should not leave this module without deleting rankings first.
+  rankings->SetData(entry->rankings()->Data());
+
   CACHE_UMA(AGE_MS, "GetRankings", 0, start);
   return true;
 }
 
+void Rankings::ConvertToLongLived(CacheRankingsBlock* rankings) {
+  if (rankings->own_data())
+    return;
+
+  // We cannot return a shared node because we are not keeping a reference
+  // to the entry that owns the buffer. Make this node a copy of the one that
+  // we have, and let the iterator logic update it when the entry changes.
+  CacheRankingsBlock temp(NULL, Addr(0));
+  *temp.Data() = *rankings->Data();
+  rankings->StopSharingData();
+  *rankings->Data() = *temp.Data();
+}
+
 void Rankings::Insert(CacheRankingsBlock* node, bool modified, List list) {
   Trace("Insert 0x%x", node->address().value());
   DCHECK(node->HasData());
@@ -414,7 +426,7 @@ void Rankings::CompleteTransaction() {
   if (!node.Load())
     return;
 
-  node.Data()->pointer = NULL;
+  node.Data()->dummy = 0;
   node.Store();
 
   Addr& my_head = heads_[control_data_->operation_list];
@@ -518,6 +530,8 @@ CacheRankingsBlock* Rankings::GetNext(CacheRankingsBlock* node, List list) {
       return NULL;
     next.reset(new CacheRankingsBlock(backend_->File(my_head), my_head));
   } else {
+    if (!node->HasData())
+      node->Load();
     Addr& my_tail = tails_[list];
     if (!my_tail.is_initialized())
       return NULL;
@@ -534,6 +548,7 @@ CacheRankingsBlock* Rankings::GetNext(CacheRankingsBlock* node, List list) {
   if (!GetRanking(next.get()))
     return NULL;
 
+  ConvertToLongLived(next.get());
   if (node && !CheckSingleLink(node, next.get()))
     return NULL;
 
@@ -548,6 +563,8 @@ CacheRankingsBlock* Rankings::GetPrev(CacheRankingsBlock* node, List list) {
       return NULL;
     prev.reset(new CacheRankingsBlock(backend_->File(my_tail), my_tail));
   } else {
+    if (!node->HasData())
+      node->Load();
     Addr& my_head = heads_[list];
     if (!my_head.is_initialized())
       return NULL;
@@ -564,6 +581,7 @@ CacheRankingsBlock* Rankings::GetPrev(CacheRankingsBlock* node, List list) {
   if (!GetRanking(prev.get()))
     return NULL;
 
+  ConvertToLongLived(prev.get());
   if (node && !CheckSingleLink(prev.get(), node))
     return NULL;
 
@@ -642,7 +660,7 @@ void Rankings::WriteTail(List list) {
 }
 
 bool Rankings::CheckEntry(CacheRankingsBlock* rankings) {
-  if (!rankings->Data()->pointer)
+  if (!rankings->Data()->dummy)
     return true;
 
   // If this entry is not dirty, it is a serious problem.
@@ -749,8 +767,7 @@ void Rankings::UpdateIterators(CacheRankingsBlock* node) {
        ++it) {
     if (it->first == address && it->second->HasData()) {
       CacheRankingsBlock* other = it->second;
-      other->Data()->next = node->Data()->next;
-      other->Data()->prev = node->Data()->prev;
+      *other->Data() = *node->Data();
     }
   }
 }
diff --git a/net/disk_cache/rankings.h b/net/disk_cache/rankings.h
index 4e94237..c9fc8d2 100644
--- a/net/disk_cache/rankings.h
+++ b/net/disk_cache/rankings.h
@@ -147,9 +147,14 @@ class Rankings {
   void WriteHead(List list);
   void WriteTail(List list);
 
-  // Gets the rankings information for a given rankings node.
+  // Gets the rankings information for a given rankings node. We may end up
+  // sharing the actual memory with a loaded entry, but we are not taking a
+  // reference to that entry, so |rankings| must be short lived.
   bool GetRanking(CacheRankingsBlock* rankings);
 
+  // Makes |rankings| suitable to live a long life.
+  void ConvertToLongLived(CacheRankingsBlock* rankings);
+
   // Finishes a list modification after a crash.
   void CompleteTransaction();
   void FinishInsert(CacheRankingsBlock* rankings);
diff --git a/net/disk_cache/storage_block-inl.h b/net/disk_cache/storage_block-inl.h
index 909b717..5e026a7 100644
--- a/net/disk_cache/storage_block-inl.h
+++ b/net/disk_cache/storage_block-inl.h
@@ -72,10 +72,16 @@ template<typename T> void  StorageBlock<T>::Discard() {
   DeleteData();
   data_ = NULL;
   modified_ = false;
-  own_data_ = false;
   extended_ = false;
 }
 
+template<typename T> void  StorageBlock<T>::StopSharingData() {
+  if (!data_ || own_data_)
+    return;
+  DCHECK(!modified_);
+  data_ = NULL;
+}
+
 template<typename T> void StorageBlock<T>::set_modified() {
   DCHECK(data_);
   modified_ = true;
@@ -91,6 +97,10 @@ template<typename T> bool StorageBlock<T>::HasData() const {
   return (NULL != data_);
 }
 
+template<typename T> bool StorageBlock<T>::own_data() const {
+  return own_data_;
+}
+
 template<typename T> const Addr StorageBlock<T>::address() const {
   return address_;
 }
diff --git a/net/disk_cache/storage_block.h b/net/disk_cache/storage_block.h
index 78d51db..0d94b82 100644
--- a/net/disk_cache/storage_block.h
+++ b/net/disk_cache/storage_block.h
@@ -50,6 +50,9 @@ class StorageBlock : public FileBlock {
   // own the memory buffer (it cannot be shared).
   void Discard();
 
+  // Stops sharing the data with another object.
+  void StopSharingData();
+
   // Sets the object to lazily save the in-memory data on destruction.
   void set_modified();
 
@@ -59,6 +62,9 @@ class StorageBlock : public FileBlock {
   // Returns true if there is data associated with this object.
   bool HasData() const;
 
+  // Returns true if this object owns the data buffer, false if it is shared.
+  bool own_data() const;
+
   const Addr address() const;
 
   // Loads and store the data.