StringBuilder with Caching, ThreadStatic

Question

I came across the code below from Writing Large, Responsive .NET Framework Apps.

The code below creates a string like SomeType<T1, T2, T3> using StringBuilder, and demonstrating caching StringBuilder to improve performance.

 public void Test3()
        {
            Console.WriteLine(GenerateFullTypeName("SomeType", 3));
        }

        // Constructs a name like "SomeType<T1, T2, T3>"  
        public string GenerateFullTypeName(string name, int arity)
        {
            //StringBuilder sb = new StringBuilder();
            StringBuilder sb = AcquireBuilder();

            sb.Append(name);
            if (arity != 0)
            {
                sb.Append("<");
                for (int i = 1; i < arity; i++)
                {
                    sb.Append("T"); sb.Append(i.ToString()); sb.Append(", ");
                }
                sb.Append("T"); sb.Append(arity.ToString()); sb.Append(">");
            }

            //return sb.ToString();
            /* Use sb as before */
            return GetStringAndReleaseBuilder(sb);
        }
        [ThreadStatic]
        private static StringBuilder cachedStringBuilder;

        private static StringBuilder AcquireBuilder()
        {
            StringBuilder result = cachedStringBuilder;
            if (result == null)
            {
                return new StringBuilder();
            }
            result.Clear();
            cachedStringBuilder = null;
            return result;
        }

        private static string GetStringAndReleaseBuilder(StringBuilder sb)
        {
            string result = sb.ToString();
            cachedStringBuilder = sb;
            return result;
        }

However, is it correct that the two modified methods below are better in terms of caching StringBuilder?? Only AcquireBuilder needs to know how to cache it.

 private static StringBuilder AcquireBuilder()
        {
            StringBuilder result = cachedStringBuilder;
            if (result == null)
            {
                //unlike the method above, assign it to the cache
                cachedStringBuilder = result = new StringBuilder();
                return result;
            }
            result.Clear();
            //no need to null it
           // cachedStringBuilder = null;
            return result;
        }

        private static string GetStringAndReleaseBuilder(StringBuilder sb)
        {
            string result = sb.ToString();
             //other method does not to assign it again.
            //cachedStringBuilder = sb;
            return result;
        }

Another issue is that the original methods are not thread-safe, why is ThreadStatic used in the demo?

Here's an even better implementation of `AcquireBuilder`: [`ObjectPool.Get`](https://learn.microsoft.com/aspnet/core/api/microsoft.extensions.objectpool.objectpool-1). It's what ASP.NET itself uses; I'm not sure why the author felt it necessary to come up with something original. — Jeroen Mostert, Aug 30 '17 at 20:20
This is already [built into the framework](https://stackoverflow.com/questions/20029868/understanding-of-net-internal-stringbuildercache-class-configuration). Looks pretty similar. Do make sure you need it, keep in mind that a cache without an expiration policy is a memory leak. — Hans Passant, Aug 30 '17 at 22:47

eocron · Answer 1 · 2017-08-30T20:52:09.730

The focus is on the line that creates a new StringBuilder instance. The code causes an allocation for sb.ToString() and internal allocations within the StringBuilder implementation, but you cannot control those allocations if you want the string result.

Well according to example, they ignored their own words. Better would be to just cache it and reuse it (clean it before use). No allocations except those what is needed:

    public static string GenerateFullTypeName(string name, int arity)
    {
        //StringBuilder sb = new StringBuilder();
        StringBuilder sb = cached.Value;
        sb.Clear();

        sb.Append(name);
        if (arity != 0)
        {
            sb.Append("<");
            for (int i = 1; i < arity; i++)
            {
                sb.Append("T"); sb.Append(i.ToString()); sb.Append(", ");
            }
            sb.Append("T"); sb.Append(arity.ToString()); sb.Append(">");
        }

        //return sb.ToString();
        /* Use sb as before */
        return sb.ToString();
    }

    [ThreadStatic]
    private static Lazy<StringBuilder> cached = new Lazy<StringBuilder>(()=> new StringBuilder());

Also, I think this is bery bad example of how GC can harm your application performance. Temporal and short string is basicaly never enter 2nd generation and will be disposed shortly. Better would be something like buffers for WCF transfer streams, with returning this buffer in pool, or how Task works in general (same idead) and allocate their intestines, but not StringBuilder, duh.

agree, using Lazy is better than my version. But I don't understand why the original methods of `AcquireBuilder` and `GetStringAndReleaseBuilder` related to `StringBuilder`, which it said is read example for VS. — Pingpong, Aug 30 '17 at 21:12
If multithreaded string building is required, you can use ThreadLocal, clearing after acquiring the built string. I use a global version of that in production code in a handful of places where allocating StringBuilders over and over again would affect performance. This does in fact help reduce GC pressure in applicable scenarios. — Koby Duck, Aug 30 '17 at 22:23

T.S. · Answer 2 · 2017-08-30T21:13:19.843

0

Here is the answer to "original methods are not thread-safe"

Basically, what author done is - marked property with ThreadStaticAttribute, which makes it thread safe because value will be avail only for this thread. Different thread will have different value/reference. And this "cache" will live only for the duration of the life of the thread itself. Even if the method itself is not thread-safe, the value it accesses is.

Now, I don't think, it is generally a great example because what is the sense of this? You keep around an instance of sting builder that you clean out anyway.

If your particular interest in per thread static value, ThreadStaticAttribute is nice thing to have. If you more interested in thread safe static methods, look into lock

private static MyClass _myClass;
private static object _lock = new object();

public static MyClass GetMyClass()
{
    if (_myClass == null)
    {
        lock(_lock)
        {
            if (_myClass == null)
            {
                _myClass = new MyClass();
            }
        }

    }
    return _myClass;
}

edited Aug 30 '17 at 21:13

answered Aug 30 '17 at 20:47

T.S.

18,195
11
58
78

I know this. Thanks. But I don't understand why the original methods of AcquireBuilder and GetStringAndReleaseBuilder related to StringBuilder, which it said is read example for VS. – Pingpong Aug 30 '17 at 21:13
@Pingpong This is just bad example. It makes no sense in real world. You don't create and keep around something like `StringBuilder` while constantly cleaning it. A good example would be something like keeping some sort of execution context, which is different for each thread. In the beginning of thread execution you set this context and use that static property along the way. – T.S. Aug 30 '17 at 21:18
You have the right idea, but it's not thread safe on some platforms. See [this thread](https://stackoverflow.com/questions/5958767/is-double-checked-locking-is-broken-a-java-only-thing). Note that Lazy achieves the same thing. – Koby Duck Aug 30 '17 at 22:18

Ivan R. · Answer 3 · 2017-08-30T22:36:29.573

This example shows only main idea and does not dive too deep. Let extend our class with new methods to add namespace.

public string GenerateFullTypeName(string name, int arity, string @namespace)
{
    StringBuilder sb = AcquireBuilder();
    sb.Append(this.GenerateNamespace(@namespace));
    sb.Append(this.GenerateFullTypeName(name, arity));
    return GetStringAndReleaseBuilder(sb);
}

public string GenerateNamespace(string @namespace)
{
    StringBuilder sb = AcquireBuilder();

    sb.Append(@namespace);
    sb.Append(".");

    return GetStringAndReleaseBuilder(sb);
}

And test it Console.WriteLine(test.GenerateFullTypeName("SomeType", 3, "SomeNamespace")); Original code works as expected (output string is SomeNamespace.SomeType<T1, T2, T3>), but what would be happen if we apply your “optimization”? Output string would be wrong (SomeType<T1, T2, T3>SomeType<T1, T2, T3>), because we would use only one (cashed) instance of StringBuilder for all methods in this class even this instance still in use. So that is why instance is stored in field only after using and deleted from field if it is in use again.

StringBuilder with Caching, ThreadStatic

3 Answers3