888074 - Unnecessary use of StringBuffer

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

Scattered throughout the codebase are uses of StringBuffer that may be safely replaced by StringBuilder. StringBuilder is functionally equivalent to StringBuffer, but is not thread safe - StringBuffer enters a synchronized block on each method call, which is fairly expensive.
Quite often, there is code like:

String someMethod() {
   StringBuffer sf = new StringBuffer(...);
   //Put stuff in the buffer...
   return sf.toString();
}

A StringBuffer in such a situation (And similar ones) cannot possibly be accessed by another thread concurrently, so it can be safely replaced with a StringBuilder at no risk, producing improved performance.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: The seemingly obviously safe migrations from StringBuffer to StringBuilder

I`m including a patch migrating all uses of StringBuffer which are very obviously not accessed from multiple threads to StringBuilder.

Some more seemingly-safe alterations exist, but they`re inside the json-simple library - it isn´t clear why they are as they are. It is possible the author of the library simply didn´t realise that he could have made this substitution, or he knew something I don´t :P. Would appreciate some input on that - it seems that it would be a nice thing to get a speedup on.

Finally, it´s maybe the case that the two StringBuffers in DoubleMetaphone.DoubleMetaphoneResult could be replaced by StringBuilders, but I´m unable to say for certain if this class is accessed from multiple threads - would appreciate input from people more familiar with that part of the code on the safety of that change.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Part 2: The seemingly, but not entirely obviously safe migrations from StringBuffer to StringBuilder

