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

Speed up interface proxy w/o target type generation significantly #573

Merged

Conversation

stakx
Copy link
Member

@stakx stakx commented Feb 2, 2021

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-generic methods on interface proxies without target. It should be possible to cover more methods — generic ones (I think), and abstract 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.

@stakx
Copy link
Member Author

stakx commented Feb 2, 2021

@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.)

Comment on lines 68 to 71
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);
}
Copy link
Member Author

@stakx stakx Feb 2, 2021

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.

Copy link
Member

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.

Copy link
Member Author

@stakx stakx Feb 3, 2021

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:

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);
+    }

Copy link
Member Author

@stakx stakx Feb 3, 2021

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.

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.

Top work Dom 👍 . Definitely want to see this one in the changelog.

Comment on lines 68 to 71
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);
}
Copy link
Member

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.

@stakx stakx added this to the v5.0.0 milestone Feb 3, 2021
@stakx stakx force-pushed the interface-proxy-type-generation-speedup branch 2 times, most recently from 0e5dc7c to e3de07e Compare February 3, 2021 20:32
@stakx stakx force-pushed the interface-proxy-type-generation-speedup branch from e3de07e to 7b6b193 Compare February 3, 2021 22:17
@stakx stakx changed the title Speed up interface proxy w/o target type generation by up to approx. 250 % Speed up interface proxy w/o target type generation significantly Feb 4, 2021
@stakx stakx merged commit 7eecd12 into castleproject:master Feb 4, 2021
@stakx stakx deleted the interface-proxy-type-generation-speedup branch February 4, 2021 22:18
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.

None yet

2 participants