IndexedDB: IDBFactory.deleteDatabase() is slow
Created attachment 162616 [details] Patch
Please take a look.
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.
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
(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.
Created attachment 162826 [details] Patch
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.
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?
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.
Created attachment 163227 [details] Patch
Comment on attachment 163227 [details] Patch dgrogan@ - all feedback incorporated. Please take another look?
Comment on attachment 163227 [details] Patch LGTM
tony@ - r? cq?
Comment on attachment 163227 [details] Patch You could write a perf test for this. See the PerformanceTests directory for examples of existing tests.
Comment on attachment 163227 [details] Patch Clearing flags on attachment: 163227 Committed r128136: <http://trac.webkit.org/changeset/128136>
All reviewed patches have been landed. Closing bug.