dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.13k stars 4.7k forks source link

JIT: obj?.GetType() == typeof(...) is not optimized to method table comparison #108355

Open DoctorKrolic opened 2 weeks ago

DoctorKrolic commented 2 weeks ago

Configuration

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;

var config = DefaultConfig.Instance
    .AddJob(
        Job.Default
            .WithEnvironmentVariable("DOTNET_TieredPGO", "0"));

BenchmarkRunner.Run<Benchmarks>(config);

[DisassemblyDiagnoser(syntax: DisassemblySyntax.Intel)]
public class Benchmarks
{
    private readonly object _secretObject = new MySealedClass();

    [Benchmark]
    public bool IsCheck() => _secretObject is MySealedClass;

    [Benchmark]
    public bool TypeOfCheck() => _secretObject?.GetType() == typeof(MySealedClass);
}

sealed class MySealedClass
{
}

Data

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4169/23H2/2023Update/SunValley3)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-rc.1.24452.12
  [Host]     : .NET 9.0.0 (9.0.24.43107), X64 RyuJIT AVX2
  Job-JQTUDK : .NET 9.0.0 (9.0.24.43107), X64 RyuJIT AVX2

EnvironmentVariables=DOTNET_TieredPGO=0
Method Mean Error StdDev Code Size
IsCheck 0.3223 ns 0.0030 ns 0.0027 ns 34 B
TypeOfCheck 1.1399 ns 0.0011 ns 0.0011 ns 46 B

Assembly:

; Benchmarks.IsCheck()
       mov       rax,[rcx+8]
       test      rax,rax
       je        short M00_L00
       mov       rcx,MT_MySealedClass
       cmp       [rax],rcx
       jne       short M00_L00
       mov       eax,1
       jmp       short M00_L01
M00_L00:
       xor       eax,eax
M00_L01:
       ret
; Total bytes of code 34
; Benchmarks.TypeOfCheck()
       sub       rsp,28
       mov       rcx,[rcx+8]
       test      rcx,rcx
       je        short M00_L00
       call      System.Object.GetType()
       jmp       short M00_L01
M00_L00:
       xor       eax,eax
M00_L01:
       mov       rcx,16380008908
       cmp       rax,rcx
       sete      al
       movzx     eax,al
       add       rsp,28
       ret
; Total bytes of code 46

Analysis

Adding a null check to a typeof via C# ?. operator seem to knock it off an optimized path (if there was no null check there wouldn't be a call System.Object.GetType()). Since these two methods do the same thing, the perf should be identical

dotnet-policy-service[bot] commented 2 weeks ago

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in area-owners.md if you want to be subscribed.

EgorBo commented 2 weeks ago

Your TypeOfCheck and IsCheckdo different things. TypeOfCheck throws NRE on null objects, I don't think it's correct to compare their perf.

DoctorKrolic commented 2 weeks ago

By adding 1 charachter to the input (namely ?) the typeof check became more than 3 times slower. I understand, that this is far less common to type check this way, but I still think the JIT should be able to put the null check first and then do the optimal type comparison

neon-sunset commented 2 weeks ago

Shouldn't .GetType be inlined here like it usually does without null-conditional operator?

e.g. ideally it could be the same as writing _secretObject != null && _secretObject.GetType() == typeof(MySealedClass);

; Benchmarks.TypeOfCheck()
       mov       rax,[rcx+8]
       test      rax,rax
       je        short M00_L00
       mov       rcx,MT_MySealedClass
       cmp       [rax],rcx
       sete      al
       movzx     eax,al
       ret
M00_L00:
       xor       eax,eax
       ret
; Total bytes of code 32
EgorBo commented 2 weeks ago

The problem that for:

o?.GetType() == typeof(...)

JIT ends up spilling o.GetType() to a temp (it does so because of basic-block boundary) - this ruins the "o.GetType() == typeof()" -> o->pMethodTalbe == clsHandle optimization. I think we should be able to fix it.

It doesn't happen for:

o != nullptr && o.GetType() == typeof(...)