1240398 - Stop using string concatenation to generate SQL (BrowserProvider & BrowserContract)

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: db-use-stringbuffer v0.1

String concatenation is bad news for GC and memory allocations. Let's stop doing it.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a78578829c&selectedJob=15546634

Comment 1

[Richard Newman [:rnewman]]

Almost all of those concatenations should be optimized away by the compiler -- indeed, the Java language spec _requires_ that ("a" + "b") must happen at compile-time.

Furthermore, most Java compilers will emit bytecode that uses StringBuilder in all of these cases, precisely to avoid having to write this kind of tortured code by hand.

What's your evidence that this is happening at run time in non-debug builds?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

After some javac and javap -c, I found the following:

String s = "a" + "b" // does indeed happen at compile time, as expected
String s = "delete from " + History.TABLE; // also concats at compile time, as expected
String s = "hello " + "there " + myName; // does create a default StringBuilder and append the parts

String s = "hello ";
s += "there";  // created two StringBuilders

Based on the above, I'll concede that not all the changes I made are needed. The problem I have is the default StringBuilder has a capacity of 16 characters. It then grows by adding half the current length to the length:
http://androidxref.com/6.0.1_r10/xref/libcore/libart/src/main/java/java/lang/AbstractStringBuilder.java#94

Some of our SQL is sizable and is called a lot. Using an explicit StringBuilder allows us to set a buffer size and reduce memory allocations.

Thoughts on reaching a compromise?

Also, this was easy to do in Android Studio. Already has a refactor for it.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8708835 [details] [diff] [review]
db-use-stringbuffer v0.1

Review of attachment 8708835 [details] [diff] [review]:
-----------------------------------------------------------------

Mm, micro-optimization.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ +56,5 @@
> +        final StringBuilder age = new StringBuilder(64);
> +        age.append("(");
> +        age.append(Combined.DATE_LAST_VISITED);
> +        age.append(" - ");
> +        age.append(System.currentTimeMillis());

The rewrite for this should probably 'unroll' `age` into the expression, capturing the time as a string, because most of the `age` expression is actually constant and can pair up with the rest of the frecency strings.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ +358,5 @@
> +        sql.append(Tabs.URL);
> +        sql.append(" FROM ");
> +        sql.append(TABLE_TABS);
> +        sql.append(")");
> +        trace("Clear thumbs using query: " + sql.toString());

Kill this trace.

@@ +889,5 @@
> +        b.append("UPDATE ");
> +        b.append(TABLE_BOOKMARKS);
> +        b.append(" SET ");
> +        b.append(Bookmarks.POSITION);
> +        b.append(" = CASE guid");

Suggested compromise: if we know we're going to be appending more stuff, use a StringBuilder, and append to it with combined common substrings. So this would be:

    final StringBuilder b = new StringBuilder(256);
    b.append("UPDATE " + TABLE_BOOKMARKS + " SET " + Bookmarks.POSITION + " = CASE guid");

This avoids a bunch of method calls to append() where the compiler will flatten the string constants for us.

@@ +907,4 @@
>          }
>  
> +        b.append(" END WHERE ");
> +        b.append(DBUtils.computeSQLInClause(processCount, Bookmarks.GUID));

If you're super keen, make a variant of computeSQLInClause that takes an external StringBuilder. It uses one internally.

@@ +918,5 @@
>       * Construct an update expression that will modify the parents of any records
>       * that match.
>       */
>      private int updateBookmarkParents(SQLiteDatabase db, ContentValues values, String selection, String[] selectionArgs) {
>          trace("Updating bookmark parents of " + selection + " (" + selectionArgs[0] + ")");

Kill this line while we're here.

@@ +926,5 @@
> +        where.append(Bookmarks.PARENT);
> +        where.append(" FROM ");
> +        where.append(TABLE_BOOKMARKS);
> +        where.append(" WHERE ");
> +        where.append(selection);

And in this case, if we know how long 'selection' is, allocate the right-size StringBuilder, append the +ed constant strings, then wrap up.

@@ -1320,5 @@
>      private int deleteUnusedImages(Uri uri) {
>          debug("Deleting all unused favicons and thumbnails for URI: " + uri);
>  
> -        String faviconSelection = Favicons._ID + " NOT IN "
> -                + "(SELECT " + History.FAVICON_ID

This is a totally constant string, so leave it (and, indeed, make it final).

@@ -1332,2 @@
>  
> -        String thumbnailSelection = Thumbnails.URL + " NOT IN "

Same.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> Also, this was easy to do in Android Studio. Already has a refactor for it.

Yeah, I don't doubt it — but it's not good to blindly apply. Long chains of 'append' calls, as this produces, will actually be worse, because they won't allow the compiler to merge adjacent constant strings. And they'll produce bigger bytecode and a trivially larger runtime footprint as a result. Always needs tuning.

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.