[Perf] Optimize CoerceResultToTaskAsync to remove MethodInfo.Invoke

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionExecutor.cs

@@ -4,27 +4,13 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
-using System.Linq;
 using System.Reflection;
-using System.Runtime.ExceptionServices;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.Mvc.Core;
-using Microsoft.Extensions.Internal;
 
 namespace Microsoft.AspNetCore.Mvc.Internal
 {
     public static class ControllerActionExecutor
     {
-        private static readonly MethodInfo _convertOfTMethod =
-            typeof(ControllerActionExecutor).GetRuntimeMethods().Single(methodInfo => methodInfo.Name == "Convert");
-
-        // Method called via reflection.
-        private static Task<object> Convert<T>(object taskAsObject)
-        {
-            var task = (Task<T>)taskAsObject;
-            return CastToObject<T>(task);
-        }
-
         public static Task<object> ExecuteAsync(
             ObjectMethodExecutor actionMethodExecutor,
             object instance,
@@ -39,57 +25,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             object instance,
             object[] orderedActionArguments)
         {
-            object invocationResult = actionMethodExecutor.Execute(instance, orderedActionArguments);
+            return actionMethodExecutor.ExecuteAsync(instance, orderedActionArguments);
-            return CoerceResultToTaskAsync(
-                invocationResult,
-                actionMethodExecutor.MethodInfo.ReturnType,
-                actionMethodExecutor.MethodInfo.Name,
-                actionMethodExecutor.MethodInfo.DeclaringType);
-        }
-
-        // We need to CoerceResult as the object value returned from methodInfo.Invoke has to be cast to a Task<T>.
-        // This is necessary to enable calling await on the returned task.
-        // i.e we need to write the following var result = await (Task<ActualType>)mInfo.Invoke.
-        // Returning Task<object> enables us to await on the result.
-        // This method is intentionally not using async pattern to keep jit time (on cold start) to a minimum.
-        private static Task<object> CoerceResultToTaskAsync(
-            object result,
-            Type returnType,
-            string methodName,
-            Type declaringType)
-        {
-            // If it is either a Task or Task<T>
-            // must coerce the return value to Task<object>
-            var resultAsTask = result as Task;
-            if (resultAsTask != null)
-            {
-                if (returnType == typeof(Task))
-                {
-                    ThrowIfWrappedTaskInstance(resultAsTask.GetType(), methodName, declaringType);
-                    return CastToObject(resultAsTask);
-                }
-
-                var taskValueType = GetTaskInnerTypeOrNull(returnType);
-                if (taskValueType != null)
-                {
-                    // for: public Task<T> Action()
-                    // constructs: return (Task<object>)Convert<T>((Task<T>)result)
-                    var genericMethodInfo = _convertOfTMethod.MakeGenericMethod(taskValueType);
-                    var convertedResult = (Task<object>)genericMethodInfo.Invoke(null, new object[] { result });
-                    return convertedResult;
-                }
-
-                // This will be the case for:
-                // 1. Types which have derived from Task and Task<T>,
-                // 2. Action methods which use dynamic keyword but return a Task or Task<T>.
-                throw new InvalidOperationException(Resources.FormatActionExecutor_UnexpectedTaskInstance(
-                    methodName,
-                    declaringType));
-            }
-            else
-            {
-                return Task.FromResult(result);
-            }
         }
 
         public static object[] PrepareArguments(
@@ -137,47 +73,5 @@ namespace Microsoft.AspNetCore.Mvc.Internal
 
             return arguments;
         }
-
-        private static void ThrowIfWrappedTaskInstance(Type actualTypeReturned, string methodName, Type declaringType)
-        {
-            // Throw if a method declares a return type of Task and returns an instance of Task<Task> or Task<Task<T>>
-            // This most likely indicates that the developer forgot to call Unwrap() somewhere.
-            if (actualTypeReturned != typeof(Task))
-            {
-                var innerTaskType = GetTaskInnerTypeOrNull(actualTypeReturned);
-                if (innerTaskType != null && typeof(Task).IsAssignableFrom(innerTaskType))
-                {
-                    throw new InvalidOperationException(
-                        Resources.FormatActionExecutor_WrappedTaskInstance(
-                            methodName,
-                            declaringType,
-                            actualTypeReturned.FullName));
-                }
-            }
-        }
-
-        /// <summary>
-        /// Cast Task to Task of object
-        /// </summary>
-        private static async Task<object> CastToObject(Task task)
-        {
-            await task;
-            return null;
-        }
-
-        /// <summary>
-        /// Cast Task of T to Task of object
-        /// </summary>
-        private static async Task<object> CastToObject<T>(Task<T> task)
-        {
-            return (object)await task;
-        }
-
-        private static Type GetTaskInnerTypeOrNull(Type type)
-        {
-            var genericType = ClosedGenericMatcher.ExtractGenericInterface(type, typeof(Task<>));
-
-            return genericType?.GenericTypeArguments[0];
-        }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs

@@ -3,15 +3,35 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc.Core;
+using Microsoft.Extensions.Internal;
 
 namespace Microsoft.AspNetCore.Mvc.Internal
 {
     public class ObjectMethodExecutor
     {
+        private ActionExecutorAsync _executorAsync;
         private ActionExecutor _executor;
 
+        private static readonly MethodInfo _convertOfTMethod =
+            typeof(ObjectMethodExecutor).GetRuntimeMethods().Single(methodInfo => methodInfo.Name == nameof(ObjectMethodExecutor.Convert));
+
+        private static readonly Expression<Func<object, Task<object>>> _createTaskFromResultExpression =
+            ((result) => Task.FromResult(result));
+
+        private static readonly MethodInfo _createTaskFromResultMethod = 
+            ((MethodCallExpression)_createTaskFromResultExpression.Body).Method;
+
+        private static readonly Expression<Func<object, string, Type, Task<object>>> _coerceTaskExpression =
+            ((result, methodName, declaringType) => ObjectMethodExecutor.CoerceTaskType(result, methodName, declaringType));
+
+        private static readonly MethodInfo _coerceMethod = ((MethodCallExpression)_coerceTaskExpression.Body).Method;
+
+
         private ObjectMethodExecutor(MethodInfo methodInfo)
         {            
             if (methodInfo == null)
@@ -21,6 +41,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             MethodInfo = methodInfo;
         }
 
+        private delegate Task<object> ActionExecutorAsync(object target, object[] parameters);
+
         private delegate object ActionExecutor(object target, object[] parameters);
 
         private delegate void VoidActionExecutor(object target, object[] parameters);
@@ -31,9 +53,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal
         {
             var executor = new ObjectMethodExecutor(methodInfo);
             executor._executor = GetExecutor(methodInfo, targetTypeInfo);
+            executor._executorAsync = GetExecutorAsync(methodInfo, targetTypeInfo);
             return executor;
         }
 
+        public Task<object> ExecuteAsync(object target, object[] parameters)
+        {
+            return _executorAsync(target, parameters);
+        }
+
         public object Execute(object target, object[] parameters)
         {
             return _executor(target, parameters);
@@ -87,5 +115,128 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                 return null;
             };
         }
+
+        private static ActionExecutorAsync GetExecutorAsync(MethodInfo methodInfo, TypeInfo targetTypeInfo)
+        {
+            // Parameters to executor
+            var targetParameter = Expression.Parameter(typeof(object), "target");
+            var parametersParameter = Expression.Parameter(typeof(object[]), "parameters");
+
+            // Build parameter list
+            var parameters = new List<Expression>();
+            var paramInfos = methodInfo.GetParameters();
+            for (int i = 0; i < paramInfos.Length; i++)
+            {
+                var paramInfo = paramInfos[i];
+                var valueObj = Expression.ArrayIndex(parametersParameter, Expression.Constant(i));
+                var valueCast = Expression.Convert(valueObj, paramInfo.ParameterType);
+
+                // valueCast is "(Ti) parameters[i]"
+                parameters.Add(valueCast);
+            }
+
+            // Call method
+            var instanceCast = Expression.Convert(targetParameter, targetTypeInfo.AsType());
+            var methodCall = Expression.Call(instanceCast, methodInfo, parameters);
+
+            // methodCall is "((Ttarget) target) method((T0) parameters[0], (T1) parameters[1], ...)"
+            // Create function
+            if (methodCall.Type == typeof(void))
+            {
+                var lambda = Expression.Lambda<VoidActionExecutor>(methodCall, targetParameter, parametersParameter);
+                var voidExecutor = lambda.Compile();
+                return WrapVoidActionAsync(voidExecutor);
+            }
+            else
+            {
+                // must coerce methodCall to match ActionExecutorAsync signature
+                var coerceMethodCall = GetCoerceMethodCallExpression(methodCall, methodInfo);
+                var lambda = Expression.Lambda<ActionExecutorAsync>(coerceMethodCall, targetParameter, parametersParameter);
+                return lambda.Compile();
+            }
+        }
+
+        // We need to CoerceResult as the object value returned from methodInfo.Invoke has to be cast to a Task<T>.
+        // This is necessary to enable calling await on the returned task.
+        // i.e we need to write the following var result = await (Task<ActualType>)mInfo.Invoke.
+        // Returning Task<object> enables us to await on the result.
+        private static Expression GetCoerceMethodCallExpression(MethodCallExpression methodCall, MethodInfo methodInfo)
+        {
+            var castMethodCall = Expression.Convert(methodCall, typeof(object));
+            var returnType = methodCall.Type;
+
+            if (typeof(Task).IsAssignableFrom(returnType))
+            {
+                if (returnType == typeof(Task))
+                {
+                    var stringExpression = Expression.Constant(methodInfo.Name);
+                    var typeExpression = Expression.Constant(methodInfo.DeclaringType);
+                    return Expression.Call(null, _coerceMethod, castMethodCall, stringExpression, typeExpression);
+                }
+
+                var taskValueType = GetTaskInnerTypeOrNull(returnType);
+                if (taskValueType != null)
+                {
+                    // for: public Task<T> Action()
+                    // constructs: return (Task<object>)Convert<T>((Task<T>)result)
+                    var genericMethodInfo = _convertOfTMethod.MakeGenericMethod(taskValueType);
+                    var genericMethodCall = Expression.Call(null, genericMethodInfo, castMethodCall);
+                    var convertedResult = Expression.Convert(genericMethodCall, typeof(Task<object>));
+                    return convertedResult;
+                }
+
+                // This will be the case for types which have derived from Task and Task<T>
+                throw new InvalidOperationException(Resources.FormatActionExecutor_UnexpectedTaskInstance(
+                    methodInfo.Name,
+                    methodInfo.DeclaringType));
+            }
+
+            return Expression.Call(null, _createTaskFromResultMethod, castMethodCall);
+        }
+
+        private static ActionExecutorAsync WrapVoidActionAsync(VoidActionExecutor executor)
+        {
+            return delegate (object target, object[] parameters)
+            {
+                executor(target, parameters);
+                return Task.FromResult<object>(null);
+            };
+        }
+
+        private static Task<object> CoerceTaskType(object result, string methodName, Type declaringType)
+        {
+            var resultAsTask = (Task)result;
+            return CastToObject(resultAsTask);
+        }
+
+        /// <summary>
+        /// Cast Task to Task of object
+        /// </summary>
+        private static async Task<object> CastToObject(Task task)
+        {
+            await task;
+            return null;
+        }
+
+        /// <summary>
+        /// Cast Task of T to Task of object
+        /// </summary>
+        private static async Task<object> CastToObject<T>(Task<T> task)
+        {
+            return (object)await task;
+        }
+
+        private static Type GetTaskInnerTypeOrNull(Type type)
+        {
+            var genericType = ClosedGenericMatcher.ExtractGenericInterface(type, typeof(Task<>));
+
+            return genericType?.GenericTypeArguments[0];
+        }
+
+        private static Task<object> Convert<T>(object taskAsObject)
+        {
+            var task = (Task<T>)taskAsObject;
+            return CastToObject<T>(task);
+        }
     }
 }

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionExecutorTests.cs

@@ -288,8 +288,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             Assert.Equal(expectedException, ex.Message);
         }
 
+        // Async actions that declares a return type of Task and returns an instance of Task<Task> or Task<Task<T>>
+        // are not handled in any special way as they are not supported. They are considered as methods that return Task type.
         [Fact]
-        public async Task AsyncAction_ReturningUnwrappedTaskThrows()
+        public async Task AsyncAction_ReturningUnwrappedTask()
         {
             // Arrange
             var inputParam1 = 1;
@@ -298,24 +300,20 @@ namespace Microsoft.AspNetCore.Mvc.Internal
 
             var methodWithUnwrappedTask = new MethodWithTaskReturnType(_controller.UnwrappedTask);
 
-            var expectedException = string.Format(
+            // Act
-                CultureInfo.CurrentCulture,
+            var result = await ExecuteAction(
-                "The method 'UnwrappedTask' on type '{0}' returned an instance of '{1}'. " +
+                methodWithUnwrappedTask,
-                "Make sure to call Unwrap on the returned value to avoid unobserved faulted Task.",
+                _controller,
-                typeof(TestController),
+                actionParameters);
-                typeof(Task<Task>).FullName);
 
-            // Act & Assert
+            // Assert
-            var ex = await Assert.ThrowsAsync<InvalidOperationException>(
+            Assert.Same(null, result);
-                () => ExecuteAction(
-                    methodWithUnwrappedTask,
-                    _controller,
-                    actionParameters));
-            Assert.Equal(expectedException, ex.Message);
         }
 
         [Fact]
-        public async Task AsyncAction_WithDynamicReturnTypeThrows()
+        // Async actions that has dynamic return type but return Task or Task<T> are not handled in any special way as,
+        // they are not supported. They are considered as regular methods that return object type.
+        public async Task AsyncAction_WithDynamicReturnType()
         {
             // Arrange
             var inputParam1 = 1;
@@ -323,18 +321,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             var actionParameters = new Dictionary<string, object> { { "i", inputParam1 }, { "s", inputParam2 } };
 
             var dynamicTaskMethod = new ReturnTaskAsDynamicValue(_controller.ReturnTaskAsDynamicValue);
-            var expectedException = string.Format(
-                CultureInfo.CurrentCulture,
-                "The method 'ReturnTaskAsDynamicValue' on type '{0}' returned a Task instance even though it is not an asynchronous method.",
-                typeof(TestController));
 
-            // Act & Assert
+            // Act
-            var ex = await Assert.ThrowsAsync<InvalidOperationException>(
+            var result = await (Task<int>)(await ExecuteAction(
-               () => ExecuteAction(
+                dynamicTaskMethod,
-                   dynamicTaskMethod,
+                _controller,
-                   _controller,
+                actionParameters));
-                   actionParameters));
+
-            Assert.Equal(expectedException, ex.Message);
+            // Assert
+            Assert.Equal(inputParam1, result);
         }
 
         [Fact]
@@ -462,12 +457,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             }
 
             /// <summary>
-            /// Returns a <see cref="Task{TResult}"/> instead of a <see cref="Task"/>. This should throw an
+            /// Returns a <see cref="Task{TResult}"/> instead of a <see cref="Task"/>.
-            /// <see cref="InvalidOperationException"/>.
             /// </summary>
             public Task UnwrappedTask(int i, string s)
             {
-                return Task.Factory.StartNew(async () => await Task.Delay(50));
+                return Task.Factory.StartNew(async () => await Task.Factory.StartNew(() => i));
             }
 
             public string Echo(string input)

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ObjectMethodExecutorTest.cs

@@ -2,13 +2,9 @@
 // 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.ComponentModel;
 using System.Globalization;
 using System.Reflection;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.Mvc.Internal;
-using Microsoft.AspNetCore.Testing;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Internal
@@ -72,6 +68,107 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                             new object[] { parameter }));
         }
 
+        [Fact]
+        public async void ExecuteValueMethodUsingAsyncMethod()
+        {
+            var executor = GetExecutorForMethod("ValueMethod");
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { 10, 20 });
+            Assert.Equal(30, (int)result);
+        }
+
+        [Fact]
+        public async void ExecuteVoidValueMethodUsingAsyncMethod()
+        {
+            var executor = GetExecutorForMethod("VoidValueMethod");
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { 10 });
+            Assert.Same(null, result);
+        }
+
+        [Fact]
+        public async void ExecuteValueMethodAsync()
+        {
+            var executor = GetExecutorForMethod("ValueMethodAsync");
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { 10, 20 });
+            Assert.Equal(30, (int)result);
+        }
+
+        [Fact]
+        public async void ExecuteVoidValueMethodAsync()
+        {
+            var executor = GetExecutorForMethod("VoidValueMethodAsync");
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { 10 });
+            Assert.Same(null, result);
+        }
+
+        [Fact]
+        public async void ExecuteValueMethodWithReturnTypeAsync()
+        {
+            var executor = GetExecutorForMethod("ValueMethodWithReturnTypeAsync");
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { 10 });
+            var resultObject = Assert.IsType<TestObject>(result);
+            Assert.Equal("Hello", resultObject.value);
+        }
+
+        [Fact]
+        public async void ExecuteValueMethodUpdateValueAsync()
+        {
+            var executor = GetExecutorForMethod("ValueMethodUpdateValueAsync");
+            var parameter = new TestObject();
+            var result = await executor.ExecuteAsync(
+                _targetObject,
+                new object[] { parameter });
+            var resultObject = Assert.IsType<TestObject>(result);
+            Assert.Equal("HelloWorld", resultObject.value);
+        }
+
+        [Fact]
+        public async void ExecuteValueMethodWithReturnTypeThrowsExceptionAsync()
+        {
+            var executor = GetExecutorForMethod("ValueMethodWithReturnTypeThrowsExceptionAsync");
+            var parameter = new TestObject();
+            await Assert.ThrowsAsync<NotImplementedException>(
+                    () => executor.ExecuteAsync(
+                            _targetObject,
+                            new object[] { parameter }));
+        }
+
+        [Fact]
+        public void ExecuteMethodOfTaskDerivedTypeReturnTypeThrowsException()
+        {
+            var expectedException = string.Format(
+                CultureInfo.CurrentCulture,
+                "The method 'TaskActionWithCustomTaskReturnType' on type '{0}' returned a Task instance even though it is not an asynchronous method.",
+                typeof(TestObject));
+
+            var ex = Assert.Throws<InvalidOperationException>(
+                    () => GetExecutorForMethod("TaskActionWithCustomTaskReturnType"));
+            Assert.Equal(expectedException, ex.Message);
+        }
+
+        [Fact]
+        public void ExecuteMethodOfTaskDerivedTypeOfTReturnTypeThrowsException()
+        {
+            var expectedException = string.Format(
+                CultureInfo.CurrentCulture,
+                "The method 'TaskActionWithCustomTaskOfTReturnType' on type '{0}' returned a Task instance even though it is not an asynchronous method.",
+                typeof(TestObject));
+
+            var ex = Assert.Throws<InvalidOperationException>(
+                    () => GetExecutorForMethod("TaskActionWithCustomTaskOfTReturnType"));
+
+            Assert.Equal(expectedException, ex.Message);
+        }
+
         private ObjectMethodExecutor GetExecutorForMethod(string methodName)
         {
             var method = typeof(TestObject).GetMethod(methodName);
@@ -106,6 +203,57 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                 parameter.value = "HelloWorld";
                 return parameter;
             }
+
+            public Task<int> ValueMethodAsync(int i, int j)
+            {
+                return Task.FromResult<int>(i + j);
+            }
+
+            public async Task VoidValueMethodAsync(int i)
+            {
+                await ValueMethodAsync(3, 4);
+            }
+            public Task<TestObject> ValueMethodWithReturnTypeAsync(int i)
+            {
+                return Task.FromResult<TestObject>(new TestObject() { value = "Hello" });
+            }
+
+            public Task<TestObject> ValueMethodWithReturnTypeThrowsExceptionAsync(TestObject i)
+            {
+                throw new NotImplementedException("Not Implemented Exception");
+            }
+
+            public Task<TestObject> ValueMethodUpdateValueAsync(TestObject parameter)
+            {
+                parameter.value = "HelloWorld";
+                return Task.FromResult<TestObject>(parameter);
+            }
+
+            public TaskDerivedType TaskActionWithCustomTaskReturnType(int i, string s)
+            {
+                return new TaskDerivedType();
+            }
+
+            public TaskOfTDerivedType<int> TaskActionWithCustomTaskOfTReturnType(int i, string s)
+            {
+                return new TaskOfTDerivedType<int>(1);
+            }
+
+            public class TaskDerivedType : Task
+            {
+                public TaskDerivedType()
+                    : base(() => Console.WriteLine("In The Constructor"))
+                {
+                }
+            }
+
+            public class TaskOfTDerivedType<T> : Task<T>
+            {
+                public TaskOfTDerivedType(T input)
+                    : base(() => input)
+                {
+                }
+            }
         }
     }
 }