Bug 225791 – Modernize / Optimize SQLiteStatement creation and preparation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Modernize / Optimize SQLiteStatement creation and preparation.

[Chris Dumez]

Created attachment 428583 [details]
Patch

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 428585 [details]
Patch

[Chris Dumez]

Created attachment 428587 [details]
Patch

[Chris Dumez]

Created attachment 428588 [details]
Patch

[Chris Dumez]

Created attachment 428592 [details]
Patch

[Chris Dumez]

This is based on a proposal from Kimmo on one of my other bugs.

[Sihui Liu]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 428596 [details]
Patch

[Chris Dumez]

Created attachment 428604 [details]
Patch

[Chris Dumez]

Created attachment 428620 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Alex Christensen]

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.

[Sam Weinig]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 428628 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> Created attachment 428628 [details]
> Patch

Rebaselined due to conflict and use a little more ASCIILiterals.

[Chris Dumez]

Created attachment 428630 [details]
Patch

[Sam Weinig]

(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.

[Chris Dumez]

Created attachment 428633 [details]
Patch

[Chris Dumez]

Created attachment 428641 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> Created attachment 428641 [details]
> Patch

WinCairo build fix.

[Chris Dumez]

Created attachment 428672 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #28)
> Created attachment 428672 [details]
> Patch

Fix the layout test that was failing in WK1 Debug.

[Chris Dumez]

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.

[Sam Weinig]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Thanks for reviewing!

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/78080269>

[Chris Dumez]

(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() ")")