From 2eba4017c1aa2f969e5a081f928c9c6a9df95327 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 21 Nov 2016 22:18:17 +0000 Subject: [PATCH] MemoryPoolIterator feedback --- global.json | 2 +- .../Internal/Http/Frame.cs | 2 +- .../Infrastructure/MemoryPoolIterator.cs | 33 ++++++++++++++----- .../RequestParsing.cs | 1 - .../MemoryPoolIteratorTests.cs | 6 ++-- .../MessageBodyTests.cs | 2 +- test/shared/MockConnection.cs | 2 +- test/shared/SocketInputExtensions.cs | 2 +- 8 files changed, 33 insertions(+), 17 deletions(-) diff --git a/global.json b/global.json index d9b4ed63ae..983ba0401e 100644 --- a/global.json +++ b/global.json @@ -1,3 +1,3 @@ { - "projects": [ "src" ] + "projects": ["src"] } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs index 59891a7355..2b7f56e882 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs @@ -23,7 +23,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { public abstract partial class Frame : IFrameControl { - // byte consts don't have a data type annotation so we pre-cast them + // byte types don't have a data type annotation so we pre-cast them; to avoid in-place casts private const byte ByteCR = (byte)'\r'; private const byte ByteLF = (byte)'\n'; private const byte ByteColon = (byte)':'; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs index 4cceb50bf7..b1b45eb6e8 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs @@ -774,10 +774,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int LocateFirstFoundByte(Vector byteEquals) { - var vector64 = Vector.AsVectorInt64(byteEquals); - long longValue = 0; + var vector64 = Vector.AsVectorUInt64(byteEquals); + ulong longValue = 0; var i = 0; - for (; i < Vector.Count; i++) + // Pattern unrolled by jit https://github.com/dotnet/coreclr/pull/8001 + for (; i < Vector.Count; i++) { longValue = vector64[i]; if (longValue == 0) continue; @@ -785,7 +786,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure } // Flag least significant power of two bit - var powerOfTwoFlag = (ulong)(longValue ^ (longValue - 1)); + var powerOfTwoFlag = (longValue ^ (longValue - 1)); // Shift all powers of two into the high byte and extract var foundByteIndex = (int)((powerOfTwoFlag * _xorPowerOfTwoToHighByte) >> 57); // Single LEA instruction with jitted const (using function result) @@ -803,7 +804,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure var block = _block; if (block == null) { - return false; + ThrowInvalidOperationException_PutPassedEndOfBlock(); } var index = _index; @@ -817,7 +818,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure return true; } - return wasLastBlock ? false : PutMultiBlock(data); + if (wasLastBlock) + { + ThrowInvalidOperationException_PutPassedEndOfBlock(); + } + + return PutMultiBlock(data); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -841,6 +847,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure } if (wasLastBlock) { + ThrowInvalidOperationException_PutPassedEndOfBlock(); return false; } } while (true); @@ -848,13 +855,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure return true; } + private static void ThrowInvalidOperationException_PutPassedEndOfBlock() + { + throw new InvalidOperationException("Attempted to put passed end of block."); + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public int GetLength(MemoryPoolIterator end) { var block = _block; if (block == null || end.IsDefault) { - return -1; + ThrowInvalidOperationException_GetLengthNullBlock(); } if (block == end._block) @@ -865,6 +877,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure return GetLengthMultiBlock(ref end); } + private static void ThrowInvalidOperationException_GetLengthNullBlock() + { + throw new InvalidOperationException("Attempted GetLength of non existent block."); + } + [MethodImpl(MethodImplOptions.NoInlining)] public int GetLengthMultiBlock(ref MemoryPoolIterator end) { @@ -1234,7 +1251,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure // Vector .ctor doesn't become an intrinsic due to detection issue // However this does cause it to become an intrinsic (with additional multiply and reg->reg copy) // https://github.com/dotnet/coreclr/issues/7459#issuecomment-253965670 - return Vector.AsVectorByte(new Vector(vectorByte * 0x0101010101010101ul)); + return Vector.AsVectorByte(new Vector(vectorByte * 0x01010101u)); } } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.Performance/RequestParsing.cs b/test/Microsoft.AspNetCore.Server.Kestrel.Performance/RequestParsing.cs index 7388c547a2..5ae06776f5 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.Performance/RequestParsing.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.Performance/RequestParsing.cs @@ -8,7 +8,6 @@ using BenchmarkDotNet.Attributes; using Microsoft.AspNetCore.Server.Kestrel.Internal; using Microsoft.AspNetCore.Server.Kestrel.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure; -using Microsoft.AspNetCore.Server.KestrelTests.TestHelpers; using Microsoft.AspNetCore.Testing; using RequestLineStatus = Microsoft.AspNetCore.Server.Kestrel.Internal.Http.Frame.RequestLineStatus; diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs index 0cb46de20d..1db21257a3 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs @@ -154,7 +154,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests } // Can't put anything by the end - Assert.False(head.Put(0xFF)); + Assert.ThrowsAny(() => head.Put(0xFF)); for (var i = 0; i < 4; ++i) { @@ -979,7 +979,6 @@ namespace Microsoft.AspNetCore.Server.KestrelTests var end = default(MemoryPoolIterator); Assert.False(default(MemoryPoolIterator).TryPeekLong(out longValue)); - Assert.False(default(MemoryPoolIterator).Put(byteCr)); Assert.Null(default(MemoryPoolIterator).GetAsciiString(ref end)); Assert.Null(default(MemoryPoolIterator).GetUtf8String(ref end)); // Assert.Equal doesn't work for default(ArraySegments) @@ -995,7 +994,8 @@ namespace Microsoft.AspNetCore.Server.KestrelTests default(MemoryPoolIterator).CopyFrom(default(ArraySegment)); default(MemoryPoolIterator).CopyFromAscii(""); - Assert.Equal(default(MemoryPoolIterator).GetLength(end), -1); + Assert.ThrowsAny(() => default(MemoryPoolIterator).Put(byteCr)); + Assert.ThrowsAny(() => default(MemoryPoolIterator).GetLength(end)); Assert.ThrowsAny(() => default(MemoryPoolIterator).Skip(1)); } diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/MessageBodyTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/MessageBodyTests.cs index a5ed8d7c9f..414bb208e3 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/MessageBodyTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/MessageBodyTests.cs @@ -10,11 +10,11 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.Kestrel; using Microsoft.AspNetCore.Server.Kestrel.Internal.Http; -using Microsoft.AspNetCore.Server.KestrelTests.TestHelpers; using Microsoft.Extensions.Internal; using Moq; using Xunit; using Xunit.Sdk; +using Microsoft.AspNetCore.Testing; namespace Microsoft.AspNetCore.Server.KestrelTests { diff --git a/test/shared/MockConnection.cs b/test/shared/MockConnection.cs index a782f51ffd..e1025c624c 100644 --- a/test/shared/MockConnection.cs +++ b/test/shared/MockConnection.cs @@ -8,7 +8,7 @@ using Microsoft.AspNetCore.Server.Kestrel; using Microsoft.AspNetCore.Server.Kestrel.Internal; using Microsoft.AspNetCore.Server.Kestrel.Internal.Http; -namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers +namespace Microsoft.AspNetCore.Testing { public class MockConnection : Connection, IDisposable { diff --git a/test/shared/SocketInputExtensions.cs b/test/shared/SocketInputExtensions.cs index 998e0552c5..d6dbbb7e88 100644 --- a/test/shared/SocketInputExtensions.cs +++ b/test/shared/SocketInputExtensions.cs @@ -4,7 +4,7 @@ using System; using Microsoft.AspNetCore.Server.Kestrel.Internal.Http; -namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers +namespace Microsoft.AspNetCore.Testing { public static class SocketInputExtensions {