Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove and change ToList<T>, Any<T>, Where<T> in Uri parser #1473

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@xuzhg
Copy link
Member

commented May 31, 2019

Issues

This pull request fixes issue #xxx.

Description

  • Remove the ToList < T > Linq call in FunctionOverloadResolver.cs
  • Change Any < T > call
  • Change Where < T > call

image

Checklist (Uncheck if it is not completed)

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

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@xuzhg xuzhg requested review from mikepizzo and KanishManuja-MS May 31, 2019

@xuzhg xuzhg force-pushed the xuzhg:LinqImprovement2 branch from cebaa20 to 2d0c8c7 Jun 3, 2019

@@ -448,11 +445,11 @@ internal static string FullNameWithParameters(this IEdmOperationImport operation
/// <param name="source">The source.</param>
/// <param name="actionItems">The action items.</param>
/// <returns>Only the functions from the operation sequence.</returns>
internal static IEnumerable<IEdmFunction> RemoveActions(this IEnumerable<IEdmOperation> source, out IList<IEdmAction> actionItems)
internal static IEnumerable<IEdmOperation> RemoveActions(this IEnumerable<IEdmOperation> source, out IList<IEdmOperation> actionItems)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

IEnumerable [](start = 24, length = 26)

Why is the return type changed to IEdmOperation? the method still only returns the functions in this list, and actions as an output param. #WontFix

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

Because the return will be used to call "FilterOperationsByParameterNames", which accepts the "IEdmOperation". We don't want to cast the operation to function, then cast the function to operation again and again. That's useless and waste of time.

Check the line 179 in FunctionOverloadResolver.cs.


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

@@ -474,9 +471,9 @@ internal static IEnumerable<IEdmFunction> RemoveActions(this IEnumerable<IEdmOpe
/// <param name="source">The source.</param>
/// <param name="actionImportItems">The action import items.</param>
/// <returns>Only the function imports from the operation Import sequence.</returns>
internal static IEnumerable<IEdmFunctionImport> RemoveActionImports(this IEnumerable<IEdmOperationImport> source, out IList<IEdmActionImport> actionImportItems)
internal static IEnumerable<IEdmOperationImport> RemoveActionImports(this IEnumerable<IEdmOperationImport> source, out IList<IEdmActionImport> actionImportItems)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

IEdmOperationImport [](start = 36, length = 19)

Why make the return type IEdmOperationImport, rather than keep it as IEdmFunctionImport, since the returned values are always function imports? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

Because the return will be used to call "FilterOperationsByParameterNames", which accepts the "IEdmOperation". We don't want to cast the operation to function, then cast the function to operation again and again. That's useless and waste of time.

Check the line 45 in FunctionOverloadResolver.cs.


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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

We don't have to cast because we are passing a more derived type. Passing a derived type to a method that is typed to receive the base type should be free, as the type checking is done at compile time, not at execution.


In reply to: 290003738 [](ancestors = 290003738,289996208)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 4, 2019

Member

As we discussed, since:

  1. .NET 3.5 requires an explicit cast to cast from IEnumerable to IEnumerable
  2. The methods that currently take the returned enumerables treat them as operations, and never have to cast to function/action
  3. The current code in EdmLibraryExtensions does cast them to the correct derived type (even though downstream we only care about the base type), and
  4. These methods are all internal, so we could change them later to return the derived type if we had a need to treat them as the derived type
    Then I'm okay, for now, changing all of the signatures to deal with operations, rather than the appropriate derived type.

In reply to: 290046571 [](ancestors = 290046571,290003738,289996208)

@@ -172,8 +172,10 @@ internal static IEdmOperation[] CalculateBindableOperationsForType(IEdmType bind
throw new ODataException(Strings.MetadataUtils_CalculateBindableOperationsForType(bindingType.FullTypeName()), exc);
}

operations.EnsureOperationsBoundWithBindingParameter();

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

Rather than looping through the operations here, and again in line 178, what if we changed EnsureOperationBoundWithBindingParameter() to take a single operation, and called it within the loop on line 178? #Resolved

@@ -177,7 +177,7 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo
}
}

possibleFunctions = possibleFunctions.EnsureOperationsBoundWithBindingParameter().ToList();
possibleFunctions.EnsureOperationsBoundWithBindingParameter();

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

Do we really need to validate all of the possibleFunctions here, or should we just wait and validate at line 199 after we have reduced the number of possible functions based on overloads. #Resolved

@mikepizzo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

        return operationsFound.ToArray();

Why do we return as an array, rather than an enumerable? #Resolved


Refers to: src/Microsoft.OData.Core/Metadata/MetadataUtils.cs:188 in 2d0c8c7. [](commit_id = 2d0c8c7, deletion_comment = False)

}

if (candidateMatchingOperationImports.Count == 0)
if (candidateMatchingOperationImports.Count() == 0)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

.Count() [](start = 49, length = 8)

is it cheaper to use FirstOrDefault() == null? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

