RESOLVED FIXED 206196
IndexedDB: speed up index records deletion
https://bugs.webkit.org/show_bug.cgi?id=206196
Summary IndexedDB: speed up index records deletion
Sihui Liu
Reported 2020-01-13 14:39:52 PST
...
Attachments
Patch (59.88 KB, patch)
2020-01-13 16:03 PST, Sihui Liu
no flags
Patch (59.15 KB, patch)
2020-01-14 13:21 PST, Sihui Liu
no flags
Patch (60.38 KB, patch)
2020-01-17 12:47 PST, Sihui Liu
no flags
Patch (58.58 KB, patch)
2020-01-21 12:13 PST, Sihui Liu
no flags
Sihui Liu
Comment 1 2020-01-13 16:03:23 PST
Sihui Liu
Comment 2 2020-01-14 09:39:02 PST
Sihui Liu
Comment 3 2020-01-14 13:21:53 PST
Sam Weinig
Comment 4 2020-01-16 21:10:33 PST
Comment on attachment 387694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387694&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:116 > + Optional<bool> ensureValidObjectStoreInfoTable(); This seems like a confusing return type? I can't tell from reading this function declaration what the return value would indicate,
Sihui Liu
Comment 5 2020-01-17 11:29:33 PST
(In reply to Sam Weinig from comment #4) > Comment on attachment 387694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387694&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:116 > > + Optional<bool> ensureValidObjectStoreInfoTable(); > > This seems like a confusing return type? I can't tell from reading this > function declaration what the return value would indicate, ensureValidObjectStoreInfoTable return nullopt if the operation fails; the boolean indicates whether there is a schema change. I will update the name.
Sihui Liu
Comment 6 2020-01-17 12:47:26 PST
Darin Adler
Comment 7 2020-01-20 12:16:37 PST
Comment on attachment 388078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388078&action=review Comment on a small coding style issue, not the overall patch/technique, which seems excellent! > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:200 > + static NeverDestroyed<WTF::String> indexRecordsRecordIndexSchemaString("CREATE INDEX IndexRecordsRecordIndex ON IndexRecords (objectStoreID, objectStoreRecordID)"); Please use _s here to use ASCIILiteral to avoid copying the string’s data, and point to it instead. Also unclear why storing this in a global is a helpful optimization. Paying even this small permanent memory cost may not be worthwhile if there is other memory allocation/deallocation going on. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:238 > +static const String v1ObjectStoreInfoSchema(const String& tableName) This should not take a const String&. Instead take an ASCIILiteral so we don’t create/destroy a String for no reason. makeString can take all kinds of types besides String, including ASCIILiteral and const char*. But also, why compute these at runtime? They could be computed at compile time if we use macros. Finally, I am really unsure that caching these in globals is a useful thing to do. Spending all the memory all the time, but are these operations so fast that we can’t take the time to create/destroy a String? All of this applies to the existing code too; just a mistake being repeated. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:255 > +static const String v2ObjectStoreInfoSchema(const String& tableName) Same comment applies here.
Sihui Liu
Comment 8 2020-01-21 12:13:24 PST
Brady Eidson
Comment 9 2020-01-28 10:39:59 PST
Comment on attachment 388332 [details] Patch I kind of cruised over the upgrade schema stuff since that is old hat boiler plate at this point. lgtm
Sihui Liu
Comment 10 2020-01-28 14:33:05 PST
(In reply to Brady Eidson from comment #9) > Comment on attachment 388332 [details] > Patch > > I kind of cruised over the upgrade schema stuff since that is old hat boiler > plate at this point. > > lgtm Thanks for the review!
WebKit Commit Bot
Comment 11 2020-01-28 15:17:42 PST
Comment on attachment 388332 [details] Patch Clearing flags on attachment: 388332 Committed r255318: <https://trac.webkit.org/changeset/255318>
WebKit Commit Bot
Comment 12 2020-01-28 15:17:44 PST
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.