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

content/browser/indexed_db/leveldb/leveldb_database.h

Index: content/browser/indexed_db/leveldb/leveldb_database.h
diff --git a/content/browser/indexed_db/leveldb/leveldb_database.h b/content/browser/indexed_db/leveldb/leveldb_database.h
index cdcabb54647c83c9d2f56690e2a84ba2fecd87c9..d305c097898560f7a6db14008b903f72a25d09e7 100644
--- a/content/browser/indexed_db/leveldb/leveldb_database.h
+++ b/content/browser/indexed_db/leveldb/leveldb_database.h
@@ -8,11 +8,13 @@
 #include <memory>
 #include <string>
 
+#include "base/containers/mru_cache.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece.h"
 #include "base/trace_event/memory_dump_provider.h"
+#include "content/browser/indexed_db/leveldb/leveldb_iterator_pool_controller.h"
 #include "content/common/content_export.h"
 #include "third_party/leveldatabase/src/include/leveldb/comparator.h"
 #include "third_party/leveldatabase/src/include/leveldb/status.h"
@@ -56,7 +58,8 @@ class CONTENT_EXPORT LevelDBLock {
 };
 
 class CONTENT_EXPORT LevelDBDatabase
-    : public base::trace_event::MemoryDumpProvider {
+    : public base::trace_event::MemoryDumpProvider,
+      NON_EXPORTED_BASE(public LevelDBIteratorPoolController) {

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

I'm not a big fan of the interface and extra name here. It seems reasonable to
me that the iterator would have a pointer/reference to its database and that
it'd call the methods on it directly.

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

That sounds fine to me - just trying to make the interface 'clean', but I see
how having more interfaces can also be more confusing.

  public:
   class ComparatorAdapter : public leveldb::Comparator {
    public:
@@ -75,10 +78,15 @@ class CONTENT_EXPORT LevelDBDatabase
     const LevelDBComparator* comparator_;
   };
 
+  enum { NO_CURSOR_EVICTION = 0 };

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

It seems to me that this option is only used for tests (directly in
content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc,
indirectly via LevelDBDatabase::OpenInMemory).

How about setting a large enough limit in that test, sticking with the default
for in-memory tests, and removing all the logic associated with not having
eviction?

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

Based on offline conversation, doing it so the behavior doesn't whether we're in
memory or not. Removed ability to disable cursor eviction.

+
+  // Set |max_open_cursors| to NO_CURSOR_EVICTION to disable eviction.
   static leveldb::Status Open(const base::FilePath& file_name,
                               const LevelDBComparator* comparator,
                               std::unique_ptr<LevelDBDatabase>* db,
+                              size_t max_open_cursors,
                               bool* is_disk_full = 0);
+
   static std::unique_ptr<LevelDBDatabase> OpenInMemory(
       const LevelDBComparator* comparator);
   static leveldb::Status Destroy(const base::FilePath& file_name);
@@ -102,8 +110,14 @@ class CONTENT_EXPORT LevelDBDatabase
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
+  // Methods for iterator pooling.
+  std::unique_ptr<leveldb::Iterator> CreateLevelDBIterator(

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

Would it make sense to have these be private and add IteratorImpl as a friend
class?

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

Sure.

+      const leveldb::Snapshot*) override;
+  void NotifyIteratorUsed(LevelDBIterator*) override;
+  void NotifyIteratorDestroyed(LevelDBIterator*) override;
+
  protected:
-  LevelDBDatabase();
+  LevelDBDatabase(size_t max_open_iterators);
 
  private:
   friend class LevelDBSnapshot;
@@ -115,6 +129,14 @@ class CONTENT_EXPORT LevelDBDatabase
   std::unique_ptr<leveldb::DB> db_;
   std::unique_ptr<const leveldb::FilterPolicy> filter_policy_;
   const LevelDBComparator* comparator_;
+
+  // Despite the type name, this object uses LRU eviction.
+  bool eviction_enabled_ = false;
+  base::MRUCache<LevelDBIterator*, LevelDBIterator*> iterator_lru_;

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

Would it make sense to use a HashingMRUCache here?

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

Sure.

+  // Recorded for UMA reporting.
+  uint32_t num_iterators_ = 0;
+  uint32_t max_iterators_ = 0;
+
   std::string file_name_for_tracing;
 };