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

Improve cache performance of MethodFinder.GetAllInstanceMethods #357

Merged
merged 7 commits into from
Jun 1, 2018
Merged

Conversation

tangdf
Copy link
Contributor

@tangdf tangdf commented May 11, 2018

No description provided.

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find. :) Just a few observations from someone passing by.

{
uniqueInfos.Add(info, null);
}
uniqueInfos.Add(info);
}
Copy link
Member

@stakx stakx May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This foreach loop probably isn't needed at all when using the HashSet(IEnumerable<T> collection, IEqualityComparer<T> comparer) ctor instead.

Copy link
Member

@jonorossi jonorossi May 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing the call site it looks like this method could be removed and the call site just replaced with:

type.GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
    .Distinct(MethodSignatureComparer.Instance)
    .ToArray();

This code is cruddy because it would have been written pre-.NET 3.5 so before LINQ and HashSet.

@@ -78,17 +78,14 @@ private static MethodInfo[] MakeFilteredCopy(MethodInfo[] methodsInCache, Bindin

private static object RemoveDuplicates(MethodInfo[] infos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Unrelated to this change, and I haven't looked at the call sites... but I wonder why this method's declared return type is object, instead of MethodInfo[]? Do call sites have to cast the return value back to MethodInfo[]?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They get stored into a field Dictionary<Type, object> cachedMethodInfosByType, so yes this field should be updated too since there is only one call site of this private method.

`KeyValuePair<Key, Value>` not implemented interface `IEquatable<>`.
Replace KeyValuePair<MethodInfo, Type> to custom struct
@jonorossi
Copy link
Member

@tangdf you've merged your cache key changes into this branch (because you used your master) which updated this pull request, so it now conflicts.

Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tangdf did you see the code review comments?

@tangdf
Copy link
Contributor Author

tangdf commented May 31, 2018

I'm so sorry,I ignored the comments.

@ghost
Copy link

ghost commented May 31, 2018

Don’t worry @tangdf we have all been there. Keep it going! ;)

@jonorossi jonorossi changed the title Replace Dictionary to HashSet Improve cache performance of MethodFinder.GetAllInstanceMethods Jun 1, 2018
@jonorossi jonorossi added this to the vNext milestone Jun 1, 2018
@jonorossi jonorossi merged commit 9ada611 into castleproject:master Jun 1, 2018
@jonorossi
Copy link
Member

Thanks @tangdf, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants