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.

Sign up for GitHub

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

Jump to bottom

Remove uses of FCThrow in interlocked helpers #96916

Merged
merged 2 commits into from
Jan 13, 2024
+115 −57
Merged

Remove uses of FCThrow in interlocked helpers #96916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ad8bd5
Remove uses of FCThrow in interlocked helpers
jkotas Jan 12, 2024
3e024ec
Stylecop
jkotas Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 101 additions & 11 deletions src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,60 @@ public static long Decrement(ref long location) =>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int Exchange(ref int location1, int value)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return Exchange(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
jkotas marked this conversation as resolved.
Show resolved Hide resolved
ThrowHelper.ThrowNullReferenceException();
return Exchange32(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int Exchange(ref int location1, int value);
private static extern int Exchange32(ref int location1, int value);

/// <summary>Sets a 64-bit signed integer to a specified value and returns the original value, as an atomic operation.</summary>
/// <param name="location1">The variable to set to the specified value.</param>
/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static long Exchange(ref long location1, long value)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return Exchange(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return Exchange64(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern long Exchange(ref long location1, long value);
private static extern long Exchange64(ref long location1, long value);

/// <summary>Sets an object to the specified value and returns a reference to the original object, as an atomic operation.</summary>
/// <param name="location1">The variable to set to the specified value.</param>
/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static extern object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value);
public static object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value)
{
if (Unsafe.IsNullRef(ref location1))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change moves the null check + throw from the C/C++ to C#. The JIT is smart enough to eliminate the null check in number of cases. I was not able to measure any actual perf improvement from that. The CAS operation dominates the perf and the extra null check is noise.

ThrowHelper.ThrowNullReferenceException();
return ExchangeObject(ref location1, value);
}

[return: NotNullIfNotNull(nameof(location1))]
[MethodImpl(MethodImplOptions.InternalCall)]
private static extern object? ExchangeObject([NotNullIfNotNull(nameof(value))] ref object? location1, object? value);

// The below whole method reduces to a single call to Exchange(ref object, object) but
// the JIT thinks that it will generate more native code than it actually does.
Expand All @@ -84,7 +117,7 @@ public static long Decrement(ref long location) =>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? =>
Unsafe.As<T>(Exchange(ref Unsafe.As<T, object?>(ref location1), value));
#endregion
#endregion

#region CompareExchange
/// <summary>Compares two 32-bit signed integers for equality and, if they are equal, replaces the first value.</summary>
Expand All @@ -94,8 +127,20 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int CompareExchange(ref int location1, int value, int comparand)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return CompareExchange(ref location1, value, comparand); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchange32(ref location1, value, comparand);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int CompareExchange(ref int location1, int value, int comparand);
private static extern int CompareExchange32(ref int location1, int value, int comparand);

/// <summary>Compares two 64-bit signed integers for equality and, if they are equal, replaces the first value.</summary>
/// <param name="location1">The destination, whose value is compared with <paramref name="comparand"/> and possibly replaced.</param>
Expand All @@ -104,8 +149,20 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static long CompareExchange(ref long location1, long value, long comparand)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return CompareExchange(ref location1, value, comparand); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchange64(ref location1, value, comparand);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern long CompareExchange(ref long location1, long value, long comparand);
private static extern long CompareExchange64(ref long location1, long value, long comparand);

/// <summary>Compares two objects for reference equality and, if they are equal, replaces the first object.</summary>
/// <param name="location1">The destination object that is compared by reference with <paramref name="comparand"/> and possibly replaced.</param>
Expand All @@ -114,9 +171,18 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static object? CompareExchange(ref object? location1, object? value, object? comparand)
{
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchangeObject(ref location1, value, comparand);
}

[MethodImpl(MethodImplOptions.InternalCall)]
[return: NotNullIfNotNull(nameof(location1))]
public static extern object? CompareExchange(ref object? location1, object? value, object? comparand);
private static extern object? CompareExchangeObject(ref object? location1, object? value, object? comparand);

// Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces
// the body of the following method with the following IL:
Expand All @@ -136,8 +202,8 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
/// <typeparam name="T">The type to be used for <paramref name="location1"/>, <paramref name="value"/>, and <paramref name="comparand"/>. This type must be a reference type.</typeparam>
[Intrinsic]
[return: NotNullIfNotNull(nameof(location1))]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static T CompareExchange<T>(ref T location1, T value, T comparand) where T : class? =>
Unsafe.As<T>(CompareExchange(ref Unsafe.As<T, object?>(ref location1), value, comparand));
#endregion
Expand All @@ -160,12 +226,36 @@ public static long Add(ref long location1, long value) =>
ExchangeAdd(ref location1, value) + value;

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int ExchangeAdd(ref int location1, int value)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return ExchangeAdd(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return ExchangeAdd32(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern int ExchangeAdd(ref int location1, int value);
private static extern int ExchangeAdd32(ref int location1, int value);

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static long ExchangeAdd(ref long location1, long value)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return ExchangeAdd(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return ExchangeAdd64(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern long ExchangeAdd(ref long location1, long value);
private static extern long ExchangeAdd64(ref long location1, long value);
#endregion

#region Read
Expand Down
36 changes: 2 additions & 34 deletions src/coreclr/vm/comutilnative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,14 +1490,10 @@ NOINLINE void GCInterface::GarbageCollectModeAny(int generation)

#include <optsmallperfcritical.h>

FCIMPL2(INT32,COMInterlocked::Exchange, INT32 *location, INT32 value)
FCIMPL2(INT32,COMInterlocked::Exchange32, INT32 *location, INT32 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchange((LONG *) location, value);
}
FCIMPLEND
Expand All @@ -1506,22 +1502,14 @@ FCIMPL2_IV(INT64,COMInterlocked::Exchange64, INT64 *location, INT64 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchange64((INT64 *) location, value);
}
FCIMPLEND

FCIMPL3(INT32, COMInterlocked::CompareExchange, INT32* location, INT32 value, INT32 comparand)
FCIMPL3(INT32, COMInterlocked::CompareExchange32, INT32* location, INT32 value, INT32 comparand)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedCompareExchange((LONG*)location, value, comparand);
}
FCIMPLEND
Expand All @@ -1530,10 +1518,6 @@ FCIMPL3_IVV(INT64, COMInterlocked::CompareExchange64, INT64* location, INT64 val
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedCompareExchange64((INT64*)location, value, comparand);
}
FCIMPLEND
Expand All @@ -1542,10 +1526,6 @@ FCIMPL2(LPVOID,COMInterlocked::ExchangeObject, LPVOID*location, LPVOID value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

LPVOID ret = InterlockedExchangeT(location, value);
#ifdef _DEBUG
Thread::ObjectRefAssign((OBJECTREF *)location);
Expand All @@ -1559,10 +1539,6 @@ FCIMPL3(LPVOID,COMInterlocked::CompareExchangeObject, LPVOID *location, LPVOID v
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

// <TODO>@todo: only set ref if is updated</TODO>
LPVOID ret = InterlockedCompareExchangeT(location, value, comparand);
if (ret == comparand) {
Expand All @@ -1579,10 +1555,6 @@ FCIMPL2(INT32,COMInterlocked::ExchangeAdd32, INT32 *location, INT32 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchangeAdd((LONG *) location, value);
}
FCIMPLEND
Expand All @@ -1591,10 +1563,6 @@ FCIMPL2_IV(INT64,COMInterlocked::ExchangeAdd64, INT64 *location, INT64 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchangeAdd64((INT64 *) location, value);
}
FCIMPLEND
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/vm/comutilnative.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,10 @@ extern "C" uint64_t QCALLTYPE GCInterface_GetGenerationBudget(int generation);
class COMInterlocked
{
public:
static FCDECL2(INT32, Exchange, INT32 *location, INT32 value);
static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value);
static FCDECL3(INT32, CompareExchange, INT32* location, INT32 value, INT32 comparand);
static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand);
static FCDECL2(INT32, Exchange32, INT32 *location, INT32 value);
static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value);
static FCDECL3(INT32, CompareExchange32, INT32* location, INT32 value, INT32 comparand);
static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand);
static FCDECL2(LPVOID, ExchangeObject, LPVOID* location, LPVOID value);
static FCDECL3(LPVOID, CompareExchangeObject, LPVOID* location, LPVOID value, LPVOID comparand);
static FCDECL2(INT32, ExchangeAdd32, INT32 *location, INT32 value);
Expand Down
16 changes: 8 additions & 8 deletions src/coreclr/vm/ecalllist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,14 +461,14 @@ FCFuncStart(gInteropMarshalFuncs)
FCFuncEnd()

FCFuncStart(gInterlockedFuncs)
FCFuncElementSig("Exchange", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::Exchange)
FCFuncElementSig("Exchange", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::Exchange64)
FCFuncElementSig("Exchange", &gsig_SM_RefObj_Obj_RetObj, COMInterlocked::ExchangeObject)
FCFuncElementSig("CompareExchange", &gsig_SM_RefInt_Int_Int_RetInt, COMInterlocked::CompareExchange)
FCFuncElementSig("CompareExchange", &gsig_SM_RefLong_Long_Long_RetLong, COMInterlocked::CompareExchange64)
FCFuncElementSig("CompareExchange", &gsig_SM_RefObj_Obj_Obj_RetObj, COMInterlocked::CompareExchangeObject)
FCFuncElementSig("ExchangeAdd", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::ExchangeAdd32)
FCFuncElementSig("ExchangeAdd", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::ExchangeAdd64)
FCFuncElement("Exchange32", COMInterlocked::Exchange32)
FCFuncElement("Exchange64", COMInterlocked::Exchange64)
FCFuncElement("ExchangeObject", COMInterlocked::ExchangeObject)
FCFuncElement("CompareExchange32", COMInterlocked::CompareExchange32)
FCFuncElement("CompareExchange64", COMInterlocked::CompareExchange64)
FCFuncElement("CompareExchangeObject", COMInterlocked::CompareExchangeObject)
FCFuncElement("ExchangeAdd32", COMInterlocked::ExchangeAdd32)
FCFuncElement("ExchangeAdd64", COMInterlocked::ExchangeAdd64)
FCFuncEnd()

FCFuncStart(gJitInfoFuncs)
Expand Down
Loading