Skip to content

Calls to Interlocked.CompareExchange<T> prevents inlining #8662

Closed

Description

@redknightlois

This is the smaller code that showcase the issue. This code is working very hard to avoid cache issues but still have to pay for the call because it refuses to inline in CoreCLR 1.1 and CoreCLR 2.0 (1w old build).

I may be missing something, but dont see any reason why it shouldn't inline.

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            var p = new Program<object>();
            p.Test();

            Console.WriteLine("Hello World!");
        }
    }

    public class Program<T> where T : class, new()
    {
        [StructLayout(LayoutKind.Sequential, Size = 128)]
        private struct CacheAwareElement
        {
            private readonly long _pad1;
            private T Value;

            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            public static bool TryClaim(ref CacheAwareElement bucket, out T item)
            {
                // Note that the initial read is optimistically not synchronized. That is intentional. 
                // We will interlock only when we have a candidate. in a worst case we may miss some
                // recently returned objects. Not a big deal.
                T inst = bucket.Value;
                if (inst != null && inst == Interlocked.CompareExchange(ref bucket.Value, null, inst))
                    goto Done;

                item = null;
                return false;

                Done:
                item = inst;
                return true;
            }
        }

        private CacheAwareElement _element;

        [MethodImpl(MethodImplOptions.NoInlining)]
        public T Test()
        {
            T instance;
            CacheAwareElement.TryClaim(ref _element, out instance);
            return instance;
        }
    }
}

cc @AndyAyersMS

category:cq
theme:inlining
skill-level:expert
cost:medium

Activity

  1. mikedn commented on Jul 31, 2017

    @mikedn
    Contributor

    Inlining isn't blocked by CompareExchange but by runtime dictionary lookup that's needed in the generic code.

  2. redknightlois commented on Jul 31, 2017

    @redknightlois
    Author

    I would have never expect a dictionary lookup underlying Interlocked.CompareExchange when T is class and therefore known to be 8 bytes on 64 bits (or 4 bytes when 32bits architecture).

  3. mikedn commented on Jul 31, 2017

    @mikedn
    Contributor

    I would have never expect a dictionary lookup underlying Interlocked.CompareExchange when T is class and therefore known to be 8 bytes on 64 bits (or 4 bytes when 32bits architecture).

    It is exactly when T is a class that you do get runtime lookups, that's how shared generic code works. But yes, the JIT could probably try a bit harder in the specific case of Interlocked.CompareExchange<T>. Probably it could make it look like a call to Interlocked.CompareExchange(ref object, object object) so runtime lookup is not required.

    For example, if you change your code to not be generic you get the following code for Test:

    G_M2434_IG01:
           56                   push     rsi
           4883EC20             sub      rsp, 32
    G_M2434_IG02:
           4883C108             add      rcx, 8
           488B31               mov      rsi, gword ptr [rcx]
           4885F6               test     rsi, rsi
           740F                 je       SHORT G_M2434_IG03
           4C8BC6               mov      r8, rsi
           33D2                 xor      rdx, rdx
           E8C5B1505F           call     System.Threading.Interlocked:CompareExchange(byref,ref,ref):ref
           483BC6               cmp      rax, rsi
           7404                 je       SHORT G_M2434_IG04
    G_M2434_IG03:
           33C0                 xor      rax, rax
           EB03                 jmp      SHORT G_M2434_IG05
    G_M2434_IG04:
           488BC6               mov      rax, rsi
    G_M2434_IG05:
           4883C420             add      rsp, 32
           5E                   pop      rsi
           C3                   ret
  4. AndyAyersMS commented on Jul 31, 2017

    @AndyAyersMS
    Member

    The inlines done are:

    Inlines into 06000003 ConsoleApp1.Program`1[__Canon][System.__Canon]:Test():ref:this
      [0 IL=0008 TR=000006 06000005] [FAILED: runtime dictionary lookup] CacheAwareElement[__Canon][System.__Canon]:TryClaim(byref,byref):bool
    
    Inlines into 06000005 CacheAwareElement[__Canon][System.__Canon]:TryClaim(byref,byref):bool
      [1 IL=0037 TR=000037 06001E87] [below ALWAYS_INLINE size] System.Threading.Interlocked:CompareExchange(byref,ref,ref):ref
    

    You often can't inline from one shared generic class into another, so Program<T> and CacheAwareElement<T> are the culprits blocking the inline of TryClaim. The jit cannot foresee that if it did this inline and then the second-level inline of CompareExchange, the runtime lookups would not be required or would go unused.

    Making the inner class non-generic or instantiating the outer class over a value type (eg via the struct wrapped ref class trick if you really wanted a ref class all along) would likely unblock this case.

    From the second inline you can see that interlocked compare exchange is inlineable (and there's already a magic transformation to call the non-generic version thanks to IL intrinsics).

    The inlined version calls into native code so there's nothing more the jit can do here. Even if all the inlines were unblocked you would still see a call to do the compare exchange.

    I haven't verified this, and my understanding of this part of the runtime is sketchy, but the fact that TryClaim thinks it needs a runtime lookup to call CompareExchange may come from fact that the runtime's code-sharing analysis reflects the IL in the original method body for CompareExchange and not the less demanding IL intrinsic version. Since we don't ever expect the original body to be used it might be interesting to replace it with something more minimal and see if that also unblocks things.

  5. mikedn commented on Jul 31, 2017

    @mikedn
    Contributor

    Since we don't ever expect the original body to be used it might be interesting to replace it with something more minimal and see if that also unblocks things.

    Tried, same result.

  6. redknightlois commented on Jul 31, 2017

    @redknightlois
    Author

    No luck either.

  7. transferred this issue fromdotnet/coreclron Jan 31, 2020
  8. added this to the Future milestone on Jan 31, 2020
  9. AndyAyersMS commented on Nov 25, 2020

    @AndyAyersMS
    Member

    This is a runtime restriction so not much the jit can do here until there is runtime support.

    @davidwrighton has prototyped some changes that would unblock such cases (#31833).

  10. AndyAyersMS commented on Aug 16, 2023

    @AndyAyersMS
    Member

    Nothing much has changed here. @EgorBo weren't you working on some changes to remove the shared-shared inlining restrictions?

  11. EgorBo commented on Aug 16, 2023

    @EgorBo
    Member

    Nothing much has changed here. @EgorBo weren't you working on some changes to remove the shared-shared inlining restrictions?

    Perhaps, for .NET 9.0 🙂

  12. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Mar 5, 2024
  13. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Mar 5, 2024
  14. added
    in-prThere is an active PR which will close this issue when it is merged
    on Mar 9, 2024
  15. self-assigned this
    on Mar 9, 2024
  16. modified the milestones: Future, 9.0.0 on Mar 9, 2024
  17. locked and limited conversation to collaborators on Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIenhancementProduct code improvement that does NOT require public API changes/additionsin-prThere is an active PR which will close this issue when it is mergedoptimizationtenet-performancePerformance related issue

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions