Lock contention on highly concurrent use of MethodBase.GetMethod

[steveharter]

Reported as a .NET Framework issue and found with Entity Framework.

The same lock (through Monitor.Enter) occurs in .NET Core, so created this issue to track a fix in .NET Core first, then perhaps port to .NET Framework pending outcome and priority.

A potential fix was suggested: The corresponding methods in MethodBase.GetProperties has a similar cache which is implemented slightly differently. In particular the method returns without trying to add the result to the cache, if the result originated from the cache. The approach should also be used in GetMethod.

cc @GrabYourPitchforks @tarekgh

[botinko]

We faced exactly the same problem on our production servers! There was ~900 threads, waiting for :

000000CA025FD688 00007ff8d0296594 [HelperMethodFrame_1OBJ: 000000ca025fd688] System.Threading.Monitor.ReliableEnter(System.Object, Boolean ByRef)
000000CA025FD7E0 00007ff84a8914e2 System.RuntimeType+RuntimeTypeCache+MemberInfoCache`1[[System.__Canon, System.Private.CoreLib]].Insert(System.__Canon[] ByRef, System.String, MemberListType)
000000CA025FD850 00007ff84ac031e6 System.RuntimeType+RuntimeTypeCache+MemberInfoCache`1[[System.__Canon, System.Private.CoreLib]].AddMethod(System.RuntimeType, System.RuntimeMethodHandleInternal, CacheType)
000000CA025FD8D0 00007ff84ac02eb3 System.RuntimeType.GetMethodBase(System.RuntimeType, System.RuntimeMethodHandleInternal)
000000CA025FD9B0 00007ff84b956b6c System.RuntimeType.GetMethodBase(System.RuntimeType, System.IRuntimeMethodInfo)
000000CA025FD9F0 00007ff84d3fa839 System.Reflection.RuntimeMethodInfo.GetGenericMethodDefinition()

[botinko]

I have made a very simple test that shows that the problem is quite serious in the following scenario. For any code that uses expressions with closure C# compiler generates a call to FieldInfo.GetFieldFromHandle on each field of the closure, which internally calls System.RuntimeType+RuntimeTypeCache+MemberInfoCache``1.Insert, which in turn has a lock.

    public class Test
    {
        public void ExprTest()
        {
            int x = 1;
            Foo(() => x);
        }

        private void Foo(Expression<Func<int>> expr)
        {
        }
    }

Turns into

  public class Test
  {
    public void ExprTest()
    {
      Test.c__DisplayClass0_0 cDisplayClass00 = new Test.c__DisplayClass0_0();
      cDisplayClass00.x = 1;
      this.Foo(Expression.Lambda<Func<int>>((Expression) Expression.Field((Expression) Expression.Constant((object) cDisplayClass00, typeof (Test.c__DisplayClass0_0)), FieldInfo.GetFieldFromHandle(__fieldref (Test.c__DisplayClass0_0.x))), Array.Empty<ParameterExpression>()));
    }

    private void Foo(Expression<Func<int>> expr)
    {
    }

    public Test()
    {
      base.ctor();
    }

    [CompilerGenerated]
    private sealed class c__DisplayClass0_0
    {
      public int x;

      public c__DisplayClass0_0()
      {
        base.ctor();
      }
    }
  }

Problematic callstack:

system.private.corelib.dll!System.RuntimeType+RuntimeTypeCache+MemberInfoCache`1[System.__Canon].Insert
system.private.corelib.dll!System.RuntimeType+RuntimeTypeCache+MemberInfoCache`1[System.__Canon].AddField
system.private.corelib.dll!System.RuntimeType.GetFieldInfo
system.private.corelib.dll!System.Reflection.FieldInfo.GetFieldFromHandle

Each field in the closure generates a call to FieldInfo.GetFieldFromHandle. Unfortunatelly, this can become a problem and even lead to a lock convoy problem. ORM solutions use expressions very widely, so many may encounter this problem.

Test is demonstrating lock contention problem:

class Program
    {
        static void Main(string[] args)
        {
            new Test().ExprTest();
            Console.WriteLine("End");
        }
    }

    public class Test
    {
        public void ExprTest()
        {
            Parallel.For(0, 100000, new ParallelOptions()
            {
                MaxDegreeOfParallelism = 30,
            }, i =>
            {
                int a,b,d,e,f;
                a = b = d = e = f = 0;
                GetExpr(c => c + a + b + d + e + f);
            });
        }

        private void GetExpr(Expression<Func<int, int>> expr)
        {
        }
    }

And here is profiling results:

[botinko]

We actually caught a lock convoy on our production servers due to this issue. Despite the fact that the bottleneck is usually the query to the database, we still ran into this. The solutions so far are as follows:

1. Generate expressions manually
2. Use the Compiled Query Technique https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/ef/language-reference/compiled-queries-linq-to-entities

For our application, these solutions will require a huge refactoring and worsen the readability of the code. Moreover, the problem is not limited to ORM. Expressions with closures are widely used.

[GrabYourPitchforks]

@tarekgh Do you have any gut feel on whether this would be a candidate for backporting? I think you were involved in some other backporting conversations.

[tarekgh]

@GrabYourPitchforks do you mean backporting to the Full Framework? or previous versions of .NET Core?

This depend if there is a customer blocked by that and there is no easy workaround can be applied. from what I am reading this is not meeting the servicing bar. we should fix it in 5.0 and if we get any complaint in the future we can back port at that time.

[GrabYourPitchforks]

Should've clarified - Full Framework servicing. Thanks for the tip. Let's try this out in 5.0 for now and see how things pan out.

[GrabYourPitchforks]

Consider port to 3.1: https://github.com/dotnet/coreclr/pull/28201