WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(59.15 KB, patch)
2020-01-14 13:21 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(60.38 KB, patch)
2020-01-17 12:47 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(58.58 KB, patch)
2020-01-21 12:13 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Sihui Liu
Comment 1
2020-01-13 16:03:23 PST
Created
attachment 387585
[details]
Patch
Sihui Liu
Comment 2
2020-01-14 09:39:02 PST
<
rdar://problem/53596307
>
Sihui Liu
Comment 3
2020-01-14 13:21:53 PST
Created
attachment 387694
[details]
Patch
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
Created
attachment 388078
[details]
Patch
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
Created
attachment 388332
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug