Bug 96036 – IndexedDB: IDBFactory.deleteDatabase() is slow

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: IDBFactory.deleteDatabase() is slow

[Joshua Bell]

Created attachment 162616 [details]
Patch

[Joshua Bell]

Please take a look.

[David Grogan]

Comment on attachment 162616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162616&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:378
> +    return m_db->remove(key);

Previously the database name and entries were deleted in one atomic WriteBatch.  It looks like here they are in two separate WriteBatches.  Accurate?  Problematic?  What would our code do if it opened a database and found a name entry but no metadata or other entries?

A more theoretically-sound solution might be to add a LevelDBNoReadTransaction that is just a light wrapper around a write batch and leaves out the whole AVLTree stuff.  But if our open code would successfully handle a lone database name entry then that's probably overkill.

[WebKit Review Bot]

Comment on attachment 162616 [details]
Patch

Attachment 162616 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13770763

New failing tests:
inspector/styles/protocol-css-regions-commands.html

[Joshua Bell]

(In reply to comment #3)
> (From update of attachment 162616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162616&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:378
> > +    return m_db->remove(key);
> 
> Previously the database name and entries were deleted in one atomic WriteBatch.  It looks like here they are in two separate WriteBatches.  Accurate?  Problematic?  What would our code do if it opened a database and found a name entry but no metadata or other entries?
> 
> A more theoretically-sound solution might be to add a LevelDBNoReadTransaction that is just a light wrapper around a write batch and leaves out the whole AVLTree stuff.  But if our open code would successfully handle a lone database name entry then that's probably overkill.

I talked about that option with alecflett@ actually, and I think I may do it just because it's safer and potentially re-usable.

[Joshua Bell]

Created attachment 162826 [details]
Patch

[Joshua Bell]

Updated with a "LevelDBWriteOnlyTransaction" type.

Note that the iteration needed to be inlined as deleteRange() can't be re-used, since that would require adding createIterator() to the transaction that iterated the database not the transaction's contents, which seemed wrong for a WriteOnlyTransaction, or passing in an iterator to deleteRange() which seemed like overkill.

[David Grogan]

Comment on attachment 162826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162826&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:363
> +    RefPtr<LevelDBWriteOnlyTransaction> transaction = LevelDBWriteOnlyTransaction::create(m_db.get());

This could be an OwnPtr?  Though I suppose there's some value in being consistent with LevelDBTransaction.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:378
> +            transaction->rollback();

2. If remove returns void we can remove this rollback call.

> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:505
> +bool LevelDBWriteOnlyTransaction::put(const LevelDBSlice& key, const Vector<char>& value)

I'd say remove this until we use it.

> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:512
> +bool LevelDBWriteOnlyTransaction::remove(const LevelDBSlice& key)

1. Should this just return void if it always returns true?

> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:531
> +void LevelDBWriteOnlyTransaction::rollback()

3. If we never call this, should we remove it?

[Joshua Bell]

Comment on attachment 162826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162826&action=review

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:363
>> +    RefPtr<LevelDBWriteOnlyTransaction> transaction = LevelDBWriteOnlyTransaction::create(m_db.get());
> 
> This could be an OwnPtr?  Though I suppose there's some value in being consistent with LevelDBTransaction.

Yeah, I was aiming for consistency.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:378
>> +            transaction->rollback();
> 
> 2. If remove returns void we can remove this rollback call.

... which is why I remove() return bool. it turns out LevelDBTransaction's remove() will only ever return true as well.

>> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:505
>> +bool LevelDBWriteOnlyTransaction::put(const LevelDBSlice& key, const Vector<char>& value)
> 
> I'd say remove this until we use it.

Will do.

>> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:531
>> +void LevelDBWriteOnlyTransaction::rollback()
> 
> 3. If we never call this, should we remove it?

Might as well.

[Joshua Bell]

Created attachment 163227 [details]
Patch

[Joshua Bell]

Comment on attachment 163227 [details]
Patch

dgrogan@ - all feedback incorporated. Please take another look?

[David Grogan]

Comment on attachment 163227 [details]
Patch

LGTM

[Joshua Bell]

tony@ - r? cq?

[Tony Chang]

Comment on attachment 163227 [details]
Patch

You could write a perf test for this.  See the PerformanceTests directory for examples of existing tests.

[WebKit Review Bot]

Comment on attachment 163227 [details]
Patch

Clearing flags on attachment: 163227

Committed r128136: <http://trac.webkit.org/changeset/128136>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.