Bug 225791 - Modernize / Optimize SQLiteStatement creation and preparation
Summary: Modernize / Optimize SQLiteStatement creation and preparation
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:
Blocks: 225855
  Show dependency treegraph
 
Reported: 2021-05-13 16:54 PDT by Chris Dumez
Modified: 2021-05-16 11:27 PDT (History)
18 users (show)

See Also:


Attachments
Patch (261.40 KB, patch)
2021-05-13 17:22 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (261.91 KB, patch)
2021-05-13 17:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (262.42 KB, patch)
2021-05-13 18:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (262.61 KB, patch)
2021-05-13 18:11 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (262.91 KB, patch)
2021-05-13 18:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (262.88 KB, patch)
2021-05-13 20:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (262.89 KB, patch)
2021-05-13 23:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (262.95 KB, patch)
2021-05-14 07:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (285.14 KB, patch)
2021-05-14 09:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (284.99 KB, patch)
2021-05-14 09:46 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (285.10 KB, patch)
2021-05-14 10:06 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (284.57 KB, patch)
2021-05-14 10:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (284.86 KB, patch)
2021-05-14 15:26 PDT, Chris Dumez
no flags 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-13 16:54:00 PDT
Modernize / Optimize SQLiteStatement creation and preparation.
Comment 1 Chris Dumez 2021-05-13 17:22:10 PDT
Created attachment 428583 [details]
Patch
Comment 2 EWS Watchlist 2021-05-13 17:22:51 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-13 17:46:35 PDT
Created attachment 428585 [details]
Patch
Comment 4 Chris Dumez 2021-05-13 18:08:08 PDT
Created attachment 428587 [details]
Patch
Comment 5 Chris Dumez 2021-05-13 18:11:05 PDT
Created attachment 428588 [details]
Patch
Comment 6 Chris Dumez 2021-05-13 18:50:36 PDT
Created attachment 428592 [details]
Patch
Comment 7 Chris Dumez 2021-05-13 19:59:12 PDT
This is based on a proposal from Kimmo on one of my other bugs.
Comment 8 Sihui Liu 2021-05-13 20:28:33 PDT
Comment on attachment 428592 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        - The SQLiteStatement constructor is now private so that we never have a
> +          SQLiteStatement that has not been "prepared". Only the SQLiteDatabase
> +          can now construct SQLiteStatement objects. We already needed to pass
> +          a SQLiteDatabase reference when constructing the SQLiteStatement anyway.

Why not just add a static SQLiteStatement::createAndPrepare()? It seems more straightforward to have a public create function in the class for use and more close to our pattern. Also with this change, SQLiteStatement class still refers to SQLiteDatabase and the SQLiteDatabase is not referring to or tracking the statements it creates so the benefits are not obvious.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:59
> +    if (m_statement)

