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

Make readonly parameters detection more robust #406

Conversation

zvirja
Copy link
Contributor

@zvirja zvirja commented Aug 30, 2018

Closes #398

Now we check the In required modifier, so the detection logic is more robust.

Additional improvements:

  • exclude Rider tmp files from git
  • do not store invocation arguments if there are no writable parameters (optimization).

I've tested that it's much quicker to test for the custom modifiers, than for attributes.

Test body:

[ClrJob, CoreJob]
public class MyBench
{
    private readonly ParameterInfo Parameter = typeof(ReadOnlyHolder).GetMethod(nameof(ReadOnlyHolder.Do)).GetParameters().Single();

    [Benchmark]
    public bool TestInModifier() => (Parameter.Attributes & ParameterAttributes.In) == ParameterAttributes.In;

    [Benchmark]
    public bool HasInReqMod() => Parameter.GetRequiredCustomModifiers().Any(x => x == typeof(InAttribute));

    [Benchmark]
    public bool HasIsReadOnlyAttribute() => Parameter.GetCustomAttributes(false).Any(x => x.GetType().FullName == "System.Runtime.CompilerServices.IsReadOnlyAttribute");
}

public abstract class ReadOnlyHolder
{
    public abstract void Do(in int value);
}

Result:

BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
Frequency=3914069 Hz, Resolution=255.4886 ns, Timer=TSC
.NET Core SDK=2.1.400
  [Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
  Clr    : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3132.0
  Core   : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT


                 Method |  Job | Runtime |         Mean |      Error |     StdDev |
----------------------- |----- |-------- |-------------:|-----------:|-----------:|
         TestInModifier |  Clr |     Clr |     1.255 ns |  0.0093 ns |  0.0077 ns |
            HasInReqMod |  Clr |     Clr |   219.762 ns |  1.4106 ns |  1.3194 ns |
 HasIsReadOnlyAttribute |  Clr |     Clr | 1,623.902 ns | 20.4394 ns | 18.1190 ns |
         TestInModifier | Core |    Core |     1.225 ns |  0.0027 ns |  0.0021 ns |
            HasInReqMod | Core |    Core |   195.929 ns |  1.3431 ns |  1.1906 ns |
 HasIsReadOnlyAttribute | Core |    Core | 1,143.240 ns |  9.8433 ns |  8.2196 ns |

CC @stakx

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.

Generally looks good to me.

  • The optimization in CopyOutAndRefParameters looks good to me.

  • I do have a few reservations about some (seemingly unnecessary) changes in IsReadOnly (and .gitignore), nothing major though.

  • Please ensure that, however the code eventually ends up, the longish introductory code comment in IsReadOnly actually agrees with (is in sync with) what the code does!

Thank you for working on this!

src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs Outdated Show resolved Hide resolved
src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs Outdated Show resolved Hide resolved
@zvirja zvirja force-pushed the make-readonly-parameters-detection-more-robust branch from 0fa3914 to 845e55d Compare August 30, 2018 22:22
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.

Looks good to me! 👍

Just two more suggestions: one is stated in the review comment below, the other would be to mention this improvement in the CHANGELOG.md (this PR's title should be sufficient as a summary). (Sorry for not thinking of these earlier.)

src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs Outdated Show resolved Hide resolved
@zvirja zvirja force-pushed the make-readonly-parameters-detection-more-robust branch 2 times, most recently from 80f2b62 to 5a41f9a Compare August 31, 2018 15:46
@zvirja
Copy link
Contributor Author

zvirja commented Aug 31, 2018

@stakx @jonorossi Many thanks for the code review and the valuable comments you left! Appreciate a lot 🍻 🙇

I fixed all of them, so we should be fine to proceed further. Let me know if something left.

P.S. Sorry for the "laconic" replies and "dirty" commits - am extremely busy these days 😖

@zvirja zvirja force-pushed the make-readonly-parameters-detection-more-robust branch from 5a41f9a to aa27233 Compare August 31, 2018 15:49
@zvirja
Copy link
Contributor Author

zvirja commented Sep 4, 2018

@jonorossi @stakx Are we waiting for somebody else's approve to merge this? Unfortunately, I don't have enough permissions to run the merge...

@jonorossi
Copy link
Member

@zvirja I didn't merge it since @stakx hadn't marked his review as complete, and since he has been more involved with these changes than myself I thought best to defer to him. Thanks for the clean set of commits.

@jonorossi
Copy link
Member

@zvirja oh, just saw the "Looks good to me!" comment by @stakx.

@jonorossi jonorossi merged commit 11bedc6 into castleproject:master Sep 4, 2018
@jonorossi jonorossi added this to the vNext milestone Sep 4, 2018
@stakx
Copy link
Member

stakx commented Sep 4, 2018

Sorry for being non-responsive for the last few days, I was feeling a little ill & took things more slowly. Good that this has been merged, thanks for the improvements!!

@jonorossi
Copy link
Member

@stakx not a problem, I hope you are feeling better.

@zvirja
Copy link
Contributor Author

zvirja commented Sep 4, 2018

@stakx Recover soon, mate 😉 Thanks for the time spent on this.

@stakx stakx mentioned this pull request Sep 5, 2018
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