Improve string builder caches

[rokonec]

Fixes #2697

Context

See #2697

Changes Made

• ReuseableStringBuilder bracketed SB allocating
  • SB is allocated with capacity bracketed to upper power of 2 bytes = 2^n where n = upper(log2(capacity)).
  • When SB is returning and its capacity has increased it indicates that bigger SB was needed. Returning SB is abandoned and new SB, with capacity upper bounded to 2^n bytes based on returning capacity, is created and used as shared instance. Prove of past needs is used as prediction of future.
  • When SB cross upper limit of 1M chars, it is abandoned, and new SB is created by next new ReuseableStringBuilder.
  • This changes results into:
    • Low ephemeral memory segments allocation - max log2(max) allocations
    • Low LOH fragmentation - max log2(max) allocations
    • Normal verbosity allocates about same as before
    • Allocation traffic during diag verbosity file loggers was measured as ~-14%
      
• ReuseableStringBuilder debug only ETW instrumentation. I have deleted old statistics as I have found new ETW more usable.
• StringBuilderCache debug only ETW instrumentation.
• ReuseableStringBuilder and StringBuilderCache moved from Shared into Framework.
• Change StringBuilderCache max size limit.

Testing

Local testing and measurements.

Notes

• Bracketing capacity into 2^n bytes was inspired by ArrayPool and "Writing High-Performance .NET Code" book.
• We shall consider to use SpanBasedStringBuilders in most places as replacements for ReuseableStringBuilder and StringBuilderCache.

[Forgind]

Looks pretty reasonable. I'm thinking about one alternative way to resize: maintaining an array of StringBuilders of various sizes and using one that seems appropriate for the given situation. Then, if we find we need a bigger one, we can either add a pointer to a smaller one (if relevant) (don't need to copy anything or allocate) or move everything to the next larger StringBuilder (need to copy, but it avoids fragmentation and is probably simpler). I'm not sure it's worth it, but I thought I'd mention it as a possibility.

[Forgind]

Should this be the power of 2 greater than capacity?

[rokonec]

I have applied power of 2 logic only to ReuseableStringBuilder as according to captured statistics, SB from StringBuilderCache uses only small capacity and is not as heavily utilized.

[drewnoakes]

Some ideas to ensure correct usage:

• Null out fields on ReuseableStringBuilder to catch use-after-free errors.
• Ensure those fields are non-null in this method, to catch double-free errors.

Maybe just in debug builds.

[rokonec]

I have nulled those fields on Release and added some asserts.

[drewnoakes]

Suggested change

	StringBuilder returningBuilder = returning._borrowedBuilder!;
	StringBuilder returningBuilder = returning._borrowedBuilder;

The ! can be avoided here by annotating VerifyThrowInternalNull as:

internal static void VerifyThrowInternalNull([NotNull] object? parameter, string parameterName)

This looks a bit weird, but it means:

• The argument may be null when it is passed in
• The argument will not be null when normal flow continues

---

As an aside (likely for another PR) if we're on C# 10 then it may be possible to use the CallerArgumentExpressionAttribute on parameterName:

internal static void VerifyThrowInternalNull([NotNull] object? parameter, [CallerArgumentExpressionAttribute] string parameterName = null)

Meaning the call could be simplified to:

FrameworkErrorUtilities.VerifyThrowInternalNull(returning._borrowedBuilder);

[rokonec]

I would rather address this in another PR.
FrameworkErrorUtilities.VerifyThrowInternalNull needs some rethinking. It should be supporting nullable, but currently the lines:

msbuild/src/Framework/ErrorUtilities.cs

Line 23 in 93e4693

private static readonly bool s_throwExceptions = string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBUILDDONOTTHROWINTERNAL"));

along with

msbuild/src/Framework/ErrorUtilities.cs

Lines 69 to 72 in 93e4693

	if (s_throwExceptions)
	{
	throw new InternalErrorException(string.Format(message, args), innerException);
	}

breaks the rules "The argument will not be null when normal flow continues".
I would like to question usefulness of MSBUILDDONOTTHROWINTERNAL as this means "lets MSBuild fail at next line when accessing null ref object".
@rainersigwald do people use it?

[drewnoakes]

I didn't realise it was conditional. Still, I think it would be ok to annotate it as though the return value is non-null given that's clearly the intent. NRT doesn't give hard guarantees. It's mostly about communicating intent in code and make bugs more obvious.

[rainersigwald]

I agree about doing this in another PR (and closing #5163 while we're at it). I pushed up a branch with a half-hearted attempt at annotating ErrorUtilities that I put aside when hitting some of these same issues.