Optimize typeof(T1).IsAssignableFrom(typeof(T2)) by EgorBo · Pull Request #1195 · dotnet/runtime (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

typeof(IEnumerable).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

Didn't expect this (current behavior):

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Currently this code handles two cases:

bool a = typeof(T1).IsAssignableFrom(typeof(T2));

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

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.

…edfrom

Conflicts:

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

src/libraries/System.Private.CoreLib/src/System/Type.Helpers.cs

@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?

not the best time to change the JIT interface

We can always come back and add that part later.

Can you run diffs?

@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.

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[])));

Choose a reason for hiding this comment

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

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

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

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.

// 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.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request

Feb 3, 2020

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).

jkotas pushed a commit that referenced this pull request

Feb 4, 2020

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).

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

EgorBo deleted the type-isassignedfrom branch

May 25, 2020 11:57

@ghost ghost locked as resolved and limited conversation to collaborators

Dec 11, 2020

Labels

area-CodeGen-coreclr

CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI