Hi @JunTaoLuo, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Looks good to me, apart from the CreateConstant method name contention.

cc @Tratcher

Do you have an initial commit for any other repo showing practical usage? Kestrel would consume it, MVC should set it.

Testing performance for different scenarios for StringValues implementation with class vs struct gave the following results:

Each test configuration was run 5 times without error and the RPS values here are an average of those runs. Unknown Headers used are ["epyT-tnetnoC"] = "/txetnialp" and ["htgneL-tnetnoC"] = "31".

A few observations. First, although testing in the afternoon showed differences between struct vs class implementations, retesting here indicate otherwise, though class implementation seems to be slightly better in all scenarios. Also, pre-encoding headers seems to have a small benefit in most scenarios but is detrimental for unknown headers in the struct implementation.

Full results here:
stringvalues.txt

cc @halter73 @lodejard @DamianEdwards @Tratcher

Thanks, @JunTaoLuo. These results are interesting, and slightly unexpected. Was the variance between runs small for both the class and the struct cases?

It seems like the source of regression here was adding additional fields. Did we experiment with designs where we try to minimize the number of fields? For instance omitting the encoding?

@rynowak We tried several things.

First we verified that making StringValues a class was significantly faster than the 2 field StringValues struct that existed prior to this change. We also tried out @lodejard's suggestion of making StringValues a single-field struct, but that still wasn't quite as fast (and not nearly as clean code-wise) as simply making StringValues a class.

The "Struct Implementation" results shown here are from the single-field version of the StringValues struct (which most efficient struct version according to our benchmarks). Those "Struct Implementation" benchmark results are far better than what we have in dev today. You can see the single-field struct version of StringValues here: https://gist.github.com/AspNetSmurfLab/61c40908bb316ca8f42c

Please forgive the sloppy coding in the gist, we were just trying to get benchmark results quickly.

@halter73 👍

@halter73 I'd retest the struct vs class in RPS; it should be a lot different now with this change in aspnet/KestrelHttpServer#497

Could there be an in place mutator? SetPreEncoded(byte[] bytes, Encoding encoding) though would need to go back to a struct so as not to change StringValues.Empty

Ignore the mutator comment; it will go horribly wrong :P

@benaadams The ClearFast change seems to make the difference between the struct/class versions of StrinvValues smaller, but the class version still performs better:

StringValues struct:

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency     7.09ms   22.17ms 553.19ms   94.38%
    Req/Sec    30.52k     2.09k   52.48k    76.88%
  14648023 requests in 15.10s, 1.80GB read
Requests/sec: 970078.67
Transfer/sec:    122.12MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency     9.91ms   30.88ms 400.11ms   92.26%
    Req/Sec    30.80k     2.13k   42.56k    68.38%
  14793210 requests in 15.10s, 1.82GB read
Requests/sec: 979714.96
Transfer/sec:    123.33MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency    10.63ms   33.91ms 381.54ms   93.81%
    Req/Sec    30.84k     2.43k   55.73k    77.87%
  14787069 requests in 15.10s, 1.82GB read
Requests/sec: 979280.70
Transfer/sec:    123.28MB

StringValues class:

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency     5.30ms   16.14ms 296.50ms   94.95%
    Req/Sec    32.74k     2.18k   59.47k    83.09%
  15704197 requests in 15.10s, 1.93GB read
Requests/sec: 1040031.02
Transfer/sec:    130.92MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency     5.81ms   21.21ms 388.86ms   95.70%
    Req/Sec    32.71k     2.29k   66.62k    83.89%
  15684105 requests in 15.10s, 1.93GB read
Requests/sec: 1038689.40
Transfer/sec:    130.76MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats Avg      Stdev Max   +/- Stdev
Latency     6.10ms   26.08ms 515.19ms   93.51%
    Req/Sec    32.82k     2.28k   60.88k    83.33%
  15738015 requests in 15.10s, 1.93GB read
Requests/sec: 1042350.33
Transfer/sec:    131.22MB

@halter73 can't argue with that.

However, if going class, then mutators would be nice; though Empty would have to change to

public static StringValues Empty => new StringValues(EmptyArray);

Which night have a perf impact; and null StringValues (rather than Empty) would have a problem :-/
Unless getters (like headers) also did a private set to Empty when null; however something might that may already be needed as null will now propagate round the system.

e.g.

StringValue.IsNullOrEmpty(null) before would return true, now it will throw.

What happens with context.Response.Headers[unknownHeaderName] that now returns null so all the StringValues instance methods will throw as they are called on a null reference.

The implicit conversions need to change to accept null values e.g.

public static implicit operator string (StringValues values)
{
    return values.GetStringValue();
}
public static implicit operator string[] (StringValues value)
{
    return value.GetArrayValue();
}

needs to change to

public static implicit operator string (StringValues values)
{
    if (values == null) return null;
    return values.GetStringValue();
}
public static implicit operator string[] (StringValues value)
{
    if (value == null) return EmptyArray;
    return value.GetArrayValue();
}

etc...

@halter73 is also an issue with the current struct's enumerator #74 which is likely to make a difference.

Both with StringValues being readonly and it passed to ctor by value (if its getting bigger)

I'd like to suggest a hold on this for now; there's a faster CopyFromAscii in Kestrel aspnet/KestrelHttpServer#527 and an even faster one to come when only pooled blocks are used aspnet/KestrelHttpServer#525 as one of the fixed can be dropped and the blocks can have raw pointers stored; but also Array.Copy and Buffer.BlockCopy in the framework are slow for byte arrays < 512 bytes; so this may end up being slower than the CopyFromAscii changes (though there are some workarounds for it)

After the many kestrel optimizations, testing the performance of this optimization showed that it makes little difference. I saw ~1% improvement in the plaintext benchmark and after I added 16 more headers the performance improvement was bigger but less stable, hovering between 4-7% (ee aspnet/KestrelHttpServer#584 for more details). Note that the headers added were for illustration only and in real scenarios, it does not make sense to pre-encode all of them. In the end, we decided that this optimization is no longer relevant and will not be made.