Skip to content
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

Type.IsAssignableTo #40326

Merged
merged 9 commits into from
Aug 11, 2020
Merged

Type.IsAssignableTo #40326

merged 9 commits into from
Aug 11, 2020

Conversation

benaadams
Copy link
Member

Resolves #30714

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@EgorBo
Copy link
Member

EgorBo commented Aug 4, 2020

I assume

static bool Test<T1, T2>() => typeof(T1).IsAssignableTo(typeof(T2));

is optimized to true/false for known types (except Nullable and shared generics) 🙂

@jkotas
Copy link
Member

jkotas commented Aug 4, 2020

is optimized to true/false for known types (except Nullable and shared generics)

That would require fixes in the JIT. I do not think the existing JIT optimization will be able to cut through ? :.

@EgorBo EgorBo closed this Aug 4, 2020
@EgorBo EgorBo reopened this Aug 4, 2020
@EgorBo
Copy link
Member

EgorBo commented Aug 4, 2020

Oops. sorry 🙁

@akoeplinger
Copy link
Member

/cc @steveharter @GrabYourPitchforks as System.Reflection area owners

@jkotas
Copy link
Member

jkotas commented Aug 10, 2020

I think this should also expand the optimizations done for IsAssignableFrom to IsAssignableTo so that the new API is no-brainer to use and people do not have to think about trade-offs. It is hard to choose between two nearly identical APIs where one has better name and the other has better optimizations.

@EgorBo
Copy link
Member

EgorBo commented Aug 10, 2020

I think this should also expand the optimizations done for IsAssignableFrom to IsAssignableTo so that the new API is no-brainer to use and people do not have to think about trade-offs. It is hard to choose between two nearly identical APIs where one has better name and the other has better optimizations.

@benaadams feel free to ping if you need any help

@jkotas
Copy link
Member

jkotas commented Aug 10, 2020

Do we need test for the optimization, similar to src/tests/JIT/Intrinsics/TypeIntrinsics.IsAssignableFrom.cs ?

@jkotas
Copy link
Member

jkotas commented Aug 10, 2020

And also general functional tests for the API

@benaadams
Copy link
Member Author

That's a new error, don't think its from this change though

/ runtime (Libraries Build Linux x64 Debug)

Build log #L1971
Child node "4" exited prematurely. Shutting down. Diagnostic information may be found in files in "/tmp/" and will be named MSBuild_*.failure.txt. This location can be changed by setting the MSBUILDDEBUGPATH environment variable to a different directory.
/tmp/MSBuild_pid-2555_75cd8686c9e7462d87e5a643d9ee969f.failure.txt:
UNHANDLED EXCEPTIONS FROM PROCESS 2555:
=====================
08/10/2020 16:31:12
System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
   at Microsoft.Build.BackEnd.BinaryTranslator.BinaryWriteTranslator.TranslateDictionary[D,T](D& dictionary, ObjectTranslator`1 objectTranslator)
   at Microsoft.Build.Execution.ProjectItemInstance.TaskItem.Microsoft.Build.BackEnd.ITranslatable.Translate(ITranslator translator)
   at Microsoft.Build.BackEnd.BinaryTranslator.BinaryWriteTranslator.Translate[T](List`1& list, ObjectTranslator`1 objectTranslator)
   at Microsoft.Build.BackEnd.LogMessagePacket.TranslateTargetFinishedEvent(ITranslator translator, TargetFinishedEventArgs finishedEvent)
   at Microsoft.Build.Shared.LogMessagePacketBase.WriteToStream(ITranslator translator)
   at Microsoft.Build.BackEnd.NodeEndpointOutOfProcBase.RunReadLoop(Stream localReadPipe, Stream localWritePipe, ConcurrentQueue`1 localPacketQueue, AutoResetEvent localPacketAvailable, AutoResetEvent localTerminatePacketPump)
===================

@benaadams
Copy link
Member Author

Added test; seems unhappy with unrelated issue

      Process: 7628 dotnet PrivateMemory: 8100003840
      Timed out at 8/10/2020 5:56:34 PM after 60000ms waiting for remote process.
      Wrote mini dump to: C:\helix\work\workitem\uploads\7628.tpwrerhj.lnu.dmp
      	Process ID: 7628
      	Handle: 1000
      	Name: dotnet
      	MainModule: C:\helix\work\correlation\dotnet.exe
      	StartTime: 8/10/2020 5:55:34 PM
      	TotalProcessorTime: 00:01:00.1875000
      	Threads:
      		Thread #1 (OS 0x6A0)   [MTA]
      			[InlinedCallFrame] (System.Buffer.__Memmove)
      			[InlinedCallFrame] (System.Buffer.__Memmove)
      			System.Buffer._Memmove(Byte*, Byte*, UIntPtr)
      			System.String.wstrcpy(Char*, Char*, Int32)
      			System.Text.StringBuilder.ThreadSafeCopy(Char*, Char[], Int32, Int32)
      			System.Text.StringBuilder.Append(Char*, Int32)
      			System.Text.StringBuilder.Append(Char[])
      			System.Text.Tests.StringBuilderTests+<>c.<FailureOnLargeString>b__168_1()
      			System.AssertExtensions.ThrowsAnyInternal(System.Action, System.Type[])
      			System.AssertExtensions.ThrowsAny[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]](System.Action)
      			System.Text.Tests.StringBuilderTests+<>c.<FailureOnLargeString>b__168_0()
      			[DebuggerU2MCatchHandlerFrame]
      			[HelperMethodFrame_PROTECTOBJ] (System.RuntimeMethodHandle.InvokeMethod)
      			System.Reflection.MethodBase.Invoke(System.Object, System.Object[])
      			Microsoft.DotNet.RemoteExecutor.Program.Main(System.String[])
      		Thread #2 (OS 0xB98) [Finalizer] [Background] [MTA]
      			[DebuggerU2MCatchHandlerFrame]
      		Thread #3 (OS 0x2210) [Thread pool worker] [Background] [MTA]
      
      Stack Trace:
        /_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs(225,0): at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose(Boolean disposing)
        /_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs(58,0): at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose()
        /_/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs(2237,0): at System.Text.Tests.StringBuilderTests.FailureOnLargeString()
  Finished:    System.Runtime.Nls.Tests

@benaadams
Copy link
Member Author

runtime (CoreCLR Product Build OSX x64 checked) broke or got chopped

/ runtime (CoreCLR Product Build OSX x64 checked)

[ 77%] Building CXX object src/jit/standalone/CMakeFiles/clrjit.dir/__/lsra.cpp.o
[ 77%] Building CXX object src/vm/crossgen/CMakeFiles/cee_crossgen.dir/__/instmethhash.cpp.o
[ 77%] Building CXX object src/jit/protononjit/CMakeFiles/protononjit.dir/__/instr.cpp.o
[ 77%] Building CXX object src/vm/wks/CMakeFiles/cee_wks_core.dir/__/encee.cpp.o
[ 77%] Building CXX object src/jit/standalone/CMakeFiles/clrjit.dir/__/lsrabuild.cpp.o
##[error]The operation was canceled.
Finishing: Build CoreCLR Runtime

@benaadams
Copy link
Member Author

OSX Issue is already reported #34472

@jkotas
Copy link
Member

jkotas commented Aug 11, 2020

Thanks

@jkotas jkotas merged commit 7128331 into dotnet:master Aug 11, 2020
@benaadams benaadams deleted the IsAssignableTo branch August 11, 2020 00:33
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

API Proposal: Type.IsAssignableTo(Type)
8 participants