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

content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/indexed_db/leveldb/leveldb_iterator_impl.h"
 
 #include <memory>
 #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());
 }
 
 static base::StringPiece MakeStringPiece(const leveldb::Slice& s) {
   return base::StringPiece(s.data(), s.size());
 }
 
 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();
 }
 
 leveldb::Status LevelDBIteratorImpl::Next() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
+  DCHECK(iterator_);
   iterator_->Next();
   CheckStatus();
   return iterator_->status();
 }
 
 leveldb::Status LevelDBIteratorImpl::Prev() {
   DCHECK(IsValid());
+  RecordUsage();
+  RestoreDBIteratorIfEvicted();
+  DCHECK(iterator_);
   iterator_->Prev();
   CheckStatus();
   return iterator_->status();
 }
 
 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