Bug 225855 - Avoid more String creations when preparing SQLite statements
Summary: Avoid more String creations when preparing SQLite statements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 225791
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-16 11:27 PDT by Chris Dumez
Modified: 2021-05-17 19:14 PDT (History)
17 users (show)

See Also:


Attachments
Patch (93.88 KB, patch)
2021-05-16 12:12 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (94.05 KB, patch)
2021-05-16 12:24 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (95.06 KB, patch)
2021-05-16 12:28 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (95.06 KB, patch)
2021-05-16 12:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (98.28 KB, patch)
2021-05-17 11:17 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-16 11:27:23 PDT
Avoid more String creations when preparing SQLite statements by using ASCIILiteral. Also rename the SQLiteDatabase::prepareString(const String&) overload so that it doesn't get called by mistake.
Comment 1 Chris Dumez 2021-05-16 12:12:11 PDT
Created attachment 428799 [details]
Patch
Comment 2 EWS Watchlist 2021-05-16 12:13:02 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Chris Dumez 2021-05-16 12:24:15 PDT
Created attachment 428800 [details]
Patch
Comment 4 Chris Dumez 2021-05-16 12:28:23 PDT
Created attachment 428801 [details]
Patch
Comment 5 Chris Dumez 2021-05-16 12:33:35 PDT
Created attachment 428802 [details]
Patch
Comment 6 Alex Christensen 2021-05-17 09:30:14 PDT
Comment on attachment 428802 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:136
> +    return "CREATE TABLE Records (objectStoreID INTEGER NOT NULL ON CONFLICT FAIL, key TEXT COLLATE IDBKEY NOT NULL ON CONFLICT FAIL, value NOT NULL ON CONFLICT FAIL, recordID INTEGER PRIMARY KEY)"_s;

We could use the preprocessor to put the common parts of this in one place.

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:73
>  static const String recordsTableSchemaAlternate()

It looks like this can be an ASCIILiteral too.

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:76
> +        "key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE"

ditto on the duplicate code.  preprocessor
Comment 7 Sihui Liu 2021-05-17 10:14:42 PDT
Comment on attachment 428802 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Avoid more String creations when preparing SQLite statements by using ASCIILiteral. Also rename the
> +        SQLiteDatabase::prepareStatement() / SQLiteDatabase::executeCommand() overloads that take in a 
> +        String to make sure they are not called by mistake.

In Source/WebCore/ChangeLog?

> Source/WebKitLegacy/ChangeLog:10
> +        Avoid more String creations when preparing SQLite statements by using ASCIILiteral. Also rename the
> +        SQLiteDatabase::prepareStatement() / SQLiteDatabase::executeCommand() overloads that take in a 
> +        String to make sure they are not called by mistake.

Ditto.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:290
> +    if (!database.executeCommandSlow(v3RecordsTableSchema("_Temp_Records"))) {

static const String v3RecordsTableSchema(const String& tableName) is only used here with this change. Maybe just change it to static ASCIILiteral v3RecordsTableSchemaTemp()?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:455
> +    if (!m_sqliteDB->executeCommandSlow(v3IndexRecordsTableSchema("_Temp_IndexRecords"))) {

Same as above. Make it static ASCIILiteral v3IndexRecordsTableSchemaTemp()?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:596
> +    if (!m_sqliteDB->executeCommandSlow(indexInfoTableSchema("IndexInfo"_s))) {

Maybe convert static String indexInfoTableSchema(ASCIILiteral tableName) to static ASCIILiteral indexInfoTableSchema() and static ASCIILiteral indexInfoTableSchemaTemp()?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:783
> +    if (!database.executeCommandSlow(v3IndexRecordsTableSchema("_Temp_IndexRecords"))) {

static ASCIILiteral v3IndexRecordsTableSchemaTemp()?
Comment 8 Chris Dumez 2021-05-17 11:17:52 PDT
Created attachment 428848 [details]
Patch
Comment 9 EWS 2021-05-17 12:26:15 PDT
Committed r277601 (237818@main): <https://commits.webkit.org/237818@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428848 [details].
Comment 10 Radar WebKit Bug Importer 2021-05-17 12:27:16 PDT
<rdar://problem/78116715>
Comment 11 Alex Christensen 2021-05-17 17:10:03 PDT
r277617
Comment 12 Fujii Hironori 2021-05-17 19:14:41 PDT
Committed r277629 (237840@main): <https://commits.webkit.org/237840@main>