Perf improvements in FunctionOverloadResolver #1602

KanishManuja-MS · 2019-12-01T10:05:00Z

Issues

This pull request fixes issue #1392

Description

This PR removes some of the expensive LINQ calls without breaking the public interface.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Results

HasAny previous for enumerables - 0.3372341
HasAny now for enumerables - 0.2167854

Additional work necessary

I would like to add tests for the changes once the initial work is reviewed. More performance improvement related data should be attached with this PR.

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

xuzhg · 2019-12-02T18:58:55Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+        /// <param name="parameters">The list of non-binding parameter names to match.</param>
+        /// <param name="caseInsensitive">Whether case insensitive.</param>
+        /// <returns>The best matching operations based on parameters.</returns>
+        internal static IList<IEdmOperation> FindBestOverloadBasedOnParameters(this IList<IEdmOperation> functions, IList<string> parameters, bool caseInsensitive = false)


Do you have the Perf telemetry about your change? #Pending

No, I will get that once the service adopt the new version. I can try creating new perf tests but that will not be accurate.

In reply to: 352770976 [](ancestors = 352770976)

xuzhg · 2019-12-02T19:01:36Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+            // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified.
+            for(int i=0; i < functions.Count; i++)
+            {
+                if (functions[i].Parameters.Count() == parameters.Count + (functions[i].IsBound ? 1 : 0))


How about to get rid of "functions[i].Parameters.Count()"

same "parameters.Count" into an int and use it here maybe better to call the property anytime. #Resolved

IEnumerable does not have a count property. A cast to List will also be expensive. Am I missing something here?

In reply to: 352772232 [](ancestors = 352772232)

Sorry. here i mean the perf about the "[i]" on the IList.

In reply to: 353939228 [](ancestors = 353939228,352772232)

It is random access. underlying structure for List is an array. Since this is internally used, we know that IList is implemented by an array based backing field.

In reply to: 363453414 [](ancestors = 363453414,353939228,352772232)

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

xuzhg · 2019-12-02T19:05:45Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+        /// <param name="parameters">The list of non-binding parameter names to match.</param>
+        /// <param name="caseInsensitive">Whether case insensitive.</param>
+        /// <returns>The best matching operations based on parameters.</returns>
+        internal static IList<IEdmOperation> FindBestOverloadBasedOnParameters(this IList<IEdmOperation> functions, IList<string> parameters, bool caseInsensitive = false)


where's usage of "caseInsensitive"? #Resolved

xuzhg · 2019-12-02T19:06:16Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+        /// <param name="parameters">The list of non-binding parameter names to match.</param>
+        /// <param name="caseInsensitive">Whether case insensitive.</param>
+        /// <returns>The best matching operations based on parameters.</returns>
+        internal static IList<IEdmOperation> FindBestOverloadBasedOnParameters(this IList<IEdmOperation> functions, IList<string> parameters, bool caseInsensitive = false)


if only compare the count of parameters, why don't you input the number of parameter? #Resolved

xuzhg · 2019-12-02T19:07:49Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+
+            foreach (var operation in operations)
+            {
+                bool suitableCandidate = true;


move this line after if(...) #Resolved

joaocpaiva · 2019-12-03T21:57:09Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

@@ -1829,20 +1926,32 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList<str
            List<IEdmOperationParameter> functionParameters = parametersToMatch.ToList();


List functionParameters = parametersToMatch.ToList(); [](start = 12, length = 77)

We can avoid this ToList allocation and use the enumerator in the below loop, since making the List copy is not giving us much in this method and the allocation is the most expensive here.

foreach (var functionParemeter in parametersToMatch)
{
...
} #Resolved

joaocpaiva · 2019-12-03T22:04:32Z

        .ToList();

Save this allocation, not needed. We can just call SingleOrDefault (if null throw ODataException). #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:129 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:04:50Z

            .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)).ToList();

Ditto, save this allocation. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:103 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:05:53Z

        var keyProperties = type.Key().ToList();

Save this allocation, use foreach loop below. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:317 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:13:38Z

        var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)).ToList();

I think we can save these allocations in these method. Something like:

var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal));
if (list.Any())
{
// if no case sensitive, try case-insensitive
list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase));
}

        var result = list.FirstOrDefault();

        if (result != null && result == list.Last())
        {
            throw new ODataException(Strings.UriParserMetadata_MultipleMatchingParametersFound(identifier));
        }

        return result; #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:412 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:14:27Z

            list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList();

Can't we always do IgnoreCase to avoid two O(n) searches? #Pending

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:416 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:18:35Z

                    var list = namedValues.Keys.Where(key => string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)).ToList();

We can also save this allocation. Calling Any(), SingleOrDefault, LastOrDefault will be better than copying an array.

#Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:373 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-03T22:21:00Z

        var keyProperties = type.Key().ToList();

If this can be large, it will still be better to save ToList() and instead resort to Count() to compare the length of the arrays. Or do the check as you go through the loop below. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:355 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

KanishManuja-MS · 2019-12-04T19:50:30Z

        var keyProperties = type.Key().ToList();

Saved this allocation but mind you, to avoid a breaking change, we will have to call Count() on the IEnumerable. However, since the these are keys, the length should be fairly small to iterate.
Expensive part here would be to get the enumerator twice.

In reply to: 561378338 [](ancestors = 561378338)

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:317 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

KanishManuja-MS · 2019-12-04T19:58:39Z

        var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)).ToList();

Slightly different implementation but I think it might be more performant.

In reply to: 561381264 [](ancestors = 561381264)

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:412 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

KanishManuja-MS · 2019-12-04T20:04:59Z

            list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList();

I suppose there is scope for improvement. However, the logic is that we first try and find the exact case matching parameter and then if nothing is found then find case insensitive parameter. If multiple are found in either of the two searching, we throw an exception. We would need to write additional logic to distinguish between multiple case sensitive and insensitive results; we can probably do that by allocating another list. Since the count of these enumerables will be small, the only cost is for getting the enumerator. The question comes down to 2 gets on enumerators vs 1 list allocation.

In reply to: 561381545 [](ancestors = 561381545)

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:416 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

joaocpaiva · 2019-12-05T03:04:53Z

        if (possibleFunctions.Count() > 1)

We seem to have multiple places where we check Count() > 1 which for collections that cannot cast to IList<> will result in O(n) algorithm.

It would be more effective to create an extension that leverages GetEnumerator to move 2 positions to find whether the array size is >1. It will also save the casts Count() does. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs:180 in 8b1a788. [](commit_id = 8b1a788, deletion_comment = False)

joaocpaiva · 2019-12-05T03:05:02Z

src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs

@@ -185,7 +185,7 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo
            // If more than one overload matches, try to select based on optional parameters
            if (possibleFunctions.Count() > 1 && parameterNames.Count > 0)


Count [](start = 34, length = 5)

Like this too. #Resolved

Reverted the CountGreaterThan change. Experimental data showed that .Count() outperforms CountGreaterThan for smaller values.

In reply to: 354091432 [](ancestors = 354091432)

joaocpaiva · 2019-12-05T03:07:43Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

+            // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL
+            bool actionExists = false;
+            bool functionExists = false;
+            for (int i = 0; i < candidatesMatchingOperations.Count; i++)


for [](start = 12, length = 3)

Should we break the loop as soon as functionExists and/or actionExists ? #Resolved

We can break when both exists.

In reply to: 354092013 [](ancestors = 354092013)

joaocpaiva · 2019-12-05T03:13:54Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

            // Throw an error if key size from url doesn't match that from model.
            // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver,
            // should override this ResolveKeys method.
-            if (keyProperties.Count != positionalValues.Count)
+            if (type.Key().Count() != positionalValues.Count)
            {
                throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName()));
            }



If we optimize for the success scenario I think we could avoid the Count().

first do the foreach loop while counting how many elements there are in Key().

after the foreach loop, check if the count of Key() != than positionalValues.Count. Throw exception if so.

Wouldn't this work? #Resolved