From the Count() function:

public static int Count<TSource>(this IEnumerable<TSource> source) {
            if (source == null) throw Error.ArgumentNull("source");
            ICollection<TSource> collectionoft = source as ICollection<TSource>;
            if (collectionoft != null) return collectionoft.Count;
            ICollection collection = source as ICollection;
            if (collection != null) return collection.Count;
            int count = 0;
            using (IEnumerator<TSource> e = source.GetEnumerator()) {
                checked {
                    while (e.MoveNext()) count++;
                }
            }
            return count;
        }

So, if the source is ICollection< T >, it's cheaper than "FirstOrDefault< T >". In our codes, internally the IEnumerable< T > is the IList< T >, which implements the ICollection< T >, So, here the Count() is cheaper than "FirstOrDefault< T >".


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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

Why not use your new HasAny() extension method?


In reply to: 290006737 [](ancestors = 290006737,290001862)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

That's internal extension method defined in OData.Edm library.


In reply to: 290047580 [](ancestors = 290047580,290006737,290001862)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

Should we have a similar method here? For the case where the enumerator is not a collection, this method will actually count all of the members when all we really need to know is if there are any members. I realize that our implementation uses collection, but your code would be more efficient for other enumerators.


In reply to: 290048582 [](ancestors = 290048582,290047580,290006737,290001862)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

Got it.


In reply to: 290049890 [](ancestors = 290049890,290048582,290047580,290006737,290001862)

@mikepizzo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

            if (parameterNames.Count() != 0)

use parameterNamesCount (or, if you change parameterNamesCount to hasParameters, use that here.) #Resolved


Refers to: src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs:215 in 2d0c8c7. [](commit_id = 2d0c8c7, deletion_comment = False)

@@ -163,42 +164,43 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable<str
throw;
}

IList<IEdmAction> foundActionsWhenLookingForFunctions = new List<IEdmAction>();
IList<IEdmOperation> foundActionsWhenLookingForFunctions = new List<IEdmOperation>();
int parameterNamesCount = parameterNames.Count();

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

int parameterNamesCount = parameterNames.Count() [](start = 12, length = 48)

would it be better to make this a bool hasParameters = (parameterNames.FirstOrDefault == null) ? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

parameterNames is IList, so, Count< T > is cheaper than FirstOrDefault == null ?


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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

still probably better to make it bool hasParameters = (parameterNames.Count() > 0), since the only thing we care about is whether there are any parameters (we don't care how many), so that saves a comparison later.


In reply to: 290007315 [](ancestors = 290007315,290003826)

{
return false;
}
Debug.Assert(parameter != null, "Should never be null");

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

not sure this assert is necessary, since previous line would have filtered out a null parameter value. #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

I think so. Removed.


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

return array.Count() > 0;
}

return enumerable.Any();

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

.Any() [](start = 29, length = 6)

is FirstOrDefault == null better than Any()? #ByDesign

This comment has been minimized.

Copy link
@xuzhg

xuzhg Jun 3, 2019

Author Member

Here we can't say the T is reference type, or it maybe Value type. So, for some scenarios (not a list, not an array), i think it's better to delegate to use "Any()". What do you think?


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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

I'm okay using Any() here.


In reply to: 290038532 [](ancestors = 290038532,290009255)

@@ -173,7 +173,25 @@ public virtual IEnumerable<IEdmOperation> FindDeclaredBoundOperations(IEdmType b
/// </returns>
public virtual IEnumerable<IEdmOperation> FindDeclaredBoundOperations(string qualifiedName, IEdmType bindingType)
{
return this.FindDeclaredOperations(qualifiedName).Where(o => o.IsBound && o.Parameters.Any() && o.HasEquivalentBindingType(bindingType));
var enumerable = this.FindDeclaredOperations(qualifiedName);

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

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

Prefer not to use var, use IEnumerable instead. #Resolved

if (operations != null)
{
IList<IEdmOperation> matchedOperation = new List<IEdmOperation>();
for (int i = 0; i < operations.Count(); i++)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Jun 3, 2019

Member

Count() [](start = 47, length = 7)

Count or Count()? If this is a list, isn't Count property better? #Resolved

@mikepizzo
Copy link
Member

left a comment

🕐

@xuzhg

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

        return operationsFound.ToArray();

That's old codes. I changed. it.


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


Refers to: src/Microsoft.OData.Core/Metadata/MetadataUtils.cs:188 in 2d0c8c7. [](commit_id = 2d0c8c7, deletion_comment = False)

@xuzhg xuzhg force-pushed the xuzhg:LinqImprovement2 branch 3 times, most recently from 18a4702 to bce6bc3 Jun 3, 2019

@xuzhg xuzhg force-pushed the xuzhg:LinqImprovement2 branch from bce6bc3 to 12b4a32 Jun 4, 2019

@xuzhg

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

As discussed with Mike. So far, the concern is good. So, Merged. Thanks.

@xuzhg xuzhg closed this Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.