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

Improve codegen for delegate cast / type check? #55841

Closed
stephentoub opened this issue Jul 16, 2021 · 21 comments
Closed

Improve codegen for delegate cast / type check? #55841

stephentoub opened this issue Jul 16, 2021 · 21 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Jul 16, 2021

Consider:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Runtime.CompilerServices;

[DisassemblyDiagnoser]
[MemoryDiagnoser]
public class Program
{
    public static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private object _o = (Action)(() => { });

    [Benchmark] public void Cast() => M1(_o);
    [Benchmark] public void TypeCheck() => M2(_o);

    private void M1(object o)
    {
        if (o is Action action)
            action();
    }

    private void M2(object o)
    {
        if (o is not null && o.GetType() == typeof(Action))
            Unsafe.As<Action>(o)();
    }
}

On my machine, this results in:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated Code Size
Cast 1.1969 ns 0.0083 ns 0.0078 ns - - - - 43 B
TypeCheck 0.7173 ns 0.0064 ns 0.0060 ns - - - - 36 B

with the disassembly:

.NET 6.0.0 (42.42.42.42424), X64 RyuJIT

; Program.Cast()
       mov       rax,[rcx+8]
       test      rax,rax
       je        short M00_L00
       mov       rcx,offset MT_System.Action
       cmp       [rax],rcx
       je        short M00_L00
       xor       eax,eax
M00_L00:
       test      rax,rax
       je        short M00_L01
       mov       rcx,[rax+8]
       mov       rax,[rax+18]
       jmp       rax
M00_L01:
       ret
; Total bytes of code 43

.NET 6.0.0 (42.42.42.42424), X64 RyuJIT

; Program.TypeCheck()
       mov       rax,[rcx+8]
       test      rax,rax
       je        short M00_L00
       mov       rcx,offset MT_System.Action
       cmp       [rax],rcx
       jne       short M00_L00
       mov       rcx,[rax+8]
       mov       rax,[rax+18]
       jmp       rax
M00_L00:
       ret
; Total bytes of code 36

Is there anything we could do to eliminate the (small) gap / extra branch?

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Jul 16, 2021
@stephentoub
Copy link
Member Author

stephentoub commented Jul 16, 2021

Note the difference is larger when the delegate is a generic, involving a call to a cast helper in one case but not the other:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Runtime.CompilerServices;

[DisassemblyDiagnoser]
[MemoryDiagnoser]
public class Program
{
    public static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private object _o = (Action<object>)(_ => { });

    [Benchmark] public void Cast() => M1(_o);
    [Benchmark] public void TypeCheck() => M2(_o);

    private void M1(object o)
    {
        if (o is Action<object> action)
            action(o);
    }

    private void M2(object o)
    {
        if (o is not null && o.GetType() == typeof(Action<object>))
            Unsafe.As<Action<object>>(o)(o);
    }
}
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated Code Size
Cast 2.1657 ns 0.0181 ns 0.0161 ns - - - - 57 B
TypeCheck 0.9235 ns 0.0092 ns 0.0086 ns - - - - 39 B

.NET 6.0.0 (42.42.42.42424), X64 RyuJIT

; Program.Cast()
       push      rsi
       sub       rsp,20
       mov       rsi,[rcx+8]
       mov       rdx,rsi
       mov       rcx,offset MT_System.Action`1[[System.Object, System.Private.CoreLib]]
       call      CORINFO_HELP_ISINSTANCEOFARRAY
       test      rax,rax
       je        short M00_L00
       mov       rcx,[rax+8]
       mov       rdx,rsi
       mov       rax,[rax+18]
       add       rsp,20
       pop       rsi
       jmp       rax
M00_L00:
       add       rsp,20
       pop       rsi
       ret
; Total bytes of code 57

.NET 6.0.0 (42.42.42.42424), X64 RyuJIT

; Program.TypeCheck()
       mov       rdx,[rcx+8]
       test      rdx,rdx
       je        short M00_L00
       mov       rcx,offset MT_System.Action`1[[System.Object, System.Private.CoreLib]]
       cmp       [rdx],rcx
       jne       short M00_L00
       mov       rax,rdx
       mov       rcx,[rax+8]
       mov       rax,[rax+18]
       jmp       rax
