Don't use readonly struct in enumerator #74

benaadams · 2015-12-24T20:17:15Z

Ref: http://codeblog.jonskeet.uk/2014/07/16/micro-optimization-the-surprising-inefficiency-of-readonly-fields/

omariom · 2015-12-25T16:19:11Z

If not mistaken, a readonly field value is only copied when the field is of value type.

benaadams · 2015-12-25T21:33:14Z

Yep, wrong fields :)

Should have been the readonly StringValues _values; in Enumerator that shouldn't be readonly

benaadams · 2015-12-25T23:38:23Z

Replaced with better change than just making it not readonly, improved MoveNext.

Before

.method public hidebysig newslot virtual final 
        instance bool  MoveNext() cil managed
{
  // Code size       113 (0x71)
  .maxstack  3
  .locals init (string[] V_0,
           string V_1)
  IL_0000:  ldarg.0
//** copy structure
  IL_0001:  ldfld      valuetype Microsoft.Extensions.Primitives.StringValues Microsoft.Extensions.Primitives.StringValues/Enumerator::_values
  IL_0006:  ldfld      string[] Microsoft.Extensions.Primitives.StringValues::_values
  IL_000b:  stloc.0
  IL_000c:  ldloc.0
  IL_000d:  brfalse.s  IL_0041
  IL_000f:  ldarg.0
  IL_0010:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0015:  ldloc.0
  IL_0016:  ldlen
  IL_0017:  conv.i4
  IL_0018:  bge.s      IL_0038
  IL_001a:  ldarg.0
  IL_001b:  ldloc.0
  IL_001c:  ldarg.0
  IL_001d:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0022:  ldelem.ref
  IL_0023:  stfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_0028:  ldarg.0
  IL_0029:  ldarg.0
  IL_002a:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_002f:  ldc.i4.1
  IL_0030:  add
  IL_0031:  stfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0036:  ldc.i4.1
  IL_0037:  ret
  IL_0038:  ldarg.0
  IL_0039:  ldnull
  IL_003a:  stfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_003f:  ldc.i4.0
  IL_0040:  ret
  IL_0041:  ldarg.0
//** copy structure
  IL_0042:  ldfld      valuetype Microsoft.Extensions.Primitives.StringValues Microsoft.Extensions.Primitives.StringValues/Enumerator::_values
  IL_0047:  ldfld      string Microsoft.Extensions.Primitives.StringValues::_value
  IL_004c:  stloc.1
  IL_004d:  ldloc.1
  IL_004e:  brfalse.s  IL_0068
  IL_0050:  ldarg.0
  IL_0051:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0056:  brtrue.s   IL_0068
  IL_0058:  ldarg.0
  IL_0059:  ldloc.1
  IL_005a:  stfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_005f:  ldarg.0
  IL_0060:  ldc.i4.m1
  IL_0061:  stfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0066:  ldc.i4.1
  IL_0067:  ret
  IL_0068:  ldarg.0
  IL_0069:  ldnull
  IL_006a:  stfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_006f:  ldc.i4.0
  IL_0070:  ret
} // end of method Enumerator::MoveNext

After

.method public hidebysig newslot virtual final 
        instance bool  MoveNext() cil managed
{
  // Code size       96 (0x60)
  .maxstack  3
  IL_0000:  ldarg.0
  IL_0001:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0006:  ldc.i4.0
  IL_0007:  bge.s      IL_000b
  IL_0009:  ldc.i4.0
  IL_000a:  ret
  IL_000b:  ldarg.0
  IL_000c:  ldfld      string[] Microsoft.Extensions.Primitives.StringValues/Enumerator::_values
  IL_0011:  brfalse.s  IL_004f
  IL_0013:  ldarg.0
  IL_0014:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0019:  ldarg.0
  IL_001a:  ldfld      string[] Microsoft.Extensions.Primitives.StringValues/Enumerator::_values
  IL_001f:  ldlen
  IL_0020:  conv.i4
  IL_0021:  bge.s      IL_0046
  IL_0023:  ldarg.0
  IL_0024:  ldarg.0
  IL_0025:  ldfld      string[] Microsoft.Extensions.Primitives.StringValues/Enumerator::_values
  IL_002a:  ldarg.0
  IL_002b:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0030:  ldelem.ref
  IL_0031:  stfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_0036:  ldarg.0
  IL_0037:  ldarg.0
  IL_0038:  ldfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_003d:  ldc.i4.1
  IL_003e:  add
  IL_003f:  stfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0044:  ldc.i4.1
  IL_0045:  ret
  IL_0046:  ldarg.0
  IL_0047:  ldc.i4.m1
  IL_0048:  stfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_004d:  ldc.i4.0
  IL_004e:  ret
  IL_004f:  ldarg.0
  IL_0050:  ldc.i4.m1
  IL_0051:  stfld      int32 Microsoft.Extensions.Primitives.StringValues/Enumerator::_index
  IL_0056:  ldarg.0
  IL_0057:  ldfld      string Microsoft.Extensions.Primitives.StringValues/Enumerator::_current
  IL_005c:  ldnull
  IL_005d:  cgt.un
  IL_005f:  ret
} // end of method Enumerator::MoveNext

Shorter code path for default single value; doesn't duplicate StringValues struct on every call (due to it being readonly) - also removes StringValues as a field.

Ref: http://codeblog.jonskeet.uk/2014/07/16/micro-optimization-the-surprising-inefficiency-of-readonly-fields/

benaadams · 2015-12-26T00:21:22Z

Also changed the enumerator ctor to take StringValues by ref rather than by copy, if its getting bigger (as per #68)

Removed the Current == null before MoveNext checks in test as isn't needed behaviour

After an enumerator is created or after the Reset method is called, the MoveNext method must be called to advance the enumerator to the first element of the collection before reading the value of the Current property; otherwise, Current is undefined.

Made Reset() a NotSupportedException as per IEnumerator.Reset Method ()

The Reset method is provided for COM interoperability. It does not necessarily need to be implemented; instead, the implementer can simply throw a NotSupportedException.

Eilon · 2015-12-28T18:16:06Z

@rynowak assigning to you to review, but also @lodejard @Tratcher FYI.

rynowak · 2015-12-28T18:19:28Z

src/Microsoft.Extensions.Primitives/StringValues.cs

-                _values = values;
-                _current = null;
+                _values = values._values;
+                _current = values._value;


When is this ever correct? Why not just leave it null, which is equally 'undefined'

StringValue with 0 and 1 count; where the array is not set. Changed move next to fit the behaviour.

rynowak · 2015-12-28T18:21:31Z

@benaadams - this seems innocuous, but is there any big picture impact here other than counting instructions?

It's likely for is always better than foreach here, so if this is showing up in profiles, that might be a a profitable optimization.

halter73 · 2015-12-28T18:28:03Z

Are we still making StringValues a class?

#68

benaadams · 2015-12-28T19:02:21Z

@rynowak yeah, MoveNext is a player aspnet/KestrelHttpServer#528 its (RPS * header count) is half the cost of the current encoding the current headers to ascii (is a higer percentage more with improved version aspnet/KestrelHttpServer#527)

rynowak · 2015-12-28T19:06:23Z

@benaadams - have you tried just using indexing?

benaadams · 2015-12-28T19:06:45Z

@halter73 might want to remeasure class vs struct with this change as it think it would change the structs performance profile significantly.

rynowak · 2015-12-28T19:07:11Z

for this PR as all the changes seem to be in the right direction

benaadams · 2015-12-28T19:09:26Z

@rynowak no; will give it a go. Still a high cost enumerator currently though :)

And much much worse if any extra fields are added to StringValues.

halter73 · 2015-12-29T00:32:11Z

Still seems to me that making StringValues a class is better:

Baseline (dev)

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.12ms    7.14ms 299.49ms   97.05%
    Req/Sec    33.35k     1.88k   51.70k    81.25%
  16003608 requests in 15.10s, 1.97GB read
Requests/sec: 1059834.71
Transfer/sec:    133.42MB


Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.58ms    9.39ms 189.98ms   96.54%
    Req/Sec    33.44k     2.42k   81.60k    89.68%
  16027269 requests in 15.10s, 1.97GB read
Requests/sec: 1061604.41
Transfer/sec:    133.64MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.41ms    8.34ms 257.14ms   96.73%
    Req/Sec    33.40k     2.29k   60.97k    86.97%
  16016600 requests in 15.10s, 1.97GB read
Requests/sec: 1060705.99
Transfer/sec:    133.53MB

Baseline w/ 4 extra fields

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     5.85ms   20.35ms 383.83ms   95.87%
    Req/Sec    30.57k     1.91k   49.79k    76.91%
  14668647 requests in 15.10s, 1.80GB read
Requests/sec: 971453.20
Transfer/sec:    122.29MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     6.50ms   21.64ms 656.77ms   94.26%
    Req/Sec    30.54k     2.15k   58.29k    80.18%
  14647515 requests in 15.10s, 1.80GB read
Requests/sec: 970087.04
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     7.48ms   23.47ms 557.37ms   92.81%
    Req/Sec    30.64k     2.35k   59.35k    81.73%
  14689311 requests in 15.10s, 1.81GB read
Requests/sec: 972838.07
Transfer/sec:    122.47MB

StringValues ref enumerator

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.48ms   15.62ms 272.92ms   95.05%
    Req/Sec    33.94k     2.02k   55.43k    77.41%
  16285974 requests in 15.10s, 2.00GB read
Requests/sec: 1078606.40
Transfer/sec:    135.78MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.53ms   14.96ms 286.60ms   95.70%
    Req/Sec    34.05k     2.11k   61.71k    79.42%
  16339947 requests in 15.10s, 2.01GB read
Requests/sec: 1082188.58

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.45ms   14.17ms 315.22ms   96.20%
    Req/Sec    33.94k     2.89k   87.37k    91.90%
  16258131 requests in 15.10s, 2.00GB read
Requests/sec: 1076719.02
Transfer/sec:    135.54MB

