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

ByRef and Stack-only support #492

Merged
merged 5 commits into from Nov 6, 2017

Conversation

Projects
None yet
4 participants
@adamsitnik
Member

adamsitnik commented Jul 7, 2017

So far our engine was internally using lambdas: Func<T> or Action.

In C# it's impossible to define a Func that returns by reference.
Stack-only types like Span<T> can not be generic arguments, so it's impossible to use Func<Span<T>>. This is very new rule, enforced only by .NET Core 2.0+ runtime, which throws at runtime. C# is going to implement the support for this rule with C# 7.2.

I have changed the Engine to use methods only. The lack of inlining effect of lambdas got preserved by using [MethodImp(NoInlining)] attribute.

I decided to implement the Idle method for byrefs as:

[MethodImp(NoInlining)]
IntPtr Idle() => default(IntPtr);

Because it includes the cost of method invocation and of returning a value of size equal to pointer for given architecture.

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jul 7, 2017

Member

Sample benchmark:

public struct BigValueType
{
    public int _1, _2, _3, _4, _5, _6;
}

[LegacyJitX86Job, LegacyJitX64Job, RyuJitX64Job]
public class IL_RefReturns
{
    private BigValueType field;

    [Benchmark]
    public ref BigValueType ReturnsByRef() => ref Initialize(ref field);

    [Benchmark]
    public BigValueType ReturnsByValue() => Initialize(field);

    private ref BigValueType Initialize(ref BigValueType reference)
    {
        reference._1 = 1;
        reference._2 = 2;
        reference._3 = 3;
        reference._4 = 4;
        reference._5 = 5;
        reference._6 = 6;

        return ref reference;
    }

    private BigValueType Initialize(BigValueType value)
    {
        value._1 = 1;
        value._2 = 2;
        value._3 = 3;
        value._4 = 4;
        value._5 = 5;
        value._6 = 6;

        return value;
    }
}

Sample results:

BenchmarkDotNet=v0.10.8.20170707-develop, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Core i7-6600U CPU 2.60GHz (Skylake), ProcessorCount=4
Frequency=2742189 Hz, Resolution=364.6722 ns, Timer=TSC
  [Host]       : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0
  LegacyJitX64 : Clr 4.0.30319.42000, 64bit LegacyJIT/clrjit-v4.7.2053.0;compatjit-v4.7.2053.0
  LegacyJitX86 : Clr 4.0.30319.42000, 32bit LegacyJIT-v4.7.2053.0
  RyuJitX64    : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0

Runtime=Clr  
Method Job Jit Platform Mean Error StdDev
ReturnsByRef LegacyJitX64 LegacyJit X64 4.282 ns 0.0563 ns 0.0527 ns
ReturnsByValue LegacyJitX64 LegacyJit X64 13.193 ns 0.1385 ns 0.1296 ns
ReturnsByRef LegacyJitX86 LegacyJit X86 5.763 ns 0.0901 ns 0.0843 ns
ReturnsByValue LegacyJitX86 LegacyJit X86 7.813 ns 0.0892 ns 0.0834 ns
ReturnsByRef RyuJitX64 RyuJit X64 4.867 ns 0.0572 ns 0.0507 ns
ReturnsByValue RyuJitX64 RyuJit X64 7.451 ns 0.0369 ns 0.0267 ns
Member

adamsitnik commented Jul 7, 2017

Sample benchmark:

public struct BigValueType
{
    public int _1, _2, _3, _4, _5, _6;
}

[LegacyJitX86Job, LegacyJitX64Job, RyuJitX64Job]
public class IL_RefReturns
{
    private BigValueType field;

    [Benchmark]
    public ref BigValueType ReturnsByRef() => ref Initialize(ref field);

    [Benchmark]
    public BigValueType ReturnsByValue() => Initialize(field);

    private ref BigValueType Initialize(ref BigValueType reference)
    {
        reference._1 = 1;
        reference._2 = 2;
        reference._3 = 3;
        reference._4 = 4;
        reference._5 = 5;
        reference._6 = 6;

        return ref reference;
    }

    private BigValueType Initialize(BigValueType value)
    {
        value._1 = 1;
        value._2 = 2;
        value._3 = 3;
        value._4 = 4;
        value._5 = 5;
        value._6 = 6;

        return value;
    }
}

Sample results:

BenchmarkDotNet=v0.10.8.20170707-develop, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Core i7-6600U CPU 2.60GHz (Skylake), ProcessorCount=4
Frequency=2742189 Hz, Resolution=364.6722 ns, Timer=TSC
  [Host]       : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0
  LegacyJitX64 : Clr 4.0.30319.42000, 64bit LegacyJIT/clrjit-v4.7.2053.0;compatjit-v4.7.2053.0
  LegacyJitX86 : Clr 4.0.30319.42000, 32bit LegacyJIT-v4.7.2053.0
  RyuJitX64    : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0

Runtime=Clr  
Method Job Jit Platform Mean Error StdDev
ReturnsByRef LegacyJitX64 LegacyJit X64 4.282 ns 0.0563 ns 0.0527 ns
ReturnsByValue LegacyJitX64 LegacyJit X64 13.193 ns 0.1385 ns 0.1296 ns
ReturnsByRef LegacyJitX86 LegacyJit X86 5.763 ns 0.0901 ns 0.0843 ns
ReturnsByValue LegacyJitX86 LegacyJit X86 7.813 ns 0.0892 ns 0.0834 ns
ReturnsByRef RyuJitX64 RyuJit X64 4.867 ns 0.0572 ns 0.0507 ns
ReturnsByValue RyuJitX64 RyuJit X64 7.451 ns 0.0369 ns 0.0267 ns
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Jul 7, 2017

Member

@adamsitnik, I really like this feature, but unfortunately the implementation is wrong for now, you have an inlining-related bug. Here is a repro:

public struct BigValueType
{
    public int _1, _2, _3, _4, _5, _6;
}

[Config(typeof(Config)), RankColumn, WelchTTestPValueColumn]
public class IL_RefReturns2
{
    private class Config : ManualConfig
    {
        public Config()
        {
            Add(Job.RyuJitX64.WithTargetCount(30));
        }
    }

    private BigValueType field;

    [Benchmark(Baseline = true)]
    public ref BigValueType ReturnsByRef() => ref Initialize(ref field);
    
    [Benchmark]
    [MethodImpl(MethodImplOptions.NoInlining)]
    public ref BigValueType ReturnsByRefNoInlining() => ref Initialize(ref field);

    private ref BigValueType Initialize(ref BigValueType reference)
    {
        reference._1 = 1;
        reference._2 = 2;
        reference._3 = 3;
        reference._4 = 4;
        reference._5 = 5;
        reference._6 = 6;

        return ref reference;
    }

    private BigValueType Initialize(BigValueType value)
    {
        value._1 = 1;
        value._2 = 2;
        value._3 = 3;
        value._4 = 4;
        value._5 = 5;
        value._6 = 6;

        return value;
    }
}
BenchmarkDotNet=v0.10.8.20170707-develop, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Core i7-6700HQ CPU 2.60GHz (Skylake), ProcessorCount=8
Frequency=2531249 Hz, Resolution=395.0619 ns, Timer=TSC
  [Host]     : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0
  Job-XSNXHD : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0

Jit=RyuJit  Platform=X64  TargetCount=30  

                 Method |     Mean |     Error |    StdDev | Scaled | ScaledSD | t-test p-value | Rank |
----------------------- |---------:|----------:|----------:|-------:|---------:|---------------:|-----:|
           ReturnsByRef | 5.816 ns | 0.0698 ns | 0.1045 ns |   1.00 |     0.00 |         1.0000 |    1 |
 ReturnsByRefNoInlining | 6.247 ns | 0.0687 ns | 0.1029 ns |   1.07 |     0.03 |         0.0000 |    2 |

[MethodImpl(MethodImplOptions.NoInlining)] shouldn't affect benchmark results. I don't know how to solve it for byref methods. =(

Member

AndreyAkinshin commented Jul 7, 2017

@adamsitnik, I really like this feature, but unfortunately the implementation is wrong for now, you have an inlining-related bug. Here is a repro:

public struct BigValueType
{
    public int _1, _2, _3, _4, _5, _6;
}

[Config(typeof(Config)), RankColumn, WelchTTestPValueColumn]
public class IL_RefReturns2
{
    private class Config : ManualConfig
    {
        public Config()
        {
            Add(Job.RyuJitX64.WithTargetCount(30));
        }
    }

    private BigValueType field;

    [Benchmark(Baseline = true)]
    public ref BigValueType ReturnsByRef() => ref Initialize(ref field);
    
    [Benchmark]
    [MethodImpl(MethodImplOptions.NoInlining)]
    public ref BigValueType ReturnsByRefNoInlining() => ref Initialize(ref field);

    private ref BigValueType Initialize(ref BigValueType reference)
    {
        reference._1 = 1;
        reference._2 = 2;
        reference._3 = 3;
        reference._4 = 4;
        reference._5 = 5;
        reference._6 = 6;

        return ref reference;
    }

    private BigValueType Initialize(BigValueType value)
    {
        value._1 = 1;
        value._2 = 2;
        value._3 = 3;
        value._4 = 4;
        value._5 = 5;
        value._6 = 6;

        return value;
    }
}
BenchmarkDotNet=v0.10.8.20170707-develop, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Core i7-6700HQ CPU 2.60GHz (Skylake), ProcessorCount=8
Frequency=2531249 Hz, Resolution=395.0619 ns, Timer=TSC
  [Host]     : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0
  Job-XSNXHD : Clr 4.0.30319.42000, 64bit RyuJIT-v4.7.2053.0

Jit=RyuJit  Platform=X64  TargetCount=30  

                 Method |     Mean |     Error |    StdDev | Scaled | ScaledSD | t-test p-value | Rank |
----------------------- |---------:|----------:|----------:|-------:|---------:|---------------:|-----:|
           ReturnsByRef | 5.816 ns | 0.0698 ns | 0.1045 ns |   1.00 |     0.00 |         1.0000 |    1 |
 ReturnsByRefNoInlining | 6.247 ns | 0.0687 ns | 0.1029 ns |   1.07 |     0.03 |         0.0000 |    2 |

[MethodImpl(MethodImplOptions.NoInlining)] shouldn't affect benchmark results. I don't know how to solve it for byref methods. =(

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jul 10, 2017

Member

I need to think about it.

I am also not sure what should be the result in case when user is running by ref benchmarks:
Maybe in that case the cost of method invocation and returning of the result should be included in the results? To make it possible to compare ByRef vs ByValue?

Member

adamsitnik commented Jul 10, 2017

I need to think about it.

I am also not sure what should be the result in case when user is running by ref benchmarks:
Maybe in that case the cost of method invocation and returning of the result should be included in the results? To make it possible to compare ByRef vs ByValue?

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Jul 10, 2017

Member

I also need to think about it. The main point here is the JIT optimizations (like inlining) shouldn't affect benchmark results.

Member

AndreyAkinshin commented Jul 10, 2017

I also need to think about it. The main point here is the JIT optimizations (like inlining) shouldn't affect benchmark results.

adamsitnik added some commits Oct 9, 2017

use Delegates to prevent from inlining, but still make it possible to…
… use ByRef methods and Stack-Only types
Merge branch 'master' into noLambdas
Conflicts:
	src/BenchmarkDotNet.Core/Validators/CompilationValidator.cs
@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 9, 2017

Member

@AndreyAkinshin once this build pass the code is ready for review. The solution was simple: use delegates, which allow ref and don't need to be generic, but type-specific so Stack-Only types are also OK. The build for .NET Core 2.0 needs to be green to confirm that.

btw the red build was my fault, sorry!

Member

adamsitnik commented Oct 9, 2017

@AndreyAkinshin once this build pass the code is ready for review. The solution was simple: use delegates, which allow ref and don't need to be generic, but type-specific so Stack-Only types are also OK. The build for .NET Core 2.0 needs to be green to confirm that.

btw the red build was my fault, sorry!

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 9, 2017

Member

btw I checked and there is no difference for inline/no inline now ;)

Member

adamsitnik commented Oct 9, 2017

btw I checked and there is no difference for inline/no inline now ;)

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 10, 2017

Member

@AndreyAkinshin it's green now, ready for the review. Please remember to squash it ;)

Member

adamsitnik commented Oct 10, 2017

@AndreyAkinshin it's green now, ready for the review. Please remember to squash it ;)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Oct 10, 2017

Member

@adamsitnik, great, will review it on this week. Could you also cherry-pick the build fix to master and make it green too?

Member

AndreyAkinshin commented Oct 10, 2017

@adamsitnik, great, will review it on this week. Could you also cherry-pick the build fix to master and make it green too?

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 10, 2017

Member

@AndreyAkinshin done. Sorry for the trouble, I have no idea how my VS was able to build it. I ofc built it and tested with xunit to make sure it works

Member

adamsitnik commented Oct 10, 2017

@AndreyAkinshin done. Sorry for the trouble, I have no idea how my VS was able to build it. I ofc built it and tested with xunit to make sure it works

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Nov 6, 2017

Member

@AndreyAkinshin I am merging it because I want to work on the benchmarks with arguments and I want to avoid super-big conflict with my own code ;)

Member

adamsitnik commented Nov 6, 2017

@AndreyAkinshin I am merging it because I want to work on the benchmarks with arguments and I want to avoid super-big conflict with my own code ;)

@adamsitnik adamsitnik merged commit 2a2e6ca into master Nov 6, 2017

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Nov 6, 2017

Member

@adamsitnik, ok. Il already tried the branch, it looks fine. Let's figure out how it works in nightly builds!

Member

AndreyAkinshin commented Nov 6, 2017

@adamsitnik, ok. Il already tried the branch, it looks fine. Let's figure out how it works in nightly builds!

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Nov 6, 2017

Member

@AndreyAkinshin great! btw our build is red now because nuget.org is offline ;)

Member

adamsitnik commented Nov 6, 2017

@AndreyAkinshin great! btw our build is red now because nuget.org is offline ;)

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Nov 6, 2017

Member

I will rerun build tomorrow morning (I hope everything will be fine)

Member

AndreyAkinshin commented Nov 6, 2017

I will rerun build tomorrow morning (I hope everything will be fine)

@AndreyAkinshin AndreyAkinshin added this to the v0.10.11 milestone Nov 9, 2017

GeorgePlotnikov added a commit to GeorgePlotnikov/BenchmarkDotNet that referenced this pull request Feb 25, 2018

ByRef and Stack-only support (#492)
* ByRef and stack only types returning methods support

* use Delegates to prevent from inlining, but still make it possible to use ByRef methods and Stack-Only types

* the build fix ;)

* stack-only types must not be generic type arguments

@adamsitnik adamsitnik deleted the noLambdas branch Mar 22, 2018

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this pull request Sep 22, 2018

ByRef and Stack-only support (#492)
* ByRef and stack only types returning methods support

* use Delegates to prevent from inlining, but still make it possible to use ByRef methods and Stack-Only types

* the build fix ;)

* stack-only types must not be generic type arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment