From 7fbd407ad57d78bbc0c382c71d45289205173bf6 Mon Sep 17 00:00:00 2001 From: mnltejaswini Date: Fri, 25 Mar 2016 10:23:32 -0700 Subject: [PATCH] [Perf] Rewrite ExpressionHelper.GetExpressionText using StringBuilder --- .../Internal/ExpressionHelper.cs | 104 +++++++++++------- .../Internal/ExpressionHelperTest.cs | 85 +++++++++++++- 2 files changed, 147 insertions(+), 42 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs index 0d51a990e5..414ff0038c 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs @@ -2,11 +2,11 @@ // 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.Globalization; using System.Linq; using System.Linq.Expressions; using System.Reflection; +using System.Text; namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { @@ -38,10 +38,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } var containsIndexers = false; - // Split apart the expression string for property/field accessors to create its name - var nameParts = new Stack(); var part = expression.Body; + // Builder to concatenate the names for property/field accessors within an expression to create a string. + var builder = new StringBuilder(); + while (part != null) { if (part.NodeType == ExpressionType.Call) @@ -54,10 +55,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal break; } - nameParts.Push( - GetIndexerInvocation( - methodExpression.Arguments.Single(), - expression.Parameters.ToArray())); + InsertIndexerInvocationText( + builder, + methodExpression.Arguments.Single(), + expression); part = methodExpression.Object; } @@ -66,10 +67,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal containsIndexers = true; var binaryExpression = (BinaryExpression)part; - nameParts.Push( - GetIndexerInvocation( - binaryExpression.Right, - expression.Parameters.ToArray())); + InsertIndexerInvocationText( + builder, + binaryExpression.Right, + expression); part = binaryExpression.Left; } @@ -87,20 +88,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal break; } - nameParts.Push("." + name); + builder.Insert(0, name); + builder.Insert(0, '.'); part = memberExpressionPart.Expression; } - else if (part.NodeType == ExpressionType.Parameter) - { - // When the expression is parameter based (m => m.Something...), we'll push an empty - // string onto the stack and stop evaluating. The extra empty string makes sure that - // we don't accidentally cut off too much of m => m.Model. - nameParts.Push(string.Empty); - - // Exit loop. Have the entire name because the parameter cannot be used as an indexer; always the - // leftmost expression node. - break; - } else { break; @@ -108,17 +99,24 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } // If parts start with "model", then strip that part away. - if (nameParts.Count > 0 && string.Equals(nameParts.Peek(), ".model", StringComparison.OrdinalIgnoreCase)) + if (part == null || part.NodeType != ExpressionType.Parameter) { - nameParts.Pop(); + var text = builder.ToString(); + if (text.StartsWith(".model", StringComparison.OrdinalIgnoreCase)) + { + // 6 is the length of the string ".model". + builder.Remove(0, 6); + } } - expressionText = string.Empty; - if (nameParts.Count > 0) + if (builder.Length > 0) { - expressionText = nameParts.Aggregate((left, right) => left + right).TrimStart('.'); + // Trim the leading "." if present. + builder.Replace(".", string.Empty, 0, 1); } + expressionText = builder.ToString(); + if (expressionTextCache != null && !containsIndexers) { expressionTextCache.Entries.TryAdd(expression, expressionText); @@ -127,21 +125,34 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return expressionText; } - private static string GetIndexerInvocation( - Expression expression, - ParameterExpression[] parameters) + private static void InsertIndexerInvocationText( + StringBuilder builder, + Expression indexExpression, + LambdaExpression parentExpression) { - if (expression == null) + if (builder == null) { - throw new ArgumentNullException(nameof(expression)); + throw new ArgumentNullException(nameof(builder)); } - if (parameters == null) + if (indexExpression == null) { - throw new ArgumentNullException(nameof(parameters)); + throw new ArgumentNullException(nameof(indexExpression)); } - var converted = Expression.Convert(expression, typeof(object)); + if (parentExpression == null) + { + throw new ArgumentNullException(nameof(parentExpression)); + } + + if (parentExpression.Parameters == null) + { + throw new ArgumentException(Resources.FormatPropertyOfTypeCannotBeNull( + nameof(parentExpression.Parameters), + nameof(parentExpression))); + } + + var converted = Expression.Convert(indexExpression, typeof(object)); var fakeParameter = Expression.Parameter(typeof(object), null); var lambda = Expression.Lambda>(converted, fakeParameter); Func func; @@ -152,12 +163,15 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } catch (InvalidOperationException ex) { + var parameters = parentExpression.Parameters.ToArray(); throw new InvalidOperationException( - Resources.FormatExpressionHelper_InvalidIndexerExpression(expression, parameters[0].Name), + Resources.FormatExpressionHelper_InvalidIndexerExpression(indexExpression, parameters[0].Name), ex); } - return "[" + Convert.ToString(func(null), CultureInfo.InvariantCulture) + "]"; + builder.Insert(0, ']'); + builder.Insert(0, Convert.ToString(func(null), CultureInfo.InvariantCulture)); + builder.Insert(0, '['); } public static bool IsSingleArgumentIndexer(Expression expression) @@ -178,9 +192,17 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } // Find default property (the indexer) and confirm its getter is the method in this expression. - return declaringType.GetRuntimeProperties().Any( - property => (string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && - property.GetMethod == methodExpression.Method)); + var runtimeProperties = declaringType.GetRuntimeProperties(); + foreach (var property in runtimeProperties) + { + if ((string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && + property.GetMethod == methodExpression.Method)) + { + return true; + } + } + + return false; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs index 0a9841b094..36ce863aff 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs @@ -12,6 +12,78 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { private readonly ExpressionTextCache _expressionTextCache = new ExpressionTextCache(); + public static IEnumerable ExpressionAndTexts + { + get + { + var i = 3; + var value = "Test"; + var Model = new TestModel(); + var key = "TestModel"; + var myModels = new List(); + + return new TheoryData + { + { + (Expression>)(model => model.SelectedCategory), + "SelectedCategory" + }, + { + (Expression>)(m => Model.SelectedCategory), + "SelectedCategory" + }, + { + (Expression>)(model => model.SelectedCategory.CategoryName), + "SelectedCategory.CategoryName" + }, + { + (Expression>)(testModel => testModel.SelectedCategory.CategoryId), + "SelectedCategory.CategoryId" + }, + { + (Expression>)(model => model.SelectedCategory.CategoryName.MainCategory), + "SelectedCategory.CategoryName.MainCategory" + }, + { + (Expression>)(model => model), + string.Empty + }, + { + (Expression>)(model => value), + "value" + }, + { + (Expression>)(m => Model), + string.Empty + }, + { + (Expression, Category>>)(model => model[2].SelectedCategory), + "[2].SelectedCategory" + }, + { + (Expression, Category>>)(model => model[i].SelectedCategory), + "[3].SelectedCategory" + }, + { + (Expression, string>>)(model => model[key].SelectedCategory.CategoryName.MainCategory), + "[TestModel].SelectedCategory.CategoryName.MainCategory" + }, + { + (Expression>)(model => model.PreferredCategories[i].CategoryId), + "PreferredCategories[3].CategoryId" + }, + { + (Expression, Category>>)(model => myModels[i].SelectedCategory), + "myModels[3].SelectedCategory" + }, + { + (Expression, int>>)(model => model[2].PreferredCategories[i].CategoryId), + "[2].PreferredCategories[3].CategoryId" + }, + }; + } + } + public static IEnumerable CachedExpressions { get @@ -162,6 +234,17 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } + [Theory] + [MemberData(nameof(ExpressionAndTexts))] + public void GetExpressionText_ReturnsExpectedExpressionText(LambdaExpression expression, string expressionText) + { + // Act + var text = ExpressionHelper.GetExpressionText(expression, _expressionTextCache); + + // Assert + Assert.Equal(expressionText, text); + } + [Theory] [MemberData(nameof(CachedExpressions))] public void GetExpressionText_CachesExpression(LambdaExpression expression) @@ -235,7 +318,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal private class CategoryName { - public string MainCategory { get; set; } + public string MainCategory { get; set; } public string SubCategory { get; set; } } }