Use ValueStringBuilder in preference to StringBuilderCache #26934

benaadams · 2019-09-28T17:40:33Z

Saves 44MB of allocations for the 20k Exceptions thrown async, caught, captured, .Throw(), caught and then throw https://gist.github.com/benaadams/49633777609ed91e8b41624b72026bf9

Before

After

src/System.Private.CoreLib/shared/System/Exception.cs

jkotas · 2019-09-28T17:52:06Z

We are trying to get rid of the existing usages of StringBuilderCache, not add more of them.

Also, the StringBuilderCache is 360 chars max, and the stacktrace is often going to bigger than that.

jkotas · 2019-09-28T18:00:18Z

the StringBuilderCache is 360 chars max, and the stacktrace is often going to bigger than that.

And when it misses this limit, it will turn into Gen2 garbage that has worse side-effect than a bit of Gen0 garbage.

benaadams · 2019-09-28T18:13:01Z

ValueStringBuilder instead then?

EgorBo · 2019-09-28T18:17:38Z

I wish "objects on stack" feature supported StringBuilder

jkotas · 2019-09-28T18:32:05Z

Yes, ValueStringBuilder would be better.

benaadams · 2019-09-28T23:56:37Z

Had to add .AppendFormat to ValueStringBuilder not to have a different set of allocations popup

benaadams · 2019-09-29T00:27:28Z

Updated summary:

Saves 44MB of allocations for the 20k Exceptions thrown async, caught, captured, .Throw(), caught and then throw https://gist.github.com/benaadams/49633777609ed91e8b41624b72026bf9

Before

After

jkotas · 2019-09-29T00:54:17Z

Had to add .AppendFormat to ValueStringBuilder

That is a lot of code to add for saving allocations on exceptional paths. It does not feel worth it to me.

benaadams · 2019-09-29T00:58:22Z

Also changed string.Format to use it rather than the StringBuilderCache and it should ease the removal of StringBuilderCache?

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.cs

src/System.Private.CoreLib/shared/System/String.Manipulation.cs

jkotas · 2019-09-29T01:19:49Z

Also changed string.Format to use it rather than the StringBuilderCache

Ok, that is a convincing argument.

benaadams · 2019-09-29T04:05:52Z

I've also changed all of the less unusual uses of StringBuilderCache to use ValueStringBuilder now (not the ones that pass the cached StringBuilder around)

src/System.Private.CoreLib/shared/System/IO/Path.cs

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

