-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Optimize comparison of unknown type with a typeof #31686
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
Conversation
d589c61 to
ccef2ae
Compare
Serves me right for trying to write an FCALL. @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. |
70764fc to
dce102f
Compare
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
```
dce102f to
6996492
Compare
|
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 @dotnet/jit-contrib could you take a look please? I recommend viewing the JIT change with whitespace disabled (append |
There was a problem hiding this 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.
|
@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. |
|
@jkotas could you please have a look at the non-JIT changes. This should be ready to go. |
|
|
||
| internal static readonly RuntimeType ValueType = (RuntimeType)typeof(System.ValueType); | ||
|
|
||
| private static readonly RuntimeType ObjectType = (RuntimeType)typeof(object); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| if (targetType == typeof(string)) | ||
| return value.ToString(provider); | ||
| if (ReferenceEquals(targetType, ConvertTypes[(int)TypeCode.Object])) | ||
| if (targetType == typeof(object)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
TODO:
The JIT currently optimizes
System.Typecomparisons that involve eithertypeofon both sides of the comparison, ortypeofon one side andobject.GetTypeon the other.This adds handling for "anything" on one side and a
typeofon the other side. The optimization removes the materialization of theSystem.Typeinstance from thetypeof. This is done by calling a new helper that can compare a raw type handle with aSystem.Typeinstance.Obligatory before/after:
Before:
After: