Convert Array.IsSimpleCopy and CanAssignArray type to managed by huoyaoyuan · Pull Request #104103 · dotnet/runtime

huoyaoyuan · 2024-06-27T15:55:40Z

Move the two routines back again.

Removes a HELPER_METHOD_FRAME and improves performance for deciding the path.

Benchmark code:

Details

        private object[] objArray_Int = { 123 };
        private object[] objArray_Str = { "abc" };
        private int[] intArray = { 123 };
        private uint[] uintArray = { 123 };
        private byte[] byteArray = { 1 };
        private string[] strArray = { "abc" };
        private int*[] intPointerArray = { (int*)0x1234 };
        private uint*[] uintPointerArray = { (uint*)0x1234 };

        [Benchmark]
        public void SimpleCopy_Derived() => Array.Copy(strArray, objArray_Str, 1);

        [Benchmark]
        public void MustCast() => Array.Copy(objArray_Str, strArray, 1);

        [Benchmark]
        public void SimpleCopy_Primitive() => Array.Copy(intArray, uintArray, 1);

        [Benchmark]
        public void PrimitiveWiden() => Array.Copy(byteArray, intArray, 1);

        [Benchmark]
        public void SimpleCopy_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

This reverts commit 4ce4f51.

dotnet-policy-service · 2024-06-27T15:56:41Z

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

huoyaoyuan · 2024-06-27T15:56:53Z

src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

+            {
+                // Only pointers are valid for TypeDesc in array element
+
+                // Compatible pointers
+                if (srcTH.CanCastTo(destTH))
+                    return ArrayAssignType.SimpleCopy;
+            }


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

jkotas · 2024-06-27T18:40:59Z

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

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs

jkotas · 2024-06-28T00:11:08Z

...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

+            if (result != CastResult.MaybeCast)
+                return result == CastResult.CanCast;
+
+            return CanCastTo_NoCacheLookup(m_asTAddr, destTH.m_asTAddr);


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

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.

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.

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

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.

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

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

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

huoyaoyuan · 2024-06-28T02:58:06Z

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 · 2024-06-28T14:59:35Z

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs

        }

+        [Fact]
+        [SkipOnMono("Mono does not support pointer compatibility in Array.Copy")]


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

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

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

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?

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.

Opened #104197 for mono.

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs

huoyaoyuan · 2024-07-06T12:48:57Z

Anything remaining for this?

jkotas · 2024-07-06T13:27:54Z

Cleaning up the cache lookups #104103 (comment) ?

jkotas · 2024-07-06T22:19:31Z

src/coreclr/vm/comutilnative.cpp

    return bResult;
 }

+extern "C" BOOL QCALLTYPE TypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)


Suggested change

extern "C" BOOL QCALLTYPE TypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)

extern "C" BOOL QCALLTYPE TypeHandle_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 · 2024-07-06T22:38:27Z

...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

+        public bool CanCastTo(TypeHandle destTH)
+        {
+            if (m_asTAddr == destTH.m_asTAddr)
+                return true;


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.

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?

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 · 2024-07-07T16:21:04Z

/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 added 5 commits June 27, 2024 19:30

Revert "Push back changes around IsSimpleCopy and CanAssignArrayType"

ea09fe2

This reverts commit 4ce4f51.

Reduce use range of CorElementType

afd067b

Rearrange TypeDesc and TypeHandle usage

057c04a

Fix

0770882

Add test for pointer array

0cd6e54

ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 27, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 27, 2024

huoyaoyuan added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 27, 2024

huoyaoyuan commented Jun 27, 2024

View reviewed changes

jkotas reviewed Jun 27, 2024

View reviewed changes

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 28, 2024

View reviewed changes

Rearrange test

dd93e95

jkotas reviewed Jun 28, 2024

View reviewed changes

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs Outdated Show resolved Hide resolved

This was referenced Jun 28, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Closed

[Mono AOT] Failed to compile System.Private.CoreLib.dll.bc #103520

Open

huoyaoyuan added 3 commits June 29, 2024 15:21

Remove comment in test

423c94b

NativeAot compat

0ef5370

Mono compat

4b6d830

huoyaoyuan requested review from MichalStrehovsky and marek-safar as code owners June 29, 2024 07:38

This was referenced Jun 29, 2024

[browser] System.Buffers.ArrayPool.Tests.ArrayPoolUnitTests fails with out of memory #101436

Closed

[browser][MT] WasmBrowserTestRunner.RunConsoleMessagesPump - abrupt disconect #101618

Closed

huoyaoyuan mentioned this pull request Jun 29, 2024

[mono] Support copying arrays of compatible pointers #104197

Closed

huoyaoyuan added 2 commits June 30, 2024 00:07

Disable test on mono

f8b8355

Try fix nativeaot

5f8783b

build-analysis bot mentioned this pull request Jun 29, 2024

[browser][MT] MONO_WASM: index out of bounds #101930

Closed

huoyaoyuan added 2 commits July 6, 2024 21:36

Merge branch 'main' into array-assign-type

88beba7

Don't lookup cast cache in QCall

f95296f

This was referenced Jul 6, 2024

LibraryImportGenerator.Unit.Tests crashing on linux-x64 mono interpreter #100800

Open

[x86] stress failure in RayTracer.GetNaturalColor with DOTNET_JitStress=2 #102590

Closed

jkotas reviewed Jul 6, 2024

View reviewed changes

huoyaoyuan added 2 commits July 7, 2024 09:59

Fix GC mode

d48f6f4

Update QCall name

4db934b

jkotas approved these changes Jul 7, 2024

View reviewed changes

jkotas mentioned this pull request Jul 7, 2024

[browser][MT] PhysicalFileProviderTests test failure #104523

Closed

build-analysis bot mentioned this pull request Jul 7, 2024

[wasm][mt] mono_wasm_load_runtime () failed RuntimeError: index out of bounds #104271

Open

jkotas mentioned this pull request Jul 7, 2024

[browser][MT] System.Net.Http.Functional.Tests failure ("Why are we setting the target on an unoccupied slot?") #104524

Open

jkotas merged commit 7b71281 into dotnet:main Jul 7, 2024

huoyaoyuan deleted the array-assign-type branch July 8, 2024 01:51

github-actions bot locked and limited conversation to collaborators Aug 7, 2024

	extern "C" BOOL QCALLTYPE TypeHandle_CanCastTo(void* fromTypeHnd, void* toTypeHnd)
	extern "C" BOOL QCALLTYPE TypeHandle_CanCastToNoCacheLookup(void* fromTypeHnd, void* toTypeHnd)

Conversation

huoyaoyuan commented Jun 27, 2024

Uh oh!

dotnet-policy-service bot commented Jun 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 27, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huoyaoyuan commented Jul 6, 2024

Uh oh!

jkotas commented Jul 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jkotas Jun 28, 2024 •

edited

Loading

jkotas Jun 29, 2024 •

edited

Loading

huoyaoyuan commented Jun 28, 2024 •

edited

Loading

jkotas Jul 7, 2024 •

edited

Loading