-
Notifications
You must be signed in to change notification settings - Fork 466
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
Speed up interface proxy w/o target type generation significantly #573
Speed up interface proxy w/o target type generation significantly #573
Conversation
@jonorossi, I realise you probably don't have time to review all my recent PRs... there have been quite a few lately. But this very short one may be worth a look. (A remark about my other refactoring-only PRs: I'll aim at leaving each one those open for at least a couple of days, in case that you find some time to review, or that I may have second thoughts about some aspects. If neither of those two things occurs, I hope I'll be forgiven for going ahead and merging them, so that I can continue with subsequent code clean-ups... I find they often depend upon one another. I'll refrain from merging PRs without review if they affect the public API significantly, or make any functional change.) |
if (canChangeTarget == false && methodInfo.IsAbstract && methodInfo.IsGenericMethod == false && methodInfo.IsGenericMethodDefinition == false) | ||
{ | ||
// We do not need to generate a custom invocation type because no custom implementation | ||
// for `InvokeMethodOnTarget` will be needed (proceeding to target isn't possible here): | ||
return typeof(InterfaceMethodWithoutTargetInvocation); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generics only matter because there are some asserts somewhere else that trigger for generic methods; I'll need to look at those more closely before allowing generic methods here.
Checking for .IsAbstract
may seem unnecessary now, but it becomes important once we support default interface implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do generic methods not already work? Looks like SetGenericMethodArguments
would be called on this invocation instance if you removed the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they'd (almost) work just like that. We do have some code that assumes there will be a generic invocation type for a generic method, which is no longer true now:
Core/src/Castle.Core/DynamicProxy/Generators/MethodWithInvocationGenerator.cs
Lines 82 to 86 in c0fba5f
if (MethodToOverride.IsGenericMethod) | |
{ | |
// bind generic method arguments to invocation's type arguments | |
genericArguments = emitter.MethodBuilder.GetGenericArguments(); | |
invocationType = invocationType.MakeGenericType(genericArguments); |
I expect that the necessary changes are going to be very small, e.g.:
if (MethodToOverride.IsGenericMethod)
{
+ genericArguments = emitter.MethodBuilder.GetGenericArguments();
// bind generic method arguments to invocation's type arguments
- genericArguments = emitter.MethodBuilder.GetGenericArguments();
+ if (invocationType.IsGenericTypeDefinition)
+ {
invocationType = invocationType.MakeGenericType(genericArguments);
constructor = TypeBuilder.GetConstructor(invocationType, constructor);
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the above change in a slightly modified form, so generic methods are now covered, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top work Dom 👍 . Definitely want to see this one in the changelog.
if (canChangeTarget == false && methodInfo.IsAbstract && methodInfo.IsGenericMethod == false && methodInfo.IsGenericMethodDefinition == false) | ||
{ | ||
// We do not need to generate a custom invocation type because no custom implementation | ||
// for `InvokeMethodOnTarget` will be needed (proceeding to target isn't possible here): | ||
return typeof(InterfaceMethodWithoutTargetInvocation); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do generic methods not already work? Looks like SetGenericMethodArguments
would be called on this invocation instance if you removed the check.
src/Castle.Core/DynamicProxy/Contributors/InterfaceProxyWithoutTargetContributor.cs
Show resolved
Hide resolved
0e5dc7c
to
e3de07e
Compare
e3de07e
to
7b6b193
Compare
DynamicProxy's runtime performance is directly tied to the amount of System.Reflection.Emit-ting done. If we do less dynamic type generation, DynamicProxy becomes faster.
I've recently realised that the main reason why DynamicProxy generates dedicated invocation types for every proxied method is because the invocation's
InvokeMethodOnTarget
implementation depends on the proxied method's signature.However, if an invocation cannot proceed to a (method on a) target, we don't need a custom implementation for
InvokeMethodOnTarget
, and thus no custom invocation type; we can use a pre-defined invocation type instead!This PR is a realization of this idea. I'm starting with
non-genericmethods on interface proxies without target. It should be possible to cover more methods —generic ones (I think), andabstract methods on class proxies without target — but those will take more work.I expect this to be very beneficial to some core DynamicProxy usage scenarios, i.e. mocking libraries used in unit testing, where interface mocks are frequently backed by interfaces proxies without target.
I've tested this PR against Moq 4's test suite. Test execution time has dropped by a factor of 2.5x for both the .NET Framework and .NET Core. Another simple
Stopwatch
benchmark suggests roughly the same speed-up.