Replace concat String to append in StringBuilder parameters

Andrej Golovnin andrej.golovnin at gmail.com
Mon Aug 11 09:42:41 UTC 2014


Hi Otávio,

the class src/share/classes/com/sun/jmx/snmp/IPAcl/Parser.java is generated
from the grammar

src/share/classes/com/sun/jmx/snmp/IPAcl/Parser.jjt
src/share/classes/com/sun/jmx/snmp/IPAcl/Parser.jj

Therefore when you are going to change the Parser class, then you must
change the grammar files too. In other case your changes maybe overwritten
in the future, when someone decides to regenerate the Parser class.

The classes

src/share/classes/com/sun/jmx/snmp/IPAcl/ParseException.java
src/share/classes/com/sun/jmx/snmp/IPAcl/TokenMgrError.java

are generated from the templates provided by JavaCC

https://java.net/projects/javacc/sources/svn/show/trunk/src/main/resources/templates?rev=689

You should consider contributing you changes to the ParseException and
TokenMgrError classes to the JavaCC project as well.


In the class
src/share/classes/javax/management/openmbean/CompositeType.java you have
added the
annotation @SuppressWarnings("StringConcatenationInsideStringBufferAppend")
instead of fixing the concatenation inside the append method. Why?


And I would like to suggest to drop explicit usage of StringBuilder in some
methods at all to improve code readability. Take look for example at the
class src/share/classes/java/lang/management/MemoryUsage.java and its
#toString()-method:

Before your change:

 239     public String toString() {
 240         StringBuilder buf = new StringBuilder();
 241         buf.append("init = " + init + "(" + (init >> 10) + "K) ");
 242         buf.append("used = " + used + "(" + (used >> 10) + "K) ");
 243         buf.append("committed = " + committed + "(" +
 244                    (committed >> 10) + "K) " );
 245         buf.append("max = " + max + "(" + (max >> 10) + "K)");
 246         return buf.toString();
 247     }

After your change:

 239     public String toString() {
 240         StringBuilder buf = new StringBuilder();
 241         buf.append("init = ").append(init).append('(').append(init >>
10).append("K) ");
 242         buf.append("used = ").append(used).append('(').append(used >>
10).append("K) ");
 243         buf.append("committed =
").append(committed).append('(').append(committed >> 10).append("K) ");
 244         buf.append("max = ").append(max).append('(').append(max >>
10).append("K)");
 245         return buf.toString();
 246     }

And if we drop explicit usage of StringBuilder and let the Java compiler do
its magic, then it looks like this:

 239     public String toString() {
 240         return "init = " + init + "(" + (init >> 10) + "K) "
 241                + "used = " + used + "(" + (used >> 10) + "K) "
 242                + "committed = " + committed + "(" + (committed >> 10)
+ "K) "
 243                + "max = " + max + "(" + (max >> 10) + "K)";
 244     }

For me this code is easier to read and understand, then the code before and
after your change. But this is only my opinion. Other candidates for such
change may be:

src/share/classes/com/sun/crypto/provider/DHParameters.java
src/share/classes/com/sun/crypto/provider/DHPublicKey.java
src/share/classes/com/sun/crypto/provider/GCMParameters.java
src/share/classes/com/sun/crypto/provider/OAEPParameters.java
src/share/classes/com/sun/crypto/provider/RC2Parameters.java
src/share/classes/java/security/CodeSigner.java
src/share/classes/java/security/cert/CollectionCertStoreParameters.java
src/share/classes/java/security/cert/LDAPCertStoreParameters.java
src/share/classes/java/security/cert/PKIXBuilderParameters.java
src/share/classes/java/security/cert/PKIXCertPathBuilderResult.java
src/share/classes/java/security/cert/PKIXCertPathValidatorResult.java
src/share/classes/java/security/cert/PolicyQualifierInfo.java
src/share/classes/java/util/Scanner.java
src/share/classes/java/util/regex/Matcher.java
src/share/classes/javax/management/relation/RoleInfo.java
src/share/classes/javax/naming/RefAddr.java
src/share/classes/sun/security/pkcs/SigningCertificateInfo.java
src/share/classes/sun/security/provider/certpath/CertId.java
src/share/classes/sun/security/provider/certpath/SunCertPathBuilderParameters.java
src/share/classes/sun/security/x509/DistributionPointName.java
src/share/classes/sun/security/x509/PolicyInformation.java
src/share/classes/sun/security/x509/X509CertImpl.java


In the #toString()-method
of src/share/classes/javax/swing/RepaintManager.java we can avoid creation
of StringBuilder if we change it from:

 992     public synchronized String toString() {
 993         StringBuilder sb = new StringBuilder();
 994         if(dirtyComponents != null)
 995             sb.append(dirtyComponents);
 996         return sb.toString();
 997     }

to:

 992     public synchronized String toString() {
 993         return dirtyComponents != null ? dirtyComponents.toString : "";
 994     }


In src/share/classes/sun/net/www/HeaderParser.java please change the line
223 to use StringBuilder instead of StringBuffer.

In the class src/share/classes/sun/security/ssl/ServerNameExtension.java
the #toString()-method can be changed from:

 268     public String toString() {
 269         StringBuilder sb = new StringBuilder();
 270         for (SNIServerName sniName : sniMap.values()) {
 271             sb.append('[').append(sniName).append(']');
 272         }
 273
 274         return "Extension " + type + ", server_name: " + sb;
 275     }

