From 0dadf56fc80f8dec89897028eb9cdd583a968b4c Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 5 Oct 2015 21:28:01 -0700 Subject: [PATCH] Reducing allocations in value providers - Don't go async in formdata providers unless we need to - Remove unnecessary defensive copy in CompositeValueProvider --- .../Internal/TaskCacheOfT.cs | 18 +++++++++++++++++ .../ModelBinding/CompositeValueProvider.cs | 4 ++-- .../ModelBinding/FormValueProviderFactory.cs | 20 +++++++++---------- .../JQueryFormValueProviderFactory.cs | 19 +++++++++++------- .../CompositeValueProviderTest.cs | 8 ++++---- 5 files changed, 45 insertions(+), 24 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/Internal/TaskCacheOfT.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/Internal/TaskCacheOfT.cs b/src/Microsoft.AspNet.Mvc.Core/Internal/TaskCacheOfT.cs new file mode 100644 index 0000000000..6a55a1b668 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/Internal/TaskCacheOfT.cs @@ -0,0 +1,18 @@ +// 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.Threading.Tasks; + +namespace Microsoft.AspNet.Mvc.Internal +{ + public static class TaskCache + { + private static readonly Task _defaultCompletedTask = Task.FromResult(default(T)); + + /// + /// Gets a completed with the value of default(T). + /// + public static Task DefaultCompletedTask => _defaultCompletedTask; + } + +} diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeValueProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeValueProvider.cs index d005cd9549..7c392a094f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeValueProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeValueProvider.cs @@ -30,8 +30,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// The sequence of to add to this instance of /// . - public CompositeValueProvider(IEnumerable valueProviders) - : base(valueProviders.ToList()) + protected CompositeValueProvider(IList valueProviders) + : base(valueProviders) { } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormValueProviderFactory.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormValueProviderFactory.cs index ddd638857b..da6e675d50 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormValueProviderFactory.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormValueProviderFactory.cs @@ -5,12 +5,13 @@ using System; using System.Globalization; using System.Threading.Tasks; using Microsoft.AspNet.Http; +using Microsoft.AspNet.Mvc.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { public class FormValueProviderFactory : IValueProviderFactory { - public async Task GetValueProviderAsync(ValueProviderFactoryContext context) + public Task GetValueProviderAsync(ValueProviderFactoryContext context) { if (context == null) { @@ -18,23 +19,20 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } var request = context.HttpContext.Request; - if (request.HasFormContentType) { - var culture = GetCultureInfo(request); - - return new ReadableStringCollectionValueProvider( - BindingSource.Form, - await request.ReadFormAsync(), - culture); + return CreateValueProviderAsync(request); } - return null; + return TaskCache.DefaultCompletedTask; } - private static CultureInfo GetCultureInfo(HttpRequest request) + private static async Task CreateValueProviderAsync(HttpRequest request) { - return CultureInfo.CurrentCulture; + return new ReadableStringCollectionValueProvider( + BindingSource.Form, + await request.ReadFormAsync(), + CultureInfo.CurrentCulture); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/JQueryFormValueProviderFactory.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/JQueryFormValueProviderFactory.cs index 4d9809afec..49812f6cc5 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/JQueryFormValueProviderFactory.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/JQueryFormValueProviderFactory.cs @@ -8,30 +8,35 @@ using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Mvc.Core; +using Microsoft.AspNet.Mvc.Internal; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNet.Mvc.ModelBinding { public class JQueryFormValueProviderFactory : IValueProviderFactory { - public async Task GetValueProviderAsync(ValueProviderFactoryContext context) + public Task GetValueProviderAsync(ValueProviderFactoryContext context) { if (context == null) { throw new ArgumentNullException(nameof(context)); } - + var request = context.HttpContext.Request; - if (request.HasFormContentType) { - return new JQueryFormValueProvider( + return CreateValueProviderAsync(request); + } + + return TaskCache.DefaultCompletedTask; + } + + private static async Task CreateValueProviderAsync(HttpRequest request) + { + return new JQueryFormValueProvider( BindingSource.Form, await GetValueCollectionAsync(request), CultureInfo.CurrentCulture); - } - - return null; } private static async Task> GetValueCollectionAsync(HttpRequest request) diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs index d8cccd27aa..04baa0e398 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs @@ -26,7 +26,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding new JQueryFormValueProvider(bindingSource, new Dictionary(), culture); var valueProvider = new JQueryFormValueProvider(bindingSource, values, culture); - return new CompositeValueProvider(new[] { emptyValueProvider, valueProvider }); + return new CompositeValueProvider() { emptyValueProvider, valueProvider }; } #if DNX451 @@ -43,7 +43,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding provider2.Setup(p => p.GetKeysFromPrefix("prefix")) .Returns(dictionary) .Verifiable(); - var provider = new CompositeValueProvider(new[] { provider1, provider2.Object }); + var provider = new CompositeValueProvider() { provider1, provider2.Object }; // Act var values = provider.GetKeysFromPrefix("prefix"); @@ -61,7 +61,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Arrange var provider1 = Mock.Of(); var provider2 = Mock.Of(); - var provider = new CompositeValueProvider(new[] { provider1, provider2 }); + var provider = new CompositeValueProvider() { provider1, provider2 }; // Act var values = provider.GetKeysFromPrefix("prefix"); @@ -89,7 +89,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var valueProvider1 = GetMockValueProvider("Test"); var valueProvider2 = GetMockValueProvider("Unrelated"); - var provider = new CompositeValueProvider(new List() { valueProvider1.Object, valueProvider2.Object }); + var provider = new CompositeValueProvider() { valueProvider1.Object, valueProvider2.Object }; // Act var result = provider.Filter(metadata.BindingSource);