M00_L00:
       ret
; Total bytes of code 39

@stephentoub
Copy link
Member Author

I'm also confused by (at least the naming of) the helper that's emitted here:

call      CORINFO_HELP_ISINSTANCEOFARRAY

given that we're testing for a delegate, not an array.

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

I'm also confused by (at least the naming of) the helper that's emitted here:

call      CORINFO_HELP_ISINSTANCEOFARRAY

given that we're testing for a delegate, not an array.

hm.. it shows IsInstanceOfAny here (same in .NET 6.0 Main locally).

@stephentoub
Copy link
Member Author

stephentoub commented Jul 16, 2021

hm.. it shows IsInstanceOfAny here (same in .NET 6.0 Main locally).

Interesting. I'm using main as well (as of yesterday). Maybe an artifact of the disassembler used by benchmarkdotnet?

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

hm.. it shows IsInstanceOfAny here (same in .NET 6.0 Main locally).

Interesting. I'm using main as well (as of yesterday). Maybe an artifact of the disassembler used by benchmarkdotnet?

Looks like, here is the asm using JitDisasm on top of most recent Main:
image

@stephentoub
Copy link
Member Author

Ok, cool, so ignore that part. Don't ignore the part where there's a helper here at all 😄

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

The first issue reminds me discussion in #47920 and looks like it could be handled via jump-threading? cc @AndyAyersMS
image

That edge from BB01 to BB04 (but a PHI is involved). V02 is V04 if it comes from BB01

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

@stephentoub regarding this case:

    private void M1(object o)
    {
        if (o is Action<object> action)
            action(o);
    }

    private void M2(object action)
    {
        if (action is object o && o.GetType() == typeof(Action<object>))
            Unsafe.As<Action<object>>(o)(o);
    }

I don't think it's a correct transformation, e.g.:

public static void Main(string[] args)
{
    Action<object> action = p => Console.WriteLine("Hello");
    M1(action); // prints Hello
    M2(action); // does not print anything
}

private static void M1(object o)
{
    if (o is Action<Program> action)
        action(null);
}

private static void M2(object action)
{
    if (action.GetType() == typeof(Action<Program>))
        Unsafe.As<Action<Program>>(action)(null);
}

but we still can optimize it to like

if (obj is Action<object> or call IsInstanceOfAny /*fallback*/)

via PGO + GDV #55325

@stephentoub
Copy link
Member Author

stephentoub commented Jul 16, 2021

I don't think it's a correct transformation

I was typing too quickly. It should have been:

        if (o is not null && o.GetType() == typeof(Action<object>))
            Unsafe.As<Action<object>>(o)(o);

I updated the posts. Doesn't change the issue, though.

@stephentoub
Copy link
Member Author

stephentoub commented Jul 16, 2021

or call IsInstanceOfAny

Why is IsInstanceOfAny required at all? The delegate is a sealed type. o is Action<object> should be identical to o is not null && o.GetType() == typeof(Action<object>), no?

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

or call IsInstanceOfAny

Why is IsInstanceOfAny required at all? The delegate is a sealed type. o is Action<object> should be identical to o is not null && o.GetType() == typeof(Action<object>), no?

The delegate is, but its T has in (contravariance)

namespace System
{
  public delegate void Action<in T>(T obj);
}

so you can cast Action<object> to Action<Program>

@stephentoub
Copy link
Member Author

The delegate is, but its T has in (contravariance)

Gaaahh, of course 😦

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2021

The delegate is, but its T has in (contravariance)

Gaaahh, of course 😦

it's not the first place where performance suffers from Covariance/Contravariance's existence 😄