jkotas · 2019-09-29T05:09:13Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.cs

        {
+            if (s is null) return;


Does enough places pass null into this to make this worth it?

(This is aggressive inlined everywhere.)

.ToString on object returning a string? doesn't help :) But no, putting the guards in the call sites.

.ToString on object returning a string? doesn't help :)

Other than in advanced implementations like Format using this, where is ToString being passed to Append on types that don't override it?

.ToString in JoinCore<T> on T; .ToString in JoinCore on object etc changes in b232228

Are basically on object or T

Right, that's what I meant by advanced implementations, helpers in core types like string itself. And such methods need to check for null, anyway. We've had bugs filed in the past because sometimes they haven't, e.g.
https://github.com/dotnet/coreclr/issues/6785
dotnet/roslyn#34085
Whether it's the caller or callee checking, someone needs to check.

There is a lot of places that need the null check. I am thinking it would be better to just keep it inside the method. @benaadams @stephentoub Do you have an opinion?

I'd prefer it inside:

Better matches StringBuilder

Easier to consume

Doesn't add a lot of complexity

Will lead to less IL (if not asm) due to number of call sites that needed the check

jkotas · 2019-09-30T16:11:03Z

src/System.Private.CoreLib/shared/System/String.Manipulation.cs

@@ -159,15 +159,16 @@ public static string Concat<T>(IEnumerable<T> values)

                    // Create the StringBuilder, add the chars we've already enumerated,
                    // add the rest, and then get the resulting string.
-                    StringBuilder result = StringBuilderCache.Acquire();
+                    Span<char> initialBuffer = stackalloc char[64];


The stack reserved sizes in this file can be probably be a bit higher, like 128 chars.

We've been fairly loose / non-prescriptive with how we do stackallocs like this in the past. In various places around corelib, we use various limits for stackalloc char when we don't know the actual upper limit, including 256, 128, 64, and 32 (lots of other sizes are used of course when we do have an upper bound). Not as part of this change, but it seems we should probably standardize with some const somewhere that everyone just uses.

I agree that a general reasonable sized buffer constant would be useful.

We often know the typical maximum size, e.g. 32-chars for a number formatting. I assume that we would keep these scenario specific maximum sizes.

I assume that we would keep these scenario specific maximum sizes.

Yup.

Might also want to harmonize with the jit's threshold for converting fixed sized locallocs to locals, which is currently 32. Back when I set this I did not see many > 32 cases.

Here we have char, so that threshold would be 16 (= 32 bytes)?

jkotas · 2019-09-30T16:11:53Z

cc @JeremyKuhne Is this change compatible with what we are planning around more efficient formatting?

JeremyKuhne · 2019-09-30T17:21:39Z

src/System.Private.CoreLib/shared/System/String.Manipulation.cs


                    do
                    {
                        currentValue = en.Current;

                        if (currentValue != null)
                        {
-                            result.Append(currentValue.ToString());
+                            string? str = currentValue.ToString();


There is a chance that the T supports ISpanFormattable. Calling the object overload should avoid the additional string allocation in that case.

Yes, it should be done in separate change.

It's certainly possible, but I have to wonder how common it actually is. Calling Concat (not Join) with a bunch of integers, floating-point values, Guids, DateTimes, and TimeSpans is going to result in a bunch of gobbledeegook, as there won't be any separators between the values. Do you have an example where this would happen and be worth optimizing for? My gut would be to avoid the interface check as it actually succeeding would be super rare.

I'm thinking more of the future- in theory we'd want to make this interface public eventually, no?

I'm thinking more of the future

Even then, what's an example where this would be valuable? It would need to be a T where performance is important and where TryFormat would include the necessary punctuation or spacing to make Concat a useful rendering.

In trying to find such examples (that might implement ISpanFormattable in the future), I'm not finding any... in fact I'm finding very little usage of this overload in general.

If you have such cases in mind, I'd be interested in seeing them.

Thanks.

It would need to be a T where performance is important ...

And then you probably wouldn't want to be using an IEnumerable<T> overload?

JeremyKuhne · 2019-09-30T17:23:48Z

Is this change compatible with what we are planning around more efficient formatting?

Yes, this would.

cc: @jaredpar

jaredpar · 2019-10-01T04:09:46Z

Is this change compatible with what we are planning around more efficient formatting?

@jcouv

stephentoub · 2019-10-01T14:50:14Z

src/System.Private.CoreLib/shared/System/Diagnostics/StackTrace.cs

@@ -299,7 +303,8 @@ internal string ToString(TraceFormat traceFormat)
                        // Append original method name e.g. +MoveNext()
                        sb.Append('+');
                        sb.Append(methodName);
-                        sb.Append('(').Append(')');
+                        sb.Append('(');
+                        sb.Append(')');


I haven't tested... is sb.Append('('); sb.Append(')'); significantly faster than sb.Append("()");, and if so, is it in the context of this method? Otherwise, I'd just assume using the simpler call.

stephentoub · 2019-10-01T14:50:59Z

src/System.Private.CoreLib/shared/System/Exception.cs

+            if (_innerException != null)
+            {
+                innerException = _innerException.ToString();
+            }


string? innerException = _innerException?.ToString();

?

stephentoub · 2019-10-01T14:51:50Z

src/System.Private.CoreLib/shared/System/Exception.cs

+                innerException = _innerException.ToString();
+            }
+
+            int initialLength = 128;


Where does this 128 come from? Would be worth a comment, even if it's just to say it's an arbitrary guess at the size required.

Might be clearer to just have + 128 when calling the constructor.

stephentoub · 2019-10-01T14:53:38Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/Vector128_1.cs

-                 .Append(' ');
+                sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant));
+                sb.Append(',');
+                sb.Append(' ');


