Less StringValue struct copies + bounds check elimination by benaadams · Pull Request #2347 · aspnet/KestrelHttpServer

benaadams · 2018-02-24T19:23:14Z

Less StringValue struct copies for header existence checks

fast in-place bitflag check
don't let null adds set bitflags in headers (as won't be output anyway)
clear for null sets in headers

Lazy calc transfer coding

calc once existence is confirmed rather than always (since existence is checked anyway)

Fast-path EnsureHostHeaderExists

for OriginForm with single host header

Skip lots of bounds checks through various methods (by helping the Jit recognize preconditions)

Update to #2014

benaadams · 2018-02-24T19:52:32Z

Pre

Post

davidfowl · 2018-02-27T23:50:56Z

        protected override bool AddValueFast(string key, StringValues value)
        {{{(loop.ClassName == "HttpResponseHeaders" ? @"
            ValidateHeaderCharacters(value);" : "")}
+            var isNotNull = value.Count > 0;


Is this the fastest way to check for emptiness?

I think so 😢

int Count => _value != null ? 1 : (_values?.Length ?? 0);

There is a pass by value static method; but looks like that does more?

public static bool IsNullOrEmpty(StringValues value) { if (value._values == null) { return string.IsNullOrEmpty(value._value); } switch (value._values.Length) { case 0: return true; case 1: return string.IsNullOrEmpty(value._values[0]); default: return false; } }

We should optimize this. Looks very branchy 😄

public bool IsNull() { var values = _values; return (_value != null) ? false : ((values == null || values.Length == 0) ? true : false); }

? 😟

dotnet/extensions#323

davidfowl · 2018-02-28T05:53:13Z

/cc @pakrym @mikeharder @Tratcher to do another pass

davidfowl · 2018-02-28T07:40:49Z

                    ThrowHeadersReadOnlyException();
                }
-                SetValueFast(key, value);
+                if (value.Count == 0)


Is this a behavior change?

No because it doesn't output null headers; however it means the bit flags can be trusted. Currently if you set the header to a null value it will set the bit flag saying it has a value; rather than changing it to say it doesn't have a value

null string/Empty.StringValues/empty array

Corner case would be if you set it to an array of null strings; then it would say it had a value with bit flag. However that's same as now (also it needs to know to clear it if its that).

However if you add nulls; StringValues coalesces them to nothing so you'd explicitly need to use the new StringValues(string[]) constructor

With dotnet/extensions#323 this changes to

value.IsNull

We should wait for this before merging.

Not flowed yet

Tratcher · 2018-02-28T23:32:12Z

        }

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private void EnsureHostHeaderExists()


Skip this method, I'm working on some changes here that will conflict.

You can fix the conflicts...

It's not that simple, my changes will negate these.

Is your change going to be done in the next few days?

It's not that simple, my changes will negate these.

Just overwrite it? Isn't a conflict (except to git) if you entirely replace it; and it give you a perf base line to measure against 😉

Want me to drop this change?

Yep, drop the change.

benaadams · 2018-03-01T18:20:22Z

NuGet unhappy

C:\projects\kestrelhttpserver\src\Kestrel\Kestrel.csproj : error NU1102: 
Unable to find package System.Threading.Tasks.Extensions with version (>= 4.5.0-preview2-26224-02) [C:\projects\kestrelhttpserver\KestrelHttpServer.sln]
C:\projects\kestrelhttpserver\src\Kestrel\Kestrel.csproj : error NU1102:   
- Found 28 version(s) in https://dotnet.myget.org/F/aspnetcore-dev/api/v3/index.json [ Nearest version: 4.5.0-preview2-26130-01 ] [C:\projects\kestrelhttpserver\KestrelHttpServer.sln]

davidfowl · 2018-03-01T18:25:54Z

Yeaaa shit is broken right now

benaadams · 2018-03-12T07:25:27Z

Rebased

benaadams · 2018-03-14T06:35:34Z

Need to validate some of my tail call Jit assertions in second commit

davidfowl · 2018-03-14T07:52:33Z

This needs to be re-reviewed. /cc @halter73 @Tratcher

davidfowl · 2018-03-14T07:54:16Z

@halter73 assigning this to you.

benaadams · 2018-03-15T00:35:51Z

[Fast tailcall decision]: Caller: HttpUtilities:ValidateHostHeader(ref)
[Fast tailcall decision]: Callee: HttpUtilities:ValidateIPv6Host(ref)
 -- Decision: Will fastTailCall (CallerStackSize: 0, CalleeStackSize: 0)

[Fast tailcall decision]: Caller: HttpUtilities:ValidateHostHeader(ref)
[Fast tailcall decision]: Callee: HttpUtilities:ValidateHostPort(ref,int)
 -- Decision: Will fastTailCall (CallerStackSize: 0, CalleeStackSize: 0)

benaadams · 2018-03-15T01:50:35Z

ResponseHeadersWritingBenchmark

Pre

 Method |                       Type |       Mean |        Op/s | Allocated |
------- |--------------------------- |-----------:|------------:|----------:|
 Output |                 LiveAspNet | 1,852.2 ns |   539,908.4 |     359 B |
 Output |           PlaintextChunked | 1,188.0 ns |   841,778.0 |     200 B |
 Output | PlaintextChunkedWithCookie | 1,618.1 ns |   618,003.6 |     491 B |
 Output |        PlaintextWithCookie | 1,260.3 ns |   793,456.3 |     432 B |
 Output |       TechEmpowerPlaintext |   845.0 ns | 1,183,439.7 |     141 B |

Post

 Method |                       Type |       Mean |        Op/s | Allocated |
------- |--------------------------- |-----------:|------------:|----------:|
 Output |                 LiveAspNet | 1,773.1 ns |   563,978.9 |     359 B |
 Output |           PlaintextChunked | 1,178.3 ns |   848,664.3 |     200 B |
 Output | PlaintextChunkedWithCookie | 1,568.7 ns |   637,472.6 |     491 B |
 Output |        PlaintextWithCookie | 1,207.4 ns |   828,224.8 |     432 B |
 Output |       TechEmpowerPlaintext |   770.1 ns | 1,298,589.7 |     141 B |

Http1ConnectionBenchmark

Pre

               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   717.2 ns | 1,394,376.1 |   1.00 |     344 B |
           LiveAspNet | 1,517.2 ns |   659,095.9 |   2.12 |    1056 B |

Post

           
               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   695.1 ns | 1,438,728.6 |   1.00 |     344 B |
           LiveAspNet | 1,507.1 ns |   663,510.4 |   2.17 |    1056 B |

davidfowl · 2018-03-15T03:14:28Z

That's a nice boost

davidfowl · 2018-03-15T03:19:41Z

        {
-            Assert.True(HttpUtilities.IsValidHostHeader(host));
+            HttpUtilities.ValidateHostHeader(host);
+            Assert.True(true);


Assert.True(true)... 😄 xunit used to have Assert.DoesNotThrow 😄

Wasn't sure whether to just put nothing in as if it threw it would catch it as an error; but that felt wrong 😄

I'd remove it and just add a comment saying this shouldn't throw.

benaadams · 2018-03-15T15:03:04Z

Did a bounds checking dance

Http1ConnectionBenchmark

Pre

               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   717.2 ns | 1,394,376.1 |   1.00 |     344 B |
           LiveAspNet | 1,517.2 ns |   659,095.9 |   2.12 |    1056 B |

Post

               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   680.0 ns | 1,470,672.2 |   1.00 |     344 B |
           LiveAspNet | 1,482.2 ns |   674,676.6 |   2.18 |    1056 B |

davidfowl · 2018-03-16T07:48:12Z

This change seems to have a very positive impact on the plaintext benchmarks.

davidfowl · 2018-03-16T07:58:46Z

-                        .MalformedRequestInvalidHeaders);
+                    BadHttpRequestException.Throw(RequestRejectionReason.MalformedRequestInvalidHeaders);
                }
-                throw;


This seems suspicious. Why is this ok? We always want to throw even in the else case?

Aha! Yes, will add back

benaadams · 2018-03-16T19:13:54Z

        protected override bool AddValueFast(string key, in StringValues value)
        {{{(loop.ClassName == "HttpResponseHeaders" ? @"
            ValidateHeaderCharacters(value);" : "")}
+            var isNotNull = value.Count > 0;


With dotnet/extensions#323 this changes to

!value.IsNull

halter73 · 2018-03-16T21:32:00Z

            {
-                if (!host.Equals(RawTarget))
+                // Tail call
+                ValidateNonOrginHostHeader(hostText);


Cool. Do we know for sure that the tail call is getting eliminated by the JIT? Or the C# compiler itself? If not, the comment could be a little misleading.

halter73 · 2018-03-16T21:58:59Z

+                                    {header.SetBit()};
+                                    _headers._{header.Identifier} = value;{(header.EnhancedSetter == false ? "" : $@"
+                                    _headers._raw{header.Identifier} = null;")}
+                                }}


This seems like a (small) breaking change to me. Prior to this, the following would throw:

context.Response.Headers.Add("Warning", new StringValues(value: null)); context.Response.Headers.Add("Warning", new StringValues(value: null));

Now it doesn't. I don't mind making this small of a breaking change, but it's not my call. @davidfowl What do you think?

If we are willing to change behavior though, we might as well just skip calling AddValueFast and completely no-op when value.IsNull instead of wasting time checking for collisions at all.

Good point; changed.
@davidfowl on technical break?

A null value is a bit of a weird one as it won't output any header for a null value, so all it does it block another value being added but doesn't change output.

Also concatenating null StringValues will coalesce null values to be a single null with a count of 0.

e.g.

context.Response.Headers.Add("Warning", new StringValues(value: null)); context.Response.Headers["Warning"].Count == 0;

Raised issue for it https://github.com/aspnet/KestrelHttpServer/issues/2402

halter73 · 2018-03-16T22:03:32Z

+            // Subtract start of range '0'
+            // Cast to uint to change negative numbers to large numbers
+            // Check if less than 10 representing chars '0' - '9'
+            return (uint)(ch - '0') < 10u;


Do we know that a subtraction then a comparison is faster than two comparisons? I ask because the earlier version certainly does read better which I guess is why you left it there as a comment. I have more or less the same question about the IsHex change.

I know you've posted some nice results from the ResponseHeadersWritingBenchmark and the Http1ConnectionBenchmark, but I'm more interested in the performance of these methods in isolation.

Its an extra branch prediction/mis-prediction per character; rather than the extra comparison that's the issue; cpu can only run with so many branches in parallel before you hit a pipeline bubble: Control_hazards_(branch_hazards)

How do you know changing '0' <= ch && ch <= '9' to (uint)(ch - '0') < 10u improves the branch prediction?

No branch vs 1 branch?

Makes sense... I'd feel much more warm and fuzzy about it if there were some BenchmarkDotNet results to back up this theory.

Have returned to my command center of my big screen monitors, so should be able to rummage something up.

halter73 · 2018-03-16T22:08:28Z

-            for (var i = offset + 1; i < hostText.Length; i++)
+            // This do+if check rather than for loop is to elimitate the bounds check, since
+            // the Jit doesn't currently pick up on it when starting at a variable offset
+            do


I don't think transforming simple for loops to complicated do/while loops is worthwhile when we can just wait for the JIT to handle this better.

Might be waiting a long time https://github.com/dotnet/coreclr/issues/15723#issuecomment-357152332

it's probably going to take a while, the rangecheck phase is a bit of a mess and it is difficult and risky to extend it to handle new patterns.

The issue here is its a range check per character; and when looking at the valid characters array that's another range check per character

halter73 · 2018-03-16T22:16:03Z

-                    break;
+                    var ch = (int)hostText[i];
+                    // Bounds check and elimiate second bounds check
+                    if ((uint)ch >= (uint)hostCharValidity.Length || !hostCharValidity[ch])


How is this faster than IsValidHostCharf? Does the second bounds check not get eliminated when ch is a char?

No, was generating the asm while changing these and refining them.

Perhaps char isn't treated as an unsigned value (e.g. short as ushort isn't cls complaint) so ch < HostCharValidity.Length could pass with a negative char?

Also using a class level array HostCharValidity rather than a local array hostCharValidity can stop bounds checks being eliminated; and they recently made elimination more conservative dotnet/coreclr#15756

halter73 · 2018-03-16T22:21:08Z

        {
-            if (offset == hostText.Length)
+            // Skip bounds check for accessing the [offset] element
+            if ((uint)offset >= (uint)hostText.Length)


So the bounds check get repeated unless you cat offset to a uint? If so, that seems crazy. It seemed odd when the offset started as a char and it even seems more odd now that the offset is the int.

You have to check its not less than zero and also not greater than or equal to length; so two tests. Which is one test with the uint. Also I think if you do it in two tests, it doesn't work - outside of for loops with a constant start.

halter73 · 2018-03-16T22:24:18Z

-            if (string.IsNullOrEmpty(hostText))
+            // This is a string.IsNullOrEmpty test, but arranged to elmininate the
+            // bounds check from accessing the firstChar of the string
+            if (hostText is null || 0u >= (uint)hostText.Length)


This is a breaking change if you look at ValidateHostHeader in isolation since now string full of nothing but whitespace could be rejected. I think this is safe though since we trim whitespace from all request header values.

Yea, is stripped by header parsing

@benaadams Does this faster than if (hostText == null || hostText.Length == 0)? If so, why? If not, why write it like this?

Still not sure about is vs ==, but I remember something about the bounds check elimination only happening with uint comparisons. It seems to me the JIT should be able to eliminate the bounds check given the more conventional if condition. I'd just wait for the JIT to fix this rather than leave these unconventional conditions around. I mean if we did leave them around, how long we should continue cargo culting the pattern?

Is kinda funny looking... thinking about why

Could check the asm, it might; range check elimination misses obvious stuff though

I went on sharplab.io to try some variations on this, and I have to admit that the conventional version is worse than I thought:

public static bool StartsWithBracket(string hostText) { if (hostText == null || 0u >= (uint)hostText.Length) { return false; } var firstChar = hostText[0]; if (firstChar == '[') { return true; } return false; } ; Desktop CLR v4.7.2563.00 (clr.dll) on amd64. Program.StartsWithBracket(System.String) L0000: test rcx, rcx L0003: jz L000c L0005: mov eax, [rcx+0x8] L0008: test eax, eax L000a: jnz L000f L000c: xor eax, eax L000e: ret L000f: cmp word [rcx+0xc], 0x5b L0014: jnz L001c L0016: mov eax, 0x1 L001b: ret L001c: xor eax, eax L001e: ret

public static bool StartsWithBracket(string hostText) { if (hostText == null || hostText.Length == 0) { return false; } var firstChar = hostText[0]; if (firstChar == '[') { return true; } return false; } ; Desktop CLR v4.7.2563.00 (clr.dll) on amd64. Program.StartsWithBracket(System.String) L0000: sub rsp, 0x28 L0004: test rcx, rcx L0007: jz L0010 L0009: mov edx, [rcx+0x8] L000c: test edx, edx L000e: jnz L0017 L0010: xor eax, eax L0012: add rsp, 0x28 L0016: ret L0017: cmp edx, 0x0 L001a: jbe L0034 L001c: cmp word [rcx+0xc], 0x5b L0021: jnz L002d L0023: mov eax, 0x1 L0028: add rsp, 0x28 L002c: ret L002d: xor eax, eax L002f: add rsp, 0x28 L0033: ret L0034: call 0x7ffaff4223c0 L0039: int3

public static bool StartsWithBracket(string hostText) { if (string.IsNullOrEmpty(hostText)) { return false; } var firstChar = hostText[0]; if (firstChar == '[') { return true; } return false; } ; Desktop CLR v4.7.2563.00 (clr.dll) on amd64. Program.StartsWithBracket(System.String) L0000: sub rsp, 0x28 L0004: test rcx, rcx L0007: jz L0017 L0009: cmp dword [rcx+0x8], 0x0 L000d: setz al L0010: movzx eax, al L0013: test eax, eax L0015: jz L001e L0017: xor eax, eax L0019: add rsp, 0x28 L001d: ret L001e: cmp dword [rcx+0x8], 0x0 L0022: jbe L003c L0024: cmp word [rcx+0xc], 0x5b L0029: jnz L0035 L002b: mov eax, 0x1 L0030: add rsp, 0x28 L0034: ret L0035: xor eax, eax L0037: add rsp, 0x28 L003b: ret L003c: call 0x7ffaff4223c0 L0041: int3

Now I'm wondering if we should just make this optimization in string.IsNullOrEmpty if it's going to take a while for the JIT to optimize this.

Now I'm wondering if we should just make this optimization in string.IsNullOrEmpty if it's going to take a while for the JIT to optimize this.

coreclr is pretty shut down to any enhancements atm until next week; but at that point any additions will be to 2.2, so a 6 month wait?

Also you need both sides to be uint as a uint to int comparison will first upcast them to long; then do a long >= long comparision :-/

Just in case dotnet/coreclr#17512

halter73 · 2018-03-16T22:24:47Z

                    ThrowHeadersReadOnlyException();
                }
-                SetValueFast(key, value);
+                if (value.Count == 0)


We should wait for this before merging.

halter73 · 2018-03-16T22:25:24Z

        }

        // The lead '[' was already checked
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]


Are you removing this attribute from all the methods you expect to be TCO'd?

Ish, making sure they are tail called (easy way, by not returning anything, no funky params and last action in function)

But mostly its not inlining where there is no overall fast-path. So origin form is fully inlined, as a the char checks; but what's the right choice for the others? Is ipv6 via raw ip address a common scenerio? Is ports a common scenerio? (might be due to https on 443; but isn't for http on 80)

halter73 · 2018-03-16T22:59:01Z

@davidfowl and I are interested in the relative performance of this change without the more esoteric parts of it that were added to avoid extra bounds checks by the JIT.

benaadams · 2018-03-17T01:20:39Z

+                // Subtract start of range 'a'
+                // Cast to uint to change negative numbers to large numbers
+                // Check if less than 6 representing chars 'a' - 'f'
+                || (uint)((ch | 32) - 'a') < 6u;


This is reducing 4 branches to 1 branch

benaadams · 2018-03-17T02:33:40Z

@davidfowl and I are interested in the relative performance of this change without the more esoteric parts of it that were added to avoid extra bounds checks by the JIT.

Http1ConnectionBenchmark

Baseline

               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   717.2 ns | 1,394,376.1 |   1.00 |     344 B |
           LiveAspNet | 1,517.2 ns |   659,095.9 |   2.12 |    1056 B |

Changes (without bounds checking removal)

           
               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   695.1 ns | 1,438,728.6 |   1.00 |     344 B |
           LiveAspNet | 1,507.1 ns |   663,510.4 |   2.17 |    1056 B |

Changes (with bounds checking removal)

               Method |       Mean |        Op/s | Scaled | Allocated |
--------------------- |-----------:|------------:|-------:|----------:|
 PlaintextTechEmpower |   680.0 ns | 1,470,672.2 |   1.00 |     344 B |
           LiveAspNet | 1,482.2 ns |   674,676.6 |   2.18 |    1056 B |

davidfowl · 2018-03-21T07:52:22Z

@dotnet-bot test OSX 10.12 Release Build

benaadams · 2018-03-21T17:43:31Z

Added IsNull use, however get

LibuvThread.cs(427,30): error CS0115: 'LibuvThread.Schedule<T>(Action<T>, T)': no suitable method found to override
LibuvThread.cs(19,18): error CS0534: 'LibuvThread' does not implement inherited abstract member 'PipeScheduler.Schedule(Action<object>, object)' 
IOQueue.cs(19,30): error CS0115: 'IOQueue.Schedule<T>(Action<T>, T)': no suitable method found to override
IOQueue.cs(11,18): error CS0534: 'IOQueue' does not implement inherited abstract member 'PipeScheduler.Schedule(Action<object>, object)'
IOQueue.cs(19,30): error CS0115: 'IOQueue.Schedule<T>(Action<T>, T)': no suitable method found to override
IOQueue.cs(11,18): error CS0534: 'IOQueue' does not implement inherited abstract member 'PipeScheduler.Schedule(Action<object>, object)'

If I update deps

davidfowl · 2018-03-21T17:45:00Z

@benaadams rebase

benaadams · 2018-03-21T17:46:56Z

rebase

Good point :)

benaadams · 2018-03-21T19:27:13Z

Is there a way to magic perf test this PR?

benaadams · 2018-03-22T13:42:16Z

Backed out the StringValues IsNull changes while I try to understand the Universe

davidfowl · 2018-04-10T14:44:25Z

Before

[07:40:36.895] RequestsPerSecond:           1918968
[07:40:36.895] Latency on load (ms):        1.7
[07:40:36.895] Max CPU (%):                 91
[07:40:36.895] WorkingSet (MB):             383
[07:40:36.896] Startup Main (ms):           422
[07:40:36.896] First Request (ms):          159.5
[07:40:36.896] Latency (ms):                0.7
[07:40:36.896] Socket Errors:               0
[07:40:36.896] Bad Responses:               0
[07:40:36.896] SDK:                         2.2.0-preview1-007522
[07:40:36.896] Runtime:                     2.1.0-preview2-26403-06
[07:40:36.896] ASP.NET Core:                2.1.0-preview3-32183

After

[07:39:07.795] RequestsPerSecond:           2010354
[07:39:07.795] Latency on load (ms):        2.1
[07:39:07.795] Max CPU (%):                 91
[07:39:07.795] WorkingSet (MB):             371
[07:39:07.795] Startup Main (ms):           423
[07:39:07.796] First Request (ms):          157.7
[07:39:07.796] Latency (ms):                0.7
[07:39:07.796] Socket Errors:               0
[07:39:07.796] Bad Responses:               0
[07:39:07.796] SDK:                         2.2.0-preview1-007522
[07:39:07.796] Runtime:                     2.1.0-preview2-26403-06
[07:39:07.796] ASP.NET Core:                2.1.0-preview3-32183

halter73 · 2018-04-10T18:49:27Z

                {
                    if (!_absoluteRequestTarget.IsDefaultPort
-                        || host != _absoluteRequestTarget.Authority + ":" + _absoluteRequestTarget.Port.ToString(CultureInfo.InvariantCulture))
+                        || hostText != _absoluteRequestTarget.Authority + ":" + _absoluteRequestTarget.Port.ToString(CultureInfo.InvariantCulture))


@Tratcher This has nothing to do with this PR, but if the problem here is "System.Uri doesn't not tell us if the port was in the original string or not", why not just compare hostText to RawTarget instead of doing this complicated condition?

HostText isn't expected to be the full value of RawTarget, only part of it.

halter73 · 2018-04-10T19:09:37Z

-            for (; i < hostText.Length; i++)
-            {
-                if (!IsValidHostChar(hostText[i]))
+                // Enregister array


Is enregister a technical term for changing a class reference to a local reference or something?

Make sure it gets stored in a register

Compiler term, put variable in register. I hadn't heard it before I strayed too close to the Jit

halter73 · 2018-04-10T19:11:55Z

-        private static bool IsValidHostChar(char ch)
-        {
-            return ch < HostCharValidity.Length && HostCharValidity[ch];
+                if (i < hostText.Length)


Isn't this condition getting repeated in ValidateHostPort? Why check twice?

Could drop the check from ValidateHostPort; but then its just going to introduce a bounds check and check it anyway, so might as well check it there for safety?

ah, is while loop there anyway so needs the check for the loop. Not sure how to get rid of one of them?

benaadams · 2018-04-11T11:37:16Z

@halter73 any specific changes you would like? I've got a bit lost in the github display of comments

halter73 · 2018-04-12T19:41:21Z

@benaadams If you can update this PR or submit a new one without the bounds check related changes, I would like to merge it for 2.1.

benaadams · 2018-04-12T20:03:22Z

@benaadams If you can update this PR or submit a new one without the bounds check related changes, I would like to merge it for 2.1.

Asked @jkotas if the string.IsNullOrEmpty is ok to merge for 2.1 dotnet/coreclr#17512

benaadams · 2018-04-12T20:38:46Z

@benaadams If you can update this PR or submit a new one without the bounds check related changes, I would like to merge it for 2.1.

Something more like this #2488

halter73 · 2018-04-14T01:23:15Z

Closing this PR since we merged #2488 which is basically this without the bounds check elimination tricks.

muratg requested a review from davidfowl February 27, 2018 23:12

davidfowl reviewed Feb 27, 2018

View reviewed changes

davidfowl approved these changes Feb 28, 2018

View reviewed changes

davidfowl reviewed Feb 28, 2018

View reviewed changes

Tratcher reviewed Feb 28, 2018

View reviewed changes

benaadams force-pushed the Less-StringValues-copies branch from c2d1e68 to eb173c3 Compare March 12, 2018 07:04

benaadams force-pushed the Less-StringValues-copies branch from eb173c3 to b46607f Compare March 14, 2018 06:34

davidfowl assigned halter73 Mar 14, 2018

davidfowl reviewed Mar 15, 2018

View reviewed changes

davidfowl reviewed Mar 16, 2018

View reviewed changes

benaadams force-pushed the Less-StringValues-copies branch 2 times, most recently from 9fdf691 to 80553a9 Compare March 16, 2018 14:09

benaadams mentioned this pull request Mar 16, 2018

StringValues Add IsNull, always set single item to _value dotnet/extensions#323

Merged

Tratcher approved these changes Mar 16, 2018

View reviewed changes

benaadams commented Mar 16, 2018

View reviewed changes

halter73 suggested changes Mar 16, 2018

View reviewed changes

benaadams commented Mar 17, 2018

View reviewed changes

benaadams force-pushed the Less-StringValues-copies branch from d54ad0d to cc7cbcc Compare March 21, 2018 17:41

benaadams added 6 commits March 21, 2018 17:45

Less StringValue struct copies for header checks

8a86e0f

EnsureHostHeaderExists perf

776cc99

Skip some bounds checks

8e1a024

Skip StringValue copy

d9f74b3

feedback

301c8f9

Hoist IsNull check

b932a4f

benaadams force-pushed the Less-StringValues-copies branch from cc7cbcc to bff8296 Compare March 21, 2018 17:46

benaadams force-pushed the Less-StringValues-copies branch from bff8296 to b932a4f Compare March 22, 2018 13:41

halter73 reviewed Apr 10, 2018

View reviewed changes

benaadams mentioned this pull request Apr 11, 2018

Use string.IsNullOrEmpty to eliminate bounds check to first char dotnet/coreclr#17512

Merged

benaadams changed the title ~~Less StringValue struct copies for header checks~~ Less StringValue struct copies + bounds check elimination Apr 11, 2018

halter73 closed this Apr 14, 2018

benaadams mentioned this pull request Dec 13, 2018

Is a Null Header Value a Set header value? dotnet/aspnetcore#4754

Closed

Conversation

benaadams commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

davidfowl commented Feb 28, 2018

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!

Tratcher Feb 28, 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!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Mar 1, 2018

Uh oh!

davidfowl commented Mar 1, 2018

Uh oh!

benaadams commented Mar 12, 2018

Uh oh!

benaadams commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Mar 14, 2018

Uh oh!

davidfowl commented Mar 14, 2018

Uh oh!

benaadams commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Mar 15, 2018

Uh oh!

davidfowl commented Mar 15, 2018

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!

benaadams commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

benaadams commented Feb 24, 2018 •

edited

Loading

benaadams commented Feb 24, 2018 •

edited

Loading

Tratcher Feb 28, 2018 •

edited

Loading

benaadams commented Mar 14, 2018 •

edited

Loading

benaadams commented Mar 15, 2018 •

edited

Loading

benaadams commented Mar 15, 2018 •

edited

Loading