@AndyAyersMS
Copy link
Member

could be handled via jump-threading?

In #49713 I started working on walking RBO back through PHIs but I hit some snags... also weren't you going to clean some of this up earlier?

@stephentoub
Copy link
Member Author

The delegate is, but its T has in (contravariance)

@EgorBo, given the explanation, why does this still emit a call to the helper?

using System;
public class C {
    public bool M(object o) => o is Action<int>;
}
C.M(System.Object)
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x7ffc06b1cb48
    L000e: call System.Runtime.CompilerServices.CastHelpers.IsInstanceOfAny(Void*, System.Object)
    L0013: test rax, rax
    L0016: setne al
    L0019: movzx eax, al
    L001c: add rsp, 0x28
    L0020: ret

@EgorBo
Copy link
Member

EgorBo commented Jul 17, 2021

could be handled via jump-threading?

In #49713 I started working on walking RBO back through PHIs but I hit some snags... also weren't you going to clean some of this up earlier?

Will try to revise that attempt (branch).

@EgorBo, given the explanation, why does this still emit a call to the helper?

From my understanding, we can optimize it when T is a value type like in your case (but not when it's a ref type even if it's sealed)

@stephentoub
Copy link
Member Author

stephentoub commented Jul 17, 2021

but not when it's a ref type even if it's sealed

Right, that's because it's contravariant, not covariant. So, remind me why the Action<object> as in my earlier example can't be optimized? The only thing that can be successfully cast to Action<D> is an Action<B> where B is a base of D... but nothing is the base of object.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7AHwAEAmARgFgAoIgZgAJT6Bhegb2vq8ZJM+45VuwxmQCcACilEyJADwRgAKxhgMAPgCUEgM70AvOvYBfTZvoBLPTPmKVarQG5+I0ZOmy5MgAxbdBozZTcytRW2VVDU1nIVc3KQkbOWY/PUMTM0trTztIpxd6Y2pjIA

@EgorBo
Copy link
Member

EgorBo commented Jul 17, 2021

So, remind me why the Action as in my earlier example can't be optimized?

Ah, "object" can also be optimized in case of contravariance 👍

PS: I don't understand how "quote" works in github's markdown :|

@AndyAyersMS
Copy link
Member

The jit only knows that some classes have variance, not any of the particulars (co- / contra- / both; which type vars can vary, etc).

So I'm not sure how we'd take advantage of this case; we might need new jit interface methods or find ways to pose this as a question that existing methods can answer.

@AndyAyersMS
Copy link
Member

I'm going to mark this as future; @EgorBo you can change if you think there's some low-risk way we could do this.

@AndyAyersMS AndyAyersMS added this to the Future milestone Jul 19, 2021
@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Jul 19, 2021
@jakobbotsch
Copy link
Member

This looks fixed, the codegen for M1 is now:

; Method C:M1(System.Object):this
G_M55036_IG01:
						;; size=0 bbWeight=1    PerfScore 0.00

G_M55036_IG02:
       test     rdx, rdx
       je       SHORT G_M55036_IG04
						;; size=5 bbWeight=1    PerfScore 1.25

G_M55036_IG03:
       mov      rcx, 0xD1FFAB1E      ; System.Action
       cmp      qword ptr [rdx], rcx
       je       SHORT G_M55036_IG05
						;; size=15 bbWeight=0.25 PerfScore 1.06

G_M55036_IG04:
       ret      
						;; size=1 bbWeight=0.50 PerfScore 0.50

G_M55036_IG05:
       mov      rcx, gword ptr [rdx+08H]
						;; size=4 bbWeight=0.50 PerfScore 1.00

G_M55036_IG06:
       tail.jmp [rdx+18H]System.Action:Invoke():this
						;; size=4 bbWeight=0.50 PerfScore 1.00
; Total bytes of code: 29

Presumably was fixed by #76283.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2023
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

No branches or pull requests

4 participants