From 8a33972c094c49dc159c18b1559b19df92af9fb2 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 28 Aug 2015 12:36:23 -0700 Subject: [PATCH] RazorViewEngine should cache transformed paths when view cannot be found Fixes #3034 --- .../DefaultViewLocationCache.cs | 22 ++-- .../IViewLocationCache.cs | 4 +- .../RazorViewEngine.cs | 66 ++++++---- .../ViewLocationCacheResult.cs | 121 ++++++++++++++++++ .../DefaultViewLocationCacheTest.cs | 20 ++- .../RazorViewEngineTest.cs | 78 ++++++++++- 6 files changed, 265 insertions(+), 46 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Razor/ViewLocationCacheResult.cs diff --git a/src/Microsoft.AspNet.Mvc.Razor/DefaultViewLocationCache.cs b/src/Microsoft.AspNet.Mvc.Razor/DefaultViewLocationCache.cs index 36e0ccf688..dae4a3ff08 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/DefaultViewLocationCache.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/DefaultViewLocationCache.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Concurrent; using System.Linq; using System.Text; -using Microsoft.AspNet.Mvc.Razor.Internal; using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Mvc.Razor @@ -18,28 +17,33 @@ namespace Microsoft.AspNet.Mvc.Razor private const char CacheKeySeparator = ':'; // A mapping of keys generated from ViewLocationExpanderContext to view locations. - private readonly ConcurrentDictionary _cache; + private readonly ConcurrentDictionary _cache; /// /// Initializes a new instance of . /// public DefaultViewLocationCache() { - _cache = new ConcurrentDictionary(StringComparer.Ordinal); + _cache = new ConcurrentDictionary(StringComparer.Ordinal); } /// - public string Get([NotNull] ViewLocationExpanderContext context) + public ViewLocationCacheResult Get([NotNull] ViewLocationExpanderContext context) { var cacheKey = GenerateKey(context); - string result; - _cache.TryGetValue(cacheKey, out result); - return result; + ViewLocationCacheResult result; + if (_cache.TryGetValue(cacheKey, out result)) + { + return result; + } + + return ViewLocationCacheResult.None; } /// - public void Set([NotNull] ViewLocationExpanderContext context, - [NotNull] string value) + public void Set( + [NotNull] ViewLocationExpanderContext context, + [NotNull] ViewLocationCacheResult value) { var cacheKey = GenerateKey(context); _cache.TryAdd(cacheKey, value); diff --git a/src/Microsoft.AspNet.Mvc.Razor/IViewLocationCache.cs b/src/Microsoft.AspNet.Mvc.Razor/IViewLocationCache.cs index 6e26866257..e3378a8890 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/IViewLocationCache.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/IViewLocationCache.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.Razor /// The for the current view location /// expansion. /// The cached location, if available, null otherwise. - string Get(ViewLocationExpanderContext context); + ViewLocationCacheResult Get(ViewLocationExpanderContext context); /// /// Adds a cache entry for values specified by . @@ -22,6 +22,6 @@ namespace Microsoft.AspNet.Mvc.Razor /// The for the current view location /// expansion. /// The view location that is to be cached. - void Set(ViewLocationExpanderContext context, string value); + void Set(ViewLocationExpanderContext context, ViewLocationCacheResult value); } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs b/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs index 855b00ab00..bb8ca27767 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs @@ -216,9 +216,10 @@ namespace Microsoft.AspNet.Mvc.Razor } } - private RazorPageResult LocatePageFromViewLocations(ActionContext context, - string pageName, - bool isPartial) + private RazorPageResult LocatePageFromViewLocations( + ActionContext context, + string pageName, + bool isPartial) { // Initialize the dictionary for the typical case of having controller and action tokens. var areaName = GetNormalizedRouteValue(context, AreaKey); @@ -240,47 +241,64 @@ namespace Microsoft.AspNet.Mvc.Razor } // 2. With the values that we've accumumlated so far, check if we have a cached result. - var pageLocation = _viewLocationCache.Get(expanderContext); - if (!string.IsNullOrEmpty(pageLocation)) + IEnumerable locationsToSearch = null; + var cachedResult = _viewLocationCache.Get(expanderContext); + if (!cachedResult.Equals(ViewLocationCacheResult.None)) { - var page = _pageFactory.CreateInstance(pageLocation); - - if (page != null) + if (cachedResult.IsFoundResult) { - // 2a. We found a IRazorPage at the cached location. - return new RazorPageResult(pageName, page); + var page = _pageFactory.CreateInstance(cachedResult.ViewLocation); + + if (page != null) + { + // 2a We have a cache entry where a view was previously found. + return new RazorPageResult(pageName, page); + } + } + else + { + locationsToSearch = cachedResult.SearchedLocations; } } - // 2b. We did not find a cached location or did not find a IRazorPage at the cached location. - // The cached value has expired and we need to look up the page. - foreach (var expander in _viewLocationExpanders) + if (locationsToSearch == null) { - viewLocations = expander.ExpandViewLocations(expanderContext, viewLocations); + // 2b. We did not find a cached location or did not find a IRazorPage at the cached location. + // The cached value has expired and we need to look up the page. + foreach (var expander in _viewLocationExpanders) + { + viewLocations = expander.ExpandViewLocations(expanderContext, viewLocations); + } + + var controllerName = GetNormalizedRouteValue(context, ControllerKey); + + locationsToSearch = viewLocations.Select( + location => string.Format( + CultureInfo.InvariantCulture, + location, + pageName, + controllerName, + areaName + )); } // 3. Use the expanded locations to look up a page. - var controllerName = GetNormalizedRouteValue(context, ControllerKey); var searchedLocations = new List(); - foreach (var path in viewLocations) + foreach (var path in locationsToSearch) { - var transformedPath = string.Format(CultureInfo.InvariantCulture, - path, - pageName, - controllerName, - areaName); - var page = _pageFactory.CreateInstance(transformedPath); + var page = _pageFactory.CreateInstance(path); if (page != null) { // 3a. We found a page. Cache the set of values that produced it and return a found result. - _viewLocationCache.Set(expanderContext, transformedPath); + _viewLocationCache.Set(expanderContext, new ViewLocationCacheResult(path, searchedLocations)); return new RazorPageResult(pageName, page); } - searchedLocations.Add(transformedPath); + searchedLocations.Add(path); } // 3b. We did not find a page for any of the paths. + _viewLocationCache.Set(expanderContext, new ViewLocationCacheResult(searchedLocations)); return new RazorPageResult(pageName, searchedLocations); } diff --git a/src/Microsoft.AspNet.Mvc.Razor/ViewLocationCacheResult.cs b/src/Microsoft.AspNet.Mvc.Razor/ViewLocationCacheResult.cs new file mode 100644 index 0000000000..cfd1241018 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Razor/ViewLocationCacheResult.cs @@ -0,0 +1,121 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Framework.Internal; + +namespace Microsoft.AspNet.Mvc.Razor +{ + /// + /// Result of lookups. + /// + public struct ViewLocationCacheResult : IEquatable + { + /// + /// Initializes a new instance of + /// for a view that was successfully found at the specified location. + /// + /// The view location. + /// Locations that were searched + /// in addition to . + public ViewLocationCacheResult( + [NotNull] string foundLocation, + [NotNull] IEnumerable searchedLocations) + : this (searchedLocations) + { + ViewLocation = foundLocation; + SearchedLocations = searchedLocations; + IsFoundResult = true; + } + + /// + /// Initializes a new instance of for a + /// failed view lookup. + /// + /// Locations that were searched. + public ViewLocationCacheResult([NotNull] IEnumerable searchedLocations) + { + SearchedLocations = searchedLocations; + ViewLocation = null; + IsFoundResult = false; + } + + /// + /// A that represents a cache miss. + /// + public static readonly ViewLocationCacheResult None = new ViewLocationCacheResult(Enumerable.Empty()); + + /// + /// The location the view was found. + /// + /// This is available if is true. + public string ViewLocation { get; } + + /// + /// The sequence of locations that were searched. + /// + /// + /// When is true this includes all paths that were search prior to finding + /// a view at . When is false, this includes + /// all search paths. + /// + public IEnumerable SearchedLocations { get; } + + /// + /// Gets a value that indicates whether the view was successfully found. + /// + public bool IsFoundResult { get; } + + /// + public bool Equals(ViewLocationCacheResult other) + { + if (IsFoundResult != other.IsFoundResult) + { + return false; + } + + if (IsFoundResult) + { + return string.Equals(ViewLocation, other.ViewLocation, StringComparison.Ordinal); + } + else + { + if (SearchedLocations == other.SearchedLocations) + { + return true; + } + + if (SearchedLocations == null || other.SearchedLocations == null) + { + return false; + } + + return Enumerable.SequenceEqual(SearchedLocations, other.SearchedLocations, StringComparer.Ordinal); + } + } + + /// + public override int GetHashCode() + { + var hashCodeCombiner = HashCodeCombiner.Start() + .Add(IsFoundResult); + + + if (IsFoundResult) + { + hashCodeCombiner.Add(ViewLocation, StringComparer.Ordinal); + } + else if (SearchedLocations != null) + { + foreach (var location in SearchedLocations) + { + hashCodeCombiner.Add(location, StringComparer.Ordinal); + } + } + + return hashCodeCombiner; + } + } +} diff --git a/test/Microsoft.AspNet.Mvc.Razor.Test/DefaultViewLocationCacheTest.cs b/test/Microsoft.AspNet.Mvc.Razor.Test/DefaultViewLocationCacheTest.cs index e6e4a100f6..3e9da762c2 100644 --- a/test/Microsoft.AspNet.Mvc.Razor.Test/DefaultViewLocationCacheTest.cs +++ b/test/Microsoft.AspNet.Mvc.Razor.Test/DefaultViewLocationCacheTest.cs @@ -53,7 +53,7 @@ namespace Microsoft.AspNet.Mvc.Razor var result = cache.Get(context); // Assert - Assert.Null(result); + Assert.Equal(result, ViewLocationCacheResult.None); } [Theory] @@ -62,13 +62,25 @@ namespace Microsoft.AspNet.Mvc.Razor { // Arrange var cache = new DefaultViewLocationCache(); - var value = Guid.NewGuid().ToString(); + var value = new ViewLocationCacheResult( + Guid.NewGuid().ToString(), + new[] + { + Guid.NewGuid().ToString(), + Guid.NewGuid().ToString() + }); - // Act + // Act - 1 cache.Set(context, value); var result = cache.Get(context); - // Assert + // Assert - 1 + Assert.Equal(value, result); + + // Act - 2 + result = cache.Get(context); + + // Assert - 2 Assert.Equal(value, result); } diff --git a/test/Microsoft.AspNet.Mvc.Razor.Test/RazorViewEngineTest.cs b/test/Microsoft.AspNet.Mvc.Razor.Test/RazorViewEngineTest.cs index 488302994d..030ae61263 100644 --- a/test/Microsoft.AspNet.Mvc.Razor.Test/RazorViewEngineTest.cs +++ b/test/Microsoft.AspNet.Mvc.Razor.Test/RazorViewEngineTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Linq; using Microsoft.AspNet.Http.Internal; using Microsoft.AspNet.Mvc.Rendering; using Microsoft.AspNet.Mvc.Routing; @@ -409,9 +410,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Test var cache = GetViewLocationCache(); var cacheMock = Mock.Get(cache); - - cacheMock.Setup(c => c.Set(It.IsAny(), "/Views/Shared/baz.cshtml")) - .Verifiable(); + cacheMock.Setup(c => c.Set(It.IsAny(), It.IsAny())) + .Callback((ViewLocationExpanderContext _, ViewLocationCacheResult cacheResult) => + { + Assert.Equal("/Views/Shared/baz.cshtml", cacheResult.ViewLocation); + Assert.Equal(new[] { "/Views/bar/baz.cshtml" }, cacheResult.SearchedLocations); + }) + .Verifiable(); var viewEngine = CreateViewEngine(pageFactory.Object, viewFactory.Object, cache: cache); var context = GetActionContext(_controllerTestContext); @@ -443,8 +448,8 @@ namespace Microsoft.AspNet.Mvc.Razor.Test .Verifiable(); var cacheMock = new Mock(); cacheMock.Setup(c => c.Get(It.IsAny())) - .Returns("some-view-location") - .Verifiable(); + .Returns(new ViewLocationCacheResult("some-view-location", Enumerable.Empty())) + .Verifiable(); var viewEngine = CreateViewEngine(pageFactory.Object, viewFactory.Object, @@ -480,7 +485,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Test var cacheMock = new Mock(); cacheMock.Setup(c => c.Get(It.IsAny())) - .Returns("expired-location"); + .Returns(new ViewLocationCacheResult("expired-location", Enumerable.Empty())); var expander = new Mock(); expander.Setup(v => v.PopulateValues(It.IsAny())) @@ -507,6 +512,65 @@ namespace Microsoft.AspNet.Mvc.Razor.Test expander.Verify(); } + // This test validates an important perf scenario of RazorViewEngine not constructing + // multiple strings for views that do not exist in the file system on a per-request basis. + [Fact] + public void FindView_DoesNotInvokeExpandViewLocations_IfCacheEntryMatchesButViewIsNotFound() + { + // Arrange + var pageFactory = Mock.Of(); + var viewFactory = Mock.Of(); + var cache = new DefaultViewLocationCache(); + var expander = new Mock(); + var expandedLocations = new[] + { + "viewlocation1", + "viewlocation2", + "viewlocation3", + }; + expander + .Setup(v => v.PopulateValues(It.IsAny())) + .Callback((ViewLocationExpanderContext expanderContext) => + { + expanderContext.Values["somekey"] = "somevalue"; + }) + .Verifiable(); + expander + .Setup(v => v.ExpandViewLocations( + It.IsAny(), It.IsAny>())) + .Returns((ViewLocationExpanderContext c, IEnumerable viewLocations) => expandedLocations) + .Verifiable(); + + + var viewEngine = CreateViewEngine( + pageFactory, + viewFactory, + expanders: new[] { expander.Object }, + cache: cache); + var context = GetActionContext(_controllerTestContext); + + // Act - 1 + var result = viewEngine.FindView(context, "myview"); + + // Assert - 1 + Assert.False(result.Success); + Assert.Equal(expandedLocations, result.SearchedLocations); + expander.Verify(); + + // Act - 2 + result = viewEngine.FindView(context, "myview"); + + // Assert - 2 + Assert.False(result.Success); + Assert.Equal(expandedLocations, result.SearchedLocations); + expander.Verify( + v => v.PopulateValues(It.IsAny()), + Times.Exactly(2)); + expander.Verify( + v => v.ExpandViewLocations(It.IsAny(), It.IsAny>()), + Times.Once()); + } + [Theory] [InlineData(null)] [InlineData("")] @@ -1045,7 +1109,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Test options.ViewLocationExpanders.Add(expander); } } - + var optionsAccessor = new Mock>(); optionsAccessor.SetupGet(v => v.Options) .Returns(options);