The second patch - contains those migrations of this flavour that I THINK are safe, but that would benefit from being checked in more detail to ensure I didn`t do something stupid.

Comment 3

[Chris Kitching [:ckitching]]

Attachment: Part 3/3 The apparently sort of obviously safe migrations from the json-simple library

The json-simple library contains a number of apparently safe-to-migrate StringBuffers. Seperate patch because the fact I can seemingly make such an obvious improvement to an open source library makes me doubt my findings...

Comment 4

[:Margaret Leibovic]

Comment on attachment 768640 [details] [diff] [review]
The seemingly obviously safe migrations from StringBuffer to StringBuilder

I'm going to punt this over to rnewman, since he's more qualified to review this type of change than I am.

One thing I notice though is that it looks like you're modifying some auto-generated files, which you shouldn't be doing. I also don't know that we want to be making changes in external libraries that we're including.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 768640 [details] [diff] [review]
The seemingly obviously safe migrations from StringBuffer to StringBuilder

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

* Don't touch anything in httpclientandroidlib, apache, etc. These are imported libraries.
* Also don't include R.java in your commits. I would be very surprised if the RTC test apps worked with this patch applied.

Around we go!

::: media/webrtc/trunk/webrtc/modules/audio_device/test/android/audio_device_android_test/gen/org/webrtc/voiceengine/test/R.java
@@ +3,5 @@
> +package org.webrtc.voiceengine.test;
> +
> +/* This stub is for using by IDE only. It is NOT the R class actually packed into APK */
> +public final class R {
> +}
\ No newline at end of file

Be very careful about the files you include in a commit.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 768646 [details] [diff] [review]
Part 2: The seemingly, but not entirely obviously safe migrations from StringBuffer to StringBuilder

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

Not our code, so let's not touch it.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 768647 [details] [diff] [review]
Part 3/3 The apparently sort of obviously safe migrations from the json-simple library

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

This is hardly the most significant flaw in json-simple, so let's stick to our own code.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman

Drat. That was pretty fail.

Here's a fixed patch - no more library commits, not more IDE silliness.

The point about the libraries is something of a pain - the libraries, in general, when published, need to be thread safe because people may end up using them in ways that require this. We don't need them to be thread safe, so we do end up paying a performance cost for a feature of the library we don't care about. (That said, I guess this problem of libraries-performing-slightly-less-well-than-they-might-because-of-being-more-general-than-you-really-need is almost ubiquitous when using libraries, and arguably something that shouldn't be fixed for reasons of neatness. Or something.)

On the topic of the somewhat extensive brokenness of json-simple - should we make a bug for that? It could be an entertaining (And I think pointful?) use of (Presumably my own) time to implement a less horrible edition of it. (It seems that it's something we use quite a lot, so perhaps have a lot to gain by making it less suckful)

Comment 9

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #8)

> Here's a fixed patch - no more library commits, not more IDE silliness.

Thanks!

> On the topic of the somewhat extensive brokenness of json-simple - should we
> make a bug for that?

Bug 709185. But this is probably a larger architectural change than you want to bite off, with some shifting contexts right now.

I would welcome benchmarks for json-simple versus org.json as shipped with Android, incidentally. If it's a huge difference, then we care; otherwise, we'd need to shift to a streaming system like Jackson.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 769781 [details] [diff] [review]
Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman

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

N.B., patch description line should read:

  Bug 888074 - Replace single-threaded uses of StringBuffer with StringBuilder. r=rnewman

Bug number and reviewer are vital, of course, and it's implicit that it's a patch :P

Comment 11

[Chris Kitching [:ckitching]]

Comment on attachment 769781 [details] [diff] [review]
Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman

># HG changeset patch
># Parent 407fd59a6ffcb69d3a719b3535ed9c12ee45b6a5
># User Chris Kitching <[e-mail]>
>Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman
>
>
>diff --git a/build/mobile/sutagent/android/WifiConfiguration.java b/build/mobile/sutagent/android/WifiConfiguration.java
>--- a/build/mobile/sutagent/android/WifiConfiguration.java
>+++ b/build/mobile/sutagent/android/WifiConfiguration.java
>@@ -300,17 +300,17 @@ public class WifiConfiguration implement
>         for (int i = 0; i < wepKeys.length; i++)
>             wepKeys[i] = null;
>         for (EnterpriseField field : enterpriseFields) {
>             field.setValue(null);
>         }
>     }
> 
>     public String toString() {
>-        StringBuffer sbuf = new StringBuffer();
>+        StringBuilder sbuf = new StringBuilder();
>         if (this.status == WifiConfiguration.Status.CURRENT) {
>             sbuf.append("* ");
>         } else if (this.status == WifiConfiguration.Status.DISABLED) {
>             sbuf.append("- ");
>         }
>         sbuf.append("ID: ").append(this.networkId).append(" SSID: ").append(this.SSID).
>                 append(" BSSID: ").append(this.BSSID).append(" PRIO: ").append(this.priority).
>                 append('\n');
>diff --git a/mobile/android/base/CrashReporter.java b/mobile/android/base/CrashReporter.java
>--- a/mobile/android/base/CrashReporter.java
>+++ b/mobile/android/base/CrashReporter.java
>@@ -278,17 +278,17 @@ public class CrashReporter extends Activ
> 
>     private String readLogcat() {
>         BufferedReader br = null;
>         try {
>             // get the last 200 lines of logcat
>             Process proc = Runtime.getRuntime().exec(new String[] {
>                 "logcat", "-v", "threadtime", "-t", "200", "-d", "*:D"
>             });
>-            StringBuffer sb = new StringBuffer();
>+            StringBuilder sb = new StringBuilder();
>             br = new BufferedReader(new InputStreamReader(proc.getInputStream()));
>             for (String s = br.readLine(); s != null; s = br.readLine()) {
>                 sb.append(s).append('\n');
>             }
>             return sb.toString();
>         } catch (Exception e) {
>             return "Unable to get logcat: " + e.toString();
>         } finally {
>@@ -329,17 +329,17 @@ public class CrashReporter extends Activ
>                         sendPart(os, boundary, key, extras.get(key));
>                 } else if (!key.equals(SERVER_URL_KEY) && !key.equals(NOTES_KEY)) {
>                     sendPart(os, boundary, key, extras.get(key));
>                 }
>             }
> 
>             // Add some extra information to notes so its displayed by
>             // crash-stats.mozilla.org. Remove this when bug 607942 is fixed.
>-            StringBuffer sb = new StringBuffer();
>+            StringBuilder sb = new StringBuilder();
>             sb.append(extras.containsKey(NOTES_KEY) ? extras.get(NOTES_KEY) + "\n" : "");
>             if (AppConstants.MOZ_MIN_CPU_VERSION < 7) {
>                 sb.append("nothumb Build\n");
>             }
>             sb.append(Build.MANUFACTURER).append(' ')
>               .append(Build.MODEL).append('\n')
>               .append(Build.FINGERPRINT);
>             sendPart(os, boundary, NOTES_KEY, sb.toString());
>diff --git a/mobile/android/base/GeckoEvent.java b/mobile/android/base/GeckoEvent.java
>--- a/mobile/android/base/GeckoEvent.java
>+++ b/mobile/android/base/GeckoEvent.java
>@@ -594,17 +594,17 @@ public class GeckoEvent {
>         event.mCharacters = subject;
>         event.mCharactersExtra = data;
>         return event;
>     }
> 
>     public static GeckoEvent createViewportEvent(ImmutableViewportMetrics metrics, DisplayPortMetrics displayPort) {
>         GeckoEvent event = new GeckoEvent(NativeGeckoEvent.VIEWPORT);
>         event.mCharacters = "Viewport:Change";
>-        StringBuffer sb = new StringBuffer(256);
>+        StringBuilder sb = new StringBuilder(256);
>         sb.append("{ \"x\" : ").append(metrics.viewportRectLeft)
>           .append(", \"y\" : ").append(metrics.viewportRectTop)
>           .append(", \"zoom\" : ").append(metrics.zoomFactor)
>           .append(", \"fixedMarginLeft\" : ").append(metrics.marginLeft)
>           .append(", \"fixedMarginTop\" : ").append(metrics.marginTop)
>           .append(", \"fixedMarginRight\" : ").append(metrics.marginRight)
>           .append(", \"fixedMarginBottom\" : ").append(metrics.marginBottom)
>           .append(", \"displayPort\" :").append(displayPort.toJSON())
>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java
>--- a/mobile/android/base/GeckoProfile.java
>+++ b/mobile/android/base/GeckoProfile.java
>@@ -233,17 +233,17 @@ public final class GeckoProfile {
>         }
>         File target = new File(dir, filename);
>         return readFile(target);
>     }
> 
>     private String readFile(File target) throws IOException {
>         FileReader fr = new FileReader(target);
>         try {
>-            StringBuffer sb = new StringBuffer();
>+            StringBuilder sb = new StringBuilder();
>             char[] buf = new char[8192];
>             int read = fr.read(buf);
>             while (read >= 0) {
>                 sb.append(buf, 0, read);
>                 read = fr.read(buf);
>             }
>             return sb.toString();
>         } finally {
>@@ -341,17 +341,17 @@ public final class GeckoProfile {
>             }
>         }
> 
>         return null;
>     }
> 
>     private static String saltProfileName(String name) {
>         String allowedChars = "abcdefghijklmnopqrstuvwxyz0123456789";
>-        StringBuffer salt = new StringBuffer(16);
>+        StringBuilder salt = new StringBuilder(16);
>         for (int i = 0; i < 8; i++) {
>             salt.append(allowedChars.charAt((int)(Math.random() * allowedChars.length())));
>         }
>         salt.append('.');
>         salt.append(name);
>         return salt.toString();
>     }
> 
>diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java
>--- a/mobile/android/base/db/LocalBrowserDB.java
>+++ b/mobile/android/base/db/LocalBrowserDB.java
>@@ -753,17 +753,17 @@ public class LocalBrowserDB implements B
>         String faviconUrl = c.getString(c.getColumnIndexOrThrow(History.FAVICON_URL));
>         c.close();
> 
>         return faviconUrl;
>     }
> 
>     @Override
>     public Cursor getFaviconsForUrls(ContentResolver cr, List<String> urls) {
>-        StringBuffer selection = new StringBuffer();
>+        StringBuilder selection = new StringBuilder();
>         String[] selectionArgs = new String[urls.size()];
> 
>         for (int i = 0; i < urls.size(); i++) {
>           final String url = urls.get(i);
> 
>           if (i > 0)
>             selection.append(" OR ");
> 
>@@ -841,17 +841,17 @@ public class LocalBrowserDB implements B
>         byte[] b = c.getBlob(thumbnailIndex);
>         c.close();
> 
>         return b;
>     }
> 
>     @Override
>     public Cursor getThumbnailsForUrls(ContentResolver cr, List<String> urls) {
>-        StringBuffer selection = new StringBuffer();
>+        StringBuilder selection = new StringBuilder();
>         String[] selectionArgs = new String[urls.size()];
> 
>         for (int i = 0; i < urls.size(); i++) {
>           final String url = urls.get(i);
> 
>           if (i > 0)
>             selection.append(" OR ");
> 
>diff --git a/mobile/android/base/gfx/DisplayPortMetrics.java b/mobile/android/base/gfx/DisplayPortMetrics.java
>--- a/mobile/android/base/gfx/DisplayPortMetrics.java
>+++ b/mobile/android/base/gfx/DisplayPortMetrics.java
>@@ -51,17 +51,17 @@ public final class DisplayPortMetrics {
>     }
> 
>     public boolean fuzzyEquals(DisplayPortMetrics metrics) {
>         return RectUtils.fuzzyEquals(mPosition, metrics.mPosition)
>             && FloatUtils.fuzzyEquals(resolution, metrics.resolution);
>     }
> 
>     public String toJSON() {
>-        StringBuffer sb = new StringBuffer(256);
>+        StringBuilder sb = new StringBuilder(256);
>         sb.append("{ \"left\": ").append(mPosition.left)
>           .append(", \"top\": ").append(mPosition.top)
>           .append(", \"right\": ").append(mPosition.right)
>           .append(", \"bottom\": ").append(mPosition.bottom)
>           .append(", \"resolution\": ").append(resolution)
>           .append('}');
>         return sb.toString();
>     }
>diff --git a/mobile/android/base/gfx/RectUtils.java b/mobile/android/base/gfx/RectUtils.java
>--- a/mobile/android/base/gfx/RectUtils.java
>+++ b/mobile/android/base/gfx/RectUtils.java
>@@ -26,17 +26,17 @@ public final class RectUtils {
>             int height = json.getInt("height");
>             return new Rect(x, y, x + width, y + height);
>         } catch (JSONException e) {
>             throw new RuntimeException(e);
>         }
>     }
> 
>     public static String toJSON(RectF rect) {
>-        StringBuffer sb = new StringBuffer(256);
>+        StringBuilder sb = new StringBuilder(256);
>         sb.append("{ \"left\": ").append(rect.left)
>           .append(", \"top\": ").append(rect.top)
>           .append(", \"right\": ").append(rect.right)
>           .append(", \"bottom\": ").append(rect.bottom)
>           .append('}');
>         return sb.toString();
>     }
> 
>diff --git a/mobile/android/base/mozglue/GeckoLoader.java.in b/mobile/android/base/mozglue/GeckoLoader.java.in
>--- a/mobile/android/base/mozglue/GeckoLoader.java.in
>+++ b/mobile/android/base/mozglue/GeckoLoader.java.in
>@@ -54,17 +54,17 @@ public final class GeckoLoader {
>         try {
>             // Check to see if plugins were blocked.
>             if (pluginDirs == null) {
>                 putenv("MOZ_PLUGINS_BLOCKED=1");
>                 putenv("MOZ_PLUGIN_PATH=");
>                 return;
>             }
> 
>-            StringBuffer pluginSearchPath = new StringBuffer();
>+            StringBuilder pluginSearchPath = new StringBuilder();
>             for (int i = 0; i < pluginDirs.length; i++) {
>                 pluginSearchPath.append(pluginDirs[i]);
>                 pluginSearchPath.append(":");
>             }
>             putenv("MOZ_PLUGIN_PATH="+pluginSearchPath);
> 
>             File pluginDataDir = context.getDir("plugins", 0);
>             putenv("ANDROID_PLUGIN_DATADIR=" + pluginDataDir.getPath());
>diff --git a/mobile/android/base/widget/AddonsSection.java b/mobile/android/base/widget/AddonsSection.java
>--- a/mobile/android/base/widget/AddonsSection.java
>+++ b/mobile/android/base/widget/AddonsSection.java
>@@ -97,17 +97,17 @@ public class AddonsSection extends About
>         }
>         return str;
>     }
> 
>     private String readStringFromStream(InputStream fileStream) {
>         String str = null;
>         try {
>             byte[] buf = new byte[32768];
>-            StringBuffer jsonString = new StringBuffer();
>+            StringBuilder jsonString = new StringBuilder();
>             int read = 0;
>             while ((read = fileStream.read(buf, 0, 32768)) != -1)
>                 jsonString.append(new String(buf, 0, read));
>             str = jsonString.toString();
>         } catch (IOException ioe) {
>             Log.i(LOGTAG, "error reading filestream", ioe);
>         } finally {
>             try {

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman

Well THAT wasn't what I wanted...

(n+1)th time lucky.... Now with commit comment that is actually as desired..

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3105f9ef5328

(In reply to Richard Newman [:rnewman] from comment #10)
> N.B., patch description line should read:
> 
>   Bug 888074 - Replace single-threaded uses of StringBuffer with
> StringBuilder. r=rnewman

I went ahead and fixed that bug # for you...

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3105f9ef5328