Nit: if is not needed.
Comment 9 Chris Dumez 2021-05-13 20:38:58 PDT
(In reply to Sihui Liu from comment #8)
> Comment on attachment 428592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428592&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        - The SQLiteStatement constructor is now private so that we never have a
> > +          SQLiteStatement that has not been "prepared". Only the SQLiteDatabase
> > +          can now construct SQLiteStatement objects. We already needed to pass
> > +          a SQLiteDatabase reference when constructing the SQLiteStatement anyway.
> 
> Why not just add a static SQLiteStatement::createAndPrepare()? It seems more
> straightforward to have a public create function in the class for use and
> more close to our pattern. Also with this change, SQLiteStatement class
> still refers to SQLiteDatabase and the SQLiteDatabase is not referring to or
> tracking the statements it creates so the benefits are not obvious.
> 
> > Source/WebCore/platform/sql/SQLiteStatement.cpp:59
> > +    if (m_statement)
> 
> Nit: if is not needed.

It is needed, see the move constructor.
Comment 10 Chris Dumez 2021-05-13 20:47:57 PDT
(In reply to Sihui Liu from comment #8)
> Comment on attachment 428592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428592&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        - The SQLiteStatement constructor is now private so that we never have a
> > +          SQLiteStatement that has not been "prepared". Only the SQLiteDatabase
> > +          can now construct SQLiteStatement objects. We already needed to pass
> > +          a SQLiteDatabase reference when constructing the SQLiteStatement anyway.
> 
> Why not just add a static SQLiteStatement::createAndPrepare()? It seems more
> straightforward to have a public create function in the class for use and
> more close to our pattern. Also with this change, SQLiteStatement class
> still refers to SQLiteDatabase and the SQLiteDatabase is not referring to or
> tracking the statements it creates so the benefits are not obvious.

Why is SQLiteStatement::createAndPrepare(db, query) better than db.prepareStatement(query)?

I don't feel super strongly but the reasons I went this way:
- We need a database to construct a statement anyway
- Having a SQLiteStatement::createAndPrepare(db, query) return a stack object is a bit uncommon and may be confusing. Our "create" functions usually return heap-allocated objects.
- This is what Kimmo suggested

Note that I am not convinced that SQLiteStatement needs or should have a database data member but this patch is large enough as it is. We could just as well have the SQLiteStatement member functions that require a database take the database as parameter (few need it and the ones that need it, it is mostly to get the last error). It is fairly common for SQLiteStatement objects to be cached nowadays and this would reduce the size of SQLiteStatement objects. Callers have to keep the SQLiteDatabase alive anyway.
Comment 11 Chris Dumez 2021-05-13 20:49:20 PDT
Comment on attachment 428592 [details]
Patch

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

>> Source/WebCore/platform/sql/SQLiteStatement.cpp:59
>> +    if (m_statement)
> 
> Nit: if is not needed.

Oh, the doc says "Invoking sqlite3_finalize() on a NULL pointer is a harmless no-op.". I'll drop the if check.
Comment 12 Chris Dumez 2021-05-13 20:50:23 PDT
Created attachment 428596 [details]
Patch
Comment 13 Chris Dumez 2021-05-13 23:08:10 PDT
Created attachment 428604 [details]
Patch
Comment 14 Chris Dumez 2021-05-14 07:23:18 PDT
Created attachment 428620 [details]
Patch
Comment 15 Alex Christensen 2021-05-14 08:33:59 PDT
Comment on attachment 428620 [details]
Patch

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

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:705
> +    return UniqueRef<SQLiteStatement>(*new SQLiteStatement(*this, sqlStatement.value()));

Why doesn't makeUniqueRef work here?
Comment 16 Chris Dumez 2021-05-14 08:35:51 PDT
Comment on attachment 428620 [details]
Patch

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

> Source/WTF/ChangeLog:12
> +        The reason I needed this is that I want to prevent call sites from allocating SQLiteStatement

@Alex: I explained it in this ChangeLog. Let me know if it needs clarification.
Comment 17 Alex Christensen 2021-05-14 08:37:26 PDT
Comment on attachment 428620 [details]
Patch

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

>> Source/WTF/ChangeLog:12
>> +        The reason I needed this is that I want to prevent call sites from allocating SQLiteStatement
> 
> @Alex: I explained it in this ChangeLog. Let me know if it needs clarification.

Oh, the old need-to-friend-unique-ptr-or-have-public-constructor trick.  Bummer.
Comment 18 Sam Weinig 2021-05-14 08:47:43 PDT
Comment on attachment 428620 [details]
Patch

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

> Source/WebCore/platform/sql/SQLiteDatabase.h:80
> +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(const String& query);
> +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(ASCIILiteral query);
> +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(const String& query);
> +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(ASCIILiteral query);

It would be cool if we could just have the ASCIILiteral one, as almost all uses of constructed strings for these are usually a mistake. Any idea how close we are to that ideal?
Comment 19 Chris Dumez 2021-05-14 08:50:14 PDT
(In reply to Sam Weinig from comment #18)
> Comment on attachment 428620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428620&action=review
> 
> > Source/WebCore/platform/sql/SQLiteDatabase.h:80
> > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(const String& query);
> > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(ASCIILiteral query);
> > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(const String& query);
> > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(ASCIILiteral query);
> 
> It would be cool if we could just have the ASCIILiteral one, as almost all
> uses of constructed strings for these are usually a mistake. Any idea how
> close we are to that ideal?

I agree but the patch is already pretty big and there are still quite a few usages of String (which you can see in the patch since I had to modify ALL call sites). It is not uncommon for call sites to build the statements on the fly using makeString(). It may be possible to switch to an ASCIILiteral and then use binding but I'd need to look more into it to be sure.
Comment 20 Chris Dumez 2021-05-14 09:12:43 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to Sam Weinig from comment #18)
> > Comment on attachment 428620 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428620&action=review
> > 
> > > Source/WebCore/platform/sql/SQLiteDatabase.h:80
> > > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(const String& query);
> > > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(ASCIILiteral query);
> > > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(const String& query);
> > > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(ASCIILiteral query);
> > 
> > It would be cool if we could just have the ASCIILiteral one, as almost all
> > uses of constructed strings for these are usually a mistake. Any idea how
> > close we are to that ideal?
> 
> I agree but the patch is already pretty big and there are still quite a few
> usages of String (which you can see in the patch since I had to modify ALL
> call sites). It is not uncommon for call sites to build the statements on
> the fly using makeString(). It may be possible to switch to an ASCIILiteral
> and then use binding but I'd need to look more into it to be sure.

Also SQLTransaction.executeSQL(stringQuery) in WebSQL seems unavoidable as long as we support WebSQL.
Comment 21 Chris Dumez 2021-05-14 09:38:43 PDT
Created attachment 428628 [details]
Patch
Comment 22 Chris Dumez 2021-05-14 09:39:09 PDT
(In reply to Chris Dumez from comment #21)
> Created attachment 428628 [details]
> Patch

Rebaselined due to conflict and use a little more ASCIILiterals.
Comment 23 Chris Dumez 2021-05-14 09:46:16 PDT
Created attachment 428630 [details]
Patch
Comment 24 Sam Weinig 2021-05-14 09:53:51 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to Sam Weinig from comment #18)
> > Comment on attachment 428620 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428620&action=review
> > 
> > > Source/WebCore/platform/sql/SQLiteDatabase.h:80
> > > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(const String& query);
> > > +    WEBCORE_EXPORT Expected<SQLiteStatement, int> prepareStatement(ASCIILiteral query);
> > > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(const String& query);
> > > +    WEBCORE_EXPORT Expected<UniqueRef<SQLiteStatement>, int> prepareHeapStatement(ASCIILiteral query);
> > 
> > It would be cool if we could just have the ASCIILiteral one, as almost all
> > uses of constructed strings for these are usually a mistake. Any idea how
> > close we are to that ideal?
> 
> I agree but the patch is already pretty big and there are still quite a few
> usages of String (which you can see in the patch since I had to modify ALL
> call sites). It is not uncommon for call sites to build the statements on
> the fly using makeString(). It may be possible to switch to an ASCIILiteral
> and then use binding but I'd need to look more into it to be sure.

Totally fair. Was much more just asking because I was curious than stating it should be done now.
Comment 25 Chris Dumez 2021-05-14 10:06:11 PDT
Created attachment 428633 [details]
Patch
Comment 26 Chris Dumez 2021-05-14 10:55:00 PDT
Created attachment 428641 [details]
Patch
Comment 27 Chris Dumez 2021-05-14 10:55:12 PDT
(In reply to Chris Dumez from comment #26)
> Created attachment 428641 [details]
> Patch

WinCairo build fix.
Comment 28 Chris Dumez 2021-05-14 15:26:04 PDT
Created attachment 428672 [details]
Patch
Comment 29 Chris Dumez 2021-05-14 15:26:28 PDT
(In reply to Chris Dumez from comment #28)
> Created attachment 428672 [details]
> Patch

Fix the layout test that was failing in WK1 Debug.
Comment 30 Chris Dumez 2021-05-15 17:45:41 PDT
Any feedback to get this patch landed would be appreciated. I understand it is big but it is not really avoidable if we change the way SQLiteTransaction objects get created. I do have to update all call sites. Call site changes are pretty mechanical though. The important bits are in SQLiteTransaction and SQLiteDatabase classes.

The patch helps us avoid a very large amount of String constructions by using ASCIILiteral for most SQL queries. It also reduces the size of SQLiteTransaction by not storing the query string anymore. It also simplifies the use of SQLiteTransaction a bit since create and prepare are no longer 2 separate steps (you now create a prepared SQLiteTransaction. If the preparation fails, you get the SQLite Error code via Expected<SQLiteTransaction, int /*sqlite error code */>.

I do not feel particularly about any given pattern or naming in the patch, as long as we maintain the improvements stated above.
Comment 31 Sam Weinig 2021-05-16 09:52:18 PDT
(In reply to Chris Dumez from comment #20)
> (In reply to Chris Dumez from comment #19)
> > (In reply to Sam Weinig from comment #18)
> > > Comment on attachment 428620 [details]
> > > Patch
> Also SQLTransaction.executeSQL(stringQuery) in WebSQL seems unavoidable as
> long as we support WebSQL.

Sure, but once we are just down to that one, we can renamed the overload that takes a String to something scary so that people know not it probably shouldn't be used.
Comment 32 Chris Dumez 2021-05-16 09:55:14 PDT
(In reply to Sam Weinig from comment #31)
> (In reply to Chris Dumez from comment #20)
> > (In reply to Chris Dumez from comment #19)
> > > (In reply to Sam Weinig from comment #18)
> > > > Comment on attachment 428620 [details]
> > > > Patch
> > Also SQLTransaction.executeSQL(stringQuery) in WebSQL seems unavoidable as
> > long as we support WebSQL.
> 
> Sure, but once we are just down to that one, we can renamed the overload
> that takes a String to something scary so that people know not it probably
> shouldn't be used.

Oh, I like the idea of a rename to avoid calling the String overload by mistake.
Comment 33 Chris Dumez 2021-05-16 09:55:27 PDT
Thanks for reviewing!
Comment 34 EWS 2021-05-16 10:18:39 PDT
Committed r277571 (237799@main): <https://commits.webkit.org/237799@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428672 [details].
Comment 35 Radar WebKit Bug Importer 2021-05-16 10:19:22 PDT
<rdar://problem/78080269>
Comment 36 Chris Dumez 2021-05-16 11:26:11 PDT
(In reply to Sam Weinig from comment #31)
> (In reply to Chris Dumez from comment #20)
> > (In reply to Chris Dumez from comment #19)
> > > (In reply to Sam Weinig from comment #18)
> > > > Comment on attachment 428620 [details]
> > > > Patch
> > Also SQLTransaction.executeSQL(stringQuery) in WebSQL seems unavoidable as
> > long as we support WebSQL.
> 
> Sure, but once we are just down to that one, we can renamed the overload
> that takes a String to something scary so that people know not it probably
> shouldn't be used.

By the way, I am looking at the call sites that use Strings and there are cases you cannot use binding:
1. You cannot bind PRAGMA statements apparently
2. You can bind table names.
3. You cannot bind lists of dynamic size. (e.g. WHERE "foo" IN (" + buildList() ")")