Razor CompilerCache allocates too much in NormalizePath

Fixes #3035

src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilerCache.cs

@@ -2,7 +2,10 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Diagnostics;
+using System.Text;
 using Microsoft.AspNet.FileProviders;
 using Microsoft.Framework.Caching.Memory;
 using Microsoft.Framework.Internal;
@@ -17,6 +20,9 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
         private readonly IFileProvider _fileProvider;
         private readonly IMemoryCache _cache;
 
+        private readonly ConcurrentDictionary<string, string> _normalizedPathLookup =
+            new ConcurrentDictionary<string, string>(StringComparer.Ordinal);
+
         /// <summary>
         /// Initializes a new instance of <see cref="CompilerCache"/>.
         /// </summary>
@@ -42,8 +48,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             foreach (var item in precompiledViews)
             {
                 var cacheEntry = new CompilerCacheResult(CompilationResult.Successful(item.Value));
-                var normalizedPath = NormalizePath(item.Key);
+                _cache.Set(GetNormalizedPath(item.Key), cacheEntry);
-                _cache.Set(normalizedPath, cacheEntry);
             }
         }
 
@@ -52,41 +57,55 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             [NotNull] string relativePath,
             [NotNull] Func<RelativeFileInfo, CompilationResult> compile)
         {
-            var normalizedPath = NormalizePath(relativePath);
             CompilerCacheResult cacheResult;
-            if (!_cache.TryGetValue(normalizedPath, out cacheResult))
+            // Attempt to lookup the cache entry using the passed in path. This will succeed if the path is already
+            // normalized and a cache entry exists.
+            if (!_cache.TryGetValue(relativePath, out cacheResult))
             {
-                var fileInfo = _fileProvider.GetFileInfo(relativePath);
+                var normalizedPath = GetNormalizedPath(relativePath);
-                MemoryCacheEntryOptions cacheEntryOptions;
+                if (!_cache.TryGetValue(normalizedPath, out cacheResult))
-                CompilerCacheResult cacheResultToCache;
-                if (!fileInfo.Exists)
                 {
-                    cacheResultToCache = CompilerCacheResult.FileNotFound;
+                    cacheResult = CreateCacheEntry(normalizedPath, compile);
-                    cacheResult = CompilerCacheResult.FileNotFound;
-
-                    cacheEntryOptions = new MemoryCacheEntryOptions();
-                    cacheEntryOptions.AddExpirationTrigger(_fileProvider.Watch(relativePath));
                 }
-                else
-                {
-                    var relativeFileInfo = new RelativeFileInfo(fileInfo, relativePath);
-                    var compilationResult = compile(relativeFileInfo).EnsureSuccessful();
-                    cacheEntryOptions = GetMemoryCacheEntryOptions(relativePath);
-
-                    // By default the CompilationResult returned by IRoslynCompiler is an instance of
-                    // UncachedCompilationResult. This type has the generated code as a string property and do not want
-                    // to cache it. We'll instead cache the unwrapped result.
-                    cacheResultToCache = new CompilerCacheResult(
-                        CompilationResult.Successful(compilationResult.CompiledType));
-                    cacheResult = new CompilerCacheResult(compilationResult);
-                }
-
-                _cache.Set(normalizedPath, cacheResultToCache, cacheEntryOptions);
             }
 
             return cacheResult;
         }
 
+        private CompilerCacheResult CreateCacheEntry(
+            string normalizedPath,
+            Func<RelativeFileInfo, CompilationResult> compile)
+        {
+            CompilerCacheResult cacheResult;
+            var fileInfo = _fileProvider.GetFileInfo(normalizedPath);
+            MemoryCacheEntryOptions cacheEntryOptions;
+            CompilerCacheResult cacheResultToCache;
+            if (!fileInfo.Exists)
+            {
+                cacheResultToCache = CompilerCacheResult.FileNotFound;
+                cacheResult = CompilerCacheResult.FileNotFound;
+
+                cacheEntryOptions = new MemoryCacheEntryOptions();
+                cacheEntryOptions.AddExpirationTrigger(_fileProvider.Watch(normalizedPath));
+            }
+            else
+            {
+                var relativeFileInfo = new RelativeFileInfo(fileInfo, normalizedPath);
+                var compilationResult = compile(relativeFileInfo).EnsureSuccessful();
+                cacheEntryOptions = GetMemoryCacheEntryOptions(normalizedPath);
+
+                // By default the CompilationResult returned by IRoslynCompiler is an instance of
+                // UncachedCompilationResult. This type has the generated code as a string property and do not want
+                // to cache it. We'll instead cache the unwrapped result.
+                cacheResultToCache = new CompilerCacheResult(
+                    CompilationResult.Successful(compilationResult.CompiledType));
+                cacheResult = new CompilerCacheResult(compilationResult);
+            }
+
+            _cache.Set(normalizedPath, cacheResultToCache, cacheEntryOptions);
+            return cacheResult;
+        }
+
         private MemoryCacheEntryOptions GetMemoryCacheEntryOptions(string relativePath)
         {
             var options = new MemoryCacheEntryOptions();
@@ -101,15 +120,28 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             return options;
         }
 
-        private static string NormalizePath(string path)
+        private string GetNormalizedPath(string relativePath)
         {
-            // We need to allow for scenarios where the application was precompiled on a machine with forward slashes
+            Debug.Assert(relativePath != null);
-            // but is being run in one with backslashes (or vice versa). To this effect, we'll normalize paths to
+            if (relativePath.Length == 0)
-            // use backslashes for lookups and storage in the dictionary.
+            {
-            path = path.Replace('/', '\\');
+                return relativePath;
-            path = path.TrimStart('\\');
+            }
 
-            return path;
+            string normalizedPath;
+            if (!_normalizedPathLookup.TryGetValue(relativePath, out normalizedPath))
+            {
+                var builder = new StringBuilder(relativePath);
+                builder.Replace('\\', '/');
+                if (builder[0] != '/')
+                {
+                    builder.Insert(0, '/');
+                }
+                normalizedPath = builder.ToString();
+                _normalizedPathLookup.TryAdd(relativePath, normalizedPath);
+            }
+
+            return normalizedPath;
         }
     }
 }

src/Microsoft.AspNet.Mvc.Razor/Precompilation/RazorPreCompiler.cs

@@ -25,6 +25,8 @@ namespace Microsoft.AspNet.Mvc.Razor.Precompilation
 {
     public class RazorPreCompiler
     {
+        private const string CacheKeyDirectorySeparator = "/";
+
         public RazorPreCompiler(
             [NotNull] BeforeCompileContext compileContext,
             [NotNull] IFileProvider fileProvider,
@@ -226,13 +228,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Precompilation
             {
                 if (fileInfo.IsDirectory)
                 {
-                    var subPath = Path.Combine(root, fileInfo.Name);
+                    var subPath = CombinePath(root, fileInfo.Name);
                     GetFileInfosRecursive(subPath, razorFiles);
                 }
                 else if (Path.GetExtension(fileInfo.Name)
                          .Equals(FileExtension, StringComparison.OrdinalIgnoreCase))
                 {
-                    var relativePath = Path.Combine(root, fileInfo.Name);
+                    var relativePath = CombinePath(root, fileInfo.Name);
                     var info = new RelativeFileInfo(fileInfo, relativePath);
                     razorFiles.Add(info);
                 }
@@ -291,6 +293,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Precompilation
             return new NonDisposableStream(stream);
         }
 
+        private static string CombinePath(string root, string name)
+        {
+            // We use string.Join instead of Path.Combine here to ensure that the path
+            // separator we produce matches the one used by the CompilerCache.
+            return string.Join(CacheKeyDirectorySeparator, root, name);
+        }
+
         private class PrecompileRazorFileInfoCollection : RazorFileInfoCollection
         {
             public PrecompileRazorFileInfoCollection(string assemblyResourceName,

test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs

@@ -4,9 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-using System.Reflection;
-using Microsoft.AspNet.Mvc.Razor.Precompilation;
-using Microsoft.Dnx.Runtime;
 using Moq;
 using Xunit;
 
@@ -14,8 +11,8 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
 {
     public class CompilerCacheTest
     {
-        private const string ViewPath = "Views/Home/Index.cshtml";
+        private const string ViewPath = "/Views/Home/Index.cshtml";
-        private const string PrecompiledViewsPath = "Views/Home/Precompiled.cshtml";
+        private const string PrecompiledViewsPath = "/Views/Home/Precompiled.cshtml";
         private readonly IDictionary<string, Type> _precompiledViews = new Dictionary<string, Type>
         {
             { PrecompiledViewsPath, typeof(PreCompile) }
@@ -66,6 +63,36 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             Assert.Same(type, actual.CompiledType);
         }
 
+        [Theory]
+        [InlineData("/Areas/Finances/Views/Home/Index.cshtml")]
+        [InlineData(@"Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views/Home\Index.cshtml")]
+        public void GetOrAdd_NormalizesPathSepartorForPaths(string relativePath)
+        {
+            // Arrange
+            var viewPath = "/Areas/Finances/Views/Home/Index.cshtml";
+            var fileProvider = new TestFileProvider();
+            fileProvider.AddFile(viewPath, "some content");
+            var cache = new CompilerCache(fileProvider);
+            var type = typeof(TestView);
+            var expected = UncachedCompilationResult.Successful(type, "hello world");
+
+            // Act - 1
+            var result1 = cache.GetOrAdd(@"Areas\Finances\Views\Home\Index.cshtml", _ => expected);
+
+            // Assert - 1
+            var compilationResult = Assert.IsType<UncachedCompilationResult>(result1.CompilationResult);
+            Assert.Same(expected, compilationResult);
+            Assert.Same(type, compilationResult.CompiledType);
+
+            // Act - 2
+            var result2 = cache.GetOrAdd(relativePath, ThrowsIfCalled);
+
+            // Assert - 2
+            Assert.Same(type, result2.CompilationResult.CompiledType);
+        }
+
         [Fact]
         public void GetOrAdd_ReturnsFileNotFoundIfFileWasDeleted()
         {
@@ -271,6 +298,52 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             Assert.Same(typeof(PreCompile), result3.CompilationResult.CompiledType);
         }
 
+        [Theory]
+        [InlineData("/Areas/Finances/Views/Home/Index.cshtml")]
+        [InlineData(@"Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views/Home\Index.cshtml")]
+        public void GetOrAdd_NormalizesPathSepartorForPathsThatArePrecompiled(string relativePath)
+        {
+            // Arrange
+            var expected = typeof(PreCompile);
+            var viewPath = "/Areas/Finances/Views/Home/Index.cshtml";
+            var cache = new CompilerCache(
+                new TestFileProvider(),
+                new Dictionary<string, Type>
+                {
+                    { viewPath, expected }
+                });
+
+            // Act
+            var result = cache.GetOrAdd(relativePath, ThrowsIfCalled);
+
+            // Assert
+            Assert.Same(expected, result.CompilationResult.CompiledType);
+        }
+
+        [Theory]
+        [InlineData(@"Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views\Home\Index.cshtml")]
+        [InlineData(@"\Areas\Finances\Views/Home\Index.cshtml")]
+        public void ConstructorNormalizesPrecompiledViewPath(string viewPath)
+        {
+            // Arrange
+            var expected = typeof(PreCompile);
+            var cache = new CompilerCache(
+                new TestFileProvider(),
+                new Dictionary<string, Type>
+                {
+                    { viewPath, expected }
+                });
+
+            // Act
+            var result = cache.GetOrAdd("/Areas/Finances/Views/Home/Index.cshtml", ThrowsIfCalled);
+
+            // Assert
+            Assert.Same(expected, result.CompilationResult.CompiledType);
+        }
+
         private class TestView
         {
         }