[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..3274dcd5d996382b5581ed16875e8378fbb5f979 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());
@@ -20,28 +21,42 @@ static base::StringPiece MakeStringPiece(const leveldb::Slice& s) {
 namespace content {
 
 LevelDBIteratorImpl::~LevelDBIteratorImpl() {
+  pool_controller_->NotifyIteratorDestroyed(this);
 }
 
-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();

[pwnall, 2017/03/24 09:16:03]

I see these two methods together everywhere. Would it make sense to group them
together in a single method that'd be either EnsureIteratorNotEvicted() or
WillUseDBIterator()?

It makes sense to me that the two concepts are coupled -- whenever you'd want to
use an iterator, you need to page it in if it's evicted, and you need to bump it
to the front of the LRU list if it's not evicted.

[dmurph, 2017/03/24 23:33:39]

Done.

+  DCHECK(iterator_);
   iterator_->SeekToLast();
   CheckStatus();
   return iterator_->status();
 }
 
 leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
+  DCHECK(iterator_);
   iterator_->Seek(MakeSlice(target));
   CheckStatus();
   return iterator_->status();
@@ -49,6 +64,9 @@ leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
 
 leveldb::Status LevelDBIteratorImpl::Next() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
+  DCHECK(iterator_);
   iterator_->Next();
   CheckStatus();
   return iterator_->status();
@@ -56,6 +74,9 @@ leveldb::Status LevelDBIteratorImpl::Next() {
 
 leveldb::Status LevelDBIteratorImpl::Prev() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
+  DCHECK(iterator_);
   iterator_->Prev();
   CheckStatus();
   return iterator_->status();
@@ -63,12 +84,53 @@ leveldb::Status LevelDBIteratorImpl::Prev() {
 
 base::StringPiece LevelDBIteratorImpl::Key() const {
   DCHECK(IsValid());
-  return MakeStringPiece(iterator_->key());
+  if (iterator_state_ == IteratorState::ACTIVE)
+    return MakeStringPiece(iterator_->key());
+  return key_before_eviction_;
 }
 
 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.
+  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 {

[pwnall, 2017/03/24 09:16:03]

It's surprising to me that you sometimes call this and sometimes compare the
state directly with IteratorState::ACTIVE. Can you migrate all the code to one
or the other?

Note: the implicit comparison in DCHECK_EQ makes sense even if you switch to
IsEvicted() everywhere, because it results in better diagnostic messages.

[dmurph, 2017/03/24 23:33:39]

Done.

+  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());
+  } else {
+    DCHECK(!iterator_->Valid());
+  }
+  iterator_state_ = IteratorState::ACTIVE;
+}
+
 }  // namespace content