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

Optimize comparison of unknown type with a typeof #31686

Closed
wants to merge 7 commits into from

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Feb 3, 2020

TODO:

  • Update JitInterface GUID. AFAIK we're waiting to batch a couple JitInterface changes so I didn't bother to avoid merge conflicts.
  • Go over System.Private.CoreLib and fix places like this one that are actually deoptimizations after this change.

The JIT currently optimizes System.Type comparisons that involve either typeof on both sides of the comparison, or typeof on one side and object.GetType on the other.

This adds handling for "anything" on one side and a typeof on the other side. The optimization removes the materialization of the System.Type instance from the typeof. This is done by calling a new helper that can compare a raw type handle with a System.Type instance.

Obligatory before/after:

class Program
{
    static Type s_Object = typeof(object);

    static int Main()
    {
        if (typeof(object) != s_Object)
            return 1;
        return 100;
    }
}

Before:

       56                   push     rsi
       4883EC20             sub      rsp, 32
       488B0D00000000       mov      rcx, qword ptr [(reloc)]
       FF1500000000         call     [CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE]
       488BF0               mov      rsi, rax
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       483B30               cmp      rsi, gword ptr [rax]
       740B                 je       SHORT G_M24376_IG05
       B801000000           mov      eax, 1
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
       B864000000           mov      eax, 100
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret

After:

       4883EC28             sub      rsp, 40
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       488B10               mov      rdx, gword ptr [rax]
       488B0D00000000       mov      rcx, qword ptr [(reloc)]
       FF1500000000         call     [CORINFO_HELP_ARE_TYPEHANDLE_AND_TYPE_EQUIVALENT]
       85C0                 test     eax, eax
       750A                 jne      SHORT G_M24376_IG05
       B801000000           mov      eax, 1
       4883C428             add      rsp, 40
       C3                   ret
       B864000000           mov      eax, 100
       4883C428             add      rsp, 40
       C3                   ret

@MichalStrehovsky MichalStrehovsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2020
@MichalStrehovsky MichalStrehovsky added this to the 5.0 milestone Feb 3, 2020
@MichalStrehovsky MichalStrehovsky force-pushed the typeOptimize branch 2 times, most recently from d589c61 to ccef2ae Compare February 4, 2020 13:18
@MichalStrehovsky
Copy link
Member Author

!CREATE_CHECK_STRING(!"Detected use of a corrupted OBJECTREF. Possible GC hole.")

Serves me right for trying to write an FCALL.

@jkotas is there any way to write JIT helpers in C#?

@jkotas
Copy link
Member

jkotas commented Feb 4, 2020

@jkotas is there any way to write JIT helpers in C#?

There is (like what Vladimir did recently for casting helpers), but I am not sure whether it is worth doing for this one at this point.

I have left one suggestion to make it match FCall coding conventions. If it is does not fix it, the problem is likely a bad codegen by RyuJIT.

The JIT currently optimizes `System.Type` comparisons that involve either `typeof` on both sides of the comparison, or `typeof` on one side and `object.GetType` on the other.

This adds handling for "anything" on one side and a `typeof` on the other side. The optimization removes the materialization of the `System.Type` instance from the `typeof`. This is done by calling a new helper that can compare a raw type handle with a `System.Type` instance.

Obligatory before/after:

```csharp
class Program
{
    static Type s_Object = typeof(object);

    static int Main()
    {
        if (typeof(object) != s_Object)
            return 1;
        return 100;
    }
}
```

Before:

```asm
       56                   push     rsi
       4883EC20             sub      rsp, 32
       488B0D00000000       mov      rcx, qword ptr [(reloc)]
       FF1500000000         call     [CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE]
       488BF0               mov      rsi, rax
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       483B30               cmp      rsi, gword ptr [rax]
       740B                 je       SHORT G_M24376_IG05
       B801000000           mov      eax, 1
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
       B864000000           mov      eax, 100
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
```

After:

```asm
       4883EC28             sub      rsp, 40
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       488B10               mov      rdx, gword ptr [rax]
       488B0D00000000       mov      rcx, qword ptr [(reloc)]
       FF1500000000         call     [CORINFO_HELP_ARE_TYPEHANDLE_AND_TYPE_EQUIVALENT]
       85C0                 test     eax, eax
       750A                 jne      SHORT G_M24376_IG05
       B801000000           mov      eax, 1
       4883C428             add      rsp, 40
       C3                   ret
       B864000000           mov      eax, 100
       4883C428             add      rsp, 40
       C3                   ret
```
@MichalStrehovsky
Copy link
Member Author

Okay, seems like this is finally passing. We're able to optimize comparisons in 145 methods within CoreLib with this, removing the need to allocate extra System.Type instances.

@dotnet/jit-contrib could you take a look please?

I recommend viewing the JIT change with whitespace disabled (append ?w=1 to the URL when viewing the diff on GitHub).

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jit changes look good.

@BruceForstall yet another GUID update in the works.

@BruceForstall
Copy link
Member

@MichalStrehovsky We're ready to take JIT-EE interface changes now (some internal processes dependent on the JIT-EE interface GUID are now stable).

Please wait until #2249 is merged. Then, either this or #32270 can come next. Make sure to update the GUID before merging.

@MichalStrehovsky
Copy link
Member Author

@jkotas could you please have a look at the non-JIT changes. This should be ready to go.

@@ -2325,7 +2325,6 @@ private static bool FilterApplyPrefixLookup(MemberInfo memberInfo, string name,
internal static readonly RuntimeType ValueType = (RuntimeType)typeof(System.ValueType);

private static readonly RuntimeType ObjectType = (RuntimeType)typeof(object);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectType doesn't seem to be used anywhere now, can it be removed like StringType was?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in RuntimeType.cs, the other part of this partial class. Removing that one would be a tiny deoptimization.

return value.ToString(provider);
if (ReferenceEquals(targetType, ConvertTypes[(int)TypeCode.Object]))
if (targetType == typeof(object))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to introduce performance regression:

using System;

class Test
{
    static void Main()
    {
        var o = (int)42; 
        int start = Environment.TickCount;
        for (int i = 0; i < 100000000; i++)
            Convert.ChangeType(o, typeof(double), null);
        int end = Environment.TickCount;
        Internal.Console.WriteLine((end - start).ToString());   
    }
}

Baseline: 2.2s With this change: 4.1s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether a better strategy would be to make typeof(...) faster by compiling it into de-reference of handle holding the object. The handle can be initialized lazily, similar to how we initialize dictionary entries lazily.

E.g. something like this:

mov rax, handle address
mov rax, [rax]
test  rax, rax
jne Done
mov rcx, MethodTable*
call CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
Done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well yeah, instead of comparing pointers, we now have to go to through a helper that is fast, but not as fast as comparing two pointers. It's a startup optimization, really. It would probably be within noise range had the cascading if in Convert been shorter, but it's not.

Redoing this into into handle is more work than I would be willing to sign up for.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
@MichalStrehovsky MichalStrehovsky deleted the typeOptimize branch January 29, 2024 07:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants