4
Vote

[Compat .NET 4.5] Lambdas/Anonymous delegates that capture no context use backing methods whose signatures differ from the delegate type (impacts Moq)

description

Customer Scenario
Moq is a popular unit testing mocking framework. As part of mock method generation, there is an option to add custom callback implementations to your mock methods. However, the scenario of accessing method parameters while implementing callbacks via lambdas/anonymous delegates no longer works once a Moq user starts compiling their unit tests with Roslyn.

Background
If you compile the following code with .NET 4.5.x vs. Roslyn, you'll see two differing console outputs
    using System;

    class Code {
        static void Main() {
            Console.WriteLine(new Action(()=> { }).Method);
        }
    }
Pre-Roslyn: Void <Main>b__0()
Roslyn: Void <Main>b__0(System.Object)

Both are verifiable code by the .NET verifier, and are in adherent to the ECMA 335 spec.

However, because the signature of the delegate's backing method is different - code that inspects the delegate's backing method's signature may have their assumptions broken.

This is exactly what is happening in the Moq case (Moq v4.0). In this stack trace,
Moq.dll!Moq.MethodCall.ThrowParameterMismatch(ParameterInfo[] expected, ParameterInfo[] actual)
Moq.dll!Moq.MethodCall.SetCallbackWithArguments(Delegate callback)
Moq.dll!Moq.MethodCall.Callback<int>(Action<int> callback)  
UnitTestExample.dll!TestClass.TestMethod()
In this case, the TestMethod is calling code in Moq to install their custom callback, but Moq is rejecting the argument because it's convinced that the lambda is incompatible with the mock method's signature.

The Roslyn compiler should consider changing the backing method generation for lambdas in this case to improve greater compatibility with existing .NET code.

Full repro code for the Moq example below, and example VS Unit Test project attached to this issue.
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;

[TestClass]
public class TestClass
{
    [TestMethod]
    public void TestMethod()
    {
        Mock<IExample> mockObj = new Mock<IExample>();

        // Now provide a mock implementation of ExampleMethod
        // using a custom callback
        mockObj.Setup(m => m.ExampleMethod(It.IsAny<int>()))
               .Callback((int param) =>
                   {
                       // custom mock method impl code
                   });

        // some code under test, that invokes mockInterface.ExampleMethod()
    }
}

public interface IExample
{
    void ExampleMethod(int param);
}

file attachments

comments

mareksafar wrote Jul 24, 2014 at 7:52 AM

This is more of Moq bug where it cannot handle curried delegates than compiler bug which generates valid (faster) code

theoy wrote Jul 24, 2014 at 7:26 PM

I agree that technically that the compiler is doing nothing wrong. I'm also the new engineering lead for the Roslyn compilers, and didn't want to present too much bias :).

We'll investigate the perf implications though. It would be a shame for our customers who are using existing versions of Moq to suddenly break their testbed when they start building with Roslyn in Visual Studio "14".

VSadov wrote Jul 24, 2014 at 10:53 PM

theoy wrote Jul 26, 2014 at 2:47 AM

FYI, I confirmed the performance gain of the currying technique. On my machine, currying results in a consistent ~33-34% speed gain for the delegate invocation overhead (tested on empty method bodies). That's pretty compelling! Users can publically verify this test with the following code (make sure to compile under release/optimized configuration):
using System;
using System.Diagnostics;

static class Code
{
    static void Uncurried() { }
    static void Curried(this object ignored) { }

    static void Main()
    {
        Action uncurried = new Action(Uncurried);
        Action curried = new Action(((object)null).Curried);

        var uncurriedTime = CollectAndReportTicks("Uncurried", uncurried);
        var curriedTime = CollectAndReportTicks("Curried", curried);

        var curriedSpeedDividedByUncurried = ((double)uncurriedTime) / curriedTime;
        if (curriedSpeedDividedByUncurried > 1)
        {
            Console.WriteLine("Curried is {0}% faster", (curriedSpeedDividedByUncurried - 1) * 100);
        }
        else
        {
            Console.WriteLine("Curried is {0}% faster", (1 - curriedSpeedDividedByUncurried) * 100);
        }
    }

    const int Iterations = 1000 * 1000 * 1000;

    static long CollectAndReportTicks(string name, Action impl)
    {
        Stopwatch timer = new Stopwatch();
        timer.Start();
        for (int i = 0; i < Iterations; i++)
        {
            impl();
        }
        timer.Stop();
        Console.WriteLine("Time for '{0}' sample: {1} ticks", name, timer.ElapsedTicks);
        return timer.ElapsedTicks;
    }
}

KrisVDM wrote Oct 28, 2014 at 8:58 AM

The real question is: why is one faster than the other?

It seems that Roslyn may be trying to optimize around an issue in the JIT or the framework. This issue doesn't only exist with lambda's, but with delegates in general, as evidenced by the benchmark. So it looks like delegates in general should be optimized. Maybe Roslyn should indeed remain compatible, and let RyuJIT (and Native .NET) or the BCL solve this.

In fact, this isn't so much about being compatible, but about doing the sensible thing. Adding an unused parameter to speed things up? It makes no sense!

leppie wrote Oct 29, 2014 at 10:45 AM

There is no need for Roslyn to break the signature. Instance closed delegates have the same speed improvement.

Consider:
class Code
{
  public void Instance() {}

  static void Main()
  {
     Action instance = (Action) Delegate.CreateDelegate(typeof(Action), null,
       typeof(Code).GetMethod("Instance"));

    // sadly you cant use the following due to silly null check in the ctor...
    Action instance = new Action(((Code) null).Instance);

    .... // same code as previously
  }
}
The Instance method has the same signature now, bar the static modifier, same speed improvement.

VSadov wrote Oct 29, 2014 at 6:21 PM

== Question: Why one faster than another.

Delegates are fairly complicated entities that handle a lot of scenarios under the hood, but yet trivial to use. In general they are not just a tuple of a {receiver, function pointer} . - That would not occupy 40+ bytes(64bit numbers) , would it?

One example of what delegates may need to do in addition to indirection – When delegates are invoked, it is essentially an instance method call (caller calls “Invoke” on the delegate instance) and the platform ABI would require a calling convention of an instance call, with various requirements like which register holds “this” and how return value is passed back to the caller, etc... Now if a particular delegate ends up calling a static method, that one would not have “this” and will need to agree with calling convention for a static call. Not agreeing with ABI can potentially break things that rely on stackwalking – like debuggers, profilers, GC, exception handling and so on.
As a result, this particular delegate instance would need to reshuffle arguments before passing them to the actual calee. – Not a simple indirection… and the whole functionality must be opaquely encapsulated within the delegate instance because from the caller prospective all delegates of the same type look the same.

This kind of reshuffling is very hard to completely avoid when delegates are bound to static methods. The overhead for very small lambdas introduced by the code like “from customer in customers where customer.Age >= 21 select . . .” could be noticeable.

There are two "convenient" cases:

1) target method is an instance method
This is the ideal case. No need to touch arguments at all, just need to replace current “this” (which contains delegate instance) with enclosed “this” for the method that delegate is representing and JMP to the method.

The catch here is that the target method must be a real instance method and the enclosed receiver must not be null.

2) target method is a static “curried” method. (the one that has an extra first parameter)
The argument layout in the calling convention of an instance call typically matches the layout of a static call with the extra “curried this” parameter.
We just need to map {this, arg1, arg2, arg3..} -- to --> {enclosed curried this, arg1, arg2, arg3…}
As you can see we again can just patch the first argument and JMP to the target without touching other arguments.

With this approach target method can be static and enclosed receiver can be null, however the signature of the target method would need to have an extra argument for the “this” which does not need to be used.

VSadov wrote Oct 29, 2014 at 6:33 PM

== New Implementation:

Note that as of the changeset (https://roslyn.codeplex.com/SourceControl/changeset/3de67f36894b8c93431619c256d77cc9cd2e291a ) lambda codegen no longer uses the approach that emits noncapturing lambdas as static methods with an extra argument.

In the new strategy all lambdas are emitted as instance methods.

When you have code like
    static void Main(string[] args)
    {
           Func<int, bool> f = (age) => age >= 21;
    }
The code that is emitted will look similar to a simplified equivalent of:
    static void Main(string[] args)
    {
        Func<int, bool> f = DisplayClass.singleton.BackingMethod;
    }

    class DisplayClass
    {
        public static readonly DisplayClass singleton = new DisplayClass();

        public bool BackingMethod(int age)
        {
            return age >= 21;
        }
    }
With this approach lambda emit hits the “convenient case” #1 – an instance method delegate, so the perf overhead is minimal.
Also, with this approach the signature of the BackingMethod is the same as the signature of the lambda so tools relying on signature of implementing methods can continue working as before.

There are tradeoffs (as usual):
1) BackingMethod is an instance method on a display class even when lambda happens to not capture any locals. – You can see it as a closure that is empty.
2) There is a small allocation of a DisplayClass singleton which is reused for all static lambdas in a given class and is mostly negligible next to the size of actual delegate instances.

.

LukasFellechner wrote Jul 20, 2015 at 9:11 PM

There is one major downside of the new approach: Since the method will now be an instance method, this can lead to memory leaks. Each delegate will now contain a pointer to the instance they were declared on. When passing this delegate to a static or long living object, you will suddenly keep a lot of objects alive!

I have just installed VS2015 RTM and I get this exact problem with my WeakEventListener. They are not weak anymore because the attach/detach delegates now all contain instance pointers. I have noticed this with an old VS preview already but thought it is a bug that is sure to be fixed. It seems I was wrong and now my code leaks memory even with RTM.

I would strongly propose to revert the change to the old behavior. Breaking a mocking framework that can easily be updated to support VS2015 is probably a much better choice than introducing memory leaks in a hidden way. If I declare a delegate as (a, b) => a.Check(b) then I assume that this will be a static delegate and will not capture the declaring instance. If the compiler changes a delegate from static to instance then it changes the resulting code and GC behavior considerably.

Sure I could start removing all these lambdas and replacing them with explicit static methods. But that seems insane to me. Also, e.g. ReSharper proposes to use such delegates and allows to easily convert between delegate and static method. Any developer will assume that both forms will have the exact same behavior. With the current VS RTM, this is not the case. Lambdas will suddenly start to leak and only explicit static methods won't.

Please reconsider! I am not sure how to proceed here. I do not want to revert all my code. And I do not want to use VS2013. But maybe I will have to use VS2013 for final compilation until this is resolved in VS2015. Oh man. I should have reported this earlier... :-/

PauloMorgado wrote Jul 20, 2015 at 10:28 PM

Discussions have moved to https://github.com/dotnet/roslyn/issues/

Nevertheless, this change was due to performance reasons and has RTMed today. I wouldn't expect it to be changed anytime soon.

VSadov wrote Jul 21, 2015 at 5:50 PM

In theory lifting/caching private empty singleton objects should not be able to cause leaks of any user-created instances.
If there is indeed a bug here, it could be caused by something else.

Could you please enter an issue at https://github.com/dotnet/roslyn/issues/ with exact scenario that causes leaks?

Thanks,
Vladimir

LukasFellechner wrote Jul 21, 2015 at 6:00 PM

After looking into the linked discussions, I now think that the code will not leak memory. The instance reference of the delegate is the singleton instance of the displayclass, not the instance where I declare the lambda. So only the display class is referenced, and since that is stored in a static singleton, it is there to stay all time anyways.

Am I right in this assumption? If I have more questions on this, I will create a post in the github site. But it would be great if you could give a short response on this, so I know if I have to worry or not.

VSadov wrote Jul 21, 2015 at 6:36 PM

It is irrelevant whether a lambda declared in an instance or a static method.

Lambda captures only what it uses. If it uses "this", then it will capture an instance of containing class, since it needs it. If lambda does not need "this", then it should not capture it.

llarsson wrote Jul 23, 2015 at 2:45 PM

So what is the conclusion about weakeventlistener?

My reference is:
https://bkiener.wordpress.com/2010/02/11/simple-weak-event-listener-for-silverlight/

"The event handler method must be static. Why this is important? The OnEventAction is a delegate to a method and this method defines the action when the event is raising. It is important that the delegate to this method has no Target set and this is the case when the target method is static."

This has made me implement a check to avoid misstakes (see below), can i safely remove this?
    private Action<TInstance, object, TEventArgs> _onEventAction = null;
    public Action<TInstance, object, TEventArgs> OnEventAction
    {
        get { return _onEventAction; }
        set
        {
            if (value != null && !value.Method.IsStatic)
                throw new ArgumentException("OnEventAction method must be static otherwise the " +
                                            "event WeakEventListner class does not prevent memory leaks.");

            _onEventAction = value;
        }
    }

LukasFellechner wrote Jul 30, 2015 at 4:16 PM

I had a similar check in my WeakEventListener implementation, which started to throw with Roslyn. I have commented out the check for now.

But there is a downside to this: The check has a purpose. It is there to make sure that I do not accidentially pass instance methods to the WeakEventHandler, causing memory leaks. This can happen easily when using lambdas and not being careful. So the check was good to be in place.

With Roslyn it is not possible anymore to do this check, because you do not know if it is a "real" instance method or just a "fake" instance method created by Roslyn.

tibel wrote Sep 6, 2015 at 6:38 AM

There is a similar discussion on GitHub
https://github.com/dotnet/roslyn/issues/4793