WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
96036
IndexedDB: IDBFactory.deleteDatabase() is slow
https://bugs.webkit.org/show_bug.cgi?id=96036
Summary
IndexedDB: IDBFactory.deleteDatabase() is slow
Joshua Bell
Reported
2012-09-06 16:20:40 PDT
IndexedDB: IDBFactory.deleteDatabase() is slow
Attachments
Patch
(5.05 KB, patch)
2012-09-06 16:26 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(6.27 KB, patch)
2012-09-07 11:14 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(5.73 KB, patch)
2012-09-10 15:23 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-09-06 16:26:00 PDT
Created
attachment 162616
[details]
Patch
Joshua Bell
Comment 2
2012-09-06 16:26:18 PDT
Please take a look.
David Grogan
Comment 3
2012-09-06 16:42:53 PDT
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 4
2012-09-07 06:29:04 PDT
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
Comment 5
2012-09-07 08:42:46 PDT
(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
Comment 6
2012-09-07 11:14:36 PDT
Created
attachment 162826
[details]
Patch
Joshua Bell
Comment 7
2012-09-07 11:16:20 PDT
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 8
2012-09-07 13:02:07 PDT
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 9
2012-09-07 14:16:15 PDT
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
Comment 10
2012-09-10 15:23:20 PDT
Created
attachment 163227
[details]
Patch
Joshua Bell
Comment 11
2012-09-10 15:24:58 PDT
Comment on
attachment 163227
[details]
Patch dgrogan@ - all feedback incorporated. Please take another look?
David Grogan
Comment 12
2012-09-10 15:56:48 PDT
Comment on
attachment 163227
[details]
Patch LGTM
Joshua Bell
Comment 13
2012-09-10 15:57:30 PDT
tony@ - r? cq?
Tony Chang
Comment 14
2012-09-10 16:15:38 PDT
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 15
2012-09-10 17:50:12 PDT
Comment on
attachment 163227
[details]
Patch Clearing flags on attachment: 163227 Committed
r128136
: <
http://trac.webkit.org/changeset/128136
>
WebKit Review Bot
Comment 16
2012-09-10 17:50:15 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug