Shrink StringValues

[benaadams]

Use a single field to shrink by 8 bytes; shrinking ResponseHeaders by 272 bytes and RequestHeaders by 352 bytes; or 624 bytes total (e.g. per connection).

Before

After

                     Method |      Mean |     Error |    StdDev |          Op/s | Allocated |
 -------------------------- |----------:|----------:|----------:|--------------:|----------:|
-              ClearHeaders | 17.307 ns | 0.0695 ns | 0.0650 ns |  57,781,381.7 |       0 B |
+              ClearHeaders |  9.013 ns | 0.0243 ns | 0.0227 ns | 110,945,805.6 |       0 B |
-               Ctor_String |  3.687 ns | 0.0737 ns | 0.0932 ns | 271,251,797.5 |       0 B |
+               Ctor_String |  1.804 ns | 0.0065 ns | 0.0057 ns | 554,381,580.7 |       0 B |
-                Ctor_Array |  3.840 ns | 0.0122 ns | 0.0115 ns | 260,437,356.7 |       0 B |
+                Ctor_Array |  2.148 ns | 0.0076 ns | 0.0071 ns | 465,469,766.3 |       0 B |
- Index_FirstElement_String |  3.518 ns | 0.0184 ns | 0.0163 ns | 284,224,537.0 |       0 B |
+ Index_FirstElement_String |  2.647 ns | 0.0079 ns | 0.0074 ns | 377,742,529.8 |       0 B |
-  Index_FirstElement_Array |  4.185 ns | 0.0156 ns | 0.0146 ns | 238,951,422.2 |       0 B |
+  Index_FirstElement_Array |  2.711 ns | 0.0092 ns | 0.0077 ns | 368,895,093.2 |       0 B |
-              Count_String |  2.364 ns | 0.0075 ns | 0.0067 ns | 423,040,364.6 |       0 B |
+              Count_String |  2.439 ns | 0.0067 ns | 0.0049 ns | 410,002,357.0 |       0 B |
-               Count_Array |  2.448 ns | 0.0157 ns | 0.0139 ns | 408,524,150.6 |       0 B |
+               Count_Array |  3.067 ns | 0.0084 ns | 0.0074 ns | 326,031,846.5 |       0 B |
-            ForEach_String | 13.759 ns | 0.0395 ns | 0.0370 ns |  72,681,850.1 |       0 B |
+            ForEach_String | 13.759 ns | 0.0256 ns | 0.0227 ns |  72,678,544.0 |       0 B |
-             ForEach_Array | 23.041 ns | 0.0673 ns | 0.0597 ns |  43,401,774.7 |       0 B |
+             ForEach_Array | 21.331 ns | 0.0388 ns | 0.0303 ns |  46,879,326.7 |       0 B |
-               PassByValue |  5.475 ns | 0.0214 ns | 0.0200 ns | 182,652,278.6 |       0 B |
+               PassByValue |  4.615 ns | 0.0049 ns | 0.0036 ns | 216,697,441.6 |       0 B |

Resolves #1290

[Tratcher]

@davidfowl we've tried this before. Why did we abandon it last time?

[davidfowl]

@davidfowl we've tried this before. Why did we abandon it last time?

The last time we made the struct bigger by adding and encoding and a byte[] along with a string and string[] fields making the struct too big which resulting in tons of large copies.

This change is good! I still want to look at what it would take to store utf8bytes as well.

[benaadams]

Follow up would be to drop the ins from Kestrel apis (which are all internal apis) as StringValues is now pointer sized.

[halter73]

The last time we made the struct bigger by adding and encoding and a byte[] along with a string and string[] fields making the struct too big which resulting in tons of large copies.

@davidfowl @benaadams @JunTaoLuo

Back then we experimented with that, we tried changing the StringValues struct to include only a single _value object and type-checking to determine if _value was null, string, string[] or StringsAndEncodedValues. Basically what this PR does. It improved perf significantly in our benchmarks.

Then we tried naively leaving three typed fields (string _value, string[] _values and byte[] _encodedValues) and changing StringValues to a class. This yielded just-as-good-if-not-better performance than making StringValues a single-object StringValues, and also made the code a bit simpler since it checked if each field was set instead of type-checking a single _value object.

I think the reason we never made the change to make StringValues a single-object struct before is the fact that we had the "better" fix of changing StringValues to class that was too breaking ready to go.

Now that we're approaching 3.0, can we just make the breaking change and make StringValues a class? We still wouldn't need to allocate for null (since the StringValue could actually be null), and in the other cases your generally allocating strings or string arrays anyway. If you're using cached strings/string arrays, you could instead use cached StringValues.

[benaadams]

Now that we're approaching 3.0, can we just make the breaking change and make StringValues a class? We still wouldn't need to allocate for null (since the StringValue could actually be null)

If you wanted to add Nullability annotations it would make it horrible as every callsite would have to be StringValues?; whereas with the struct you only need to annotate StringValues itself.

Also it pushes null up into every usage e.g. value.Count vs value?.Count ?? 0

[benaadams]

Another expression of this is you can happily do

var charCount = RequestHeaders["random-header"].ToString().Length;
var elementCount = RequestHeaders["random-header"].ToArray().Count();

Whereas if it was nullable then it should be more like?

var charCount = RequestHeaders["random-header"]?.ToString()?.Length ?? 0;
var elementCount = RequestHeaders["random-header"]?.ToArray()?.Count ?? 0;

[halter73]

Even without nullability annotations, it's probably too big a change to be worth trying this late in the 3.0 dev cycle.

I also cannot guarantee that a StringValues class still benchmarks better than a single-object StringValues struct, but that's how I remember things from over three years ago before 1.0 when we first looked into it, and I don't have any specific reason to suspect this has changed.

I just figured I'd bring it up the possibility, so everyone remembers making StringValues a class might be an option. Who knows, maybe the change is easier than I think it would be.

[halter73]

Also, if we're making breaking changes you could make a ToArray() extension method that allows for null. You're probably SOL for ToString() though 😉