Is there a faster method then StringBuilder for a max 9-10 step string concatenation?

Question

I have this code to concate some array elements:

StringBuilder sb = new StringBuilder();
private RatedMessage joinMessage(int step, boolean isresult) {
        sb.delete(0, sb.length());
        RatedMessage rm;
        for (int i = 0; i <= step; i++) {
            if (mStack[i] == null)
                continue;
            rm = mStack[i].getCurrentMsg();// msg is built upfront, this just returns, it's a getter method call
            if (rm == null || rm.msg.length() == 0)
                continue;
            if (sb.length() != 0) {
                sb.append(", ");
            }
            sb.append(rm.msg);
        }
        rm.msg=sb.toString();
        return rm;
    }

Important the array holds max 10 items, so it's not quite much.

My trace output tells me this method is called 18864 times, 16% of the runtime was spent in this method. Can I optimize more?

Answer 1

First of all I won't reuse StringBuilder and always create new instance. That will be certainly faster, because it would allow GC to use young generation heap area.

Another little trick that allows to eliminate at least one if statement is to rewrite your code like this:

    String separator = "";
    for (int i = 0; i <= step; i++) {
        ...
        sb.append(separator);
        sb.append(rm.msg);
        separator = ", ";
    }

Answer 2

If your function is supposed to concatenate array elements, why are you passing in all these crazy values and unused parameters?

private string joinMessage( string[] myArray)
{
  StringBuilder sbr = new StringBuilder();
  for(int i = 0; i < myArray.Length; i++)
  {
     if(!string.IsNullOrEmpty(myArray[i])
     {
       sbr.Append(myArray[i]);
       sbr.Append(",")
     }
  }
  return sbr.ToString();
}

Answer 3

You can make the following change (showing only the differences):

    String separator = "";
    for (int i = 0; i <= step; i++) {
    // ...
        sb.append(separator).append(rm.msg);
        separator = ", ";
    }

It gets rid if an extra if 9 times at the cost of adding an empty string once. You should measure if it helps at all with the data you are using before you decide to keep this change :-)

Answer 4

Take a step through each element in the stack first, taking a count of the sum of all the string lengths.

Then you can use

sb.ensureCapacity(totalEndLength);

String builder works like an array list, so you might be rebuilding that array with most of your appends.

Answer 5

some ideas:

1) Do you initialize the StringBuilder with the estimated max capacity? This can save the time spent in the inner array re-allocation & copying.

2) Maybe you can append a trailing comma in the loop, and avoid the condition for string length inside the loop. Instead, add a single condition at the end of the method, and remove the trailing comma if needed.

Answer 6

A bit of a mini-optimization... take the test-for-comma outside of the loop.

private RatedMessage joinMessage(int step, boolean isresult) {
    sb.delete(0, sb.length());
    for (int i = 0; i <= step; i++) {
        if (mStack[i] == null)
            continue;
        rm = mStack[i].getCurrentMsg();
        if (rm == null || rm.msg.length() == 0)
            continue;
        sb.append(rm.msg).append(", ");
    }
    if (sb.length() > 2) {
        sb.delete(sb.length() - 2, 2);
    }
    return sb.toString();
}

Other suggestions would be:

• Make sure that when the StringBuilder is constructed you set its initial length to a decent value
• I'm not sure of the context of the rest of the code, but maybe you can pre-ensure that mStack[i] will not be null, and that mStack[i].getCurrentMessage() is not null or empty - this will allow you to take more if statements outside of the loop.

Answer 7

Have a separate copy of the mStack array with the string representation, by default initialized with empty String, so your loop would be:

String [] mStackCopy = new String[]{"","","","","","","","","","",};
// or mstackCopy = new String[mStack.length]; 
// for( int i = 0 ; i < mStackCopy.lenght ; i++ ) { mStack[i] = "" }

Also, create the StringBuilder with enough capacity:

StringBuilder sb = new StringBuilder( 10000 );// 10k chars or whatever makes sense.

So, when you need to create the message you would simply:

for (int i = 0; i <= step; i++) {
   sb.append( mStackCopy[i] );
}

And empty parts won't cause a problem because they are blank already:

You may even hard code it:

 sb.append( mStackCopy[0]);
 sb.append( mStackCopy[1]);
 sb.append( mStackCopy[2]);
 sb.append( mStackCopy[3]);
 sb.append( mStackCopy[4]);
 sb.append( mStackCopy[5]);
 sb.append( mStackCopy[6]);
 sb.append( mStackCopy[7]);
 sb.append( mStackCopy[8]);
 sb.append( mStackCopy[9]);

But this would cause more pain than relief in the future, guaranteed.

When you add something to your mStack:

MStack item = new MStack();
item.setCurrentMessage("Some message");

 .... 

Just make a copy of the message and append the ", " already.

  addToMStack(int position,  MStackItem item ) {
    mStack[position] = item;
    mStackCopy[position] = item.getCurrentMessage() + ", ";
}

And depending on the occurrence of nulls ( if its low ) you can catch them

  addToMStack(int position,  MStackItem item ) {
    if( item == null ) { return; }
    mStack[position] = item;
    try {
        mStackCopy[position] = item.getCurrentMessage() + ", ";
    } catch( NullPointerException npe ){}
 }

Which is horrendous

Or validate it:

  addToMStack(int position,  MStackItem item ) {
    if( item == null ) { return; }
    mStack[position] = item;
    mStackCopy[position] = item.getCurrentMessage() + ", ";
 }

I'm pretty sure your method is doing something else that you don't show us. Probably the reason is there.

Also, 16% is not that bad, if 100% is 1sec.

Answer 8

If your mStack is a Collection instead of an array, you can just do mStack.toString(), which will print a readable string of the array. That might be easier than writing your own.

Answer 9

16% runtime in this method including or excluding called methods? The getCurrentMsg() call might be a hidden problem, if it creates lots of objects.

Besides that, I suggest to take all the needed Strings out of your stack and afterwards call

StringUtils.join(myStrings, ", ")

using the Apache commons library. Try relying on tested code for such low level things instead of optimizing it yourself every other day. In the end that will give you better optimization results because you will be able to concentrate on the big picture (i.e. the overall design of your software).

Answer 10

Sometimes there's just nothing else to optimize. I think this is one of such cases. You can try to shave off an instruction or two maybe, but you won't get it much faster in principle.

I think the only thing left to optimize is to consider why you are calling it 18864 times and whether some of those calls can be avoided altogether. Perhaps some are not needed, or perhaps you can cache the result in some cases.

Answer 11

Use StringBuilder + StringUtils from Apache Commons Lang. Looping through a String with a separator and chomping is what StringUtils is all about!

private RatedMessage joinMessage(int step, boolean isresult) {
        StringBuilder builder = new StringBuilder();
        for (int i = 0; i <= step; i++) {
            WhateverTypeIsFromMStackVariable stackVariable = mStack[i];
            String message = getMessage(stackVariable);
            if(StringUtils.isNotEmpty(message)) {
                builder.append(message).append(", ");
            }
        }
        RatedMessage rm = new RatedMessage();
        rm.msg = StringUtils.chomp(builder.toString(), ", ");
        return rm;
    }

private static String getMessage(WhateverTypeIsFromMStackVariable stackVariable) {
    if(stackVariable != null) {
        RatedMessage message = stackVariable.getCurrentMsg();
        if(message != null) {
            return message.msg;
        }
     }
     return null;
 }

Apache Commons Lang is here: http://commons.apache.org/lang/