@joaocpaiva It might be counted as a breaking change. In that case, we will end up throwing the other exception before we could throw key count mismatch.
I know this sounds like a bad request scenario anyway and people shouldn't care about it but we have had customers complain because their tests started failing after an upgrade.

The other option is to hold off on throwing the second exception before we could loop through completely which might be an overkill and reducing the readability of the code. #Resolved

Yeah that makes sense. It is ok to leave as is then. Alternatively, we could also move the Syntax Error exception after the loop but it does not seem worth the trouble. #Resolved

joaocpaiva · 2019-12-05T03:14:43Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs


            // Throw an error if key size from url doesn't match that from model.
            // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver,
            // should override this ResolveKeys method.
-            if (keyProperties.Count != namedValues.Count)
+            if (type.Key().Count() != namedValues.Count)


Count() [](start = 27, length = 8)

Same comment applies here. #Resolved

joaocpaiva

It is looking really good! Just left a few more comments that looks like a few more small wins.

xuzhg · 2019-12-30T22:07:12Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

-                .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)).ToList();
+                .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase));
+
+            IEdmNavigationSource resolvedNavigationSource = default(IEdmNavigationSource);


default(IEdmNavigationSource); [](start = 60, length = 30)

I don't understand what's the performance advantage to use "default(interface)" and "null"? #Resolved

To keep the parity. Previously, it was SingleOrDefault, to avoid any breaking changes, I relied on Default here rather than null.

In reply to: 362107858 [](ancestors = 362107858)

xuzhg · 2019-12-30T22:09:31Z

        IEdmNavigationSource navSource = model.FindDeclaredNavigationSource(identifier);

Currently, the "resolve" process is two steps:

using case sensitive to resolve
not find at EdmError/EdmLocation does not provide file name #1, search using case insensitive.

I think consider to combine 2 steps into one can improve the performance a lot. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:90 in 8b1a788. [](commit_id = 8b1a788, deletion_comment = False)

xuzhg · 2019-12-30T22:13:28Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

            {
-                string valueText = positionalValues[i];
-                IEdmProperty keyProperty = keyProperties[i];
+                string valueText = positionalValues[i++];


[i++] [](start = 51, length = 5)

random acess using '[]' in the list maybe a performance problem #Resolved

Random access is allowed in lists. It does not iterate through the entire list. LinkedList would be the one where random access would be a problem.

In reply to: 362108940 [](ancestors = 362108940)

xuzhg · 2019-12-30T22:13:49Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

-            for (int i = 0; i < keyProperties.Count; i++)
+
+            int i = 0; 
+            foreach (IEdmProperty keyProperty in type.Key())


type.Key() [](start = 49, length = 10)

why call Key() twice in one method. It's also performance #Resolved

That's a good point @xuzhg - random access is preferable when IList objects boil down to List, since it implements random access as O(1) operation. However, when we are not absolutely sure what's the underlying implementation we should probably do foreach(...) instead of for(...) loop. Moreover, foreach(...) VS for(...) loop is a micro optimization that I don't feel too strong about it either.

To help with this, we should start using the underlying class, instead of the interface (unless when there is a need to send down the interface). #Resolved

xuzhg · 2019-12-30T22:15:23Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

            {
                // if no case sensitive, try case-insensitive
-                list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList();
+                list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase));
            }


can we combine them as one? #Resolved

As per previous reply, Ordinal string comparison will be cheaper than Ignore case.

In reply to: 362109307 [](ancestors = 362109307)

xuzhg · 2019-12-30T22:18:35Z

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

@@ -3182,19 +3182,12 @@ internal static bool TryGetStaticEntitySet(this IEdmPathExpression pathExpressio

        internal static bool HasAny<T>(this IEnumerable<T> enumerable) where T : class
        {
-            IList<T> list = enumerable as IList<T>;
-            if (list != null)
+            if (enumerable != null && enumerable.GetEnumerator().MoveNext())


&& enumerable.GetEnumerator().MoveNext()) [](start = 35, length = 41)

can you point me the materials to say to use this to test "HasAny" #Resolved

Refer to Line 804 in ExtensionMethodTests.

In reply to: 362109873 [](ancestors = 362109873)

xuzhg · 2020-01-06T19:56:34Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                bool matched = false;
+                for (int j = 0; j < parameterNameList.Count; j++)
+                {
+                    if (string.Equals(parameterNameList[j], functionParameter.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal))


parameterNameList[j] [](start = 38, length = 20)

same here for the [j] on IList, it's not random access. What's the benefit to access using "index"? Maybe I miss something. #Resolved

random access is faster. Lists do support random access.

In reply to: 363456913 [](ancestors = 363456913)

xuzhg

🕐

KanishManuja-MS · 2020-01-23T20:23:01Z

        IEdmNavigationSource navSource = model.FindDeclaredNavigationSource(identifier);

I think we had discussed this offline. Ignore case is twice as expensive check compared to case sensitive. Let's not degrade the performance when someone is specifying the correct case.

In reply to: 569810729 [](ancestors = 569810729)

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:90 in 8b1a788. [](commit_id = 8b1a788, deletion_comment = False)

KanishManuja-MS · 2020-01-24T22:48:47Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+
+        private static void EnsureOperationBoundWithBindingParameter(this IEdmOperation operation)
+        {
+            if (!operation.IsBound)


Make an assert instead. #Resolved

Converted to assert for functionoverload resolver but did not revert for select segment code path.

In reply to: 370876004 [](ancestors = 370876004)

KanishManuja-MS · 2020-01-24T22:58:54Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

                return true;
            }

-            if (foundActionsWhenLookingForFunctions.Count > 0)
+            if (foundActionsWhenLookingForFunctions != null && foundActionsWhenLookingForFunctions.Count > 0)


foundActionsWhenLookingForFunctions [](start = 16, length = 35)

Make this a boolean instead; verify. #Closed

KanishManuja-MS · 2020-01-24T23:05:20Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                        if (actionItems == null)
+                        {
+                            actionItems = new List<IEdmOperation>();
+                        }


Set a boolean here. #Closed

KanishManuja-MS · 2020-01-24T23:11:36Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                        {
+                            if (count > 0 && !(param is IEdmOptionalParameter))
+                            {
+                                suitableCandidate = false;


See if edmlib enforces optional parameters at the end then just look for the first optional parameter.
Set suitable to false if we see another required parameter. #Closed

EdmLib does not validate the ordering. Just checking for the first optional parameter fails some of our tests.

In reply to: 370881673 [](ancestors = 370881673)

KanishManuja-MS · 2020-01-24T23:20:08Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                    {
+                        suitableCandidate = false;
+                    }
+                }


Combine as per Mike's previous comment. #Closed

KanishManuja-MS · 2020-01-24T23:24:55Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                    if (string.Equals(parameterNameList[j], functionParameter.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal))
+                    {
+                        matched = true;
+                        parameterExists[j] = true;


break; #Resolved

It doesn't look like you added the break; is there a reason you have to continue looping through the remaining parameter names once you find one that matches?

In reply to: 370884636 [](ancestors = 370884636)

As discussed, we need to continue looping to mark other parameters as matched. The current logic mimics the old one, the existing bug is documented here - #1649

In reply to: 371602187 [](ancestors = 371602187,370884636)

More specifically; this matches the current (flawed) behavior of flagging all case-insensitive matches. Kanish to open a separate issue to track fixing this existing logic.

In reply to: 371616803 [](ancestors = 371616803,371602187,370884636)

#1649

In reply to: 371618735 [](ancestors = 371618735,371616803,371602187,370884636)

KanishManuja-MS · 2020-01-24T23:35:55Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

            {
-                return false;
+                if (!parameterExists[i])


parameterExists [](start = 21, length = 15)

Can i do it in O(1)? #Closed

Nope.

In reply to: 370886886 [](ancestors = 370886886)

KanishManuja-MS · 2020-01-24T23:39:01Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

            {
-                // Filter out functions with any parameters
-                candidateMatchingOperations = candidateMatchingOperations.Where(o => (o.IsFunction() && !o.Parameters.Any()) || o.IsAction());
+                candidatesMatchingOperations = candidatesMatchingOperations.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(bindingType) as IList<IEdmOperation>;
            }


Add null check if list casting fails. #Resolved

KanishManuja-MS · 2020-01-24T23:43:24Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

+                if (functionExists && actionExists)
+                {
+                    break;
+                }


split and move to 185; and 190? #Resolved

Can't move both the exceptions to above as we need to traverse the entire list before deciding if both action and function exist vs only actions exists.

In reply to: 370888279 [](ancestors = 370888279)

KanishManuja-MS · 2020-01-24T23:45:48Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

@@ -217,45 +215,58 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable<str
                    throw ExceptionUtil.CreateBadRequestError(ODataErrorStrings.RequestUriProcessor_SegmentDoesNotSupportKeyPredicates(identifier));
                }

-                matchingOperation = candidateMatchingOperations.Single();
+                matchingOperation = candidatesMatchingOperations.Count > 0 ? candidatesMatchingOperations[0] : null;
                return true;
            }


Move these up. #Resolved

Can't, refactored into a separate function now. Moving both of the exceptions does not improve the code as we need to iterate through all the operations anyway.

In reply to: 370888720 [](ancestors = 370888720)

KanishManuja-MS · 2020-01-24T23:46:23Z

This should be an assert. #Resolved

Refers to: src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs:214 in 1795eb9. [](commit_id = 1795eb9, deletion_comment = False)

KanishManuja-MS · 2020-01-24T23:49:42Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

                return true;
            }

-            if (foundActionsWhenLookingForFunctions.Count > 0)
+            if (foundActionsWhenLookingForFunctions != null && foundActionsWhenLookingForFunctions.Count > 0)
            {
                throw ExceptionUtil.CreateBadRequestError(ODataErrorStrings.RequestUriProcessor_SegmentDoesNotSupportKeyPredicates(identifier));
            }


Check the validity of this in terms of the position. #Resolved

Instead of being smart about not matching an action when request uri has parameters, this code throws even if there might be functions matching. Now, we only throw when there are no functions matched as well.

In reply to: 370889470 [](ancestors = 370889470)

KanishManuja-MS · 2020-01-25T00:10:13Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+            }
+
+            return exactMatches == null ? operations : exactMatches;
+        }


Should return a single item and throw an exception if multiple operations match? #Resolved

Instead of throwing, I decided to change the name of the function to indicate filtering rather than finding the best overload.

In reply to: 370893088 [](ancestors = 370893088)

KanishManuja-MS · 2020-01-25T00:16:43Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

@@ -151,12 +169,17 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName)

            IList<IEdmTerm> results = FindAcrossModels<IEdmTerm>(model, termName, /*caseInsensitive*/ true);

+            if (results == null)


) [](start = 31, length = 1)

Add check for count == 0 and make the return statement cleaner. #Closed

KanishManuja-MS · 2020-01-25T00:26:54Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

+
+                        string caseInsensitiveKey = string.Empty;
+                        int count = 0;
+                        foreach (var key in list)


make it a boolean. #Closed

KanishManuja-MS · 2020-01-25T00:30:18Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

        {
-            return model.SchemaElements.OfType<T>()
-            .Where(e => string.Equals(qualifiedName, e.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal));
+            foreach (var schema in model.SchemaElements)


var [](start = 21, length = 3)

IEDMSchemaElement, look for type in above changes as well. #Resolved

KanishManuja-MS · 2020-01-25T00:35:04Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

@@ -438,22 +480,36 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container)
            return container.GetRequiredService<ODataUriResolver>();
        }

