diff --git a/src/Microsoft.Net.Http.Headers/CacheControlHeaderValue.cs b/src/Microsoft.Net.Http.Headers/CacheControlHeaderValue.cs index b8b6d2726c..94793efbea 100644 --- a/src/Microsoft.Net.Http.Headers/CacheControlHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/CacheControlHeaderValue.cs @@ -49,7 +49,7 @@ namespace Microsoft.Net.Http.Headers private ICollection _privateHeaders; private bool _mustRevalidate; private bool _proxyRevalidate; - private ICollection _extensions; + private IList _extensions; public CacheControlHeaderValue() { @@ -158,7 +158,7 @@ namespace Microsoft.Net.Http.Headers set { _proxyRevalidate = value; } } - public ICollection Extensions + public IList Extensions { get { diff --git a/src/Microsoft.Net.Http.Headers/ContentDispositionHeaderValue.cs b/src/Microsoft.Net.Http.Headers/ContentDispositionHeaderValue.cs index 25fe03cd9a..6a3cb271c9 100644 --- a/src/Microsoft.Net.Http.Headers/ContentDispositionHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/ContentDispositionHeaderValue.cs @@ -24,7 +24,7 @@ namespace Microsoft.Net.Http.Headers = new GenericHeaderParser(false, GetDispositionTypeLength); // Use list instead of dictionary since we may have multiple parameters with the same name. - private ICollection _parameters; + private ObjectCollection _parameters; private string _dispositionType; private ContentDispositionHeaderValue() @@ -48,7 +48,7 @@ namespace Microsoft.Net.Http.Headers } } - public ICollection Parameters + public IList Parameters { get { diff --git a/src/Microsoft.Net.Http.Headers/HeaderUtilities.cs b/src/Microsoft.Net.Http.Headers/HeaderUtilities.cs index 3885477aa1..eee2ea9c7d 100644 --- a/src/Microsoft.Net.Http.Headers/HeaderUtilities.cs +++ b/src/Microsoft.Net.Http.Headers/HeaderUtilities.cs @@ -13,7 +13,7 @@ namespace Microsoft.Net.Http.Headers private const string QualityName = "q"; internal const string BytesUnit = "bytes"; - internal static void SetQuality(ICollection parameters, double? value) + internal static void SetQuality(IList parameters, double? value) { Contract.Requires(parameters != null); @@ -50,7 +50,7 @@ namespace Microsoft.Net.Http.Headers } } - internal static double? GetQuality(ICollection parameters) + internal static double? GetQuality(IList parameters) { Contract.Requires(parameters != null); diff --git a/src/Microsoft.Net.Http.Headers/MediaTypeHeaderValue.cs b/src/Microsoft.Net.Http.Headers/MediaTypeHeaderValue.cs index 23ed8b0cec..b8b7c09e9e 100644 --- a/src/Microsoft.Net.Http.Headers/MediaTypeHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/MediaTypeHeaderValue.cs @@ -59,7 +59,7 @@ namespace Microsoft.Net.Http.Headers // Remove charset parameter if (charsetParameter != null) { - _parameters.Remove(charsetParameter); + Parameters.Remove(charsetParameter); } } else @@ -123,7 +123,7 @@ namespace Microsoft.Net.Http.Headers // Remove charset parameter if (boundaryParameter != null) { - _parameters.Remove(boundaryParameter); + Parameters.Remove(boundaryParameter); } } else @@ -140,7 +140,7 @@ namespace Microsoft.Net.Http.Headers } } - public ICollection Parameters + public IList Parameters { get { @@ -161,8 +161,12 @@ namespace Microsoft.Net.Http.Headers public double? Quality { - get { return HeaderUtilities.GetQuality(Parameters); } - set { HeaderUtilities.SetQuality(Parameters, value); } + get { return HeaderUtilities.GetQuality(_parameters); } + set + { + HeaderUtilities.ThrowIfReadOnly(IsReadOnly); + HeaderUtilities.SetQuality(Parameters, value); + } } public string MediaType @@ -210,7 +214,7 @@ namespace Microsoft.Net.Http.Headers { get { - return SubType.Equals("*", StringComparison.Ordinal); + return string.Compare(_mediaType, _mediaType.IndexOf('/') + 1, "*", 0, 1, StringComparison.Ordinal) == 0; } } @@ -241,15 +245,31 @@ namespace Microsoft.Net.Http.Headers return false; } + // PERF: Avoid doing anything here that allocates a substring, this is a very hot path + // for content-negotiation. + var indexOfSlash = _mediaType.IndexOf('/'); + // "text/plain" is a subset of "text/plain", "text/*" and "*/*". "*/*" is a subset only of "*/*". - if (!Type.Equals(otherMediaType.Type, StringComparison.OrdinalIgnoreCase)) + if (string.Compare( + strA: _mediaType, + indexA: 0, + strB: otherMediaType._mediaType, + indexB: 0, + length: indexOfSlash, + comparisonType: StringComparison.OrdinalIgnoreCase) != 0) { if (!otherMediaType.MatchesAllTypes) { return false; } } - else if (!SubType.Equals(otherMediaType.SubType, StringComparison.OrdinalIgnoreCase)) + else if (string.Compare( + strA: MediaType, + indexA: indexOfSlash + 1, + strB: otherMediaType._mediaType, + indexB: indexOfSlash + 1, // We know the Type is equal, so the index of '/' is the same in both strings. + length: _mediaType.Length - indexOfSlash, + comparisonType: StringComparison.OrdinalIgnoreCase) != 0) { if (!otherMediaType.MatchesAllSubTypes) { @@ -457,17 +477,17 @@ namespace Microsoft.Net.Http.Headers // If there is no whitespace between and in / get the media type using // one Substring call. Otherwise get substrings for and and combine them. - var mediatTypeLength = current + subtypeLength - startIndex; - if (typeLength + subtypeLength + 1 == mediatTypeLength) + var mediaTypeLength = current + subtypeLength - startIndex; + if (typeLength + subtypeLength + 1 == mediaTypeLength) { - mediaType = input.Substring(startIndex, mediatTypeLength); + mediaType = input.Substring(startIndex, mediaTypeLength); } else { mediaType = input.Substring(startIndex, typeLength) + "/" + input.Substring(current, subtypeLength); } - return mediatTypeLength; + return mediaTypeLength; } private static void CheckMediaTypeFormat(string mediaType, string parameterName) diff --git a/src/Microsoft.Net.Http.Headers/NameValueHeaderValue.cs b/src/Microsoft.Net.Http.Headers/NameValueHeaderValue.cs index 3031d877c0..fed6261a4d 100644 --- a/src/Microsoft.Net.Http.Headers/NameValueHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/NameValueHeaderValue.cs @@ -173,7 +173,7 @@ namespace Microsoft.Net.Http.Headers } internal static void ToString( - ICollection values, + IList values, char separator, bool leadingSeparator, StringBuilder destination) @@ -185,18 +185,18 @@ namespace Microsoft.Net.Http.Headers return; } - foreach (var value in values) + for (var i = 0; i < values.Count; i++) { if (leadingSeparator || (destination.Length > 0)) { destination.Append(separator); destination.Append(' '); } - destination.Append(value.ToString()); + destination.Append(values[i].ToString()); } } - internal static string ToString(ICollection values, char separator, bool leadingSeparator) + internal static string ToString(IList values, char separator, bool leadingSeparator) { if ((values == null) || (values.Count == 0)) { @@ -210,7 +210,7 @@ namespace Microsoft.Net.Http.Headers return sb.ToString(); } - internal static int GetHashCode(ICollection values) + internal static int GetHashCode(IList values) { if ((values == null) || (values.Count == 0)) { @@ -218,9 +218,9 @@ namespace Microsoft.Net.Http.Headers } var result = 0; - foreach (var value in values) + for (var i = 0; i < values.Count; i++) { - result = result ^ value.GetHashCode(); + result = result ^ values[i].GetHashCode(); } return result; } @@ -282,7 +282,7 @@ namespace Microsoft.Net.Http.Headers string input, int startIndex, char delimiter, - ICollection nameValueCollection) + IList nameValueCollection) { Contract.Requires(nameValueCollection != null); Contract.Requires(startIndex >= 0); @@ -320,7 +320,7 @@ namespace Microsoft.Net.Http.Headers } } - public static NameValueHeaderValue Find(ICollection values, string name) + public static NameValueHeaderValue Find(IList values, string name) { Contract.Requires((name != null) && (name.Length > 0)); @@ -329,8 +329,9 @@ namespace Microsoft.Net.Http.Headers return null; } - foreach (var value in values) + for (var i = 0; i < values.Count; i++) { + var value = values[i]; if (string.Compare(value.Name, name, StringComparison.OrdinalIgnoreCase) == 0) { return value; diff --git a/src/Microsoft.Net.Http.Headers/ObjectCollection.cs b/src/Microsoft.Net.Http.Headers/ObjectCollection.cs index 24237b6ff1..f6d91e1508 100644 --- a/src/Microsoft.Net.Http.Headers/ObjectCollection.cs +++ b/src/Microsoft.Net.Http.Headers/ObjectCollection.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; @@ -12,15 +11,28 @@ namespace Microsoft.Net.Http.Headers // type to throw if 'null' gets added. Collection internally uses List which comes at some cost. In addition // Collection.Add() calls List.InsertItem() which is an O(n) operation (compared to O(1) for List.Add()). // This type is only used for very small collections (1-2 items) to keep the impact of using Collection small. - internal class ObjectCollection : ICollection where T : class + internal class ObjectCollection : Collection where T : class { internal static readonly Action DefaultValidator = CheckNotNull; internal static readonly ObjectCollection EmptyReadOnlyCollection = new ObjectCollection(DefaultValidator, isReadOnly: true); - private readonly Collection _collection = new Collection(); private readonly Action _validator; - private readonly bool _isReadOnly; + + // We need to create a 'read-only' inner list for Collection to do the right + // thing. + private static IList CreateInnerList(bool isReadOnly, IEnumerable other = null) + { + var list = other == null ? new List() : new List(other); + if (isReadOnly) + { + return new ReadOnlyCollection(list); + } + else + { + return list; + } + } public ObjectCollection() : this(DefaultValidator) @@ -28,58 +40,45 @@ namespace Microsoft.Net.Http.Headers } public ObjectCollection(Action validator, bool isReadOnly = false) + : base(CreateInnerList(isReadOnly)) { _validator = validator; - _isReadOnly = isReadOnly; } public ObjectCollection(IEnumerable other, bool isReadOnly = false) + : base(CreateInnerList(isReadOnly, other)) { _validator = DefaultValidator; - foreach (T item in other) + foreach (T item in Items) { - Add(item); + _validator(item); } - _isReadOnly = isReadOnly; } - public int Count + public bool IsReadOnly => ((ICollection)this).IsReadOnly; + + protected override void ClearItems() { - get { return _collection.Count; } + base.ClearItems(); } - public bool IsReadOnly + protected override void InsertItem(int index, T item) { - get { return _isReadOnly; } - } - - public void Add(T item) - { - HeaderUtilities.ThrowIfReadOnly(IsReadOnly); _validator(item); - _collection.Add(item); + base.InsertItem(index, item); } - public bool Remove(T item) + protected override void RemoveItem(int index) { - HeaderUtilities.ThrowIfReadOnly(IsReadOnly); - return _collection.Remove(item); + base.RemoveItem(index); } - public void Clear() + protected override void SetItem(int index, T item) { - HeaderUtilities.ThrowIfReadOnly(IsReadOnly); - _collection.Clear(); + _validator(item); + base.SetItem(index, item); } - public bool Contains(T item) => _collection.Contains(item); - - public void CopyTo(T[] array, int arrayIndex) => _collection.CopyTo(array, arrayIndex); - - public IEnumerator GetEnumerator() => _collection.GetEnumerator(); - - IEnumerator IEnumerable.GetEnumerator() => _collection.GetEnumerator(); - private static void CheckNotNull(T item) { // null values cannot be added to the collection. diff --git a/test/Microsoft.Net.Http.Headers.Tests/MediaTypeHeaderValueTest.cs b/test/Microsoft.Net.Http.Headers.Tests/MediaTypeHeaderValueTest.cs index 48a4bfab12..ba3a31b7d2 100644 --- a/test/Microsoft.Net.Http.Headers.Tests/MediaTypeHeaderValueTest.cs +++ b/test/Microsoft.Net.Http.Headers.Tests/MediaTypeHeaderValueTest.cs @@ -122,9 +122,9 @@ namespace Microsoft.Net.Http.Headers Assert.False(mediaType0.Parameters.IsReadOnly); Assert.True(mediaType1.Parameters.IsReadOnly); Assert.Equal(mediaType0.Parameters.Count, mediaType1.Parameters.Count); - Assert.Throws(() => mediaType1.Parameters.Add(new NameValueHeaderValue("name"))); - Assert.Throws(() => mediaType1.Parameters.Remove(new NameValueHeaderValue("name"))); - Assert.Throws(() => mediaType1.Parameters.Clear()); + Assert.Throws(() => mediaType1.Parameters.Add(new NameValueHeaderValue("name"))); + Assert.Throws(() => mediaType1.Parameters.Remove(new NameValueHeaderValue("name"))); + Assert.Throws(() => mediaType1.Parameters.Clear()); var pair0 = mediaType0.Parameters.First(); var pair1 = mediaType1.Parameters.First();