Optimize typeof(T1).IsAssignableFrom(typeof(T2)) #1195

EgorBo · 2019-12-27T23:57:16Z

Optimize typeof(T1).IsAssignableFrom(typeof(T2)) to true/false. E.g.

typeof(IEnumerable<object>).IsAssignableFrom(typeof(string[])); // to `true`

compareTypesForCast seems does everything I need for IsAssignableFrom: handles COMObjects, __Canon, covariance/contravariance, etc. The only thing - it gives up on Nullable<>.

Contributes to https://github.com/dotnet/coreclr/issues/2591

src/coreclr/src/jit/compiler.h

EgorBo · 2019-12-28T00:02:16Z

Didn't expect this (current behavior):

typeof(SimpleEnum_int).IsAssignableFrom(typeof(SimpleEnum_uint)); // False
typeof(SimpleEnum_int[]).IsAssignableFrom(typeof(SimpleEnum_uint[])); // True

AndyAyersMS

Would also be interesting to run diffs, and perhaps see what percentage of IsAssignableFrom get optimized.

src/coreclr/src/jit/importer.cpp

AndyAyersMS · 2019-12-28T00:38:51Z

src/coreclr/src/jit/importer.cpp

+
+                if (typeTo->IsCall() && typeFrom->IsCall())
+                {
+                    // make sure both arguments are `typeof()`


Do we know what typical uses of IsAssignableFrom look like? Are they usually via typeof or does GetType show up with any frequency?

Good point, I started with typeof()+typeof() but it seems it makes sense to handle more cases (e.g. a.GetType().IsAssignableFrom(b.GetType())) will analyze usages in popular projects.

This is the scenario I was thinking about
https://github.com/dotnet/coreclr/issues/2591#issuecomment-170209816

Currently this code handles two cases:

bool a = typeof(T1).IsAssignableFrom(typeof(T2)); bool a = variable.GetType().IsAssignableFrom(typeof(T2)); // for ValueType variables

src/coreclr/src/jit/importer.cpp

src/coreclr/tests/src/JIT/Intrinsics/TypeIntrinsics.IsAssignableFrom.cs

jkotas · 2019-12-28T01:02:16Z

handles COMObjects,

It handles COMObjects by giving up. It can do better than that.

the only thing - it gives up on Nullable<>

It may be a good idea to add an extra argument to compareTypesForCast that would tell it to use the reflection-like logic.

AndyAyersMS · 2020-01-25T22:13:16Z

@EgorBo any update on this?

…edfrom # Conflicts: # src/coreclr/tests/src/JIT/Intrinsics/TypeIntrinsics.cs # src/libraries/System.Private.CoreLib/src/System/Type.Helpers.cs

EgorBo · 2020-02-01T11:49:29Z

@AndyAyersMS well, it already works. It just doesn't optimize IsAssignableFrom for cases when one of the types are Nullable<> or COMObject (see here) - it fallbacks to runtime check for those cases. @jkotas suggests to extends the JIT interface method compareTypesForCast with "use reflection" argument to handle them but it looks like it's not the best time to change the JIT interface, is it?

AndyAyersMS · 2020-02-01T16:45:16Z

not the best time to change the JIT interface

We can always come back and add that part later.

Can you run diffs?

EgorBo · 2020-02-01T18:51:18Z

@AndyAyersMS jit-diff: (-f --pmi)

Total bytes of diff: -10450 (-0.03% of base)
    diff is an improvement.

Top file improvements by size (bytes):
       -7272 : System.Data.Common.dasm (-0.48% of base)
       -2180 : System.ComponentModel.TypeConverter.dasm (-0.73% of base)
        -998 : System.Private.CoreLib.dasm (-0.02% of base)

3 total files with size differences (3 improved, 0 regressed), 106 unchanged.

Top method improvements by size (bytes):
       -1166 (-91.31% of base) : System.Data.Common.dasm - System.Data.EnumerableRowCollection`1[Byte][System.Byte]:GetLinqDataView():System.Data.LinqDataView:this
       -1166 (-91.31% of base) : System.Data.Common.dasm - System.Data.EnumerableRowCollection`1[Int16][System.Int16]:GetLinqDataView():System.Data.LinqDataView:this
       -1166 (-91.31% of base) : System.Data.Common.dasm - System.Data.EnumerableRowCollection`1[Int32][System.Int32]:GetLinqDataView():System.Data.LinqDataView:this
       -1166 (-91.31% of base) : System.Data.Common.dasm - System.Data.EnumerableRowCollection`1[Double][System.Double]:GetLinqDataView():System.Data.LinqDataView:this
       -1166 (-91.31% of base) : System.Data.Common.dasm - System.Data.EnumerableRowCollection`1[Vector`1][System.Numerics.Vector`1[System.Single]]:GetLinqDataView():System.Data.LinqDataView:this

Top method improvements by size (percentage):
        -311 (-92.56% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.BindingList`1[Double][System.Double]:Initialize():this
        -304 (-92.40% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.BindingList`1[Int64][System.Int64]:Initialize():this
        -303 (-92.38% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.BindingList`1[Byte][System.Byte]:Initialize():this
        -303 (-92.38% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.BindingList`1[Int16][System.Int16]:Initialize():this
        -303 (-92.38% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.BindingList`1[Int32][System.Int32]:Initialize():this

31 total methods with size differences (31 improved, 0 regressed), 262922 unchanged.

The diff comes from:

EnumerableRowCollection.GetLinqDataView

BindingList.Initialize

ArraySortHelper.CreateArraySortHelper

I think I saw some usages in aspnetcore and ef but not sure.

…edfrom

AndyAyersMS · 2020-02-01T19:21:03Z

Those look like valid diffs, but I would guess PMI is asking for some unusual instantiations and we may not see this optimization fire much in our normal testing.

Can you add in test cases for the primitive typed array assignments? Something like:

        IsTrue(typeof(byte[]).IsAssignableFrom(typeof(sbyte[])));
        IsTrue(typeof(sbyte[]).IsAssignableFrom(typeof(byte[])));
        IsTrue(typeof(short[]).IsAssignableFrom(typeof(ushort[])));
        IsTrue(typeof(ushort[]).IsAssignableFrom(typeof(short[])));
        IsTrue(typeof(int[]).IsAssignableFrom(typeof(uint[])));
        IsTrue(typeof(uint[]).IsAssignableFrom(typeof(int[])));
        IsTrue(typeof(long[]).IsAssignableFrom(typeof(ulong[])));
        IsTrue(typeof(ulong[]).IsAssignableFrom(typeof(long[])));

        IsFalse(typeof(int[]).IsAssignableFrom(typeof(byte[])));
        IsFalse(typeof(int[]).IsAssignableFrom(typeof(sbyte[])));
        IsFalse(typeof(int[]).IsAssignableFrom(typeof(short[])));
        IsFalse(typeof(int[]).IsAssignableFrom(typeof(ushort[])));
        IsFalse(typeof(int[]).IsAssignableFrom(typeof(float[])));
        IsFalse(typeof(int[]).IsAssignableFrom(typeof(double[])));

        IsFalse(typeof(long[]).IsAssignableFrom(typeof(double[])));

AndyAyersMS

This looks good.

Thanks!

jkotas · 2020-02-03T03:41:12Z

This was merged with test failures that were most likely introduced by this change. Reverting in #31643

AndyAyersMS · 2020-02-03T08:19:29Z

Sorry about that. For some reason I thought those were pre-existing failures.

AndyAyersMS · 2020-02-03T17:13:07Z

Failing case is asking about assigning __Canon to INotifyPropertyChanged, code in this PR decides this is not possible. This is an issue with the canCastTo logic, specifically this clause that tries to return some MustNot results when casting from a shared type.

runtime/src/coreclr/src/vm/jitinterface.cpp

Lines 4502 to 4510 in 6000488

    
           // However, CanCastTo will report failure in such cases since 
        
           // __Canon won't match the instantiated type on the 
        
           // interface (which can't be __Canon since we screened out 
        
           // canonical subtypes for toClass above). So only report 
        
           // failure if the interface is not instantiated. 
        
           else if (!toHnd.HasInstantiation()) 
        
           { 
        
               result = TypeCompareState::MustNot; 
        
           }

Existing callers avoid drawing the wrong conclusions from this; they either have more constrained inputs or are more cautious with MustNot results.

Fix is either to refine the clause or just drop it all together. Will investigate.

We weren't careful enough with `__Canon` in some cases, which lead to unsafely returning `MustNot` when the cast outcome was not determined at jit time. Add an extra check, update comments, and add some test cases. Addresses the failures seen in dotnet#1195 (which was reverted).

We weren't careful enough with `__Canon` in some cases, which lead to unsafely returning `MustNot` when the cast outcome was not determined at jit time. Add an extra check, update comments, and add some test cases. Addresses the failures seen in #1195 (which was reverted).

AndyAyersMS · 2020-02-04T07:45:19Z

@EgorBo can you resubmit this PR?

AndyAyersMS · 2020-02-04T07:50:09Z

Ah, looks like Jan already did this in #31705.

Optimize Type.IsAssignableFrom

9818396

EgorBo commented Dec 27, 2019

View reviewed changes

src/coreclr/src/jit/compiler.h Outdated Show resolved Hide resolved

EgorBo added 2 commits December 28, 2019 03:06

Add TypeIntrinsics.IsAssignableFrom to TypeIntrinsics.csproj

4d15e44

Formatting

a7c741d

AndyAyersMS reviewed Dec 28, 2019

View reviewed changes

jkotas reviewed Dec 28, 2019

View reviewed changes

src/coreclr/tests/src/JIT/Intrinsics/TypeIntrinsics.IsAssignableFrom.cs Show resolved Hide resolved

jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 28, 2019

EgorBo added 3 commits December 28, 2019 15:17

Add License header

55fc28c

Remove assert

81efe86

fix "mustexpand assert"

ce60887

Merge branch 'master' of github.com:dotnet/runtime into type-isassign…

d4e9acd

…edfrom # Conflicts: # src/coreclr/tests/src/JIT/Intrinsics/TypeIntrinsics.cs # src/libraries/System.Private.CoreLib/src/System/Type.Helpers.cs

Merge branch 'master' of github.com:dotnet/runtime into type-isassign…

eed7c90

…edfrom

Add tests for Arrays of vt

78a0a3b

AndyAyersMS approved these changes Feb 1, 2020

View reviewed changes

AndyAyersMS merged commit 9582ecc into dotnet:master Feb 2, 2020

jkotas mentioned this pull request Feb 3, 2020

Revert "Optimize typeof(T1).IsAssignableFrom(typeof(T2))" #31643

Merged

AndyAyersMS mentioned this pull request Feb 3, 2020

Fix logic in compareTypesForCast #31681

Merged

EgorBo deleted the type-isassignedfrom branch May 25, 2020 11:57

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize typeof(T1).IsAssignableFrom(typeof(T2)) #1195

Optimize typeof(T1).IsAssignableFrom(typeof(T2)) #1195

EgorBo commented Dec 27, 2019 •

edited

Loading

EgorBo commented Dec 28, 2019

AndyAyersMS left a comment

AndyAyersMS Dec 28, 2019

EgorBo Dec 28, 2019 •

edited

Loading

omariom Dec 28, 2019

EgorBo Dec 28, 2019 •

edited

Loading

jkotas commented Dec 28, 2019 •

edited

Loading

AndyAyersMS commented Jan 25, 2020

EgorBo commented Feb 1, 2020 •

edited

Loading

AndyAyersMS commented Feb 1, 2020

EgorBo commented Feb 1, 2020 •

edited

Loading

AndyAyersMS commented Feb 1, 2020

AndyAyersMS left a comment

jkotas commented Feb 3, 2020

AndyAyersMS commented Feb 3, 2020

AndyAyersMS commented Feb 3, 2020

AndyAyersMS commented Feb 4, 2020

AndyAyersMS commented Feb 4, 2020

Optimize typeof(T1).IsAssignableFrom(typeof(T2)) #1195

Optimize typeof(T1).IsAssignableFrom(typeof(T2)) #1195

Conversation

EgorBo commented Dec 27, 2019 • edited Loading

EgorBo commented Dec 28, 2019

AndyAyersMS left a comment

Choose a reason for hiding this comment

AndyAyersMS Dec 28, 2019

Choose a reason for hiding this comment

EgorBo Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

omariom Dec 28, 2019

Choose a reason for hiding this comment

EgorBo Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas commented Dec 28, 2019 • edited Loading

AndyAyersMS commented Jan 25, 2020

EgorBo commented Feb 1, 2020 • edited Loading

AndyAyersMS commented Feb 1, 2020

EgorBo commented Feb 1, 2020 • edited Loading

AndyAyersMS commented Feb 1, 2020

AndyAyersMS left a comment

Choose a reason for hiding this comment

jkotas commented Feb 3, 2020

AndyAyersMS commented Feb 3, 2020

AndyAyersMS commented Feb 3, 2020

AndyAyersMS commented Feb 4, 2020

AndyAyersMS commented Feb 4, 2020

EgorBo commented Dec 27, 2019 •

edited

Loading

EgorBo Dec 28, 2019 •

edited

Loading

EgorBo Dec 28, 2019 •

edited

Loading

jkotas commented Dec 28, 2019 •

edited

Loading

EgorBo commented Feb 1, 2020 •

edited

Loading

EgorBo commented Feb 1, 2020 •

edited

Loading