Improve ReuseableStringBuilderFactory

[davkean]

There's a bunch of improvements that I've noticed will help a lot:

• Don't evict the "current" stringbuilder if the requested capacity is larger than the max size, we'll never store it anyway
• Allow reusable string builders to be increased in capacity after created (will help this case: ExpandExpressionCaptureIntoStringBuilder is spending ~4.8 of allocations resizing the string builder #2696)
• Make sure everyone is setting a default capacity size that is sensible - it's currently 16. Expanding the string builder after it is created is about 6.6% of allocations

[danmoseley]

There is already some diagnostic logging in there in debug -- improving that and running some scenarios would be a great way to tune it to first order. Analogous to how the OpportunisticIntern class can dump stats to help adjust its thresholds. So much easier than iterating with a profiler right away.

In particular it would be interesting to log how large (both length and capacity) the builders are when they are returned (which indicates how large they needed to be, and how often they are discarded). My guess is that they are often getting discarded as too large (>1024 chars). If the threshold was higher, the default size wouldn't matter much as they would rarely be recreated. It could even be tuned, ie., if there's a lot of discards, then increase the size you accept; if not many, then there's likely less value in keeping it around.

[ladipro]

Labeling with size:1 as the cost of the initial investigation. Will open a follow-up issue if more work is identified.
Can we deprecate ReuseableStringBuilder in favor of SpanBasedStringBuilder?

[rokonec]

MSBuild currently uses two string builder reusing caches:

1. ReuseableStringBuilder/Factory - it is static process wide cache of one StringBuilder wrapper with think façade of ReuseableStringBuilder. When returning SB capacity exceed max limit of 1024 chars, returning SB is abandoned and eventually collected. If capacity is bellow that, SB is Cleared in preparation for next reuse and rooted by stored into static variable. Although ReuseableStringBuilderFactory is designed to thread safe, using it heavily concurrently by multiple threads would invalidate its purpose.
2. StringBuilderCache.Acquire/Release - this is clone of internal StringBuilderCache used at many places in runtime. It holds one SB per thread and its returning capacity limit is 360 chars.

I have instrumented code and captured returning length of both in various scenarios.
It turned out SB are mostly used for logging purposes.

In default verbosity both SB behaves acceptably although I'd recommend to increase size limit of StringBuilderCache to 1024 to get into 66% percentile of data reuse. Also MSBuild does not need to be as memory careful as Runtime.

Bellow is "histogram" of ReuseableStringBuilder returning length. Y axis is sum of returning length for given bucket.

With diagnostics logging verbosity, the situation for ReuseableStringBuilder is very different and much less optimal.

Only 47% percent of SB is reused. In other words comparing to create SB instance per call we safe only 47% of SB allocations. If we talk about allocated bytes we safes only 1.2% of allocated bytes.

Histogram looks like this:

Conclusion

It is worth to devise a better way to invalidate/reuse ReuseableStringBuilder. The solution should take into consideration different usage patterns during normal vs diagnostics.

[danmoseley]

Would it help if StringBuilder could get (and return) its backing arrays through an array pool? This would just move the caching down of course but it could be tuned to avoid any that go on LOH. It might be easier to reason about the caching strategy.
It would have to be a copy of the existing SB for now since SB is not IDisposable unfortunately. If it was generally valuable (which is not clear eg it might well make it slower) one could imagine changing SB proper eg by having Trim() return to the pool.

[rokonec]

Having SB which is backed up by array pool would be ideal, IMHO. I asked Stephen that question about half a year ago.
InterpolatedStringHandler of C# 10 uses array pool, but having ArrayPoolBackedStringBuilder which would have most of its API identical to StringBuilder would make it easy for people to migrate to it in memory heavy applications.
Although I already have some solution for our ReuseableStringBuilder, PR in day or two, having one well tuned shared ArrayPool looks appealing to me. Another interesting idea is to use @ladipro SpanBasedStringBuilder

msbuild/src/StringTools/SpanBasedStringBuilder.cs

Lines 9 to 18 in a59d7a5

	/// <summary>
	/// A StringBuilder replacement that keeps a list of <see cref="ReadOnlyMemory{T}"/> spans making up the intermediate string rather
	/// than a copy of its characters. This has positive impact on both memory (no need to allocate space for the intermediate string)
	/// and time (no need to copy characters to the intermediate string).
	/// </summary>
	/// <remarks>
	/// The <see cref="ToString"/> method tries to intern the resulting string without even allocating it if it's already interned.
	/// Use <see cref="Strings.GetSpanBasedStringBuilder"/> to take advantage of pooling to eliminate allocation overhead of this class.
	/// </remarks>
	public class SpanBasedStringBuilder : IDisposable

As this is even better, in certain uses cases, than theoretical ArrayPoolBackedStringBuilder. SpanBasedStringBuilder has one less arraycopy than ArrayPoolBackedStringBuilder which has to arraycopy from source to buffer and then from buffer to final string char[len] while SpanBasedStringBuilder just arraycopy from spans to final string char[len]. It might make difference with some huge strings. SpanBasedStringBuilder have some missing API (AppendFormat, Append(char), ...) to be used in some niche use cases though.

[danmoseley]

cc @stephentoub in case he is interested in this discussion.

I am mainly interested in case anything is learned that would be of general interest in the core libraries. (I doubt SpanBasedStringBuilder would be)

it could be tuned to avoid any that go on LOH

Never mind that part. I remembered that SB already has a max chunk size of 8KB, unless you initialize it with something smaller. So it would be necessary to establish that backing with an ArrayPool actually is helpful or is merely moving the caching around.