[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..b06f23565de52b347fddf2ee381cad990baf35e3 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_database.h"
 
 static leveldb::Slice MakeSlice(const base::StringPiece& s) {
   return leveldb::Slice(s.begin(), s.size());
@@ -20,28 +21,37 @@ static base::StringPiece MakeStringPiece(const leveldb::Slice& s) {
 namespace content {
 
 LevelDBIteratorImpl::~LevelDBIteratorImpl() {
+  db_->NotifyIteratorDestroyed(this);
 }
 
-LevelDBIteratorImpl::LevelDBIteratorImpl(std::unique_ptr<leveldb::Iterator> it)
-    : iterator_(std::move(it)) {}
+LevelDBIteratorImpl::LevelDBIteratorImpl(std::unique_ptr<leveldb::Iterator> it,
+                                         LevelDBDatabase* db,
+                                         const leveldb::Snapshot* snapshot)
+    : iterator_(std::move(it)), db_(db), snapshot_(snapshot) {}
 
 void LevelDBIteratorImpl::CheckStatus() {
+  DCHECK(!IsDetached());
   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() {
+  WillUseDBIterator();
+  DCHECK(iterator_);
   iterator_->SeekToLast();
   CheckStatus();

[pwnall, 2017/03/27 17:08:04]

I don't know how accepted this is in Chromium / google3... but if you're
returning iterator_->status() every time after you call CheckStatus(), would it
make sense to have CheckStatus() return the status, so you can use "return
CheckStatus()"?

[dmurph, 2017/03/27 21:32:05]

sure.

   return iterator_->status();
 }
 
 leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
+  WillUseDBIterator();
+  DCHECK(iterator_);
   iterator_->Seek(MakeSlice(target));
   CheckStatus();
   return iterator_->status();
@@ -49,6 +59,8 @@ leveldb::Status LevelDBIteratorImpl::Seek(const base::StringPiece& target) {
 
 leveldb::Status LevelDBIteratorImpl::Next() {
   DCHECK(IsValid());
+  WillUseDBIterator();
+  DCHECK(iterator_);
   iterator_->Next();
   CheckStatus();
   return iterator_->status();
@@ -56,6 +68,8 @@ leveldb::Status LevelDBIteratorImpl::Next() {
 
 leveldb::Status LevelDBIteratorImpl::Prev() {
   DCHECK(IsValid());
+  WillUseDBIterator();
+  DCHECK(iterator_);
   iterator_->Prev();
   CheckStatus();
   return iterator_->status();
@@ -63,12 +77,49 @@ leveldb::Status LevelDBIteratorImpl::Prev() {
 
 base::StringPiece LevelDBIteratorImpl::Key() const {
   DCHECK(IsValid());
+  if (IsDetached())
+    return key_before_eviction_;
   return MakeStringPiece(iterator_->key());
 }
 
 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/27 15:35:34]

nit: dealing with -> implementing  ??

[dmurph, 2017/03/27 21:32:05]

Done.

+  LevelDBIteratorImpl* non_const = const_cast<LevelDBIteratorImpl*>(this);
+  non_const->WillUseDBIterator();
   return MakeStringPiece(iterator_->value());
 }
 
+void LevelDBIteratorImpl::Detach() {
+  DCHECK(!IsDetached());
+  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::IsDetached() const {
+  return iterator_state_ != IteratorState::ACTIVE;
+}
+
+void LevelDBIteratorImpl::WillUseDBIterator() {
+  db_->NotifyIteratorUsed(this);
+  if (!IsDetached())
+    return;
+
+  iterator_ = db_->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