Convert Array.IsSimpleCopy and CanAssignArray type to managed

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them

[huoyaoyuan]

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

privateobject[]objArray_Int={123};privateobject[]objArray_Str={"abc"};privateint[]intArray={123};privateuint[]uintArray={123};privatebyte[]byteArray={1};privatestring[]strArray={"abc"};privateint*[]intPointerArray={(int*)0x1234};privateuint*[]uintPointerArray={(uint*)0x1234};[Benchmark]publicvoidSimpleCopy_Derived()=>Array.Copy(strArray,objArray_Str,1);[Benchmark]publicvoidMustCast()=>Array.Copy(objArray_Str,strArray,1);[Benchmark]publicvoidSimpleCopy_Primitive()=>Array.Copy(intArray,uintArray,1);[Benchmark]publicvoidPrimitiveWiden()=>Array.Copy(byteArray,intArray,1);[Benchmark]publicvoidSimpleCopy_Pointer()=>Array.Copy(intPointerArray,uintPointerArray,1);

Result:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
SimpleCopy_Derived	Job-RIAPVG	\PR\corerun.exe	10.092 ns	0.0322 ns	0.0269 ns	0.87
SimpleCopy_Derived	Job-PSZNKI	\main\corerun.exe	11.586 ns	0.1421 ns	0.1329 ns	1.00
MustCast	Job-RIAPVG	\PR\corerun.exe	10.382 ns	0.0319 ns	0.0282 ns	0.43
MustCast	Job-PSZNKI	\main\corerun.exe	23.951 ns	0.1351 ns	0.1198 ns	1.00
SimpleCopy_Primitive	Job-RIAPVG	\PR\corerun.exe	7.974 ns	0.0679 ns	0.0635 ns	0.69
SimpleCopy_Primitive	Job-PSZNKI	\main\corerun.exe	11.608 ns	0.0954 ns	0.0893 ns	1.00
PrimitiveWiden	Job-RIAPVG	\PR\corerun.exe	12.496 ns	0.0779 ns	0.0691 ns	0.44
PrimitiveWiden	Job-PSZNKI	\main\corerun.exe	28.334 ns	0.5434 ns	0.5083 ns	1.00
SimpleCopy_Pointer	Job-RIAPVG	\PR\corerun.exe	8.105 ns	0.1383 ns	0.1226 ns	0.80
SimpleCopy_Pointer	Job-PSZNKI	\main\corerun.exe	10.155 ns	0.0799 ns	0.0747 ns	1.00

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[huoyaoyuan]

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.

[jkotas]

Could you please take a look at the Mono test failures?

[jkotas]

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

[jkotas]

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

[huoyaoyuan]

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

[jkotas]

Yes (it is fine to do it in a follow up PR).

[huoyaoyuan]

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

[jkotas]

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

[jkotas]

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

[huoyaoyuan]

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

[huoyaoyuan]

Could you please take a look at the Mono test failures?

Yes, I expect the same pointer case should also be disabled for mono.
Well it may be because mono doesn't support compatible pointer case. I don't think enabling it on mono is worthy.

[jkotas]

Could you please open an issue on this and disable the test against this issue?

[huoyaoyuan]

Is it something we'd ever want to support on mono?

[jkotas]

We do not want to have behavior differences between different runtime. Every behavior difference between runtime is a bug. We may choose to not fix some of these bugs, but I do not see a good reason for it here.

For example, similar Mono-specific issue in casting logic was fixed just a few days ago: #103841

[huoyaoyuan]

Currently the behavior is different among all three runtimes. NativeAOT allows conversion between any pointers.

The current coreclr behavior is really complex to support. Can we make a breaking change instead to support only exactly same pointers?

[jkotas]

The current coreclr behavior is really complex to support.

Why is it complex to support?

Can we make a breaking change instead to support only exactly same pointers?

I do not see how we would justify this breaking change.

[huoyaoyuan]

Opened #104197 for mono.

[huoyaoyuan]

Anything remaining for this?

[jkotas]

Cleaning up the cache lookups #104103 (comment) ?

[jkotas]

Suggested change

	extern"C"BOOLQCALLTYPETypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern"C"BOOLQCALLTYPETypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

I think this would be a better name for the QCall to make it clear what it does. Matches convention used for cast helpers (ChkCastAny_NoCacheLookup, etc.)

[jkotas]

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

[huoyaoyuan]

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

[jkotas]

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.

[jkotas]

Thank you!

[jkotas]

/ba-g All failures have known issues opened for them. I am not able to tell why BA is not able to match them