Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(190)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2712713005: Revert of IndexedDB: Optimize range deletion operations (e.g. clearing a store) (Closed)

Created:
3 years, 10 months ago by Marc Treib
Modified:
3 years, 10 months ago
Reviewers:
jsbell, cmumford, dmurph
CC:
chromium-reviews, jam, jsbell+idb_chromium.org, darin-cc_chromium.org, cmumford
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of IndexedDB: Optimize range deletion operations (e.g. clearing a store) (patchset #1 id:1 of https://codereview.chromium.org/2708223002/ ) Reason for revert: Suspected cause for lots of indexeddb webkit_tests failures, e.g. on the following builders: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty%20MSAN https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7 https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29 Original issue's description: > IndexedDB: Optimize range deletion operations (e.g. clearing a store) > > Range deletion in leveldb involves walking over the range to identify > keys to remove. The wrapper class used by IndexedDB to implement > transactions was applying this same logic, which was inefficient due > to (1) complexity in walking two iterators (one for the uncommitted > transaction data and one for the backing store data) and (2) notifying > iterators for every record. > > This was optimized by clearing the range in the uncommitted data > first, then simply walking the range in the backing store, and > deferring notifying iterators until the end. This required removing > the |delete_count| out param plumbed through various methods, but it > was only used in tests. > > On a test for clearing a store with 100k index entries, a 4x speedup > was observed. > > BUG=693799 > TEST=content_unittests --gtest_filter='LevelDBTransactionTest.*' > > Review-Url: https://codereview.chromium.org/2708223002 > Cr-Commit-Position: refs/heads/master@{#452324} > Committed: https://chromium.googlesource.com/chromium/src/+/339d49e89f42347c7b85ac31cb0e9ebd8163025d TBR=cmumford@chromium.org,dmurph@chromium.org,jsbell@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=693799

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+227 lines, -432 lines) Patch
M content/browser/indexed_db/indexed_db_backing_store.h View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/indexed_db/indexed_db_backing_store.cc View 10 chunks +37 lines, -12 lines 0 comments Download
M content/browser/indexed_db/indexed_db_backing_store_unittest.cc View 4 chunks +22 lines, -10 lines 0 comments Download
M content/browser/indexed_db/indexed_db_database.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_transaction.h View 2 chunks +4 lines, -15 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_transaction.cc View 2 chunks +1 line, -24 lines 0 comments Download
D content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc View 1 chunk +0 lines, -368 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_unittest.cc View 2 chunks +159 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 7 (2 generated)
Marc Treib
Created Revert of IndexedDB: Optimize range deletion operations (e.g. clearing a store)
3 years, 10 months ago (2017-02-23 09:43:16 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2712713005/1
3 years, 10 months ago (2017-02-23 09:43:33 UTC) #3
commit-bot: I haz the power
Failed to apply patch for content/browser/indexed_db/indexed_db_backing_store.cc: While running git apply --index -p1; error: patch failed: ...
3 years, 10 months ago (2017-02-23 09:44:14 UTC) #5
Marc Treib
On 2017/02/23 09:44:14, commit-bot: I haz the power wrote: > Failed to apply patch for ...
3 years, 10 months ago (2017-02-23 10:02:59 UTC) #6
jsbell
3 years, 10 months ago (2017-02-23 17:26:41 UTC) #7
Message was sent while issue was closed. 
On 2017/02/23 10:02:59, Marc Treib wrote:
> On 2017/02/23 09:44:14, commit-bot: I haz the power wrote:
> > Failed to apply patch for
> > content/browser/indexed_db/indexed_db_backing_store.cc:
> > While running git apply --index -p1;
> >   error: patch failed:
> > content/browser/indexed_db/indexed_db_backing_store.cc:699
> >   error: content/browser/indexed_db/indexed_db_backing_store.cc: patch does
> not
> > apply
> > 
> > Patch:       content/browser/indexed_db/indexed_db_backing_store.cc
> > Index: content/browser/indexed_db/indexed_db_backing_store.cc
> > diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc
> > b/content/browser/indexed_db/indexed_db_backing_store.cc
> > index
> >
>
29934dc7c94e5b22ca6ed9a50dd4720ab839b083..ff549bbaaa137614e37efb46e097efa13047fc85
> > 100644
> > --- a/content/browser/indexed_db/indexed_db_backing_store.cc
> > +++ b/content/browser/indexed_db/indexed_db_backing_store.cc
> > @@ -699,8 +699,20 @@
> >  Status DeleteRangeBasic(LevelDBTransaction* transaction,
> >                          const std::string& begin,
> >                          const std::string& end,
> > -                        bool upper_open) {
> > -  return transaction->RemoveRange(begin, end, upper_open);
> > +                        bool upper_open,
> > +                        size_t* delete_count) {
> > +  DCHECK(delete_count);
> > +  std::unique_ptr<LevelDBIterator> it = transaction->CreateIterator();
> > +  Status s;
> > +  *delete_count = 0;
> > +  for (s = it->Seek(begin); s.ok() && it->IsValid() &&
> > +                            (upper_open ? CompareKeys(it->Key(), end) < 0
> > +                                        : CompareKeys(it->Key(), end) <=
0);
> > +       s = it->Next()) {
> > +    if (transaction->Remove(it->Key()))
> > +      (*delete_count)++;
> > +  }
> > +  return s;
> >  }
> >  
> >  Status DeleteBlobsInRange(IndexedDBBackingStore::Transaction* transaction,
> > @@ -1988,6 +2000,7 @@
> >    LevelDBTransaction* leveldb_transaction = transaction->transaction();
> >  
> >    base::string16 object_store_name;
> > +  size_t delete_count = 0;
> >    bool found = false;
> >    Status s = GetString(leveldb_transaction, ObjectStoreMetaDataKey::Encode(
> >                                                  database_id,
object_store_id,
> > @@ -2011,7 +2024,8 @@
> >    s = DeleteRangeBasic(
> >        leveldb_transaction,
> >        ObjectStoreMetaDataKey::Encode(database_id, object_store_id, 0),
> > -      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id),
> > true);
> > +      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id),
> true,
> > +      &delete_count);
> >  
> >    if (s.ok()) {
> >      leveldb_transaction->Remove(
> > @@ -2020,14 +2034,16 @@
> >      s = DeleteRangeBasic(
> >          leveldb_transaction,
> >          IndexFreeListKey::Encode(database_id, object_store_id, 0),
> > -        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id),
true);
> > +        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id), true,
> > +        &delete_count);
> >    }
> >  
> >    if (s.ok()) {
> >      s = DeleteRangeBasic(
> >          leveldb_transaction,
> >          IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0),
> > -        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id),
true);
> > +        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id), true,
> > +        &delete_count);
> >    }
> >  
> >    if (!s.ok()) {
> > @@ -2174,8 +2190,9 @@
> >        KeyPrefix(database_id, object_store_id).Encode();
> >    const std::string stop_key =
> >        KeyPrefix(database_id, object_store_id + 1).Encode();
> > -  Status s =
> > -      DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
> true);
> > +  size_t delete_count = 0;
> > +  Status s = DeleteRangeBasic(transaction->transaction(), start_key,
> stop_key,
> > +                              true, &delete_count);
> >    if (!s.ok()) {
> >      INTERNAL_WRITE_ERROR(CLEAR_OBJECT_STORE);
> >      return s;
> > @@ -2211,8 +2228,11 @@
> >      IndexedDBBackingStore::Transaction* transaction,
> >      int64_t database_id,
> >      int64_t object_store_id,
> > -    const IndexedDBKeyRange& key_range) {
> > +    const IndexedDBKeyRange& key_range,
> > +    size_t* exists_delete_count) {
> > +  DCHECK(exists_delete_count);
> >    Status s;
> > +  *exists_delete_count = 0;
> >    std::unique_ptr<IndexedDBBackingStore::Cursor> start_cursor =
> >        OpenObjectStoreCursor(transaction, database_id, object_store_id,
> >                              key_range, blink::WebIDBCursorDirectionNext,
&s);
> > @@ -2249,7 +2269,9 @@
> >                           false);
> >    if (!s.ok())
> >      return s;
> > -  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
> false);
> > +  size_t data_delete_count = 0;
> > +  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
> false,
> > +                       &data_delete_count);
> >    if (!s.ok())
> >      return s;
> >    start_key =
> > @@ -2257,7 +2279,9 @@
> >    stop_key =
> >        ExistsEntryKey::Encode(database_id, object_store_id,
> end_cursor->key());
> >  
> > -  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
> false);
> > +  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
> false,
> > +                       exists_delete_count);
> > +  DCHECK_EQ(data_delete_count, *exists_delete_count);
> >    return s;
> >  }
> >  
> > @@ -3005,12 +3029,13 @@
> >      return InvalidDBKeyStatus();
> >    LevelDBTransaction* leveldb_transaction = transaction->transaction();
> >  
> > +  size_t delete_count = 0;
> >    const std::string index_meta_data_start =
> >        IndexMetaDataKey::Encode(database_id, object_store_id, index_id, 0);
> >    const std::string index_meta_data_end =
> >        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id,
index_id);
> >    Status s = DeleteRangeBasic(leveldb_transaction, index_meta_data_start,
> > -                              index_meta_data_end, true);
> > +                              index_meta_data_end, true, &delete_count);
> >  
> >    if (s.ok()) {
> >      const std::string index_data_start =
> > @@ -3018,7 +3043,7 @@
> >      const std::string index_data_end =
> >          IndexDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
> >      s = DeleteRangeBasic(leveldb_transaction, index_data_start,
> index_data_end,
> > -                         true);
> > +                         true, &delete_count);
> >    }
> >  
> >    if (!s.ok())
> 
> keishi's revert landed juust before this one :)
> https://codereview.chromium.org/2710203002

Thanks for the revert!

Powered by Google App Engine
This is Rietveld 408576698