Same question/comment as earlier... I wonder if sb.Append(", "); would be better, just for simplicity reasons.

stephentoub · 2019-10-01T14:58:48Z

src/System.Private.CoreLib/shared/System/String.Manipulation.cs

@@ -646,17 +667,41 @@ public static string Join(string? separator, IEnumerable<string?> values)
                }

                // Null separator and values are handled by the StringBuilder


Per the other conversation we should probably change Append to accept nulls again. But if we don't, this comment will need to be updated.

stephentoub · 2019-10-01T15:00:55Z

src/System.Private.CoreLib/shared/System/String.Manipulation.cs

+                            result.Append(str);
+                        }
+                    }
+                    while (en.MoveNext());


I assume this duplication can be reverted once Append is changed to allow null? (Even if it's not, I think it's worth avoiding the duplication and just having the explicit null check on each append for the separator.)

stephentoub · 2019-10-01T15:05:07Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+            if (provider != null)
+            {
+                cf = (ICustomFormatter?)provider.GetFormat(typeof(ICustomFormatter));
+            }


var cf = (ICustomFormatter?)provider?.GetFormat(typeof(ICustomFormatter));

?

stephentoub · 2019-10-01T15:06:13Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+
+            int pos = 0;
+            int len = format.Length;
+            char ch = '\x0';


Nit: '\0'?

Was just a copy paste from StringBuilder https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Text/StringBuilder.cs#L1549

But makes sense to change.

stephentoub · 2019-10-01T15:07:23Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                            break;
+                        }
+                    }
+                    // If it neither then treat the character as just text.


Nit: "If it neither" => "If it's neither"
Also it'd be nice if there were a blank line above this.

stephentoub · 2019-10-01T15:07:41Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                        }
+                    }
+                    // Is it a opening brace?
+                    if (ch == '{')


stephentoub · 2019-10-01T15:08:17Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                //  Index ::= ('0'-'9')+ WS*
+                //
+                pos++;
+                // If reached end of text then error (Unexpected end of text)


Nit: blank line above this

stephentoub · 2019-10-01T15:08:33Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                    ch = format[pos];
+
+                    pos++;
+                    // Is it a closing brace?


Nit: blank line above this

stephentoub · 2019-10-01T15:09:04Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                //
+                // Start of parsing of Argument Hole.
+                // Argument Hole ::= { Index (, WS* Alignment WS*)? (: Formatting)? }
+                //


Nit: can you remove the //s followed by nothing?

stephentoub · 2019-10-01T15:09:23Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                    {
+                        FormatError();
+                    }
+                    ch = format[pos];


Nit: blank line above this

stephentoub · 2019-10-01T15:09:48Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                    {
+                        FormatError();
+                    }
+                    // Parse alignment digits.


Nit: blank line above this

stephentoub · 2019-10-01T15:10:00Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+                    {
+                        width = width * 10 + ch - '0';
+                        pos++;
+                        // If reached end of text then error. (Unexpected end of text)


Nit: blank line above this. I'll stop commenting on these.

stephentoub · 2019-10-01T15:11:08Z

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs

+            }
+        }
+
+        private static void FormatError()


Nit: "FormatError" => "ThrowFormatException"

stephentoub · 2019-10-01T15:12:17Z

src/System.Private.CoreLib/shared/System.Private.CoreLib.Shared.projitems

@@ -822,6 +822,7 @@
    <Compile Include="$(MSBuildThisFileDirectory)System\Text\UTF7Encoding.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\Text\UTF8Encoding.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\Text\ValueStringBuilder.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Text\ValueStringBuilder.AppendFormat.cs" />


