"Don't use StringBuilder or foreach in this hot code path"

Question

I am browsing the source code of the Open Source SignalR project, and I see this diff code which is entitled "Don't use StringBuilder or foreach in this hot code path" :

-           public static string MakeCursor(IEnumerable<Cursor> cursors)
+           public static string MakeCursor(IList<Cursor> cursors)
            { 
-               var sb = new StringBuilder();
-               bool first = true;
-               foreach (var c in cursors)
+               var result = "";
+               for (int i = 0; i < cursors.Count; i++)
                {
-                   if (!first)
+                   if (i > 0)
                    {
-                       sb.Append('|');
+                       result += '|';
                    }
-                   sb.Append(Escape(c.Key));
-                   sb.Append(',');
-                   sb.Append(c.Id);
-                   first = false;
+                   result += Escape(cursors[i].Key);
+                   result += ',';
+                   result += cursors[i].Id;
                }
-               return sb.ToString();
+               return result;
            }

I understand why foreach could be less efficient sometimes, and why it is replaced by for.

However, I learned and experienced that the StringBuilder is the most efficient way to concatenate strings. So I am wondering why the author decided to replace it with standard concatenation.

What's wrong in here and in general about using StringBuilder ?

Answer 1

I made the code change and yes it made a huge difference in number of allocations (GetEnumerator()) calls vs not. Imagine this code is millions of times per second. The number of enumerators allocated is ridiculous and can be avoided.

edit: We now invert control in order to avoid any allocations (writing to the writer directly): https://github.com/SignalR/SignalR/blob/2.0.2/src/Microsoft.AspNet.SignalR.Core/Messaging/Cursor.cs#L36

Answer 2

Just hope the guy who changed this actually measured the difference.

• There is overhead in instantiating a new stringbuilder every time. This also puts pressure on memory/garbage collection.
• The compiler can generate 'stringbuilderlike' code for simple concatenations
• The FOR might actually be slower because it might require bounds checking which is not done with foreach loops as the compilers 'knows' they are within bounds.

Answer 3

It depends on the number of Cursors provided to the function.

Most comparisons between the two apporaches seems to favour StringBuilder over string concatination when concatinating 4-10 strings. I would most likely favour StringBuilder if I didn't have explicit reasons not too (e.g. performance comparison of the two approaches for my problem/application). I would consider to pre-allocate a buffer in the StringBuilder to avoid (many) reallocations.

See String concatenation vs String Builder. Performance and Concatenating with StringBuilders vs. Strings for some discussion about the subject.

Answer 4

How many concatenations are you doing? If numerous, use StringBuilder. If just a few, then the overhead of creating the StringBuilder will outweigh any advantage.

Answer 5

I will put my money on

           StringBuilder sb = new StringBuilder();
           bool first = true;
           foreach (Cursor c in cursors)
           {
                if (first)
                {
                   first = false;  // only assign it once
                }
                else
                {
                    sb.Append('|');
                }
                sb.Append(Escape(c.Key) + ',' + c.Id);
            }
            return sb.ToString();

But I would put my money and the update from dfowler. Check out the link in his answer.