Merge pull request #105 from SixLabors/js/code-quality

Various Code Quality Fixes

Author: JimBobSquarePants

Directory.Build.props

@@ -13,13 +13,14 @@
   <!-- Default settings that are used by other settings -->
   <PropertyGroup>
     <BaseArtifactsPath>$(MSBuildThisFileDirectory)artifacts/</BaseArtifactsPath>
-    <BaseArtifactsPathSuffix>$(ImageSharpProjectCategory)/$(MSBuildProjectName)</BaseArtifactsPathSuffix>
+    <BaseArtifactsPathSuffix>$(SixLaborsProjectCategory)/$(MSBuildProjectName)</BaseArtifactsPathSuffix>
     <RepositoryUrl Condition="'$(RepositoryUrl)' == ''">https://github.com/SixLabors/ImageSharp.Web/</RepositoryUrl>
   </PropertyGroup>
 
   <!-- Default settings that explicitly differ from the Sdk.props defaults  -->
   <PropertyGroup>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <BaseIntermediateOutputPath>$(BaseArtifactsPath)obj/$(BaseArtifactsPathSuffix)/</BaseIntermediateOutputPath>
     <DebugType>portable</DebugType>
     <DebugType Condition="'$(codecov)' != ''">full</DebugType>
     <NullableContextOptions>disable</NullableContextOptions>
@@ -58,6 +59,7 @@
   <!-- Default settings that explicitly differ from the Sdk.targets defaults-->
   <PropertyGroup>
     <Authors>Six Labors and contributors</Authors>
+    <BaseOutputPath>$(BaseArtifactsPath)bin/$(BaseArtifactsPathSuffix)/</BaseOutputPath>
     <Company>Six Labors</Company>
     <PackageOutputPath>$(BaseArtifactsPath)pkg/$(BaseArtifactsPathSuffix)/$(Configuration)/</PackageOutputPath>
     <Product>SixLabors.ImageSharp.Web</Product>

Directory.Build.targets

@@ -25,7 +25,7 @@
     <PackageReference Update="Azure.Storage.Blobs" Version="12.4.0" />
     <PackageReference Update="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All"/>
     <PackageReference Update="MinVer" PrivateAssets="All" Version="2.3.0" />
-    <PackageReference Update="SixLabors.ImageSharp" Version="1.0.0" />
+    <PackageReference Update="SixLabors.ImageSharp" Version="1.0.1" />
   </ItemGroup>
 
   <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1'">

src/Directory.Build.props

@@ -43,7 +43,7 @@
     <!-- DynamicProxyGenAssembly2 is needed so Moq can use our internals -->
     <InternalsVisibleTo Include="DynamicProxyGenAssembly2" PublicKey="0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7" />
     <InternalsVisibleTo Include="SixLabors.ImageSharp.Web.Tests"  PublicKey="$(SixLaborsPublicKey)" />
-    <InternalsVisibleTo Include="SixLabors.ImageSharp.Web.Benchmarks"  PublicKey="$(SixLaborsPublicKey)" />
+    <InternalsVisibleTo Include="ImageSharp.Web.Benchmarks"  PublicKey="$(SixLaborsPublicKey)" />
   </ItemGroup>
 
 </Project>

src/ImageSharp.Web.Providers.Azure/Resolvers/AzureBlobStorageImageResolver.cs

@@ -33,6 +33,8 @@ public async Task<ImageMetadata> GetMetaDataAsync()
         /// <inheritdoc/>
         public async Task<Stream> OpenReadAsync()
         {
+            // Copy to a MemoryStream first because RetriableStreamImpl
+            // doesn't support Position.
             Stream blobStream = (await this.blob.DownloadAsync()).Value.Content;
             var memoryStream = new ChunkedMemoryStream();
 

src/ImageSharp.Web/Caching/CacheHash.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0.
 
 using System;
+using System.Runtime.CompilerServices;
 using System.Security.Cryptography;
 using System.Text;
 using Microsoft.Extensions.Options;
@@ -33,17 +34,34 @@ public CacheHash(IOptions<ImageSharpMiddlewareOptions> options, MemoryAllocator
         }
 
         /// <inheritdoc/>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public string Create(string value, uint length)
         {
-            int len = (int)length;
             int byteCount = Encoding.ASCII.GetByteCount(value);
-            using (var hashAlgorithm = SHA256.Create())
-            using (IManagedByteBuffer buffer = this.memoryAllocator.AllocateManagedByteBuffer(byteCount))
+
+            // Allocating a buffer from the pool is ~27% slower than stackalloc so use
+            // that for short strings
+            if (byteCount < 257)
             {
-                Encoding.ASCII.GetBytes(value, 0, byteCount, buffer.Array, 0);
-                byte[] hash = hashAlgorithm.ComputeHash(buffer.Array, 0, byteCount);
-                return $"{HexEncoder.Encode(new Span<byte>(hash).Slice(0, len / 2))}";
+                return HashValue(value, length, stackalloc byte[byteCount]);
             }
+
+            using IManagedByteBuffer buffer = this.memoryAllocator.AllocateManagedByteBuffer(byteCount);
+            return HashValue(value, length, buffer.Memory.Span);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static string HashValue(ReadOnlySpan<char> value, uint length, Span<byte> bufferSpan)
+        {
+            using var hashAlgorithm = SHA256.Create();
+            Encoding.ASCII.GetBytes(value, bufferSpan);
+
+            // Hashed output maxes out at 32 bytes @ 256bit/8 so we're safe to use stackalloc.
+            Span<byte> hash = stackalloc byte[32];
+            hashAlgorithm.TryComputeHash(bufferSpan, hash, out int _);
+
+            // length maxes out at 64 since we throw if options is greater.
+            return HexEncoder.Encode(hash.Slice(0, (int)(length / 2)));
         }
     }
 }