Can we remove StringBuilderCache.cs from corelib as well, or are there still other uses?

There's still a few other uses; they are more problematic ones as one method gets the cached string builder, then it returns it, passes to other methods, then releases it later. Didn't tackle them as it would over-complicate the review.

stephentoub

Thanks for working on this.

src/System.Private.CoreLib/shared/System/Exception.cs

stephentoub · 2019-10-25T16:57:04Z

Replaced by #27441

EgorBo reviewed Sep 28, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Exception.cs Outdated Show resolved Hide resolved

benaadams added 4 commits September 29, 2019 00:18

Use StringBuilderCache for Exceptions

3d6a7a3

Feedback

861e503

Use ValueStringBuilder instead

8a93a3f

Add .AppendFormat to ValueStringBuilder

d2be6bc

benaadams force-pushed the Exceptions-StringBuilderCache branch from 89c0a10 to d2be6bc Compare September 28, 2019 23:44

benaadams changed the title ~~Use StringBuilderCache for Exceptions~~ Use ValueStringBuilder for Exceptions Sep 28, 2019

jkotas reviewed Sep 29, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.cs Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/String.Manipulation.cs Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/String.Manipulation.cs Outdated Show resolved Hide resolved

benaadams changed the title ~~Use ValueStringBuilder for Exceptions~~ Use ValueStringBuilder in preference to StringBuilderCache Sep 29, 2019

Use ValueStringBuilder in preference to StringBuilderCache

f0c2a71

jkotas reviewed Sep 29, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/IO/Path.cs Outdated Show resolved Hide resolved

Increase path to 260

3ac1100

jkotas reviewed Sep 29, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Text/ValueStringBuilder.AppendFormat.cs Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2019

View reviewed changes

Adjust naullablitiy

b232228

jkotas reviewed Sep 30, 2019

View reviewed changes

JeremyKuhne reviewed Sep 30, 2019

View reviewed changes

stephentoub reviewed Oct 1, 2019

View reviewed changes

danmoseley reviewed Oct 1, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Exception.cs Show resolved Hide resolved

stephentoub mentioned this pull request Oct 25, 2019

Use ValueStringBuilder instead of StringBuidler{Cache} in multiple places #27441

Merged

stephentoub closed this Oct 25, 2019

benaadams deleted the Exceptions-StringBuilderCache branch October 25, 2019 17:00

		@@ -646,17 +667,41 @@ public static string Join(string? separator, IEnumerable<string?> values)
		}

		// Null separator and values are handled by the StringBuilder

Use ValueStringBuilder in preference to StringBuilderCache #26934

Use ValueStringBuilder in preference to StringBuilderCache #26934

Conversation

benaadams commented Sep 28, 2019 • edited Loading

jkotas commented Sep 28, 2019

jkotas commented Sep 28, 2019 • edited Loading

benaadams commented Sep 28, 2019

EgorBo commented Sep 28, 2019

jkotas commented Sep 28, 2019

benaadams commented Sep 28, 2019

benaadams commented Sep 29, 2019 • edited Loading

jkotas commented Sep 29, 2019

benaadams commented Sep 29, 2019

jkotas commented Sep 29, 2019

benaadams commented Sep 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfoidl Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne commented Sep 30, 2019

jaredpar commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub commented Oct 25, 2019

benaadams commented Sep 28, 2019 •

edited

Loading

jkotas commented Sep 28, 2019 •

edited

Loading

benaadams commented Sep 29, 2019 •

edited

Loading

stephentoub Sep 29, 2019 •

edited

Loading

stephentoub Sep 30, 2019 •

edited

Loading

gfoidl Sep 30, 2019 •

edited

Loading

stephentoub Oct 1, 2019 •

edited

Loading

stephentoub Oct 1, 2019 •

edited

Loading