Consider pooling buffer within StringBuilder

[AlexRadch]

Some issues request string builders with less memory allocations. See dotnet/corefx#13875 (comment) and https://github.com/dotnet/corefx/issues/13893
I suggest rewrite StringBuilder to use ArrayPool internal and also add next API to StringBuilder

public class StringBuilder
{

    public string ToStringAndRelease(); // return result String and release buffers to ArrayPool 
    public string ToStringAndRelease(int index, int count); // return result String and release buffers to ArrayPool
}

[stephentoub]

I don't think we should modify StringBuilder in place to use ArrayPool. There are likely millions of lines of code that use StringBuilder today with its existing API surface area, none of which would release the internal buffer back to the pool, which means such a change would cause every existing usage to become a leak from the pool. We should instead make whatever we do opt-in in some fashion.

[AlexRadch]

@stephentoub yes, pool will have leak in user code. But this will not down grade existing code. And if standard library will use ToStringAndRelease, then in many cases buffers will return to pool. So I think it will increase performance.
Also StringBuilder can return such buffers to pool before destruction to prevent leak.

[stephentoub]

But this will not down grade existing code

It will likely degrade existing code (I'm assuming that's what you meant by down grade). Existing code using StringBuilder and ArrayPool will find that ArrayPool has to allocate more buffers. Whereas previously StringBuilder was having to allocate buffers that were more likely to end in lower GC generations, the buffers getting thrown away from ArrayPool are more likely to be in higher generations, so we'd essentially end up getting rid of "better" buffers than we would before.

[AlexRadch]

Also StringBuilder can return buffers to pool before destruction to prevent leak.

[stephentoub]

Also StringBuilder can return buffers to pool before destruction to prevent leak.

StringBuilder is not IDisposable, so no existing code is doing any kind of cleanup with it. Nor is it finalizable, and making it finalizable would add cost at run time.

[stephentoub]

I've not completely thought it through, so there may be some issues with this, but we could potentially explore doing something like using pooled buffers, and when ToString is called, release the buffer but hold on to the resulting string. Then if the StringBuilder is manipulated again, get another buffer, store the previous string into it, and then continue manipulations on that. That would only leak in cases where ToString (or other such operations we hooked, maybe with interop) wasn't used, which is probably the minority case, though I've not investigated. (Note, though, that even with this it wouldn't completely address the linked comments, as there would still be the allocation of the StringBuilder itself.)

[AlexRadch]

StringBuilder is not IDisposable, so no existing code is doing any kind of cleanup with it. Nor is it finalizable, and making it finalizable would add cost at run time.

I think class can have method that called before destruction ~StringBuilder().

[stephentoub]

I think class can have method that called before destruction ~StringBuilder().

That's what I was referring to when I talked about it being finalizable. That "~StringBuilder()" is a finalizer, which gets run when the GC cleans up the instance, but itself makes every instance more expensive as it needs to be tracked specially by the runtime, guarantees that the instance will survive at least one GC generation, needs to have its finalizer invoked by a single dedicated thread for doing so, etc.

[ianhays]

I agree with Stephen here. Doing this would require a lot of effort, probably introduce bugs somewhere, and not provide all that much benefit. Using the pooled buffers and release it on ToString would probably work, but I question whether it is worth the effort just to use ArrayPool.

Closing this.