-        private static List<T> FindAcrossModels<T>(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement
+        private static IList<T> FindAcrossModels<T>(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement


private [](start = 8, length = 7)

Is it okay to return null vs an empty list? #Resolved

KanishManuja-MS · 2020-01-25T00:36:50Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

        {
-            List<T> results = FindSchemaElements<T>(model, qualifiedName, caseInsensitive).ToList();
+            IList<T> results = null;


Remove empty list. #Resolved

KanishManuja-MS · 2020-01-25T00:39:07Z

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

@@ -3182,19 +3182,12 @@ internal static bool TryGetStaticEntitySet(this IEdmPathExpression pathExpressio

        internal static bool HasAny<T>(this IEnumerable<T> enumerable) where T : class
        {
-            IList<T> list = enumerable as IList<T>;
-            if (list != null)
+            if (enumerable != null && enumerable.GetEnumerator().MoveNext())


enumerable.GetEnumerator().MoveNext() [](start = 38, length = 37)

return the movenext. #Closed

KanishManuja-MS · 2020-01-25T00:42:59Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

+            return false;
+        }
+
+        internal static bool CountGreaterThan<T>(this IEnumerable<T> enumerable, int count) where T : class


move as private. #Closed

not marked as private.

In reply to: 370898092 [](ancestors = 370898092)

I changed CountGreater than to private but ended up removing that. This is a shared function that gets used in SelectPathSegmentTokenBinder, therefore keeping as internal.

In reply to: 371615196 [](ancestors = 371615196,370898092)

KanishManuja-MS · 2020-01-25T00:44:51Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

+        internal static bool CountGreaterThan<T>(this IEnumerable<T> enumerable, int count) where T : class
+        {
+            int currentCount = 0;
+            foreach (T element in enumerable)
            {


While move next to avoid the null check. #Closed

could also do if (enumerable != null) { return enumerable.GetEnumerator().MoveNext();}

In reply to: 370898326 [](ancestors = 370898326)

KanishManuja-MS · 2020-01-25T00:45:40Z

src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs

+                    continue;
+                }
+
+                currentCount++;


make it ++currentCount > count instead. #Closed

KanishManuja-MS · 2020-01-25T00:52:09Z

src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs

+                            results = new List<T>();
+                        }
+
+                        results.Add((T)schema);


(( [](start = 35, length = 2)

Try and get rid of this repeated cast. Removing var might help with that. #Resolved

Does not help. If it is a class, we can but not with an interface.

In reply to: 370899362 [](ancestors = 370899362)

mikepizzo · 2020-01-28T03:55:19Z

src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs

+                    {
+                        // Binding type requires atleast one parameter for a function. 
+                        bool parameterFound = bindingType != null ? false : true;
+                        foreach (var param in operation.Parameters)


var [](start = 33, length = 3)

try to avoid use of var #Closed

mikepizzo · 2020-01-28T05:02:01Z

            list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList();

Right. The logic for case insensitive is to first match case sensitive. If that fails, attempt to match case insensitive, in which case there can be only one case-insensitive match. This is by protocol (and best practice for handling case-insensitive comparison)

In reply to: 561816744 [](ancestors = 561816744,561381545)

Refers to: src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs:416 in 5adb8e8. [](commit_id = 5adb8e8, deletion_comment = False)

KanishManuja-MS · 2020-01-28T18:27:55Z

        if (possibleFunctions.Count() > 1)

Reverted the CountGreaterThan change. Experimental data showed that .Count() outperforms CountGreaterThan for smaller values.

In reply to: 561946259 [](ancestors = 561946259)

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs:180 in 8b1a788. [](commit_id = 8b1a788, deletion_comment = False)

mikepizzo

KanishManuja-MS requested review from xuzhg and mikepizzo December 1, 2019 10:05

xuzhg reviewed Dec 2, 2019

View reviewed changes

joaocpaiva reviewed Dec 3, 2019

View reviewed changes

joaocpaiva reviewed Dec 5, 2019

View reviewed changes

joaocpaiva suggested changes Dec 5, 2019

View reviewed changes

KanishManuja-MS requested a review from Sreejithpin December 30, 2019 18:31

xuzhg reviewed Dec 30, 2019

View reviewed changes

xuzhg reviewed Jan 6, 2020

View reviewed changes

xuzhg requested changes Jan 6, 2020

View reviewed changes

KanishManuja-MS commented Jan 24, 2020

View reviewed changes

KanishManuja-MS commented Jan 25, 2020

View reviewed changes

mikepizzo reviewed Jan 28, 2020

View reviewed changes

mikepizzo approved these changes Jan 29, 2020

View reviewed changes

Performance Improvements in function overload resolver

558d4b1

KanishManuja-MS force-pushed the PerfImprovements branch from 9ee197c to 558d4b1 Compare January 29, 2020 19:02

KanishManuja-MS merged commit c1c2344 into OData:master Jan 29, 2020

		@@ -1829,20 +1926,32 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList<str
		List<IEdmOperationParameter> functionParameters = parametersToMatch.ToList();

		@@ -185,7 +185,7 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo
		// If more than one overload matches, try to select based on optional parameters
		if (possibleFunctions.Count() > 1 && parameterNames.Count > 0)

		@@ -151,12 +169,17 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName)

		IList<IEdmTerm> results = FindAcrossModels<IEdmTerm>(model, termName, /caseInsensitive/ true);

		if (results == null)

Perf improvements in FunctionOverloadResolver #1602

Perf improvements in FunctionOverloadResolver #1602

Conversation

KanishManuja-MS commented Dec 1, 2019 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Results

Additional work necessary

xuzhg Dec 2, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Dec 2, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Dec 2, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

xuzhg Dec 2, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

xuzhg Dec 2, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

joaocpaiva Dec 3, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

joaocpaiva commented Dec 3, 2019 • edited by KanishManuja-MS Loading

KanishManuja-MS commented Dec 4, 2019

KanishManuja-MS commented Dec 4, 2019

KanishManuja-MS commented Dec 4, 2019

joaocpaiva commented Dec 5, 2019 • edited by KanishManuja-MS Loading

joaocpaiva Dec 5, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaocpaiva Dec 5, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaocpaiva Dec 5, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

KanishManuja-MS Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

joaocpaiva Dec 5, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

joaocpaiva Dec 5, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

joaocpaiva left a comment

Choose a reason for hiding this comment

xuzhg Dec 30, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg commented Dec 30, 2019 • edited by KanishManuja-MS Loading

xuzhg Dec 30, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Dec 30, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

joaocpaiva Jan 23, 2020 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

xuzhg Dec 30, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Dec 30, 2019 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Jan 6, 2020 • edited by KanishManuja-MS Loading

Choose a reason for hiding this comment

KanishManuja-MS Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

xuzhg left a comment

Choose a reason for hiding this comment

KanishManuja-MS commented Jan 23, 2020

KanishManuja-MS Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KanishManuja-MS Jan 24, 2020 • edited by mikepizzo Loading

KanishManuja-MS commented Dec 1, 2019 •

edited

Loading

xuzhg Dec 2, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 2, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 2, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 2, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 2, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 3, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva commented Dec 5, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Dec 5, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Dec 5, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Dec 5, 2019 •

edited by KanishManuja-MS

Loading

KanishManuja-MS Dec 5, 2019 •

edited

Loading

joaocpaiva Dec 5, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Dec 5, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 30, 2019 •

edited by KanishManuja-MS

Loading

xuzhg commented Dec 30, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 30, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 30, 2019 •

edited by KanishManuja-MS

Loading

joaocpaiva Jan 23, 2020 •

edited by KanishManuja-MS

Loading

xuzhg Dec 30, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Dec 30, 2019 •

edited by KanishManuja-MS

Loading

xuzhg Jan 6, 2020 •

edited by KanishManuja-MS

Loading

KanishManuja-MS Jan 23, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 24, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 24, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 24, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 28, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS commented Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 24, 2020 •

edited

Loading

KanishManuja-MS Jan 25, 2020 •

edited

Loading

KanishManuja-MS Jan 25, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 25, 2020 •

edited by mikepizzo

Loading

KanishManuja-MS Jan 25, 2020 •

edited

Loading

KanishManuja-MS Jan 25, 2020 •

edited

Loading

KanishManuja-MS Jan 25, 2020 •

edited

Loading