[IndexedDB] Pool and evict leveldb iterators, to save memory

content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc

Index: content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc
diff --git a/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc b/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc
index 2052f437168c65e580b49211ec45ff32d97d5384..098837a3c8cbedae1992dc2afba24257fd5b4f43 100644
--- a/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc
+++ b/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc
@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/logging.h"
+#include "content/browser/indexed_db/leveldb/leveldb_iterator_pool_controller.h"
 
 static leveldb::Slice MakeSlice(const base::StringPiece& s) {
   return leveldb::Slice(s.begin(), s.size());
@@ -22,26 +23,37 @@ namespace content {
 LevelDBIteratorImpl::~LevelDBIteratorImpl() {
 }
 
-LevelDBIteratorImpl::LevelDBIteratorImpl(std::unique_ptr<leveldb::Iterator> it)
-    : iterator_(std::move(it)) {}
+LevelDBIteratorImpl::LevelDBIteratorImpl(
+    std::unique_ptr<leveldb::Iterator> it,
+    LevelDBIteratorPoolController* pool_controller,
+    const leveldb::Snapshot* snapshot)
+    : iterator_(std::move(it)),
+      pool_controller_(pool_controller),
+      snapshot_(snapshot) {}
 
 void LevelDBIteratorImpl::CheckStatus() {
+  DCHECK_EQ(iterator_state_, IteratorState::ACTIVE);
   const leveldb::Status& s = iterator_->status();
   if (!s.ok())
     LOG(ERROR) << "LevelDB iterator error: " << s.ToString();
 }
 
 bool LevelDBIteratorImpl::IsValid() const {
-  return iterator_->Valid();
+  return iterator_state_ == IteratorState::EVICTED_AND_VALID ||
+         iterator_->Valid();
 }
 
 leveldb::Status LevelDBIteratorImpl::SeekToLast() {
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
   iterator_->SeekToLast();
   CheckStatus();
   return iterator_->status();
 }
 
 leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
   iterator_->Seek(MakeSlice(target));
   CheckStatus();
   return iterator_->status();
@@ -49,6 +61,8 @@ leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
 
 leveldb::Status LevelDBIteratorImpl::Next() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
   iterator_->Next();
   CheckStatus();
   return iterator_->status();
@@ -56,6 +70,8 @@ leveldb::Status LevelDBIteratorImpl::Next() {
 
 leveldb::Status LevelDBIteratorImpl::Prev() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
   iterator_->Prev();
   CheckStatus();
   return iterator_->status();
@@ -68,7 +84,44 @@ base::StringPiece LevelDBIteratorImpl::Key() const {
 
 base::StringPiece LevelDBIteratorImpl::Value() const {
   DCHECK(IsValid());
+  // Always need to update the LRU, so we always call this. Const-cast needed,
+  // as we're dealing with a caching layer.

[jsbell, 2017/03/23 21:48:21]

Can you see if making iterator_state_, key_before_eviction_, and iterator_
mutable is enough to avoid the const_cast ?

(They achieve the same thing, so whichever ends up being the most readable)

[dmurph, 2017/03/23 22:48:47]

Since this would require making all member variables (except snapshot - which is
4/5) mutable, I'm going to stick with const_cast.

+  LevelDBIteratorImpl* non_const = const_cast<LevelDBIteratorImpl*>(this);
+  non_const->RecordUsage();
+  non_const->RestoreDBIteratorIfEvicted();
   return MakeStringPiece(iterator_->value());
 }
 
+void LevelDBIteratorImpl::PurgeMemory() {
+  DCHECK_EQ(iterator_state_, IteratorState::ACTIVE);
+  if (iterator_->Valid()) {
+    iterator_state_ = IteratorState::EVICTED_AND_VALID;
+    key_before_eviction_ = iterator_->key().ToString();
+  } else {
+    iterator_state_ = IteratorState::EVICTED_AND_INVALID;
+  }
+  iterator_.reset();
+}
+
+bool LevelDBIteratorImpl::IsEvicted() const {
+  return iterator_state_ != IteratorState::ACTIVE;
+}
+
+void LevelDBIteratorImpl::RecordUsage() {
+  pool_controller_->NotifyIteratorUsed(this);
+}
+
+void LevelDBIteratorImpl::RestoreDBIteratorIfEvicted() {
+  if (iterator_state_ == IteratorState::ACTIVE)
+    return;
+
+  iterator_ = pool_controller_->CreateLevelDBIterator(snapshot_);
+  if (iterator_state_ == IteratorState::EVICTED_AND_VALID) {
+    iterator_->Seek(key_before_eviction_);
+    key_before_eviction_.clear();
+    DCHECK(IsValid());
+  }

[jsbell, 2017/03/23 21:48:21]

else clause that DCHECK(!IsValid()) ?

[dmurph, 2017/03/23 22:48:47]

tautology - we have to be in state EVICTED_AND_INVALID in the else clause - so
it would always return true.

[jsbell, 2017/03/24 00:30:21]

Oops - I meant DCHECK(!iterator_->Valid())

+  iterator_state_ = IteratorState::ACTIVE;
+}
+
 }  // namespace content