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

Use Castle to generate delegate proxies #537

Merged
merged 1 commit into from
Apr 13, 2019

Conversation

zvirja
Copy link
Contributor

@zvirja zvirja commented Apr 8, 2019

Recently @stakx did a favor to all of us and added possibility to generate delegate proxies to the Castle.Core library. Simplified our internal logic to use the Castle.Core generator for both delegates and classes.

Notice, I haven't removed the deprecated classes we already have, as it's not allowed by SemVer I'm trying to follow 😉 Instead, just deprecated all the types to remove them in future.

@dtchepak Please review the PR and let me know if you are fine. Don't merge it yet - I want to spend some time and benchmark the performance (after #536 is merged). Created it early, so we can discuss the approach 😉

UPD:

According to the benchmark results the activation performance is comparable:

BEFORE:

                           Method |  Job | Runtime |      Mean |     Error |    StdDev |    Median |  Gen 0 | Allocated |
--------------------------------- |----- |-------- |----------:|----------:|----------:|----------:|-------:|----------:|
        CreateInterfaceSubstitute |  Clr |     Clr |  4.970 us | 0.0993 us | 0.1816 us |  4.933 us | 0.9308 |   4.32 KB |
    CreateAbstractClassSubstitute |  Clr |     Clr |  4.545 us | 0.0859 us | 0.0882 us |  4.522 us | 0.8774 |   4.05 KB |
 CreateNonAbstractClassSubstitute |  Clr |     Clr |  4.496 us | 0.0817 us | 0.0764 us |  4.523 us | 0.8774 |   4.05 KB |
         CreateDelegateSubstitute |  Clr |     Clr | 11.112 us | 0.2137 us | 0.2099 us | 11.097 us | 0.9460 |   4.42 KB |
        CreateInterfaceSubstitute | Core |    Core |  4.864 us | 0.0965 us | 0.0991 us |  4.882 us | 1.0300 |   4.77 KB |
    CreateAbstractClassSubstitute | Core |    Core |  4.777 us | 0.0820 us | 0.0640 us |  4.776 us | 0.9766 |   4.51 KB |
 CreateNonAbstractClassSubstitute | Core |    Core |  4.926 us | 0.1127 us | 0.3324 us |  5.112 us | 0.9766 |   4.51 KB |
         CreateDelegateSubstitute | Core |    Core |  8.343 us | 0.1622 us | 0.2620 us |  8.296 us | 1.0529 |   4.87 KB |

AFTER:


                           Method |  Job | Runtime |      Mean |     Error |    StdDev |    Median |  Gen 0 | Allocated |
--------------------------------- |----- |-------- |----------:|----------:|----------:|----------:|-------:|----------:|
        CreateInterfaceSubstitute |  Clr |     Clr |  5.003 us | 0.0957 us | 0.0940 us |  4.975 us | 0.9384 |   4.33 KB |
    CreateAbstractClassSubstitute |  Clr |     Clr |  4.587 us | 0.0900 us | 0.1170 us |  4.579 us | 0.8774 |   4.06 KB |
 CreateNonAbstractClassSubstitute |  Clr |     Clr |  4.604 us | 0.0798 us | 0.0707 us |  4.590 us | 0.8774 |   4.06 KB |
         CreateDelegateSubstitute |  Clr |     Clr | 11.512 us | 0.1608 us | 0.1342 us | 11.488 us | 0.9918 |   4.58 KB |
        CreateInterfaceSubstitute | Core |    Core |  4.914 us | 0.0859 us | 0.0761 us |  4.907 us | 1.0376 |    4.8 KB |
    CreateAbstractClassSubstitute | Core |    Core |  4.426 us | 0.0856 us | 0.0801 us |  4.423 us | 0.9842 |   4.54 KB |
 CreateNonAbstractClassSubstitute | Core |    Core |  4.360 us | 0.0654 us | 0.0579 us |  4.358 us | 0.9842 |   4.54 KB |
         CreateDelegateSubstitute | Core |    Core |  8.090 us | 0.1618 us | 0.4591 us |  7.918 us | 1.0834 |   5.04 KB |

@zvirja zvirja requested a review from dtchepak April 8, 2019 21:18
Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

src/NSubstitute/Core/Extensions.cs Show resolved Hide resolved
@zvirja zvirja force-pushed the use-castle-to-generate-delegate-proxy branch from 3ce3a59 to a1cf685 Compare April 13, 2019 21:04
@zvirja zvirja force-pushed the use-castle-to-generate-delegate-proxy branch from a1cf685 to 1e665fd Compare April 13, 2019 21:06
@zvirja
Copy link
Contributor Author

zvirja commented Apr 13, 2019

@dtchepak Added benchmark results - everything is fine. Please take a look at the latest changes and fi you are fine - feel free to merge the code.

Thanks!

@zvirja zvirja changed the title [WIP] Use Castle to generate delegate proxies Use Castle to generate delegate proxies Apr 13, 2019
@dtchepak dtchepak merged commit 9a2e885 into master Apr 13, 2019
@dtchepak dtchepak deleted the use-castle-to-generate-delegate-proxy branch April 13, 2019 22:10
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.

2 participants