Constant Encoded StringValues #64 #68

JunTaoLuo · 2015-12-11T02:40:09Z

Issue #64

dnfclas · 2015-12-11T02:40:15Z

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;

JunTaoLuo · 2015-12-11T02:40:42Z

src/Microsoft.Extensions.Primitives/StringValues.cs

+            _bytes = null;
+        }
+
+        public static StringValues CreateConstant(string value, Encoding encoding)


I believe there is still some contention on the naming of this method?

@lodejard @davidfowl @DamianEdwards

I don't think it should have the word "Constant" in it, as constants are a special thing already and this can't create them (despite the intent). I'm happy to just have it be Create and TryGetConstant be renamed to TryGetBytes

I think "CreateXxx" and "TryGetXxx" should have the same "Xxx" in the name. The "Xxx" doesn't have to be "Constant".

That term was chosen originally because of how it was used: internal static class Constants { public static readonly StringValues TextPlain = StringValues.CreateConstant("text/plain"); }

Actually, wasn't it originally CreateBinaryConstant? How about CreateBinary and TryGetBinary? Or like you said CreateBytes and TryGetBytes?

The problem is: we don't want people to use the Create method unless they are creating a static readonly instance used the same way you use a constant.

If you have someone call StringValues.Create(foo) per-request - as if it was just an alias for the ctor - they'll actually be increasing their allocations per-request. That's why the "Constant" hint was in there: this method is for creating constants...

If you have this CreateXxx method do you even need the associated constructor?

Create/TryGetPreEncoded?

Not really, the constructor could be internal

I don't mind CreatePreEncoded/TryGetPreEncoded

muratg · 2015-12-11T16:31:04Z

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

cc @Tratcher

pranavkm · 2015-12-14T18:17:57Z

src/Microsoft.Extensions.Primitives/StringValues.cs

+        /// <param name="bytes">If successful, <paramref name="bytes"/> contains the pre-encoded bytes of the <see cref="StringValues"/>.</param>
+        /// <param name="encoding">If successful, <paramref name="encoding"/> contains the <see cref="Encoding"/> used to compute the pre-encoded bytes.</param>
+        /// <returns><c>true</c> if <see cref="StringValues"/> contains pre-encoded bytes, otherwise <c>false</c>.</returns>
+        public bool TryGetPreEncoded(out byte[] bytes, out Encoding encoding)


This method looks kinda gross.

Gross by design. Any recommendations for alternative designs?

Couldn't we make EncodedBytes a public property and add a remark that says its only available for pre-encoded values? Two outs feels like bad design.

It's better to keep them linked than in two separate properties. Otherwise you need to keep explaining the relationship between the two.

Tratcher · 2015-12-14T19:02:53Z

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

JunTaoLuo · 2015-12-16T04:12:23Z

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

Test Headers	Class Implementation	Struct Implementation
Known Headers Only, Using PreEncoding	863888.71	855962.316
Known Headers Only, No PreEncoding	849639.634	849207.826
Known and Unknown Headers, Using PreEncoding	721453.136	685609.094
Known and Unknown Headers, No PreEncoding	715319.652	704673.458

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

muratg · 2015-12-16T16:26:06Z

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

Tratcher · 2015-12-16T18:41:44Z

src/Microsoft.Extensions.Primitives/StringValues.cs

+        /// <param name="encoding">The <see cref="Encoding"/> to be use when computing pre-encoded bytes.</param>
+        /// <returns>A <see cref="StringValues"/> which contains the pre-encoded bytes.</returns>
+        public static StringValues CreatePreEncoded(string value, Encoding encoding)
+        {


We should null check the args in this scenario, we actually use them.

rynowak · 2015-12-16T20:01:12Z

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?

halter73 · 2015-12-16T20:10:06Z

@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.

rynowak · 2015-12-16T20:14:54Z

@halter73 👍

pranavkm · 2015-12-16T20:17:44Z

src/Microsoft.Extensions.Primitives/StringValues.cs


        public StringValues(string value)
        {
            _value = value;
            _values = null;
+            _encoding = null;


You shouldn't need to initialize these anymore.

benaadams · 2015-12-17T00:20:14Z

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

benaadams · 2015-12-17T00:28:47Z

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

benaadams · 2015-12-17T00:31:46Z

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

halter73 · 2015-12-17T23:09:22Z

@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

benaadams · 2015-12-17T23:35:51Z

@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...

benaadams · 2015-12-24T20:25:35Z

@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)

benaadams · 2015-12-29T10:11:14Z

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)

JunTaoLuo · 2016-01-21T23:13:55Z

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.

using statement duplicated

