IndexedDB: Optimize range deletion operations (e.g. clearing a store)

content/browser/indexed_db/leveldb/leveldb_transaction.cc

Index: content/browser/indexed_db/leveldb/leveldb_transaction.cc
diff --git a/content/browser/indexed_db/leveldb/leveldb_transaction.cc b/content/browser/indexed_db/leveldb/leveldb_transaction.cc
index 016e7dea72b90ed23f1247f28813119d1f615f56..77258585df46c73de81b7f423bd960fa8bc1ccae 100644
--- a/content/browser/indexed_db/leveldb/leveldb_transaction.cc
+++ b/content/browser/indexed_db/leveldb/leveldb_transaction.cc
@@ -22,15 +22,14 @@ LevelDBTransaction::LevelDBTransaction(LevelDBDatabase* db)
       snapshot_(db),
       comparator_(db->Comparator()),
       data_comparator_(comparator_),
-      data_(data_comparator_),
-      finished_(false) {}
+      data_(data_comparator_) {}
 
-LevelDBTransaction::Record::Record() : deleted(false) {}
+LevelDBTransaction::Record::Record() {}
 LevelDBTransaction::Record::~Record() {}
 
 LevelDBTransaction::~LevelDBTransaction() {}
 
-bool LevelDBTransaction::Set(const StringPiece& key,
+void LevelDBTransaction::Set(const StringPiece& key,
                              std::string* value,
                              bool deleted) {
   DCHECK(!finished_);
@@ -43,21 +42,47 @@ bool LevelDBTransaction::Set(const StringPiece& key,
     record->deleted = deleted;
     data_[record->key] = std::move(record);
     NotifyIterators();
-    return false;
+    return;
   }
-  bool replaced_deleted_value = it->second->deleted;
   it->second->value.swap(*value);
   it->second->deleted = deleted;
-  return replaced_deleted_value;
 }
 
 void LevelDBTransaction::Put(const StringPiece& key, std::string* value) {
   Set(key, value, false);
 }
 
-bool LevelDBTransaction::Remove(const StringPiece& key) {
+void LevelDBTransaction::Remove(const StringPiece& key) {
   std::string empty;
-  return !Set(key, &empty, true);
+  Set(key, &empty, true);
+}
+
+leveldb::Status LevelDBTransaction::RemoveRange(const StringPiece& begin,
+                                                const StringPiece& end,
+                                                bool upper_open) {
+  leveldb::Status s;
+  bool dirty = false;
+  {
+    // Scope this iterator so it is deleted before other iterators are
+    // notified.
+    std::unique_ptr<TransactionIterator> it = TransactionIterator::Create(this);
+    for (s = it->Seek(begin);
+         s.ok() && it->IsValid() &&
+         (upper_open ? comparator_->Compare(it->Key(), end) < 0

[dmurph, 2017/03/01 20:06:35]

Can you pull this out into a static helper method? It's really hard to
understand in it's current form, and it could benefit from a nice descriptive
method name.

[jsbell, 2017/03/01 22:45:25]

Done. Feedback welcome!

+                     : comparator_->Compare(it->Key(), end) <= 0);
+         s = it->Next()) {
+      if (!it->DeleteData()) {
+        std::unique_ptr<Record> record = base::MakeUnique<Record>();
+        record->key = it->Key().as_string();
+        record->deleted = true;
+        data_[record->key] = std::move(record);
+      }
+      dirty = true;
+    }
+  }
+  if (dirty)
+    NotifyIterators();
+  return s;
 }
 
 leveldb::Status LevelDBTransaction::Get(const StringPiece& key,
@@ -178,6 +203,12 @@ bool LevelDBTransaction::DataIterator::IsDeleted() const {
   return iterator_->second->deleted;
 }
 
+void LevelDBTransaction::DataIterator::Delete() {
+  DCHECK(IsValid());
+  iterator_->second->deleted = true;
+  iterator_->second->value.clear();
+}
+
 LevelDBTransaction::DataIterator::~DataIterator() {}
 
 LevelDBTransaction::DataIterator::DataIterator(LevelDBTransaction* transaction)
@@ -195,10 +226,8 @@ LevelDBTransaction::TransactionIterator::TransactionIterator(
     : transaction_(transaction),
       comparator_(transaction_->comparator_),
       data_iterator_(DataIterator::Create(transaction_.get())),
-      db_iterator_(transaction_->db_->CreateIterator(&transaction_->snapshot_)),
-      current_(0),
-      direction_(FORWARD),
-      data_changed_(false) {
+      db_iterator_(
+          transaction_->db_->CreateIterator(&transaction_->snapshot_)) {
   transaction_->RegisterIterator(this);
 }
 
@@ -331,6 +360,15 @@ void LevelDBTransaction::TransactionIterator::DataChanged() {
   data_changed_ = true;
 }
 
+bool LevelDBTransaction::TransactionIterator::DeleteData() {
+  DCHECK(IsValid());
+  if (current_ == data_iterator_.get()) {
+    data_iterator_->Delete();
+    return true;
+  }
+  return false;
+}
+
 void LevelDBTransaction::TransactionIterator::RefreshDataIterator() const {
   DCHECK(data_changed_);
 
@@ -403,7 +441,7 @@ void LevelDBTransaction::TransactionIterator::HandleConflictsAndDeletes() {
 
 void
 LevelDBTransaction::TransactionIterator::SetCurrentIteratorToSmallestKey() {
-  LevelDBIterator* smallest = 0;
+  LevelDBIterator* smallest = nullptr;
 
   if (data_iterator_->IsValid())
     smallest = data_iterator_.get();
@@ -418,7 +456,7 @@ LevelDBTransaction::TransactionIterator::SetCurrentIteratorToSmallestKey() {
 }
 
 void LevelDBTransaction::TransactionIterator::SetCurrentIteratorToLargestKey() {
-  LevelDBIterator* largest = 0;
+  LevelDBIterator* largest = nullptr;
 
   if (data_iterator_->IsValid())
     largest = data_iterator_.get();
@@ -453,7 +491,7 @@ std::unique_ptr<LevelDBDirectTransaction> LevelDBDirectTransaction::Create(
 }
 
 LevelDBDirectTransaction::LevelDBDirectTransaction(LevelDBDatabase* db)
-    : db_(db), write_batch_(LevelDBWriteBatch::Create()), finished_(false) {}
+    : db_(db), write_batch_(LevelDBWriteBatch::Create()) {}
 
 LevelDBDirectTransaction::~LevelDBDirectTransaction() {
   write_batch_->Clear();