Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

huoyaoyuan · 2024-01-29T13:56:47Z

Contributes to #95695

huoyaoyuan · 2024-01-29T13:57:25Z

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

+            uint size = pMT->GetNumInstanceFieldBytes();
+            return SpanHelpers.SequenceEqual(
+                ref GetRawData(o1),
+                ref GetRawData(o2),
+                size);


I'm really not sure if this is GC safe for object field.

I presume it needs a nogc region for that

Right, this does not work. It needs to stay as FCall or QCall w/ SuppressGCTransition.

huoyaoyuan · 2024-01-29T14:01:49Z

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

+            // Throws NullReferenceException as expected
+            MethodTable* pMT = RuntimeHelpers.GetMethodTable(this);
+
+            Type? type = GetTypeIfExists(pMT);


Would reusing the same variable pessimize JIT? The value may be seen as address exposed.

This method is potentially sharable with GetTypeFromHandleUnsafe, but the unnecessary check for TypeDesc may hurt performance.

EgorBo · 2024-01-29T14:32:17Z

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

@@ -9,8 +10,14 @@ public partial class Object
    {
        // Returns a Type object which represent this object instance.
        [Intrinsic]
-        [MethodImpl(MethodImplOptions.InternalCall)]
-        public extern Type GetType();
+        public unsafe Type GetType()


First of all, it needs NoInlining on it. It's a jit intrinsic and if it gets inlined it might ruin some opts.
Also, it feels like it's potentially doing more work prior calling runtime helper - are there any benchmarks? Presumably, GetType() is perf critical. I was thinking of optimizing Object.GetType in jit here #88631

The helper is also doing unnecessary check for null and TypeDesc. With NoInling, the performance is likely to regress a lot.

It's not so clear about the desired approach - expanding in JIT or exposing more fields to managed code? Would GetHashCode also benefit from this?

The helper is also doing unnecessary check for null and TypeDesc. With NoInling, the performance is likely to regress a lot.

I don't think I follow, I meant NoInlining on top of GetType() you can check JIT codebase for NI_System_Object_GetType intrinsics - all those opts won't work if it gets inlined (JIT will have to special case GetTypeFromHandleUnsafe)

Would GetHashCode also benefit from this?

Object.GetHashCode can also be optimized on C# side like this #55273, it just needs a jit intrinsic to guarantee GC-safe access for object's header (exterior pointer). or can be expanded in JIT as well.

It's a jit intrinsic and if it gets inlined it might ruin some opts.

JIT intrinsics take precedence over inlining, no? Many JIT intrinsics are inlineable, but we do not have them marked as no inlining.

Also, we use similar managed implementation in native AOT. If this has a problem, the native AOT implementation has the same problem.

JIT intrinsics take precedence over inlining, no? Many JIT intrinsics are inlineable, but we do not have them marked as no inlining.

Nope, JIT is free to inline them. In 95% of cases it's not a problem because most intrinsic-related optimizations happen directly in importer prior inlining. There are only a few which are special and GetObject is one of those
E.g.

case NI_System_Array_Clone: case NI_System_Collections_Generic_Comparer_get_Default: case NI_System_Collections_Generic_EqualityComparer_get_Default: case NI_System_Object_MemberwiseClone: case NI_System_Threading_Thread_get_CurrentThread: case NI_System_Object_GetType: case NI_System_Text_UTF8Encoding_UTF8EncodingSealed_ReadUtf8: case NI_System_SpanHelpers_SequenceEqual: case NI_System_Buffer_Memmove:

Some of them are just too big to be inlined anyway, internal calls or it's just not a big deal if jit inlines them.

With AuxiliaryData being exposed in #97590, all the necessary information for fast path seems to be exposed to managed code. Non-collectible RuntimeType objects are now allocated on FOH so it will always be GC safe.

I'm concerned about the code being fragile and depend more implementation details like the FOH optimization. It's also hard to do optimization for collectible types.

Can we disable the inlining for these in the JIT instead if the JIT does not want them inlined?

Disabled inlining has secondary effect on code quality (forces stackwalkable frame, etc.). It will be hard to avoid perf regressions if the fast path has to be marked as disabled inlining.

Can we disable the inlining for these in the JIT instead if the JIT does not want them inlined?

Yeah, it can it be done

AustinWise · 2024-01-29T20:53:47Z

src/coreclr/classlibnative/bcltype/objectnative.cpp


-    FC_INNER_RETURN(INT32, GetHashCodeHelper(objRef));
+    END_QCALL;


Suggested change

END_QCALL;

END_QCALL;

return ret;

huoyaoyuan · 2024-02-22T08:22:31Z

I think it's better to leave GetType as-is and convert the easy ones first. Will do an performance measurement for GetHashCode.

huoyaoyuan · 2024-02-23T18:08:11Z

There is also non-trivial regression for GetHashCode:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio	RatioSD
GetHashCodeFast	Job-VYLGUH	\FCall\corerun.exe	3.151 ns	0.0345 ns	0.0323 ns	1.99	0.02
GetHashCodeFast	Job-APSUVV	\ManagedHeader\corerun.exe	10.712 ns	0.0876 ns	0.0819 ns	6.77	0.05
GetHashCodeFast	Job-RNEPRL	\main\corerun.exe	1.580 ns	0.0031 ns	0.0024 ns	1.00	0.00

GetHashCodeSyncBlock	Job-VYLGUH	\FCall\corerun.exe	3.822 ns	0.0100 ns	0.0094 ns	1.64	0.01
GetHashCodeSyncBlock	Job-APSUVV	\ManagedHeader\corerun.exe	12.829 ns	0.0649 ns	0.0575 ns	5.51	0.03
GetHashCodeSyncBlock	Job-RNEPRL	\main\corerun.exe	2.328 ns	0.0044 ns	0.0041 ns	1.00	0.00

Reverting GetHashCode changes too.

jkotas · 2024-02-23T22:22:32Z

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

+                return false;
+
+            // If it's not a value class, don't compare by value
+            if (!o1.GetType().IsValueType)


Is this going to end up calling reflection? It would be a good idea to check for perf regression.

Note that this FCALL does not have a helper method frame and your change is not deleting, just making it a bit smaller. FCALLs without a helper method frame are not something we are trying to actively eliminate.

I haven't checked if JIT treats this as intrinsic. Only typeof(T).IsValueType?

jkotas · 2024-02-24T15:33:06Z

Tests are crashing:

Assert failure(PID 24 [0x00000018], Thread: 24 [0x0018]): !"OBJECTREF being accessed while thread is in preemptive GC mode."
    File: /__w/1/s/src/coreclr/vm/object.cpp:487

jkotas · 2024-02-24T15:33:36Z

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

@@ -139,8 +139,32 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
        [MethodImpl(MethodImplOptions.InternalCall)]
        internal static extern int TryGetHashCode(object o);

+        public static new unsafe bool Equals(object? o1, object? o2)


We do not seem to have any test coverage for this method. Could you please add some?

I'm not sure how to express the mentioned issue about padding bits.

If we had any regular test coverage for this API, we would likely see the tests fail intermittently due to the padding issue.

I do not think you need to bother with adding a test that fails deterministically due the padding issue.

MichalPetryka · 2024-02-25T11:23:57Z

...em.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs

+            // Boolean RuntimeHelpers.Equals(Object, Object)
+
+            Assert.True(RuntimeHelpers.Equals(Guid.Empty, Guid.Empty));
+            Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.NewGuid()));


