Remove some of usages of StringBuilderCache from System.Runtime.Extensions by leotsarev · Pull Request #30072 · dotnet/corefx

leotsarev · 2018-06-02T09:03:46Z

fixes #29997
That's a work in progress to get initial feedback about removing.
Checklist

System.ApplicationId
System.Environment.ExpandEnvironmentVariablesCore()

ExpandEnvironmentVariablesCore benchmark

	Metric	Unit	Iterations	Average	STDEV.S	Min	Max
BEFORE	Duration	msec	16	655,7176231	83,74468942	616,708306	923,3642414
AFTER	Duration	msec	16	644,7916818	41,45682354	613,3610511	778,3421872
BEFORE	Allocation	bytes	16	48000000	0	48000000	48000000
AFTER	Allocation	bytes	16	48000000	0	48000000	48000000

…y.HtmlDecode

ViktorHofer

The JIT is pretty good with optimizing stackallocing constant buffer sizes. Because of that I would remove the variable part and always alloc up to a the threshold like 255, as it makes no measurable difference if you alloc 128 or 255 chars.

…nt.Unix.ExpandEnvironmentVariablesCore

ViktorHofer · 2018-06-02T09:16:22Z

src/System.Runtime.Extensions/src/System/ApplicationId.cs

+                hexOrder[j++] = HexDigit(digit);
            }
-            return result;
+            return new string(hexOrder);


If you know the exact number of chars you can use the new string.Create overload with Span which is the absolute fastest way to build a string.

Are you talking about
public static string Create<TState>(int length, TState state, SpanAction<char, TState> action) ?
How I can use it — I couldn't find any clue for now...

From unit test:

char[] input = expected.ToCharArray(); string result = string.Create(input.Length, input, (span, state) => { Assert.Same(input, state); for (int i = 0; i < state.Length; i++) { span[i] = state[i]; } });

That's really the faster than new string(span) ??

Yes, that's the fastest way to create a string. For more info around that read the midsection about Span in the Runtime from @stephentoub: https://msdn.microsoft.com/en-us/magazine/mt814808.aspx

Of course without creating the char array from the unit test... You just need the length, you want to avoid heap allocations where possible

Changes pushed, thanks for enlightening.
Am I right that we are relying that JIT will optimize out allocation of lambda here?

cc @stephentoub for delegate costs.

…y.HtmlEncode

…r of constant buffer

ViktorHofer · 2018-06-02T10:40:26Z

src/System.Runtime.Extensions/src/System/ApplicationId.cs

-                result = new string(hexOrder);
+            if (sArray == null) return null;
+
+            Span<char> hexOrder = stackalloc char[sArray.Length * 2];


you want to be defensive here to avoid a stack overflow. I don't know what are the usual sizes of the given array is, but a good rule is to limit the stackalloc size to ~255 chars. As already commented below, if you know the fixed size to begin with and it's not too large for allocating on the stack you probably want to use string.Create instead.

stackalloc removed here

stephentoub · 2018-06-02T11:34:32Z

src/System.Runtime.Extensions/src/System/ApplicationId.cs

        public override string ToString ()
        {
-            StringBuilder sb = StringBuilderCache.Acquire();
+            Span<char> charSpan = stackalloc char[255];


What's a typical length of these ApplicationIds? 255 is fine, but if they're all going to be much less than that, it'd be nice to save on the stack space by using 128 or something smaller (but only if we think that the 99% case will fit into that smaller size).

culture="en-us", version="1.0.0.0", publicKeyToken="43cb1e8e7a352766", processorArchitecture="x86"
has 98 chars, leaving 30 chars for application name. I don't have a statictics for application names, but I met already 38 char exe name in wild. I'm rather go to 255. What's your decision?

Works for me. thanks!

I met already 38 char exe name in wild

As Stephen said, this is about optimizing for the 99% case. I think that it is safe to say that 99% of the .exes out there have name shorter than 38 chars.

@ViktorHofer you said

The JIT is pretty good with optimizing stackallocing constant buffer sizes

Could you explain? I'm not familiar.

https://github.com/dotnet/coreclr/issues/8542

stephentoub · 2018-06-02T11:36:26Z

src/System.Runtime.Extensions/src/System/ApplicationId.cs

-                result = new string(hexOrder);
-            }
-            return result;
+            });


Nice use of string.Create to avoid the char[] allocation.

Kudos to @ViktorHofer

It would be better to append the characters directly to the ValueStringBuilder, and completely avoid the String allocation.

stephentoub · 2018-06-02T11:40:23Z

src/System.Runtime.Extensions/src/System/Net/WebUtility.cs

-            StringBuilder sb = StringBuilderCache.Acquire(value.Length);
-            HtmlEncode(value, index, sb);
-            return StringBuilderCache.GetStringAndRelease(sb);
+            Span<char> charSpan = stackalloc char[value.Length];


This is dangerous. value could be really big, in which case we'd try to stackalloc that much space and could easily result in a stack overflow / crash. This should instead just using a fixed amount, e.g. 256, or not use stack space at all and just pass an empty span to ValueStringBuilder. Or update ValueStringBuilder with a capacity constructor.

Note, though, that @benjamin-hodgson already has a PR that addresses WebUtility at #27250, so it probably makes sense to just undo these WebUtility.cs changes and let his PR handle that.

For the sake of this PR I would recommend to just pass a stackalloc char[255] to the ValueStringBuilder and let @benjamin-hodgson fine grain the stack allocation in his PR.

@ViktorHofer, that makes me nervous, given that these changes showed a regression on throughput in his PR: #27250 (comment).

I see, that's unfortunate, I didn't know about the regression.

So, do you like me to undo WebUtility changes?

Yes, but you also need to undo the removal of the StringBuilderCache from the assembly.

of course: done

This reverts commit 79183c0.

…ebUtility.HtmlEncode" This reverts commit 5bba334.

…ebUtility.HtmlDecode" This reverts commit b745a31.

ViktorHofer · 2018-06-02T13:50:18Z

For the ApplicationId.ToString() and the Environment.ExpandEnvironmentVariablesCore (Unix) changes we would need perf measurements. Could you please do that?

jkotas · 2018-06-02T14:18:12Z

src/System.Runtime.Extensions/src/System/Environment.Unix.cs

@@ -51,7 +52,7 @@ private static string ExpandEnvironmentVariablesCore(string name)
            }
            result.Append(name.Substring(lastPos));