src/ImageSharp.Web/Caching/HexEncoder.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Six Labors.
+// Copyright (c) Six Labors.
 // Licensed under the Apache License, Version 2.0.
 
 using System;
@@ -23,29 +23,38 @@ internal static class HexEncoder
         private static readonly char[] HexLutLo = Enumerable.Range(0, 256).Select(x => HexLutBase[x % 0x10]).ToArray();
 
         /// <summary>
-        /// Converts a <see cref="T:Span{byte}"/> to a hexidecimal formatted <see cref="string"/> padded to 2 digits.
+        /// Converts a <see cref="ReadOnlySpan{Byte}"/> to a hexidecimal formatted <see cref="string"/> padded to 2 digits.
         /// </summary>
         /// <param name="bytes">The bytes.</param>
         /// <returns>The <see cref="string"/>.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public static string Encode(Span<byte> bytes)
+        public static unsafe string Encode(ReadOnlySpan<byte> bytes)
+        {
+            fixed (byte* bytesPtr = bytes)
+            {
+                return string.Create(bytes.Length * 2, (Ptr: (IntPtr)bytesPtr, bytes.Length), (chars, args) =>
+                {
+                    var ros = new ReadOnlySpan<byte>((byte*)args.Ptr, args.Length);
+                    EncodeToUtf16(ros, chars);
+                });
+            }
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static void EncodeToUtf16(ReadOnlySpan<byte> bytes, Span<char> chars)
         {
-            int length = bytes.Length;
-            char[] chars = new char[length * 2];
-            ref char charRef = ref MemoryMarshal.GetReference<char>(chars);
             ref byte bytesRef = ref MemoryMarshal.GetReference(bytes);
+            ref char charRef = ref MemoryMarshal.GetReference(chars);
             ref char hiRef = ref MemoryMarshal.GetReference<char>(HexLutHi);
             ref char lowRef = ref MemoryMarshal.GetReference<char>(HexLutLo);
 
             int index = 0;
-            for (int i = 0; i < length; i++)
+            for (int i = 0; i < bytes.Length; i++)
             {
                 byte byteIndex = Unsafe.Add(ref bytesRef, i);
                 Unsafe.Add(ref charRef, index++) = Unsafe.Add(ref hiRef, byteIndex);
                 Unsafe.Add(ref charRef, index++) = Unsafe.Add(ref lowRef, byteIndex);
             }
-
-            return new string(chars, 0, chars.Length);
         }
     }
-}
+}

src/ImageSharp.Web/Caching/PhysicalFileSystemCache.cs

@@ -1,7 +1,10 @@
 // Copyright (c) Six Labors.
 // Licensed under the Apache License, Version 2.0.
 
+using System;
 using System.IO;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.Extensions.FileProviders;
@@ -21,6 +24,11 @@ public class PhysicalFileSystemCache : IImageCache
         /// </summary>
         private readonly string cacheRootPath;
 
+        /// <summary>
+        /// The length of the filename to use (minus the extension) when storing images in the image cache.
+        /// </summary>
+        private readonly int cachedNameLength;
+
         /// <summary>
         /// The file provider abstraction.
         /// </summary>
