[Perf] Rewrite ExpressionHelper.GetExpressionText using StringBuilder

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<string>();
             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(
+                    InsertIndexerInvocationText(
-                        GetIndexerInvocation(
+                        builder,
-                            methodExpression.Arguments.Single(),
+                        methodExpression.Arguments.Single(),
-                            expression.Parameters.ToArray()));
+                        expression);
 
                     part = methodExpression.Object;
                 }
@@ -66,10 +67,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
                     containsIndexers = true;
                     var binaryExpression = (BinaryExpression)part;
 
-                    nameParts.Push(
+                    InsertIndexerInvocationText(
-                        GetIndexerInvocation(
+                        builder,
-                            binaryExpression.Right,
+                        binaryExpression.Right,
-                            expression.Parameters.ToArray()));
+                        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 (builder.Length > 0)
-            if (nameParts.Count > 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(
+        private static void InsertIndexerInvocationText(
-            Expression expression,
+            StringBuilder builder,
-            ParameterExpression[] parameters)
+            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<Func<object, object>>(converted, fakeParameter);
             Func<object, object> 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(
+            var runtimeProperties = declaringType.GetRuntimeProperties();
-                property => (string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) &&
+            foreach (var property in runtimeProperties)
-                    property.GetMethod == methodExpression.Method));
+            {
+                if ((string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) &&
+                    property.GetMethod == methodExpression.Method))
+                {
+                    return true;
+                }
+            }
+
+            return false;
         }
     }
 }

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<object[]> ExpressionAndTexts
+        {
+            get
+            {
+                var i = 3;
+                var value = "Test";
+                var Model = new TestModel();
+                var key = "TestModel";
+                var myModels = new List<TestModel>();
+
+                return new TheoryData<Expression, string>
+                {
+                    {
+                        (Expression<Func<TestModel, Category>>)(model => model.SelectedCategory),
+                        "SelectedCategory"
+                    },
+                    {
+                        (Expression<Func<TestModel, Category>>)(m => Model.SelectedCategory),
+                        "SelectedCategory"
+                    },
+                    {
+                        (Expression<Func<TestModel, CategoryName>>)(model => model.SelectedCategory.CategoryName),
+                        "SelectedCategory.CategoryName"
+                    },
+                    {
+                        (Expression<Func<TestModel, int>>)(testModel => testModel.SelectedCategory.CategoryId),
+                        "SelectedCategory.CategoryId"
+                    },
+                    {
+                        (Expression<Func<TestModel, string>>)(model => model.SelectedCategory.CategoryName.MainCategory),
+                        "SelectedCategory.CategoryName.MainCategory"
+                    },
+                    {
+                        (Expression<Func<TestModel, TestModel>>)(model => model),
+                        string.Empty
+                    },
+                    {
+                        (Expression<Func<TestModel, string>>)(model => value),
+                        "value"
+                    },
+                    {
+                        (Expression<Func<TestModel, TestModel>>)(m => Model),
+                        string.Empty
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, Category>>)(model => model[2].SelectedCategory),
+                        "[2].SelectedCategory"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, Category>>)(model => model[i].SelectedCategory),
+                        "[3].SelectedCategory"
+                    },
+                    {
+                        (Expression<Func<IDictionary<string, TestModel>, string>>)(model => model[key].SelectedCategory.CategoryName.MainCategory),
+                        "[TestModel].SelectedCategory.CategoryName.MainCategory"
+                    },
+                    {
+                        (Expression<Func<TestModel, int>>)(model => model.PreferredCategories[i].CategoryId),
+                        "PreferredCategories[3].CategoryId"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, Category>>)(model => myModels[i].SelectedCategory),
+                        "myModels[3].SelectedCategory"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, int>>)(model => model[2].PreferredCategories[i].CategoryId),
+                        "[2].PreferredCategories[3].CategoryId"
+                    },
+                };
+            }
+        }
+
         public static IEnumerable<object[]> 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; }
         }
     }