Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: optimize case where box feeds GetType#13710

Merged
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:UpdateTypeEqualityOptimization
Sep 12, 2017
Merged

JIT: optimize case where box feeds GetType#13710
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:UpdateTypeEqualityOptimization

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

If the only use of a box is in a call to Type:GetType, we can remove the
box and just construct the type directly.

Also add some logging to the type optimizations done in morph.

Closes #13187.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

Jit-diffs results

Total bytes of diff: -3104 (-0.02 % of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
       -3104 : System.Private.CoreLib.dasm (-0.09 % of base)

1 total files with size differences (1 improved, 0 regressed), 78 unchanged.

Top method improvements by size (bytes):
        -120 : System.Private.CoreLib.dasm - System.ValueTuple`8[__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,ValueTuple`8][System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`8[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`7[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon]]]:System.IComparable.CompareTo(ref):int:this
        -115 : System.Private.CoreLib.dasm - System.ValueTuple`8[__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,ValueTuple`8][System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`8[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`7[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon]]]:System.Collections.IStructuralComparable.CompareTo(ref,ref):int:this
        -115 : System.Private.CoreLib.dasm - System.ValueTuple`8[__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,ValueTuple`8][System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`8[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`6[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon]]]:System.IComparable.CompareTo(ref):int:this
        -110 : System.Private.CoreLib.dasm - System.ValueTuple`8[__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,ValueTuple`8][System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`8[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`6[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon]]]:System.Collections.IStructuralComparable.CompareTo(ref,ref):int:this
        -110 : System.Private.CoreLib.dasm - System.ValueTuple`8[__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,__Canon,ValueTuple`8][System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`8[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.ValueTuple`5[System.__Canon,System.__Canon,System.__Canon,System.__Canon,System.__Canon]]]:System.IComparable.CompareTo(ref):int:this

46 total methods with size differences (46 improved, 0 regressed), 78402 unchanged.

Copy link
Copy Markdown

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

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

Looks good. I'm curious why not do the same for non-box trees that we can find an exact type for and know are non-null... what are the relative costs of GetType() and TypeHandleToRuntimeType()? Also, would doing the rewrite potentially open further optimizations (like if we go on to compare the type...)?

@AndyAyersMS
Copy link
Copy Markdown
Member Author

The one test failure looks like a legitimate issue with this change. In shared contexts the class handle may be a representative type and not the exact type. In other words we end up creating

MyGenStruct1`1[System.__Canon]

instead of

MyGenStruct1`1[System.Collections.Generic.Dictionary`2[MyClass0,System.Int32]]

So this needs some revision; likely we need to repurpose the handle that is passed to the newobj helper instead of trying to create one from scratch.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Will rework this once #13748 is in.

If the only use of a box is in a call to Type:GetType, remove the box and
obtain the type directly. Get the handle needed for obtaining the type from
the newobj call in the original box, via a new box removal option.

Also add some logging to the type optimizations done in morph.

Closes #13187.
@AndyAyersMS AndyAyersMS force-pushed the UpdateTypeEqualityOptimization branch from e61f846 to 2e78bfc Compare September 12, 2017 03:20
@AndyAyersMS
Copy link
Copy Markdown
Member Author

Reworked this a fair amount.... jit-diffs very similar to the above, but should avoid the bug in the earlier version.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Windows x64 debug timed out in tailcall_v4/hijack. It passes locally, so will retry.

@dotnet-bot retest Windows_NT x64 Debug Build and Test

Copy link
Copy Markdown

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Ah, I don't need the changes in value numbering any more. No point introducing dead code.

Let me snip that bit out.

@AndyAyersMS AndyAyersMS merged commit 81adc9d into dotnet:master Sep 12, 2017
@AndyAyersMS AndyAyersMS deleted the UpdateTypeEqualityOptimization branch September 12, 2017 22:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants