From d37f5aeb31553d4f1519009a0ef55e9185515b10 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sun, 27 Sep 2015 22:53:59 -0700 Subject: [PATCH] Fix #3217 - Optimize IList.GetEnumerator allocations This change fixes call sites on the main request path for a simple site (model binding, validation, views) that allocate boxed list enumerators. Some cases aren't addressed by this change because the fix is too invasive or requires changing an important contract to take IList instead of IEnumerable. Will follow up on those case by case in order of importance. --- .../DefaultControllerActionArgumentBinder.cs | 6 ++-- .../Filters/DefaultFilterProvider.cs | 5 ++-- .../Infrastructure/DefaultActionSelector.cs | 24 ++++++++++++---- .../ModelBinding/CompositeModelBinder.cs | 4 ++- .../CompositeClientModelValidatorProvider.cs | 5 ++-- .../CompositeModelValidatorProvider.cs | 6 ++-- .../DefaultModelValidatorProvider.cs | 5 ++-- .../Routing/ActionSelectionDecisionTree.cs | 8 ++++-- .../DefaultClientModelValidatorProvider.cs | 5 ++-- .../RazorViewEngine.cs | 28 ++++++++++++------- .../ViewFeatures/DefaultHtmlGenerator.cs | 4 ++- 11 files changed, 68 insertions(+), 32 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs b/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs index 25ea0482f0..40c2e1091b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs @@ -162,10 +162,12 @@ namespace Microsoft.AspNet.Mvc.Controllers OperationBindingContext operationContext, ModelStateDictionary modelState, IDictionary arguments, - IEnumerable parameterMetadata) + IList parameterMetadata) { - foreach (var parameter in parameterMetadata) + // Perf: Avoid allocations + for (var i = 0; i < parameterMetadata.Count; i++) { + var parameter = parameterMetadata[i]; var modelBindingResult = await BindModelAsync(parameter, modelState, operationContext); if (modelBindingResult.IsModelSet) { diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs index bcb509675e..0100367a14 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs @@ -20,9 +20,10 @@ namespace Microsoft.AspNet.Mvc.Filters { if (context.ActionContext.ActionDescriptor.FilterDescriptors != null) { - foreach (var item in context.Results) + // Perf: Avoid allocations + for (var i = 0; i < context.Results.Count; i++) { - ProvideFilter(context, item); + ProvideFilter(context, context.Results[i]); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs index 61dbf6cac4..7b5560aeb9 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs @@ -41,8 +41,11 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var matchingRouteConstraints = tree.Select(context.RouteData.Values); var candidates = new List(); - foreach (var action in matchingRouteConstraints) + + // Perf: Avoid allocations + for (var i = 0; i < matchingRouteConstraints.Count; i++) { + var action = matchingRouteConstraints[i]; var constraints = GetConstraints(context.HttpContext, action); candidates.Add(new ActionSelectorCandidate(action, constraints)); } @@ -54,8 +57,10 @@ namespace Microsoft.AspNet.Mvc.Infrastructure if (matchingActionConstraints != null) { matchingActions = new List(matchingActionConstraints.Count); - foreach (var candidate in matchingActionConstraints) + // Perf: Avoid allocations + for (var i = 0; i < matchingActionConstraints.Count; i++) { + var candidate = matchingActionConstraints[i]; matchingActions.Add(candidate.Action); } } @@ -107,12 +112,16 @@ namespace Microsoft.AspNet.Mvc.Infrastructure // Find the next group of constraints to process. This will be the lowest value of // order that is higher than startingOrder. int? order = null; - foreach (var candidate in candidates) + + // Perf: Avoid allocations + for (var i = 0; i < candidates.Count; i++) { + var candidate = candidates[i]; if (candidate.Constraints != null) { - foreach (var constraint in candidate.Constraints) + for (var j = 0; j < candidate.Constraints.Count; j++) { + var constraint = candidate.Constraints[j]; if ((startingOrder == null || constraint.Order > startingOrder) && (order == null || constraint.Order < order)) { @@ -137,16 +146,19 @@ namespace Microsoft.AspNet.Mvc.Infrastructure constraintContext.Candidates = candidates; constraintContext.RouteContext = context; - foreach (var candidate in candidates) + // Perf: Avoid allocations + for (var i = 0; i < candidates.Count; i++) { + var candidate = candidates[i]; var isMatch = true; var foundMatchingConstraint = false; if (candidate.Constraints != null) { constraintContext.CurrentCandidate = candidate; - foreach (var constraint in candidate.Constraints) + for (var j = 0; j < candidate.Constraints.Count; j++) { + var constraint = candidate.Constraints[j]; if (constraint.Order == order) { foundMatchingConstraint = true; diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs index 3bd49f42b9..cfa6d338e0 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs @@ -49,8 +49,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { RuntimeHelpers.EnsureSufficientExecutionStack(); - foreach (var binder in ModelBinders) + // Perf: Avoid allocations + for (var i = 0; i < ModelBinders.Count; i++) { + var binder = ModelBinders[i]; var result = await binder.BindModelAsync(bindingContext); if (result != ModelBindingResult.NoResult) { diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeClientModelValidatorProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeClientModelValidatorProvider.cs index b9ec81fd03..fa1b7effd8 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeClientModelValidatorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeClientModelValidatorProvider.cs @@ -30,9 +30,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation /// public void GetValidators(ClientValidatorProviderContext context) { - foreach (var validatorProvider in ValidatorProviders) + // Perf: Avoid allocations + for (var i = 0; i < ValidatorProviders.Count; i++) { - validatorProvider.GetValidators(context); + ValidatorProviders[i].GetValidators(context); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeModelValidatorProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeModelValidatorProvider.cs index 10ca4fc4fd..ab4661b452 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeModelValidatorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/CompositeModelValidatorProvider.cs @@ -27,11 +27,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation /// public IReadOnlyList ValidatorProviders { get; } + /// public void GetValidators(ModelValidatorProviderContext context) { - foreach (var validatorProvider in ValidatorProviders) + // Perf: Avoid allocations + for (var i = 0; i < ValidatorProviders.Count; i++) { - validatorProvider.GetValidators(context); + ValidatorProviders[i].GetValidators(context); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultModelValidatorProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultModelValidatorProvider.cs index b2b100a20a..939851a961 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultModelValidatorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultModelValidatorProvider.cs @@ -15,9 +15,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation /// public void GetValidators(ModelValidatorProviderContext context) { - foreach (var metadata in context.ValidatorMetadata) + //Perf: Avoid allocations here + for (var i = 0; i < context.ValidatorMetadata.Count; i++) { - var validator = metadata as IModelValidator; + var validator = context.ValidatorMetadata[i] as IModelValidator; if (validator != null) { context.Validators.Add(validator); diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectionDecisionTree.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectionDecisionTree.cs index ce9272da30..fcc923d2dd 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectionDecisionTree.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectionDecisionTree.cs @@ -47,13 +47,17 @@ namespace Microsoft.AspNet.Mvc.Routing // The common case for MVC has no catch-alls, so avoid allocating. List filtered = null; - foreach (var action in results) + // Perf: Avoid allocations + for (var i = 0; i < results.Count; i++) { + var action = results[i]; + var actionHasCatchAll = false; if (action.RouteConstraints != null) { - foreach (var constraint in action.RouteConstraints) + for (var j = 0; j < action.RouteConstraints.Count; j++) { + var constraint = action.RouteConstraints[j]; if (constraint.KeyHandling == RouteKeyHandling.CatchAll) { actionHasCatchAll = true; diff --git a/src/Microsoft.AspNet.Mvc.DataAnnotations/DefaultClientModelValidatorProvider.cs b/src/Microsoft.AspNet.Mvc.DataAnnotations/DefaultClientModelValidatorProvider.cs index 8cb1c3f87f..d79c870a9b 100644 --- a/src/Microsoft.AspNet.Mvc.DataAnnotations/DefaultClientModelValidatorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.DataAnnotations/DefaultClientModelValidatorProvider.cs @@ -22,9 +22,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation throw new ArgumentNullException(nameof(context)); } - foreach (var metadata in context.ValidatorMetadata) + // Perf: Avoid allocations + for (var i = 0; i < context.ValidatorMetadata.Count; i++) { - var validator = metadata as IClientModelValidator; + var validator = context.ValidatorMetadata[i] as IClientModelValidator; if (validator != null) { context.Validators.Add(validator); diff --git a/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs b/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs index 3eab873e1b..874aa14aa0 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs @@ -198,17 +198,24 @@ namespace Microsoft.AspNet.Mvc.Razor } else { - // For traditional routes, lookup the key in RouteConstraints if the key is RequireKey. - var match = actionDescriptor.RouteConstraints.FirstOrDefault( - constraint => string.Equals(constraint.RouteKey, key, StringComparison.OrdinalIgnoreCase)); - if (match != null && match.KeyHandling != RouteKeyHandling.CatchAll) + // Perf: Avoid allocations + for (var i = 0; i < actionDescriptor.RouteConstraints.Count; i++) { - if (match.KeyHandling == RouteKeyHandling.DenyKey) + var constraint = actionDescriptor.RouteConstraints[i]; + if (string.Equals(constraint.RouteKey, key, StringComparison.Ordinal)) { - return null; - } + if (constraint.KeyHandling == RouteKeyHandling.DenyKey) + { + return null; + } + else if (constraint.KeyHandling == RouteKeyHandling.RequireKey) + { + normalizedValue = constraint.RouteValue; + } - normalizedValue = match.RouteValue; + // Duplicate keys in RouteConstraints are not allowed. + break; + } } } @@ -266,9 +273,10 @@ namespace Microsoft.AspNet.Mvc.Razor expanderContext.Values = new Dictionary(StringComparer.Ordinal); // 1. Populate values from viewLocationExpanders. - foreach (var expander in _viewLocationExpanders) + // Perf: Avoid allocations + for( var i = 0; i < _viewLocationExpanders.Count; i++) { - expander.PopulateValues(expanderContext); + _viewLocationExpanders[i].PopulateValues(expanderContext); } } diff --git a/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs b/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs index bdd6e8be7d..4bc6b6d5f8 100644 --- a/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs +++ b/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs @@ -800,8 +800,10 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures var htmlSummary = new TagBuilder("ul"); foreach (var modelState in modelStates) { - foreach (var modelError in modelState.Errors) + // Perf: Avoid allocations + for (var i = 0; i < modelState.Errors.Count; i++) { + var modelError = modelState.Errors[i]; var errorText = ValidationHelpers.GetUserErrorMessageOrDefault(modelError, modelState: null); if (!string.IsNullOrEmpty(errorText))