Suggested change

Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.NewGuid()));

Guid guid = Guid.NewGuid();

Assert.Equals(Guid.Empty == guid, RuntimeHelpers.Equals(Guid.Empty, guid));

In case we'd generate an empty GUID.

Guid.NewGuid won't return empty.
The real thing I'm concerned is that we are taking a dependency of the behavior/contract of NewGuid.

jkotas · 2024-02-25T14:23:29Z

...em.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs

+        [Fact]
+        public static void EqualsTest()
+        {
+            // Boolean RuntimeHelpers.Equals(Object, Object)


This should also cover comparing non-valuetypes and other paths that are special-cased in the implementation.

jkotas

Thank you!

huoyaoyuan added 4 commits January 29, 2024 19:00

Equals

d108638

GetHashCode

5c827a5

AllocateUninitializedClone

581bb9c

GetClass

d3b9dcc

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 29, 2024

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 29, 2024

huoyaoyuan commented Jan 29, 2024

View reviewed changes

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 Jan 29, 2024

huoyaoyuan commented Jan 29, 2024

View reviewed changes

Use GetTypeFromHandleUnsafe instead to reduce code duplication

62fa673

EgorBo reviewed Jan 29, 2024

View reviewed changes

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

[8.0] Failing to build native components #94823

Closed

AustinWise reviewed Jan 29, 2024

View reviewed changes

huoyaoyuan added 7 commits February 22, 2024 23:54

Merge branch 'main'

4cc5699

Revert change of GetType

6a56bd6

Merge diff for AllocateUninitializedClone

fb27520

Compare object content in native code

8c45bd3

Fix return

84ff6d1

Revert changes around GetHashCode

46e9b7c

Remove unused functions

1bac399

jkotas reviewed Feb 23, 2024

View reviewed changes

huoyaoyuan mentioned this pull request Feb 24, 2024

RuntimeHelpers.Equals incorrectly compares an extra four bytes on x64 #98875

Closed

Do not compare padding in Equals

e041418

jkotas reviewed Feb 24, 2024

View reviewed changes

huoyaoyuan added 2 commits February 25, 2024 16:37

Fix assert touching OBJECTREF in preemptive mode

e7bbdd4

Add test coverage for Equals

2d46bc0

MichalPetryka reviewed Feb 25, 2024

View reviewed changes

build-analysis bot mentioned this pull request Feb 25, 2024

[browser][MT] Assert failed: Cannot find Promise for JSHandle -2 #98406

Closed

jkotas reviewed Feb 25, 2024

View reviewed changes

huoyaoyuan added 2 commits February 26, 2024 22:27

Fix FCall method binding

633f525

Add more test cases

f2a0fc7

build-analysis bot mentioned this pull request Feb 26, 2024

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

jkotas approved these changes Feb 26, 2024

View reviewed changes

jkotas merged commit 5d14a8d into dotnet:main Feb 26, 2024
149 of 153 checks passed

huoyaoyuan deleted the object-native branch February 27, 2024 04:35

build-analysis bot mentioned this pull request Feb 28, 2024

profiler\\multiple\\multiple\\multiple.cmd failing on windows arm64 #98817

Closed

github-actions bot locked and limited conversation to collaborators Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

huoyaoyuan commented Jan 29, 2024

huoyaoyuan Jan 29, 2024

EgorBo Jan 29, 2024

jkotas Jan 29, 2024

huoyaoyuan Jan 29, 2024

huoyaoyuan Jan 29, 2024

EgorBo Jan 29, 2024 •

edited

Loading

huoyaoyuan Jan 29, 2024

EgorBo Jan 29, 2024 •

edited

Loading

jkotas Jan 29, 2024

jkotas Jan 29, 2024

EgorBo Jan 29, 2024

huoyaoyuan Jan 29, 2024

jkotas Jan 29, 2024

EgorBo Jan 29, 2024

AustinWise Jan 29, 2024

huoyaoyuan commented Feb 22, 2024

huoyaoyuan commented Feb 23, 2024

jkotas Feb 23, 2024

huoyaoyuan Feb 24, 2024

jkotas Feb 24, 2024

jkotas commented Feb 24, 2024

jkotas Feb 24, 2024

huoyaoyuan Feb 24, 2024

jkotas Feb 24, 2024

MichalPetryka Feb 25, 2024 •

edited

Loading

huoyaoyuan Feb 25, 2024

jkotas Feb 25, 2024

jkotas left a comment


		FC_INNER_RETURN(INT32, GetHashCodeHelper(objRef));
		END_QCALL;

	Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.NewGuid()));
	Guid guid = Guid.NewGuid();
	Assert.Equals(Guid.Empty == guid, RuntimeHelpers.Equals(Guid.Empty, guid));

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Conversation

huoyaoyuan commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huoyaoyuan commented Feb 22, 2024

huoyaoyuan commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Feb 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalPetryka Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

EgorBo Jan 29, 2024 •

edited

Loading

EgorBo Jan 29, 2024 •

edited

Loading

MichalPetryka Feb 25, 2024 •

edited

Loading