The default bug view has changed. See this FAQ.

Unnecessary use of StringBuffer

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

Trunk
Firefox 25
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → ckitching
(Assignee)

Comment 1

4 years ago
Created attachment 768640 [details] [diff] [review]
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.
Attachment #768640 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 2

4 years ago
Created attachment 768646 [details] [diff] [review]
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.
Attachment #768646 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 3

4 years ago
Created attachment 768647 [details] [diff] [review]
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...
Attachment #768647 - Flags: review?(margaret.leibovic)

Comment 4

4 years ago
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.
Attachment #768640 - Flags: review?(margaret.leibovic) → review?(rnewman)

Updated

4 years ago
Attachment #768646 - Flags: review?(margaret.leibovic) → review?(rnewman)

Updated

4 years ago
Attachment #768647 - Flags: review?(margaret.leibovic) → review?(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.
Attachment #768640 - Flags: review?(rnewman) → feedback+
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.
Attachment #768646 - Flags: review?(rnewman) → review-
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.
Attachment #768647 - Flags: review?(rnewman)
Status: NEW → ASSIGNED
(Assignee)

Comment 8

4 years ago
Created attachment 769781 [details] [diff] [review]
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)
Attachment #768640 - Attachment is obsolete: true
Attachment #768646 - Attachment is obsolete: true
Attachment #768647 - Attachment is obsolete: true
Attachment #769781 - Flags: review?(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 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
Attachment #769781 - Flags: review?(rnewman) → review+
(Assignee)

Updated

4 years ago
Attachment #769781 - Attachment description: Patch replacing single-threaded uses of StringBuffer with StringBuilder. V2. → Bug 769781 = Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman
(Assignee)

Updated

4 years ago
Attachment #769781 - Attachment description: Bug 769781 = Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman → Bug 769781 - Replacing single-threaded uses of StringBuffer with StringBuilder. r=rnewman
(Assignee)

Comment 11

4 years ago
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 <ckitching@mozilla.com>
>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 {
(Assignee)

Comment 12

4 years ago
Created attachment 769865 [details] [diff] [review]
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..
Attachment #769781 - Attachment is obsolete: true
Attachment #769865 - Flags: review+
Keywords: checkin-needed
OS: Linux → Android
Hardware: x86_64 → All
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...
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3105f9ef5328
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.