-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641
Changes from 12 commits
d108638
5c827a5
581bb9c
d3b9dcc
62fa673
4cc5699
6a56bd6
fb27520
8c45bd3
84ff6d1
46e9b7c
1bac399
e041418
e7bbdd4
2d46bc0
633f525
f2a0fc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,8 +139,30 @@ 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) | ||
{ | ||
// Compare by ref for normal classes, by value for value types. | ||
|
||
if (ReferenceEquals(o1, o2)) | ||
return true; | ||
|
||
if (o1 is null || o2 is null) | ||
return false; | ||
|
||
// If it's not a value class, don't compare by value | ||
if (!o1.GetType().IsValueType) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't checked if JIT treats this as intrinsic. Only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right |
||
return false; | ||
|
||
// Make sure they are the same type. | ||
if (o1.GetType() != o2.GetType()) | ||
return false; | ||
|
||
// Compare the contents | ||
return SequenceEqualWithReferences(o1, o2); | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.InternalCall)] | ||
public static extern new bool Equals(object? o1, object? o2); | ||
private static extern unsafe bool SequenceEqualWithReferences(object o1, object o2); | ||
|
||
[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")] | ||
public static int OffsetToStringData | ||
|
@@ -194,8 +216,8 @@ public static object GetUninitializedObject( | |
return rt.GetUninitializedObject(); | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.InternalCall)] | ||
internal static extern object AllocateUninitializedClone(object obj); | ||
[LibraryImport(QCall, EntryPoint = "ObjectNative_AllocateUninitializedClone")] | ||
internal static partial void AllocateUninitializedClone(ObjectHandleOnStack objHandle); | ||
|
||
/// <returns>true if given type is reference type or value type that contains references</returns> | ||
[Intrinsic] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not seem to have any test coverage for this method. Could you please add some?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to express the mentioned issue about padding bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.