StringValues ref enum w/ 4 extra fields

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.18ms   33.90ms 485.72ms   91.90%
    Req/Sec    31.26k     2.71k   56.26k    77.29%
  14997417 requests in 15.10s, 1.84GB read
Requests/sec: 993234.05
Transfer/sec:    125.03MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    10.94ms   32.47ms 396.85ms   90.72%
    Req/Sec    31.24k     2.24k   43.23k    73.50%
  15002333 requests in 15.10s, 1.84GB read
Requests/sec: 993526.13
Transfer/sec:    125.07MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    10.65ms   34.30ms 392.07ms   91.64%
    Req/Sec    31.25k     2.63k   47.19k    77.83%
  14990570 requests in 15.10s, 1.84GB read
Requests/sec: 992801.37
Transfer/sec:    124.98MB

StringValues class

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.18ms    8.18ms 189.68ms   95.02%
    Req/Sec    34.79k     1.95k   50.79k    81.27%
  16695118 requests in 15.10s, 2.05GB read
Requests/sec: 1105653.43
Transfer/sec:    139.19MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.56ms    9.51ms 238.83ms   95.85%
    Req/Sec    34.77k     1.84k   58.18k    77.92%
  16695874 requests in 15.10s, 2.05GB read
Requests/sec: 1105717.48
Transfer/sec:    139.19MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.03ms    7.36ms 229.03ms   97.05%
    Req/Sec    34.84k     2.55k   79.86k    92.67%
  16694074 requests in 15.10s, 2.05GB read
Requests/sec: 1105555.81
Transfer/sec:    139.17M

StringValues class w/ 4 extra fields

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.28ms    7.99ms 221.08ms   97.14%
    Req/Sec    33.44k     2.17k   70.60k    88.46%
  16031292 requests in 15.10s, 1.97GB read
Requests/sec: 1061707.31
Transfer/sec:    133.65MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.49ms    9.15ms 220.30ms   93.86%
    Req/Sec    33.50k     1.83k   49.45k    84.28%
  16080556 requests in 15.10s, 1.98GB read
Requests/sec: 1064935.72
Transfer/sec:    134.06MB

Running 15s test @ http://10.0.0.100:5001/plaintext
  32 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.48ms    9.30ms 219.51ms   93.54%
    Req/Sec    33.50k     1.91k   56.23k    82.87%
  16077789 requests in 15.10s, 1.98GB read
Requests/sec: 1064806.70
Transfer/sec:    134.04M

Generally only one value

benaadams · 2015-12-29T09:49:19Z

@rynowak sped up Count for the common case so indexing works better

rynowak · 2015-12-29T16:30:18Z

8c5e87a

rynowak · 2015-12-29T16:30:36Z

Merged for now. If this becomes a class again, we can simplify the enumerator code.

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

This was referenced Dec 24, 2015

Faster output header handling aspnet/KestrelHttpServer#528

Closed

Constant Encoded StringValues #64 #68

Closed

benaadams changed the title ~~Don't make struct fields readonly~~ Don't use readonly struct in enumerator Dec 25, 2015

benaadams force-pushed the patch-1 branch from 90b6c63 to c61ecab Compare December 25, 2015 23:36

Don't use readonly struct in enumerator

1c1e21b

Ref: http://codeblog.jonskeet.uk/2014/07/16/micro-optimization-the-surprising-inefficiency-of-readonly-fields/

benaadams force-pushed the patch-1 branch from c61ecab to 1c1e21b Compare December 26, 2015 00:18

benaadams mentioned this pull request Dec 26, 2015

Xmas combo aspnet/KestrelHttpServer#519

Closed

Eilon assigned rynowak Dec 28, 2015

rynowak reviewed Dec 28, 2015
View reviewed changes

Faster Count

adcbdff

Generally only one value

rynowak closed this Dec 29, 2015

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

Don't use readonly struct in enumerator #74

Don't use readonly struct in enumerator #74

Uh oh!

Conversation

benaadams commented Dec 24, 2015

Uh oh!

omariom commented Dec 25, 2015

Uh oh!

benaadams commented Dec 25, 2015

Uh oh!

benaadams commented Dec 25, 2015

Uh oh!

benaadams commented Dec 26, 2015

Uh oh!

Eilon commented Dec 28, 2015

Uh oh!

rynowak Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

benaadams Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

rynowak Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

rynowak commented Dec 28, 2015

Uh oh!

halter73 commented Dec 28, 2015

Uh oh!

benaadams commented Dec 28, 2015

Uh oh!

rynowak commented Dec 28, 2015

Uh oh!

benaadams commented Dec 28, 2015

Uh oh!

rynowak commented Dec 28, 2015

Uh oh!

benaadams commented Dec 28, 2015

Uh oh!

halter73 commented Dec 29, 2015

Baseline (dev)

Baseline w/ 4 extra fields

StringValues ref enumerator

StringValues ref enum w/ 4 extra fields

StringValues class

StringValues class w/ 4 extra fields

Uh oh!

benaadams commented Dec 29, 2015

Uh oh!

rynowak commented Dec 29, 2015

Uh oh!

rynowak commented Dec 29, 2015

Uh oh!

Uh oh!