Bug 225855

Summary: Avoid more String creations when preparing SQLite statements
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, berto, cgarcia, darin, ews-watchlist, galpeter, ggaren, gustavo, Hironori.Fujii, japhet, jsbell, mkwst, sam, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 225791    
Bug Blocks:    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch ews-feeder: commit-queue-

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>