to:

 268     public String toString() {
 269         StringBuilder sb = new StringBuilder();
 270         sb.append("Extension ").append(type).append(", server_name: ");
 271         for (SNIServerName sniName : sniMap.values()) {
 272             sb.append('[').append(sniName).append(']');
 273         }
 274
 275         return sb.toString();
 276     }

to avoid creation of an additional StringBuilder-object in the line 274.
Similar change can be applied to the class
 src/share/classes/sun/security/ssl/SignatureAlgorithmsExtension.java.

Best regards,
Andrej Golovnin

On Mon, Aug 11, 2014 at 5:29 AM, Otávio Gonçalves de Santana <
otaviojava at java.net> wrote:

> Done.
>
>
> https://dl.dropboxusercontent.com/u/16109193/open_jdk/string_builder_concat_2.zip
>
> obs: stay the 2 chars to better performance.
>
>
> On Sun, Aug 10, 2014 at 8:03 PM, Claes Redestad <claes.redestad at oracle.com
> >
> wrote:
>
> > +1
> >
> > Some suggestions (mostly nits):
> >
> > - in places like src/share/classes/java/util/regex/Pattern.java you
> > introducesingle-char
> >   strings which might use a char instead:
> >
> > -                result.append("|"+next);
> > +                result.append('|').append(next);
> >
> > - in places like src/share/classes/sun/security/provider/PolicyFile.java
> >   you end up with a sequence of String literal appends which could be
> > merged into one:
> >
> > -                sb.append(principalInfo[i][0] + " " +
> > -                    "\"" + principalInfo[i][1] + "\"");
> > +                sb.append(principalInfo[i][0]).append(" \"")
> > +                    .append(principalInfo[i][1]).append('"');
> >
> > - in some places like src/share/classes/java/text/ChoiceFormat.java
> >   you end up doing append(""). I guess the empty string concatenation was
> > used as a form
> >   of primitive-to-string coercion and was probably always redundant
> > already, but care needs
> >   to be taken that doing the append directly behave as intended.
> >
> >   I think it should be safe to just remove the empty string append in
> most
> > cases:
> >
> > -                result.append(""+choiceLimits[i]);
> > +                result.append(choiceLimits[i]);
> >
> > Thanks!
> >
> > /Claes
> >
> > On 2014-08-10 23:33, Otávio Gonçalves de Santana wrote:
> >
> >> *Motivation:* Make another append instead of concat String inside of
> >> append
> >>
> >> parameter in StringBuilder class. To avoid an extra StringBuilder
> created
> >> for the purpose of concatenating. So it will save memory and will faster
> >> than concat String.
> >> Doing a code to benchMark[1], the result is:
> >>
> >> Benchmark                                                  Mode
> Samples
> >>        Mean   Mean error    Units
> >> m.StringBuilderConcatBenchMark.stringBuilder              thrpt
>  10
> >>   6317444.705   108673.584    ops/s
> >> m.StringBuilderConcatBenchMark.stringBuilderWithConcat    thrpt
>  10
> >>   3354554.435    68353.924    ops/s
> >>
> >> The webrev of all code is:
> >> https://dl.dropboxusercontent.com/u/16109193/open_jdk/
> >> string_builder_concat.zip
> >> <https://dl.dropboxusercontent.com/u/16109193/open_jdk/string_
> >> builder_concat.zip>
> >>
> >> [1]
> >>
> >> @State(Scope.Thread)
> >> @OutputTimeUnit(TimeUnit.SECONDS)
> >> public class StringBuilderConcatBenchMark {
> >>
> >>
> >>          private static final String F = "!!!!";
> >>   private static final String E = " running in Java ";
> >> private static final String D = " in the code ";
> >>   private static final String C = " to try impact ";
> >> private static final String B = " with some text ";
> >>   private static final String A = "Doing a test";
> >>
> >> @GenerateMicroBenchmark
> >> public void stringBuilder(BlackHole bh) {
> >>   bh.consume(createBuilder(A, B, C, D, E, F));
> >> }
> >>
> >> @GenerateMicroBenchmark
> >>   public void stringBuilderWithConcat(BlackHole bh) {
> >> bh.consume(createBuilderWithConcat(A, B, C, D, E, F));
> >>   }
> >>
> >> private StringBuilder createBuilder(String... values) {
> >> StringBuilder text = new StringBuilder();
> >>   text.append(values[0]).append(values[1])
> >> .append(values[2]).append(values[3])
> >> .append(values[4]).append(values[5]);
> >>   return text;
> >> }
> >>   private StringBuilder createBuilderWithConcat(String... values) {
> >>   StringBuilder text = new StringBuilder();
> >> text.append(values[0] + values[1])
> >> .append(values[2] + values[3])
> >>   .append(values[4]+ values[5]);
> >> return text;
> >> }
> >> }
> >>
> >>
> >
>
>
> --
> Otávio Gonçalves de Santana
>
> blog:     http://otaviosantana.blogspot.com.br/
> twitter: http://twitter.com/otaviojava
> site:     *http://about.me/otaviojava <http://about.me/otaviojava>*
> 55 (11) 98255-3513
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20140811/9e52c27c/attachment-0001.html>


More information about the security-dev mailing list