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

content/browser/indexed_db/leveldb/leveldb_database.h

 // Copyright (c) 2013 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.
 
 #ifndef CONTENT_BROWSER_INDEXED_DB_LEVELDB_LEVELDB_DATABASE_H_
 #define CONTENT_BROWSER_INDEXED_DB_LEVELDB_LEVELDB_DATABASE_H_
 
 #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"
 
 namespace leveldb {
 class Comparator;
 class DB;
 class FilterPolicy;
 class Env;
 class Snapshot;
@@ skipping 23 matching lines @@
 class CONTENT_EXPORT LevelDBLock {
  public:
   LevelDBLock() {}
   virtual ~LevelDBLock() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(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:
     explicit ComparatorAdapter(const LevelDBComparator* comparator);
 
     int Compare(const leveldb::Slice& a,
                 const leveldb::Slice& b) const override;
 
     const char* Name() const override;
 
     void FindShortestSeparator(std::string* start,
                                const leveldb::Slice& limit) const override;
     void FindShortSuccessor(std::string* key) const override;
 
    private:
     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);
   static std::unique_ptr<LevelDBLock> LockForTesting(
       const base::FilePath& file_name);
   ~LevelDBDatabase() override;
 
   leveldb::Status Put(const base::StringPiece& key, std::string* value);
   leveldb::Status Remove(const base::StringPiece& key);
   virtual leveldb::Status Get(const base::StringPiece& key,
                               std::string* value,
                               bool* found,
                               const LevelDBSnapshot* = 0);
   leveldb::Status Write(const LevelDBWriteBatch& write_batch);
   std::unique_ptr<LevelDBIterator> CreateIterator(const LevelDBSnapshot* = 0);
   const LevelDBComparator* Comparator() const;
   void Compact(const base::StringPiece& start, const base::StringPiece& stop);
   void CompactAll();
 
   // base::trace_event::MemoryDumpProvider implementation.
   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;
 
   void CloseDatabase();
 
   std::unique_ptr<leveldb::Env> env_;
   std::unique_ptr<leveldb::Comparator> comparator_adapter_;
   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;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_INDEXED_DB_LEVELDB_LEVELDB_DATABASE_H_