From ea1cc52fb1e35b1f46b7472cf2dc66bd74a51ffb Mon Sep 17 00:00:00 2001 From: Tim M <49349513+TimothyMakkison@users.noreply.github.com> Date: Sat, 29 Jun 2024 01:05:07 +0100 Subject: [PATCH] feat: optimize `RestMethodInfo`, reduce dictionary allocations and linq iterations (#1742) --- Refit.Tests/RequestBuilder.cs | 48 +++++----- Refit/EnumerableExtensions.cs | 7 ++ Refit/RequestBuilderImplementation.cs | 16 ++-- Refit/RestMethodInfo.cs | 130 ++++++++++++++------------ 4 files changed, 108 insertions(+), 93 deletions(-) diff --git a/Refit.Tests/RequestBuilder.cs b/Refit.Tests/RequestBuilder.cs index 656833acf..5b8f300da 100644 --- a/Refit.Tests/RequestBuilder.cs +++ b/Refit.Tests/RequestBuilder.cs @@ -1062,8 +1062,8 @@ public void DynamicHeaderCollectionShouldWork() Assert.Equal("1", fixture.Headers["Api-Version"]); Assert.Equal(4, fixture.Headers.Count); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); } [Theory] @@ -1085,8 +1085,8 @@ public void DynamicHeaderCollectionShouldWorkWithBody(string interfaceMethodName Assert.NotNull(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(2)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(2)); } [Theory] @@ -1110,8 +1110,8 @@ public void DynamicHeaderCollectionShouldWorkWithoutBody(string interfaceMethodN Assert.Null(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); } [Theory] @@ -1141,8 +1141,8 @@ public void DynamicHeaderCollectionShouldWorkWithInferredBody(string interfaceMe Assert.NotNull(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); Assert.Equal(2, fixture.BodyParameterInfo.Item3); } @@ -1168,8 +1168,8 @@ public void DynamicHeaderCollectionShouldWorkWithAuthorize(string interfaceMetho Assert.Null(fixture.BodyParameterInfo); Assert.NotNull(fixture.AuthorizeParameterInfo); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(2)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(2)); } [Theory] @@ -1195,8 +1195,8 @@ public void DynamicHeaderCollectionShouldWorkWithDynamicHeader(string interfaceM Assert.Single(fixture.HeaderParameterMap); Assert.Equal("Authorization", fixture.HeaderParameterMap[1]); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(2)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(2)); input = typeof(IRestMethodInfoTests); fixture = new RestMethodInfoInternal( @@ -1220,8 +1220,8 @@ public void DynamicHeaderCollectionShouldWorkWithDynamicHeader(string interfaceM Assert.Single(fixture.HeaderParameterMap); Assert.Equal("Authorization", fixture.HeaderParameterMap[2]); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); } [Theory] @@ -1253,8 +1253,8 @@ string interfaceMethodName Assert.Single(fixture.HeaderParameterMap); Assert.Equal("X-PathMember", fixture.HeaderParameterMap[0]); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); } [Theory] @@ -1274,8 +1274,8 @@ public void DynamicHeaderCollectionInMiddleOfParamsShouldWork(string interfaceMe Assert.Null(fixture.BodyParameterInfo); Assert.Equal("baz", fixture.QueryParameterMap[2]); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(1)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(1)); } [Theory] @@ -1321,8 +1321,8 @@ public void DynamicHeaderCollectionShouldWorkWithProperty(string interfaceMethod Assert.Single(fixture.PropertyParameterMap); - Assert.Equal(1, fixture.HeaderCollectionParameterMap.Count); - Assert.True(fixture.HeaderCollectionParameterMap.Contains(0)); + Assert.True(fixture.HasHeaderCollection); + Assert.True(fixture.HeaderCollectionAt(0)); } [Theory] @@ -1393,7 +1393,7 @@ public void DynamicRequestPropertyShouldWorkWithBody() Assert.Empty(fixture.HeaderParameterMap); Assert.NotNull(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Empty(fixture.HeaderCollectionParameterMap); + Assert.False(fixture.HasHeaderCollection); Assert.Equal("SomeProperty", fixture.PropertyParameterMap[2]); } @@ -1420,7 +1420,7 @@ public void DynamicRequestPropertiesShouldWorkWithBody() Assert.Empty(fixture.HeaderParameterMap); Assert.NotNull(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Empty(fixture.HeaderCollectionParameterMap); + Assert.False(fixture.HasHeaderCollection); Assert.Equal("SomeProperty", fixture.PropertyParameterMap[2]); Assert.Equal("SomeOtherProperty", fixture.PropertyParameterMap[3]); @@ -1449,7 +1449,7 @@ public void DynamicRequestPropertyShouldWorkWithoutBody(string interfaceMethodNa Assert.Empty(fixture.HeaderParameterMap); Assert.Null(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Empty(fixture.HeaderCollectionParameterMap); + Assert.False(fixture.HasHeaderCollection); Assert.Equal("SomeProperty", fixture.PropertyParameterMap[1]); } @@ -1477,7 +1477,7 @@ public void DynamicRequestPropertyShouldWorkWithInferredBody(string interfaceMet Assert.Empty(fixture.HeaderParameterMap); Assert.NotNull(fixture.BodyParameterInfo); Assert.Null(fixture.AuthorizeParameterInfo); - Assert.Empty(fixture.HeaderCollectionParameterMap); + Assert.False(fixture.HasHeaderCollection); Assert.Equal("SomeProperty", fixture.PropertyParameterMap[1]); Assert.Equal(2, fixture.BodyParameterInfo.Item3); diff --git a/Refit/EnumerableExtensions.cs b/Refit/EnumerableExtensions.cs index b4c131fb3..e612ca01c 100644 --- a/Refit/EnumerableExtensions.cs +++ b/Refit/EnumerableExtensions.cs @@ -19,6 +19,13 @@ internal static EnumerablePeek TryGetSingle(this IEnumerable enumerable, o } } +internal static class EmptyDictionary +{ + private static Dictionary Value = new Dictionary(); + + internal static Dictionary Get() => Value; +} + internal enum EnumerablePeek { Empty, diff --git a/Refit/RequestBuilderImplementation.cs b/Refit/RequestBuilderImplementation.cs index 53e67fbe9..1e47bbe3e 100644 --- a/Refit/RequestBuilderImplementation.cs +++ b/Refit/RequestBuilderImplementation.cs @@ -689,8 +689,8 @@ bool paramsContainsCancellationToken Uri.EscapeDataString( settings.UrlParameterFormatter.Format( s, - restMethod.ParameterInfoMap[i], - restMethod.ParameterInfoMap[i].ParameterType + restMethod.ParameterInfoArray[i], + restMethod.ParameterInfoArray[i].ParameterType ) ?? string.Empty ) ) @@ -702,8 +702,8 @@ bool paramsContainsCancellationToken replacement = Uri.EscapeDataString( settings.UrlParameterFormatter.Format( param, - restMethod.ParameterInfoMap[i], - restMethod.ParameterInfoMap[i].ParameterType + restMethod.ParameterInfoArray[i], + restMethod.ParameterInfoArray[i].ParameterType ) ?? string.Empty ); } @@ -738,7 +738,7 @@ bool paramsContainsCancellationToken } //if header collection, add to request headers - if (restMethod.HeaderCollectionParameterMap.Contains(i)) + if (restMethod.HeaderCollectionAt(i)) { if (param is IDictionary headerCollection) { @@ -776,7 +776,7 @@ bool paramsContainsCancellationToken // or if is an object bound to the path add any non-path bound properties to query string // or if it's an object with a query attribute var queryAttribute = restMethod - .ParameterInfoMap[i] + .ParameterInfoArray[i] .GetCustomAttribute(); if ( !restMethod.IsMultipart @@ -897,7 +897,7 @@ void AddQueryParameters(RestMethodInfoInternal restMethod, QueryAttribute? query queryParamsToAdd.AddRange( ParseQueryParameter( param, - restMethod.ParameterInfoMap[i], + restMethod.ParameterInfoArray[i], restMethod.QueryParameterMap[i], attr ) @@ -913,7 +913,7 @@ void AddQueryParameters(RestMethodInfoInternal restMethod, QueryAttribute? query queryParamsToAdd.AddRange( ParseQueryParameter( kvp.Value, - restMethod.ParameterInfoMap[i], + restMethod.ParameterInfoArray[i], path, attr ) diff --git a/Refit/RestMethodInfo.cs b/Refit/RestMethodInfo.cs index cb2ef6afa..a22dd5262 100644 --- a/Refit/RestMethodInfo.cs +++ b/Refit/RestMethodInfo.cs @@ -27,6 +27,7 @@ Type ReturnType [DebuggerDisplay("{MethodInfo}")] internal class RestMethodInfoInternal { + private int HeaderCollectionParameterIndex { get; set; } public string Name { get; set; } public Type Type { get; set; } public MethodInfo MethodInfo { get; set; } @@ -38,13 +39,12 @@ internal class RestMethodInfoInternal public UriFormat QueryUriFormat { get; set; } public Dictionary Headers { get; set; } public Dictionary HeaderParameterMap { get; set; } - public ISet HeaderCollectionParameterMap { get; set; } public Dictionary PropertyParameterMap { get; set; } public Tuple? BodyParameterInfo { get; set; } public Tuple? AuthorizeParameterInfo { get; set; } public Dictionary QueryParameterMap { get; set; } public Dictionary> AttachmentNameMap { get; set; } - public Dictionary ParameterInfoMap { get; set; } + public ParameterInfo[] ParameterInfoArray { get; set; } public Dictionary ParameterMap { get; set; } public Type ReturnType { get; set; } public Type ReturnResultType { get; set; } @@ -86,59 +86,59 @@ public RestMethodInfoInternal( DetermineIfResponseMustBeDisposed(); // Exclude cancellation token parameters from this list - var parameterArray = methodInfo + ParameterInfoArray = methodInfo .GetParameters() .Where(static p => p.ParameterType != typeof(CancellationToken)) .ToArray(); - ParameterInfoMap = parameterArray - .Select((parameter, index) => new { index, parameter }) - .ToDictionary(x => x.index, x => x.parameter); - ParameterMap = BuildParameterMap(RelativePath, parameterArray); - BodyParameterInfo = FindBodyParameter(parameterArray, IsMultipart, hma.Method); - AuthorizeParameterInfo = FindAuthorizationParameter(parameterArray); + ParameterMap = BuildParameterMap(RelativePath, ParameterInfoArray); + BodyParameterInfo = FindBodyParameter(ParameterInfoArray, IsMultipart, hma.Method); + AuthorizeParameterInfo = FindAuthorizationParameter(ParameterInfoArray); Headers = ParseHeaders(methodInfo); - HeaderParameterMap = BuildHeaderParameterMap(parameterArray); - HeaderCollectionParameterMap = RestMethodInfoInternal.BuildHeaderCollectionParameterMap( - parameterArray + HeaderParameterMap = BuildHeaderParameterMap(ParameterInfoArray); + HeaderCollectionParameterIndex = RestMethodInfoInternal.GetHeaderCollectionParameterIndex( + ParameterInfoArray ); - PropertyParameterMap = BuildRequestPropertyMap(parameterArray); + PropertyParameterMap = BuildRequestPropertyMap(ParameterInfoArray); // get names for multipart attachments - AttachmentNameMap = []; + Dictionary>? attachmentDict = null; if (IsMultipart) { - for (var i = 0; i < parameterArray.Length; i++) + for (var i = 0; i < ParameterInfoArray.Length; i++) { if ( ParameterMap.ContainsKey(i) || HeaderParameterMap.ContainsKey(i) || PropertyParameterMap.ContainsKey(i) - || HeaderCollectionParameterMap.Contains(i) + || HeaderCollectionAt(i) ) { continue; } - var attachmentName = GetAttachmentNameForParameter(parameterArray[i]); + var attachmentName = GetAttachmentNameForParameter(ParameterInfoArray[i]); if (attachmentName == null) continue; - AttachmentNameMap[i] = Tuple.Create( + attachmentDict ??= new Dictionary>(); + attachmentDict[i] = Tuple.Create( attachmentName, - GetUrlNameForParameter(parameterArray[i]) + GetUrlNameForParameter(ParameterInfoArray[i]) ); } } - QueryParameterMap = []; - for (var i = 0; i < parameterArray.Length; i++) + AttachmentNameMap = attachmentDict ?? EmptyDictionary>.Get(); + + Dictionary? queryDict = null; + for (var i = 0; i < ParameterInfoArray.Length; i++) { if ( ParameterMap.ContainsKey(i) || HeaderParameterMap.ContainsKey(i) || PropertyParameterMap.ContainsKey(i) - || HeaderCollectionParameterMap.Contains(i) + || HeaderCollectionAt(i) || (BodyParameterInfo != null && BodyParameterInfo.Item3 == i) || (AuthorizeParameterInfo != null && AuthorizeParameterInfo.Item2 == i) ) @@ -146,9 +146,12 @@ public RestMethodInfoInternal( continue; } - QueryParameterMap.Add(i, GetUrlNameForParameter(parameterArray[i])); + queryDict ??= new Dictionary(); + queryDict.Add(i, GetUrlNameForParameter(ParameterInfoArray[i])); } + QueryParameterMap = queryDict ?? EmptyDictionary.Get(); + var ctParamEnumerable = methodInfo .GetParameters() .Where(p => p.ParameterType == typeof(CancellationToken)) @@ -174,9 +177,13 @@ public RestMethodInfoInternal( || ReturnResultType == typeof(IApiResponse); } - static HashSet BuildHeaderCollectionParameterMap(ParameterInfo[] parameterArray) + public bool HasHeaderCollection => HeaderCollectionParameterIndex >= 0; + + public bool HeaderCollectionAt(int index) => HeaderCollectionParameterIndex >= 0 && HeaderCollectionParameterIndex == index; + + static int GetHeaderCollectionParameterIndex(ParameterInfo[] parameterArray) { - var headerCollectionMap = new HashSet(); + var headerIndex = -1; for (var i = 0; i < parameterArray.Length; i++) { @@ -191,7 +198,11 @@ static HashSet BuildHeaderCollectionParameterMap(ParameterInfo[] parameterA //opted for IDictionary semantics here as opposed to the looser IEnumerable> because IDictionary will enforce uniqueness of keys if (param.ParameterType.IsAssignableFrom(typeof(IDictionary))) { - headerCollectionMap.Add(i); + // throw if there is already a HeaderCollection parameter + if(headerIndex >= 0) + throw new ArgumentException("Only one parameter can be a HeaderCollection parameter"); + + headerIndex = i; } else { @@ -201,12 +212,7 @@ static HashSet BuildHeaderCollectionParameterMap(ParameterInfo[] parameterA } } - if (headerCollectionMap.Count > 1) - throw new ArgumentException( - "Only one parameter can be a HeaderCollection parameter" - ); - - return headerCollectionMap; + return headerIndex; } public RestMethodInfo ToRestMethodInfo() => @@ -214,7 +220,7 @@ public RestMethodInfo ToRestMethodInfo() => static Dictionary BuildRequestPropertyMap(ParameterInfo[] parameterArray) { - var propertyMap = new Dictionary(); + Dictionary? propertyMap = null; for (var i = 0; i < parameterArray.Length; i++) { @@ -229,11 +235,12 @@ static Dictionary BuildRequestPropertyMap(ParameterInfo[] parameter var propertyKey = !string.IsNullOrEmpty(propertyAttribute.Key) ? propertyAttribute.Key : param.Name!; + propertyMap ??= new Dictionary(); propertyMap[i] = propertyKey!; } } - return propertyMap; + return propertyMap ?? EmptyDictionary.Get(); } static IEnumerable GetParameterProperties(ParameterInfo parameter) @@ -413,7 +420,7 @@ static string GetAttachmentNameForParameter(ParameterInfo paramInfo) } Tuple? FindBodyParameter( - IList parameterList, + ParameterInfo[] parameterArray, bool isMultipart, HttpMethod method ) @@ -423,16 +430,15 @@ HttpMethod method // 2) POST/PUT/PATCH: Reference type other than string // 3) If there are two reference types other than string, without the body attribute, throw - var bodyParamEnumerable = parameterList + var bodyParamEnumerable = parameterArray .Select( x => - new - { - Parameter = x, - BodyAttribute = x.GetCustomAttributes(true) - .OfType() - .FirstOrDefault() - } + ( + Parameter: x, + BodyAttribute: x.GetCustomAttributes(true) + .OfType() + .FirstOrDefault() + ) ) .Where(x => x.BodyAttribute != null) .TryGetSingle(out var bodyParam); @@ -460,7 +466,7 @@ HttpMethod method return Tuple.Create( bodyParam!.BodyAttribute!.SerializationMethod, bodyParam.BodyAttribute.Buffered ?? RefitSettings.Buffered, - parameterList.IndexOf(bodyParam.Parameter) + Array.IndexOf(parameterArray, bodyParam.Parameter) ); } @@ -476,7 +482,7 @@ HttpMethod method // see if we're a post/put/patch // explicitly skip [Query], [HeaderCollection], and [Property]-denoted params - var refParamEnumerable = parameterList + var refParamEnumerable = parameterArray .Where( pi => !pi.ParameterType.GetTypeInfo().IsValueType @@ -500,25 +506,24 @@ HttpMethod method return Tuple.Create( BodySerializationMethod.Serialized, RefitSettings.Buffered, - parameterList.IndexOf(refParam!) + Array.IndexOf(parameterArray, refParam!) ); } return null; } - static Tuple? FindAuthorizationParameter(IList parameterList) + static Tuple? FindAuthorizationParameter(ParameterInfo[] parameterArray) { - var authorizeParamsEnumerable = parameterList + var authorizeParamsEnumerable = parameterArray .Select( x => - new - { - Parameter = x, - AuthorizeAttribute = x.GetCustomAttributes(true) - .OfType() - .FirstOrDefault() - } + ( + Parameter: x, + AuthorizeAttribute: x.GetCustomAttributes(true) + .OfType() + .FirstOrDefault() + ) ) .Where(x => x.AuthorizeAttribute != null) .TryGetSingle(out var authorizeParam); @@ -532,7 +537,7 @@ HttpMethod method { return Tuple.Create( authorizeParam!.AuthorizeAttribute!.Scheme, - parameterList.IndexOf(authorizeParam.Parameter) + Array.IndexOf(parameterArray, authorizeParam.Parameter) ); } @@ -541,8 +546,6 @@ HttpMethod method static Dictionary ParseHeaders(MethodInfo methodInfo) { - var ret = new Dictionary(); - var inheritedAttributes = methodInfo.DeclaringType != null ? methodInfo @@ -565,11 +568,15 @@ HttpMethod method .OfType() .SelectMany(ha => ha.Headers); + Dictionary? ret = null; + foreach (var header in headers) { if (string.IsNullOrWhiteSpace(header)) continue; + ret ??= new Dictionary(); + // NB: Silverlight doesn't have an overload for String.Split() // with a count parameter, but header values can contain // ':' so we have to re-join all but the first part to get the @@ -579,12 +586,12 @@ HttpMethod method parts.Length > 1 ? string.Join(":", parts.Skip(1)).Trim() : null; } - return ret; + return ret ?? EmptyDictionary.Get(); } static Dictionary BuildHeaderParameterMap(ParameterInfo[] parameterArray) { - var ret = new Dictionary(); + Dictionary? ret = null; for (var i = 0; i < parameterArray.Length; i++) { @@ -596,11 +603,12 @@ static Dictionary BuildHeaderParameterMap(ParameterInfo[] parameter if (!string.IsNullOrWhiteSpace(header)) { + ret ??= new Dictionary(); ret[i] = header.Trim(); } } - return ret; + return ret ?? EmptyDictionary.Get(); } void DetermineReturnTypeInfo(MethodInfo methodInfo)