@@ -62,6 +70,7 @@ public PhysicalFileSystemCache(
             Guard.NotNull(options, nameof(options));
             Guard.NotNullOrWhiteSpace(environment.WebRootPath, nameof(environment.WebRootPath));
 
+            // Allow configuration of the cache without having to register everything.
             this.cacheOptions = cacheOptions != null ? cacheOptions.Value : new PhysicalFileSystemCacheOptions();
             this.cacheRootPath = Path.Combine(environment.WebRootPath, this.cacheOptions.CacheFolder);
             if (!Directory.Exists(this.cacheRootPath))
@@ -71,13 +80,14 @@ public PhysicalFileSystemCache(
 
             this.fileProvider = new PhysicalFileProvider(this.cacheRootPath);
             this.options = options.Value;
+            this.cachedNameLength = (int)this.options.CachedNameLength;
             this.formatUtilies = formatUtilities;
         }
 
         /// <inheritdoc/>
         public async Task<IImageCacheResolver> GetAsync(string key)
         {
-            string path = this.ToFilePath(key);
+            string path = ToFilePath(key, this.cachedNameLength);
 
             IFileInfo metaFileInfo = this.fileProvider.GetFileInfo(this.ToMetaDataFilePath(path));
             if (!metaFileInfo.Exists)
@@ -105,7 +115,7 @@ public async Task<IImageCacheResolver> GetAsync(string key)
         /// <inheritdoc/>
         public async Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
         {
-            string path = Path.Combine(this.cacheRootPath, this.ToFilePath(key));
+            string path = Path.Combine(this.cacheRootPath, ToFilePath(key, this.cachedNameLength));
             string imagePath = this.ToImageFilePath(path, metadata);
             string metaPath = this.ToMetaDataFilePath(path);
             string directory = Path.GetDirectoryName(path);
@@ -146,8 +156,35 @@ private string ToImageFilePath(string path, in ImageCacheMetadata metaData)
         /// Converts the key into a nested file path.
         /// </summary>
         /// <param name="key">The cache key.</param>
+        /// <param name="cachedNameLength">The length of the cached file name minus the extension.</param>
         /// <returns>The <see cref="string"/>.</returns>
-        private string ToFilePath(string key) // TODO: Avoid the allocation here.
-            => $"{string.Join("/", key.Substring(0, (int)this.options.CachedNameLength).ToCharArray())}/{key}";
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        internal static unsafe string ToFilePath(string key, int cachedNameLength)
+        {
+            // Each key substring char + separator + key
+            int length = (cachedNameLength * 2) + key.Length;
+            fixed (char* keyPtr = key)
+            {
+                return string.Create(length, (Ptr: (IntPtr)keyPtr, key.Length), (chars, args) =>
+                {
+                    const char separator = '/';
+                    var keySpan = new ReadOnlySpan<char>((char*)args.Ptr, args.Length);
+                    ref char keyRef = ref MemoryMarshal.GetReference(keySpan);
+                    ref char charRef = ref MemoryMarshal.GetReference(chars);
+
+                    int index = 0;
+                    for (int i = 0; i < cachedNameLength; i++)
+                    {
+                        Unsafe.Add(ref charRef, index++) = Unsafe.Add(ref keyRef, i);
+                        Unsafe.Add(ref charRef, index++) = separator;
+                    }
+
+                    for (int i = 0; i < keySpan.Length; i++)
+                    {
+                        Unsafe.Add(ref charRef, index++) = Unsafe.Add(ref keyRef, i);
+                    }
+                });
+            }
+        }
     }
 }

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

@@ -230,7 +230,7 @@ private async Task ProcessRequestAsync(HttpContext context, bool processRequest,
                         {
                             // Has the cached image expired or has the source image been updated?
                             if (cachedImageMetadata.SourceLastWriteTimeUtc == sourceImageMetadata.LastWriteTimeUtc
-                                && cachedImageMetadata.CacheLastWriteTimeUtc > DateTimeOffset.Now.AddDays(-this.options.MaxCacheDays))
+                                && cachedImageMetadata.CacheLastWriteTimeUtc > DateTimeOffset.UtcNow.AddDays(-this.options.MaxCacheDays))
                             {
                                 // We're pulling the image from the cache.
                                 using (Stream cachedBuffer = await cachedImageResolver.OpenReadAsync())
@@ -265,7 +265,7 @@ private async Task ProcessRequestAsync(HttpContext context, bool processRequest,
                                 // No commands? We simply copy the stream across.
                                 if (commands.Count == 0)
                                 {
-                                    format = Image.DetectFormat(this.options.Configuration, inStream);
+                                    format = await Image.DetectFormatAsync(this.options.Configuration, inStream);
                                     await inStream.CopyToAsync(outStream);
                                 }
                                 else
@@ -316,11 +316,33 @@ private async Task ProcessRequestAsync(HttpContext context, bool processRequest,
                 }
                 finally
                 {
-                    outStream?.Dispose();
+                    await this.StreamDisposeAsync(outStream);
                 }
             }
         }
 
+        private ValueTask StreamDisposeAsync(Stream stream)
+        {
+            if (stream is null)
+            {
+                return default;
+            }
+
+#if NETCOREAPP2_1
+            try
+            {
+                stream.Dispose();
+                return default;
+            }
+            catch (Exception ex)
+            {
+                return new ValueTask(Task.FromException(ex));
+            }
+#else
+            return stream.DisposeAsync();
+#endif
+        }
+
         private async Task SendResponseAsync(ImageContext imageContext, string key, Stream stream, ImageCacheMetadata metadata)
         {
             imageContext.ComprehendRequestHeaders(metadata.CacheLastWriteTimeUtc, stream.Length);

…harp.Web.Benchmarks/CacheHashBaseline.cs ….Benchmarks/Caching/CacheHashBaseline.cstests/ImageSharp.Web.Benchmarks/CacheHashBaseline.cs renamed to tests/ImageSharp.Web.Benchmarks/Caching/CacheHashBaseline.cs tests/ImageSharp.Web.Benchmarks/CacheHashBaseline.cs renamed to tests/ImageSharp.Web.Benchmarks/Caching/CacheHashBaseline.cs

@@ -6,7 +6,7 @@
 using System.Text;
 using SixLabors.ImageSharp.Web.Caching;
 
-namespace SixLabors.ImageSharp.Web.Benchmarks
+namespace SixLabors.ImageSharp.Web.Benchmarks.Caching
 {
     /// <summary>
     /// A baseline naive SHA256 hashing implementation
@@ -16,11 +16,11 @@ public class CacheHashBaseline : ICacheHash
         /// <inheritdoc/>
         public string Create(string value, uint length)
         {
-            if (length.CompareTo(2) < 0 || length.CompareTo(64) > 0)
-            {
-                throw new ArgumentOutOfRangeException(nameof(length), $"Value must be greater than or equal to {2} and less than or equal to {64}.");
-            }
-
+            // Don't use in benchmark.
+            // if (length.CompareTo(2) < 0 || length.CompareTo(64) > 0)
+            // {
+            //    throw new ArgumentOutOfRangeException(nameof(length), $"Value must be greater than or equal to {2} and less than or equal to {64}.");
+            // }
             using (var hashAlgorithm = SHA256.Create())
             {
                 // Concatenate the hash bytes into one long string.

tests/ImageSharp.Web.Benchmarks/Caching/CacheHashBenchmarks.cs

@@ -12,14 +12,28 @@ namespace SixLabors.ImageSharp.Web.Benchmarks.Caching
     public class CacheHashBenchmarks
     {
         private const string URL = "http://testwebsite.com/image-12345.jpeg?width=400";
-        private static readonly IOptions<ImageSharpMiddlewareOptions> Options = Microsoft.Extensions.Options.Options.Create(new ImageSharpMiddlewareOptions());
-        private static readonly CacheHash Sha256Hasher = new CacheHash(Options, Options.Value.Configuration.MemoryAllocator);
+        private static readonly IOptions<ImageSharpMiddlewareOptions> MWOptions = Options.Create(new ImageSharpMiddlewareOptions());
+        private static readonly CacheHash Sha256Hasher = new CacheHash(MWOptions, MWOptions.Value.Configuration.MemoryAllocator);
         private static readonly CacheHashBaseline NaiveSha256Hasher = new CacheHashBaseline();
 
         [Benchmark(Baseline = true, Description = "Baseline Sha256Hasher")]
         public string HashUsingBaselineHash() => NaiveSha256Hasher.Create(URL, 12);
 
         [Benchmark(Description = "Sha256Hasher")]
         public string HashUsingSha256() => Sha256Hasher.Create(URL, 12);
+
+        /*
+        BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
+        Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
+        .NET Core SDK=3.1.401
+          [Host]     : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
+          DefaultJob : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
+
+
+        |                  Method |       Mean |   Error |  StdDev | Ratio |  Gen 0 | Gen 1 | Gen 2 | Allocated |
+        |------------------------ |-----------:|--------:|--------:|------:|-------:|------:|------:|----------:|
+        | 'Baseline Sha256Hasher' | 1,065.7 ns | 8.95 ns | 8.37 ns |  1.00 | 0.1564 |     - |     - |     656 B |
+        |            Sha256Hasher |   786.7 ns | 9.77 ns | 9.14 ns |  0.74 | 0.0420 |     - |     - |     176 B |
+        */
     }
 }