Fix #68 - use trace for header logging

dnfclas added the cla-already-signed label Dec 11, 2015

JunTaoLuo reviewed Dec 11, 2015
View reviewed changes

pranavkm reviewed Dec 14, 2015
View reviewed changes

JunTaoLuo force-pushed the johluo/constant-stringvalues branch from 909604e to 4befddb Compare December 16, 2015 04:25

JunTaoLuo mentioned this pull request Dec 16, 2015

Use pre-encoded headers if possible aspnet/KestrelHttpServer#491

Closed

halter73 mentioned this pull request Dec 16, 2015

Don't use Vector<byte>.Dot aspnet/KestrelHttpServer#494

Merged

Tratcher reviewed Dec 16, 2015
View reviewed changes

pranavkm reviewed Dec 16, 2015
View reviewed changes

benaadams mentioned this pull request Dec 26, 2015

Don't use readonly struct in enumerator #74

Closed

Tratcher mentioned this pull request Jan 4, 2016

Store header values as byte[] aspnet/KestrelHttpServer#169

Closed

JunTaoLuo force-pushed the johluo/constant-stringvalues branch from 5802cb3 to 9020e65 Compare January 15, 2016 00:27

JunTaoLuo added 2 commits January 21, 2016 12:02

Constant Encoded StringValues #64

c14eea0

PR

fc8e5c7

JunTaoLuo added 2 commits January 21, 2016 12:02

Change StringValues to class

9c608c8

WIP

4671bc8

JunTaoLuo force-pushed the johluo/constant-stringvalues branch from 9020e65 to 4671bc8 Compare January 21, 2016 20:03

JunTaoLuo closed this Jan 21, 2016

JunTaoLuo deleted the johluo/constant-stringvalues branch January 22, 2016 19:35

natemcmaster pushed a commit that referenced this pull request Nov 6, 2018

Merge pull request #68 from ryan1234/patch-1

b412bd4

using statement duplicated

natemcmaster pushed a commit that referenced this pull request Nov 16, 2018

Fix #68 - use trace for header logging

0ac7e42

natemcmaster pushed a commit that referenced this pull request Nov 16, 2018

Merge pull request #83 from aspnet/release/2.1

1145a51

Fix #68 - use trace for header logging

halter73 mentioned this pull request Apr 10, 2019

Shrink StringValues #1283

Merged

ghost locked as resolved and limited conversation to collaborators May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constant Encoded StringValues #64 #68

Constant Encoded StringValues #64 #68

JunTaoLuo commented Dec 11, 2015

dnfclas commented Dec 11, 2015

JunTaoLuo Dec 11, 2015

JunTaoLuo Dec 11, 2015

DamianEdwards Dec 11, 2015

lodejard Dec 11, 2015

Tratcher Dec 11, 2015

Tratcher Dec 11, 2015

lodejard Dec 11, 2015

DamianEdwards Dec 11, 2015

lodejard Dec 11, 2015

muratg commented Dec 11, 2015

pranavkm Dec 14, 2015

Tratcher Dec 14, 2015

pranavkm Dec 14, 2015

Tratcher Dec 14, 2015

Tratcher commented Dec 14, 2015

JunTaoLuo commented Dec 16, 2015

muratg commented Dec 16, 2015

Tratcher Dec 16, 2015

halter73 Dec 16, 2015

rynowak commented Dec 16, 2015

halter73 commented Dec 16, 2015

rynowak commented Dec 16, 2015

pranavkm Dec 16, 2015

benaadams commented Dec 17, 2015

benaadams commented Dec 17, 2015

benaadams commented Dec 17, 2015

halter73 commented Dec 17, 2015

benaadams commented Dec 17, 2015

benaadams commented Dec 24, 2015

benaadams commented Dec 29, 2015

JunTaoLuo commented Jan 21, 2016

Constant Encoded StringValues #64 #68

Constant Encoded StringValues #64 #68

Conversation

JunTaoLuo commented Dec 11, 2015

dnfclas commented Dec 11, 2015

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

muratg commented Dec 11, 2015

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

Tratcher commented Dec 14, 2015

JunTaoLuo commented Dec 16, 2015

muratg commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Dec 16, 2015

halter73 commented Dec 16, 2015

rynowak commented Dec 16, 2015

Choose a reason for hiding this comment

benaadams commented Dec 17, 2015

benaadams commented Dec 17, 2015

benaadams commented Dec 17, 2015

halter73 commented Dec 17, 2015

benaadams commented Dec 17, 2015

benaadams commented Dec 24, 2015

benaadams commented Dec 29, 2015

JunTaoLuo commented Jan 21, 2016