You can replace result.Append(name.Substring will result.Append(name.AsSpan to void unnecessary allocations while you are on it.

there's still one left in line 53

danmoseley · 2018-06-02T14:27:07Z

Thanks @leotsarev

leotsarev · 2018-06-02T15:06:49Z

Thanks everybody for input.
@ViktorHofer I'll try to do add perfomance tests

ViktorHofer · 2018-06-02T15:10:21Z

src/System.Runtime.Extensions/src/System/ApplicationId.cs

        public override string ToString ()
        {
-            StringBuilder sb = StringBuilderCache.Acquire();
+            Span<char> charSpan = stackalloc char[127];


127 is a weird number. Why not use 128 which spans from 0 - 127?

The space on the stack is aligned to pointer sized chunks anyway. If you use 127, there will be one unused byte.

128 is better.

ViktorHofer

Perf tests would be nice but not a must for me. Thanks a lot! 👍

leotsarev · 2018-06-02T17:05:58Z

@ViktorHofer I measured old & new implementation of ExpandEnvironmentVariablesCore (using existing perf test)
It shows negligible perfomance win (near margin of error), but no win on allocations. Latter makes me suspicions about should we trust this result.
However, artificially adding allocations (by allocing array into volatile field) does show difference on this benchmark.
Does anybody have a clue why this could happen?
Do you have cloud infra to run perf test "before" and "after" PR?

jkotas · 2018-06-02T17:30:02Z

Have you measured it on Unix? Note that the implementation you are changing is Unix specific. Could you please share the source to your perf test?

leotsarev · 2018-06-02T17:38:49Z

@jkotas no, but I temporary changed code to use Unix-specfic code on Windows

jkotas · 2018-06-02T17:48:26Z

Thank you for your contribution. If you get to the bottom of why the perf tests do not work properly for you, please share the results. But as Victor said we are not really worried about it given the nature of the change.

…sions (dotnet/corefx#30072) * Remove StringBuilderCache from ApplicationId * Remove StringBuilderCache in favor of ValueStringBuilder in Environment.Unix.ExpandEnvironmentVariablesCore Commit migrated from dotnet/corefx@b987a3b

leotsarev added 2 commits June 2, 2018 12:01

Remove StringBuilderCache from ApplicationId

6c472f2

Remove StringBuilderCache in favor of ValueStringBuilder in WebUtilit…

b745a31

…y.HtmlDecode

ViktorHofer reviewed Jun 2, 2018

View reviewed changes

Remove StringBuilderCache in favor of ValueStringBuilder in Environme…

b988a31

…nt.Unix.ExpandEnvironmentVariablesCore

ViktorHofer reviewed Jun 2, 2018

View reviewed changes

leotsarev added 3 commits June 2, 2018 12:19

Remove StringBuilderCache in favor of ValueStringBuilder in WebUtilit…

5bba334

…y.HtmlEncode

Remove link to StringBuilderCache

79183c0

Remove attempt to guess buffer size in ApplicationId.ToString in favo…

be76d59

…r of constant buffer

ViktorHofer reviewed Jun 2, 2018

View reviewed changes

ViktorHofer requested review from JeremyKuhne and stephentoub June 2, 2018 10:42

ViktorHofer added the area-System.Runtime.Extensions label Jun 2, 2018

ViktorHofer assigned leotsarev Jun 2, 2018

using string.Create to implement EncodeHexString

902c171

leotsarev changed the title ~~[WIP] Remove StringBuilderCache from System.Runtime.Extensions~~ Remove StringBuilderCache from System.Runtime.Extensions Jun 2, 2018

stephentoub reviewed Jun 2, 2018

View reviewed changes

leotsarev added 3 commits June 2, 2018 16:39

Revert "Remove link to StringBuilderCache"

7358c4c

This reverts commit 79183c0.

Revert "Remove StringBuilderCache in favor of ValueStringBuilder in W…

62b3cea

…ebUtility.HtmlEncode" This reverts commit 5bba334.

Revert "Remove StringBuilderCache in favor of ValueStringBuilder in W…

9ff7917

…ebUtility.HtmlDecode" This reverts commit b745a31.

leotsarev changed the title ~~Remove StringBuilderCache from System.Runtime.Extensions~~ Remove some of usages of StringBuilderCache from System.Runtime.Extensions Jun 2, 2018

jkotas reviewed Jun 2, 2018

View reviewed changes

leotsarev added 2 commits June 2, 2018 18:02

code review changes

c561aea

move ApplicationId to smaller buffer

54c2cbb

ViktorHofer reviewed Jun 2, 2018

View reviewed changes

fix buffer size for ApplicationId

c091e60

remove another substring

bd897cf

jkotas approved these changes Jun 2, 2018

View reviewed changes

ViktorHofer approved these changes Jun 2, 2018

View reviewed changes

karelz added the Hackathon Issues picked for Hackathon label Jun 2, 2018

karelz assigned AlexGhiondea and joperezr Jun 2, 2018

stephentoub approved these changes Jun 2, 2018

View reviewed changes

jkotas merged commit b987a3b into dotnet:master Jun 2, 2018

karelz added this to the 3.0 milestone Jun 2, 2018

leotsarev deleted the remove-stringbuildercache-from-system-runtime-extensions branch June 3, 2018 16:52

leotsarev mentioned this pull request Jun 3, 2018

Optimise HtmlEncode and HtmlDecode to produce less garbage #27250

Merged

		@@ -51,7 +52,7 @@ private static string ExpandEnvironmentVariablesCore(string name)
		}
		result.Append(name.Substring(lastPos));

Conversation

leotsarev commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leotsarev Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leotsarev commented Jun 2, 2018 •

edited

Loading

danmoseley Jun 2, 2018 •

edited

Loading

jkotas Jun 2, 2018 •

edited

Loading

ViktorHofer Jun 2, 2018 •

edited

Loading

ViktorHofer commented Jun 2, 2018 •

edited

Loading

jkotas Jun 2, 2018 •

edited

Loading

leotsarev Jun 2, 2018 •

edited

Loading

jkotas commented Jun 2, 2018 •

edited

Loading