From 1312ed8b7171a73a6c80b3e6a9bc91e8cf95942e Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 28 Oct 2015 18:12:24 -0700 Subject: [PATCH] Remove Linq and boxed Enumerator allocations from conneg --- .../Formatters/OutputFormatter.cs | 80 +++++++-- .../Infrastructure/ObjectResultExecutor.cs | 157 +++++++++++++----- .../ObjectResultExecutorTest.cs | 2 +- 3 files changed, 181 insertions(+), 58 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatter.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatter.cs index 967b524fcf..9a980455ad 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatter.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Http; @@ -251,21 +250,20 @@ namespace Microsoft.AspNet.Mvc.Formatters { if (acceptCharsetHeaders != null && acceptCharsetHeaders.Count > 0) { - var sortedAcceptCharsetHeaders = acceptCharsetHeaders - .Where(acceptCharset => acceptCharset.Quality != HeaderQuality.NoMatch) - .OrderByDescending(m => m, StringWithQualityHeaderValueComparer.QualityComparer); - - foreach (var acceptCharset in sortedAcceptCharsetHeaders) + var acceptValues = Sort(acceptCharsetHeaders); + for (var i = 0; i < acceptValues.Count; i++) { - var charset = acceptCharset.Value; + var charset = acceptValues[i].Value; if (!string.IsNullOrWhiteSpace(charset)) { - var encoding = SupportedEncodings.FirstOrDefault(supportedEncoding => - charset.Equals(supportedEncoding.WebName, StringComparison.OrdinalIgnoreCase) || - charset.Equals("*", StringComparison.Ordinal)); - if (encoding != null) + for (var j = 0; j < SupportedEncodings.Count; j++) { - return encoding; + var encoding = SupportedEncodings[j]; + if (charset.Equals(encoding.WebName, StringComparison.OrdinalIgnoreCase) || + charset.Equals("*", StringComparison.Ordinal)) + { + return encoding; + } } } } @@ -273,5 +271,63 @@ namespace Microsoft.AspNet.Mvc.Formatters return null; } + + // There's no allocation-free way to sort an IList and we may have to filter anyway, + // so we're going to have to live with the copy + insertion sort. + private IList Sort(IList values) + { + var sortNeeded = false; + var count = 0; + + for (var i = 0; i < values.Count; i++) + { + var value = values[i]; + if (value.Quality == HeaderQuality.NoMatch) + { + // Exclude this one + } + else if (value.Quality != null) + { + count++; + sortNeeded = true; + } + else + { + count++; + } + } + + if (!sortNeeded) + { + return values; + } + + var sorted = new List(); + for (var i = 0; i < values.Count; i++) + { + var value = values[i]; + if (value.Quality == HeaderQuality.NoMatch) + { + // Exclude this one + } + else + { + // Doing an insertion sort. + var position = sorted.BinarySearch(value, StringWithQualityHeaderValueComparer.QualityComparer); + if (position >= 0) + { + sorted.Insert(position + 1, value); + } + else + { + sorted.Insert(~position, value); + } + } + } + + // We want a descending sort, but BinarySearch does ascending + sorted.Reverse(); + return sorted; + } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/ObjectResultExecutor.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/ObjectResultExecutor.cs index 160a5a016c..6d3a627c09 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/ObjectResultExecutor.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/ObjectResultExecutor.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.IO; -using System.Linq; using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Http; @@ -163,7 +162,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure protected virtual IOutputFormatter SelectFormatter( OutputFormatterWriteContext formatterContext, IList contentTypes, - IEnumerable formatters) + IList formatters) { if (formatterContext == null) { @@ -191,14 +190,15 @@ namespace Microsoft.AspNet.Mvc.Infrastructure return SelectFormatterUsingAnyAcceptableContentType(formatterContext, formatters, contentTypes); } - var sortedAcceptHeaderMediaTypes = GetSortedAcceptHeaderMediaTypes(formatterContext); + var request = formatterContext.HttpContext.Request; + var acceptValues = PrepareAcceptValues(request.GetTypedHeaders().Accept); IOutputFormatter selectedFormatter = null; if (contentTypes == null || contentTypes.Count == 0) { // Check if we have enough information to do content-negotiation, otherwise get the first formatter // which can write the type. Let the formatter choose the Content-Type. - if (!sortedAcceptHeaderMediaTypes.Any()) + if (acceptValues == null || acceptValues.Count == 0) { Logger.LogVerbose("No information found on request to perform content negotiation."); @@ -213,7 +213,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure selectedFormatter = SelectFormatterUsingSortedAcceptHeaders( formatterContext, formatters, - sortedAcceptHeaderMediaTypes); + acceptValues); // 2. No formatter was found based on Accept header. Fallback to the first formatter which can write // the type. Let the formatter choose the Content-Type. @@ -230,18 +230,32 @@ namespace Microsoft.AspNet.Mvc.Infrastructure } else { - if (sortedAcceptHeaderMediaTypes.Any()) + if (acceptValues != null && acceptValues.Count > 0) { // Filter and remove accept headers which cannot support any of the user specified content types. // That is, confirm this result supports a more specific media type than requested e.g. OK if // "text/*" requested and result supports "text/plain". - var filteredAndSortedAcceptHeaders = sortedAcceptHeaderMediaTypes - .Where(acceptHeader => contentTypes.Any(contentType => contentType.IsSubsetOf(acceptHeader))); + for (var i = acceptValues.Count - 1; i >= 0; i--) + { + var isCompatible = false; + for (var j = 0; j < contentTypes.Count; j++) + { + if (contentTypes[j].IsSubsetOf(acceptValues[i])) + { + isCompatible = true; + } + } + + if (!isCompatible) + { + acceptValues.RemoveAt(i); + } + } selectedFormatter = SelectFormatterUsingSortedAcceptHeaders( formatterContext, formatters, - filteredAndSortedAcceptHeaders); + acceptValues); } if (selectedFormatter == null) @@ -275,7 +289,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure /// protected virtual IOutputFormatter SelectFormatterNotUsingAcceptHeaders( OutputFormatterWriteContext formatterContext, - IEnumerable formatters) + IList formatters) { if (formatterContext == null) { @@ -315,8 +329,8 @@ namespace Microsoft.AspNet.Mvc.Infrastructure /// protected virtual IOutputFormatter SelectFormatterUsingSortedAcceptHeaders( OutputFormatterWriteContext formatterContext, - IEnumerable formatters, - IEnumerable sortedAcceptHeaders) + IList formatters, + IList sortedAcceptHeaders) { if (formatterContext == null) { @@ -364,8 +378,8 @@ namespace Microsoft.AspNet.Mvc.Infrastructure /// protected virtual IOutputFormatter SelectFormatterUsingAnyAcceptableContentType( OutputFormatterWriteContext formatterContext, - IEnumerable formatters, - IEnumerable acceptableContentTypes) + IList formatters, + IList acceptableContentTypes) { if (formatterContext == null) { @@ -397,60 +411,113 @@ namespace Microsoft.AspNet.Mvc.Infrastructure return null; } - private IEnumerable GetSortedAcceptHeaderMediaTypes( - OutputFormatterWriteContext formatterContext) + // There's no allocation-free way to sort an IList so we're going to have to live with the + // copy + insertion sort. + private IList PrepareAcceptValues(IList values) { - var request = formatterContext.HttpContext.Request; - var incomingAcceptHeaderMediaTypes = request.GetTypedHeaders().Accept ?? new MediaTypeHeaderValue[] { }; + if (values == null || values.Count == 0) + { + return null; + } // By default we want to ignore considering accept headers for content negotiation when // they have a media type like */* in them. Browsers typically have these media types. // In these cases we would want the first formatter in the list of output formatters to // write the response. This default behavior can be changed through options, so checking here. - var respectAcceptHeader = true; - if (RespectBrowserAcceptHeader == false - && incomingAcceptHeaderMediaTypes.Any(mediaType => mediaType.MatchesAllTypes)) + if (!RespectBrowserAcceptHeader) { - respectAcceptHeader = false; + for (var i = 0; i < values.Count; i++) + { + if (values[i].MatchesAllTypes) + { + return null; + } + } } - var sortedAcceptHeaderMediaTypes = Enumerable.Empty(); - if (respectAcceptHeader) + // Degenerate case, we can avoid copying anything. + if (values.Count == 1) { - sortedAcceptHeaderMediaTypes = SortMediaTypeHeaderValues(incomingAcceptHeaderMediaTypes) - .Where(header => header.Quality != HeaderQuality.NoMatch); + return values; } - return sortedAcceptHeaderMediaTypes; + var sortNeeded = false; + var count = 0; + + for (var i = 0; i < values.Count; i++) + { + var value = values[i]; + if (value.Quality == HeaderQuality.NoMatch) + { + // Exclude this one + } + else if (value.Quality != null) + { + count++; + sortNeeded = true; + } + else + { + count++; + } + } + + if (!sortNeeded) + { + return values; + } + + var sorted = new List(count); + for (var i = 0; i < values.Count; i++) + { + var value = values[i]; + if (value.Quality == HeaderQuality.NoMatch) + { + // Exclude this one + } + else + { + var position = sorted.BinarySearch(value, MediaTypeHeaderValueComparer.QualityComparer); + if (position >= 0) + { + sorted.Insert(position + 1, value); + } + else + { + sorted.Insert(~position, value); + } + } + } + + // We want a descending sort, but BinarySearch does ascending + sorted.Reverse(); + return sorted; } private void ValidateContentTypes(IList contentTypes) { - var matchAllContentType = contentTypes?.FirstOrDefault( - contentType => contentType.MatchesAllSubTypes || contentType.MatchesAllTypes); - if (matchAllContentType != null) + if (contentTypes == null) { - throw new InvalidOperationException( - Resources.FormatObjectResult_MatchAllContentType( - matchAllContentType, - nameof(ObjectResult.ContentTypes))); + return; + } + + for (var i = 0; i < contentTypes.Count; i++) + { + var contentType = contentTypes[i]; + if (contentType.MatchesAllTypes || contentType.MatchesAllSubTypes) + { + var message = Resources.FormatObjectResult_MatchAllContentType( + contentType, + nameof(ObjectResult.ContentTypes)); + throw new InvalidOperationException(message); + } } } - // This can't be cached, because + // This can't be cached, because the BindingContext is per-request. private IList GetDefaultFormatters() { return BindingContext?.OutputFormatters ?? OptionsFormatters; } - - private static IEnumerable SortMediaTypeHeaderValues( - IEnumerable headerValues) - { - // Use OrderBy() instead of Array.Sort() as it performs fewer comparisons. In this case the comparisons - // are quite expensive so OrderBy() performs better. - return headerValues.OrderByDescending( - headerValue => headerValue, - MediaTypeHeaderValueComparer.QualityComparer); - } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/ObjectResultExecutorTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/ObjectResultExecutorTest.cs index de8b84005d..422164aa9e 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/ObjectResultExecutorTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/ObjectResultExecutorTest.cs @@ -526,7 +526,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure public new IOutputFormatter SelectFormatter( OutputFormatterWriteContext formatterContext, IList contentTypes, - IEnumerable formatters) + IList formatters) { return base.SelectFormatter(formatterContext, contentTypes, formatters); }