Skip to content

Commit 8a57ce2

Browse files
stephentoubstephentoub
committed
Remove class constraint from Interlocked.{Compare}Exchange
Today `Interlocked.CompareExchange<T>` and `Interlocked.Exchange<T>` support only reference type `T`s. Now that we have corresponding {Compare}Exchange methods that support types of size 1, 2, 4, and 8, we can remove the constraint and support any `T` that's either a reference type, a primitive type, or an enum type, making the generic overload more useful and avoiding consumers needing to choose less-than-ideal types just because of the need for atomicity with Interlocked.{Compare}Exchange.
1 parent 5ab200c commit 8a57ce2

File tree

60 files changed

+920
-555
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

60 files changed

+920
-555
lines changed

‎src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs

Lines changed: 1 addition & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Diagnostics;
45
using System.Diagnostics.CodeAnalysis;
56
using System.Runtime.CompilerServices;
67
using System.Runtime.InteropServices;
@@ -102,21 +103,6 @@ public static long Exchange(ref long location1, long value)
102103
[return: NotNullIfNotNull(nameof(location1))]
103104
[MethodImpl(MethodImplOptions.InternalCall)]
104105
private static extern object? ExchangeObject([NotNullIfNotNull(nameof(value))] ref object? location1, object? value);
105-
106-
// The below whole method reduces to a single call to Exchange(ref object, object) but
107-
// the JIT thinks that it will generate more native code than it actually does.
108-
109-
/// <summary>Sets a variable of the specified type <typeparamref name="T"/> to a specified value and returns the original value, as an atomic operation.</summary>
110-
/// <param name="location1">The variable to set to the specified value.</param>
111-
/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
112-
/// <returns>The original value of <paramref name="location1"/>.</returns>
113-
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
114-
/// <typeparam name="T">The type to be used for <paramref name="location1"/> and <paramref name="value"/>. This type must be a reference type.</typeparam>
115-
[Intrinsic]
116-
[return: NotNullIfNotNull(nameof(location1))]
117-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
118-
public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? =>
119-
Unsafe.As<T>(Exchange(ref Unsafe.As<T, object?>(ref location1), value));
120106
#endregion
121107

122108
#region CompareExchange
@@ -183,29 +169,6 @@ public static long CompareExchange(ref long location1, long value, long comparan
183169
[MethodImpl(MethodImplOptions.InternalCall)]
184170
[return: NotNullIfNotNull(nameof(location1))]
185171
private static extern object? CompareExchangeObject(ref object? location1, object? value, object? comparand);
186-
187-
// Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces
188-
// the body of the following method with the following IL:
189-
// ldarg.0
190-
// ldarg.1
191-
// ldarg.2
192-
// call System.Threading.Interlocked::CompareExchange(ref Object, Object, Object)
193-
// ret
194-
// The workaround is no longer strictly necessary now that we have Unsafe.As but it does
195-
// have the advantage of being less sensitive to JIT's inliner decisions.
196-
197-
/// <summary>Compares two instances of the specified reference type <typeparamref name="T"/> for reference equality and, if they are equal, replaces the first one.</summary>
198-
/// <param name="location1">The destination, whose value is compared by reference with <paramref name="comparand"/> and possibly replaced.</param>
199-
/// <param name="value">The value that replaces the destination value if the comparison by reference results in equality.</param>
200-
/// <param name="comparand">The object that is compared by reference to the value at <paramref name="location1"/>.</param>
201-
/// <returns>The original value in <paramref name="location1"/>.</returns>
202-
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
203-
/// <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>
204-
[Intrinsic]
205-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
206-
[return: NotNullIfNotNull(nameof(location1))]
207-
public static T CompareExchange<T>(ref T location1, T value, T comparand) where T : class? =>
208-
Unsafe.As<T>(CompareExchange(ref Unsafe.As<T, object?>(ref location1), value, comparand));
209172
#endregion
210173

211174
#region Add

‎src/coreclr/nativeaot/System.Private.CoreLib/src/System/EventReporter.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ private static unsafe void ClrReportEvent(string eventSource, short type, ushort
171171
Interop.Advapi32.DeregisterEventSource(handle);
172172
}
173173

174-
private static byte s_once;
174+
private static bool s_once;
175175

176176
public static bool ShouldLogInEventLog
177177
{
@@ -180,7 +180,7 @@ public static bool ShouldLogInEventLog
180180
if (Interop.Kernel32.IsDebuggerPresent())
181181
return false;
182182

183-
if (s_once == 1 || Interlocked.Exchange(ref s_once, 1) == 1)
183+
if (s_once || Interlocked.Exchange(ref s_once, true))
184184
return false;
185185

186186
return true;

‎src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,6 @@ public static long CompareExchange(ref long location1, long value, long comparan
3636
#endif
3737
}
3838

39-
[Intrinsic]
40-
[return: NotNullIfNotNull(nameof(location1))]
41-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
42-
public static T CompareExchange<T>(ref T location1, T value, T comparand) where T : class?
43-
{
44-
return Unsafe.As<T>(CompareExchange(ref Unsafe.As<T, object?>(ref location1), value, comparand));
45-
}
46-
4739
[Intrinsic]
4840
[MethodImpl(MethodImplOptions.AggressiveInlining)]
4941
[return: NotNullIfNotNull(nameof(location1))]
@@ -92,16 +84,6 @@ public static long Exchange(ref long location1, long value)
9284
#endif
9385
}
9486

95-
[Intrinsic]
96-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
97-
[return: NotNullIfNotNull(nameof(location1))]
98-
public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class?
99-
{
100-
if (Unsafe.IsNullRef(ref location1))
101-
ThrowHelper.ThrowNullReferenceException();
102-
return Unsafe.As<T>(RuntimeImports.InterlockedExchange(ref Unsafe.As<T, object?>(ref location1), value));
103-
}
104-
10587
[Intrinsic]
10688
[MethodImpl(MethodImplOptions.AggressiveInlining)]
10789
[return: NotNullIfNotNull(nameof(location1))]

‎src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,22 +30,45 @@ public static MethodIL EmitIL(
3030
if (compilationModuleGroup.ContainsType(method.OwningType))
3131
#endif // READYTORUN
3232
{
33-
TypeDesc objectType = method.Context.GetWellKnownType(WellKnownType.Object);
34-
MethodDesc compareExchangeObject = method.OwningType.GetKnownMethod("CompareExchange",
35-
new MethodSignature(
36-
MethodSignatureFlags.Static,
37-
genericParameterCount: 0,
38-
returnType: objectType,
39-
parameters: new TypeDesc[] { objectType.MakeByRefType(), objectType, objectType }));
40-
41-
ILEmitter emit = new ILEmitter();
42-
ILCodeStream codeStream = emit.NewCodeStream();
43-
codeStream.EmitLdArg(0);
44-
codeStream.EmitLdArg(1);
45-
codeStream.EmitLdArg(2);
46-
codeStream.Emit(ILOpcode.call, emit.NewToken(compareExchangeObject));
47-
codeStream.Emit(ILOpcode.ret);
48-
return emit.Link(method);
33+
// Rewrite the generic Interlocked.CompareExchange<T> to be a call to one of the non-generic overloads.
34+
TypeDesc ceArgType = null;
35+
36+
TypeDesc tType = method.Instantiation[0];
37+
if (!tType.IsValueType)
38+
{
39+
ceArgType = method.Context.GetWellKnownType(WellKnownType.Object);
40+
}
41+
else if (tType.IsPrimitive || tType.IsEnum)
42+
{
43+
int size = tType.GetElementSize().AsInt;
44+
Debug.Assert(size is 1 or 2 or 4 or 8);
45+
ceArgType = size switch
46+
{
47+
1 => method.Context.GetWellKnownType(WellKnownType.Byte),
48+
2 => method.Context.GetWellKnownType(WellKnownType.UInt16),
49+
4 => method.Context.GetWellKnownType(WellKnownType.UInt32),
50+
_ => method.Context.GetWellKnownType(WellKnownType.UInt64),
51+
};
52+
}
53+
54+
if (ceArgType is not null)
55+
{
56+
MethodDesc compareExchangeNonGeneric = method.OwningType.GetKnownMethod("CompareExchange",
57+
new MethodSignature(
58+
MethodSignatureFlags.Static,
59+
genericParameterCount: 0,
60+
returnType: ceArgType,
61+
parameters: [ceArgType.MakeByRefType(), ceArgType, ceArgType]));
62+
63+
ILEmitter emit = new ILEmitter();
64+
ILCodeStream codeStream = emit.NewCodeStream();
65+
codeStream.EmitLdArg(0);
66+
codeStream.EmitLdArg(1);
67+
codeStream.EmitLdArg(2);
68+
codeStream.Emit(ILOpcode.call, emit.NewToken(compareExchangeNonGeneric));
69+
codeStream.Emit(ILOpcode.ret);
70+
return emit.Link(method);
71+
}
4972
}
5073
}
5174

‎src/coreclr/vm/corelib.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,10 @@ DEFINE_METHOD(MEMORY_MARSHAL, GET_ARRAY_DATA_REFERENCE_MDARRAY, GetArrayDa
694694
DEFINE_CLASS(INTERLOCKED, Threading, Interlocked)
695695
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_T, CompareExchange, GM_RefT_T_T_RetT)
696696
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_OBJECT,CompareExchange, SM_RefObject_Object_Object_RetObject)
697+
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_BYTE, CompareExchange, SM_RefByte_Byte_Byte_RetByte)
698+
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_USHRT, CompareExchange, SM_RefUShrt_UShrt_UShrt_RetUShrt)
699+
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_UINT, CompareExchange, SM_RefUInt_UInt_UInt_RetUInt)
700+
DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_ULONG, CompareExchange, SM_RefULong_ULong_ULong_RetULong)
697701

698702
DEFINE_CLASS(RAW_DATA, CompilerServices, RawData)
699703
DEFINE_FIELD(RAW_DATA, DATA, Data)

‎src/coreclr/vm/jitinterface.cpp

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7239,8 +7239,34 @@ bool getILIntrinsicImplementationForInterlocked(MethodDesc * ftn,
72397239
if (ftn->GetMemberDef() != CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_T)->GetMemberDef())
72407240
return false;
72417241

7242-
// Get MethodDesc for non-generic System.Threading.Interlocked.CompareExchange()
7243-
MethodDesc* cmpxchgObject = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_OBJECT);
7242+
// Determine the type of the generic T method parameter
7243+
_ASSERTE(ftn->HasMethodInstantiation());
7244+
_ASSERTE(ftn->GetNumGenericMethodArgs() == 1);
7245+
TypeHandle typeHandle = ftn->GetMethodInstantiation()[0];
7246+
MethodTable* methodTable = typeHandle.GetMethodTable();
7247+
7248+
MethodDesc* cmpxchg;
7249+
7250+
// Based on the generic method parameter, determine which overload of CompareExchange
7251+
// to delegate to, or if we can't handle the type at all.
7252+
if (!typeHandle.IsValueType())
7253+
{
7254+
cmpxchg = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_OBJECT);
7255+
}
7256+
else if (CorTypeInfo::IsPrimitiveType(typeHandle.GetVerifierCorElementType()))
7257+
{
7258+
UINT size = typeHandle.GetSize();
7259+
_ASSERTE(size == 1 || size == 2 || size == 4 || size == 8);
7260+
cmpxchg =
7261+
size == 1 ? CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_BYTE) :
7262+
size == 2 ? CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_USHRT) :
7263+
size == 4 ? CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_UINT) :
7264+
CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_ULONG);
7265+
}
7266+
else
7267+
{
7268+
return false;
7269+
}
72447270

72457271
// Setup up the body of the method
72467272
static BYTE il[] = {
@@ -7251,12 +7277,12 @@ bool getILIntrinsicImplementationForInterlocked(MethodDesc * ftn,
72517277
CEE_RET
72527278
};
72537279

7254-
// Get the token for non-generic System.Threading.Interlocked.CompareExchange(), and patch [target]
7255-
mdMethodDef cmpxchgObjectToken = cmpxchgObject->GetMemberDef();
7256-
il[4] = (BYTE)((int)cmpxchgObjectToken >> 0);
7257-
il[5] = (BYTE)((int)cmpxchgObjectToken >> 8);
7258-
il[6] = (BYTE)((int)cmpxchgObjectToken >> 16);
7259-
il[7] = (BYTE)((int)cmpxchgObjectToken >> 24);
7280+
// Get the token for the relevant System.Threading.Interlocked.CompareExchange overload, and patch [target]
7281+
mdMethodDef cmpxchgToken = cmpxchg->GetMemberDef();
7282+
il[4] = (BYTE)((int)cmpxchgToken >> 0);
7283+
il[5] = (BYTE)((int)cmpxchgToken >> 8);
7284+
il[6] = (BYTE)((int)cmpxchgToken >> 16);
7285+
il[7] = (BYTE)((int)cmpxchgToken >> 24);
72607286

72617287
// Initialize methInfo
72627288
methInfo->ILCode = const_cast<BYTE*>(il);

‎src/coreclr/vm/metasig.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,10 @@ DEFINE_METASIG_T(SM(RefDec_RetVoid, r(g(DECIMAL)), v))
586586

587587
DEFINE_METASIG(GM(RefT_T_T_RetT, IMAGE_CEE_CS_CALLCONV_DEFAULT, 1, r(M(0)) M(0) M(0), M(0)))
588588
DEFINE_METASIG(SM(RefObject_Object_Object_RetObject, r(j) j j, j))
589+
DEFINE_METASIG(SM(RefByte_Byte_Byte_RetByte, r(b) b b, b))
590+
DEFINE_METASIG(SM(RefUShrt_UShrt_UShrt_RetUShrt, r(H) H H, H))
591+
DEFINE_METASIG(SM(RefUInt_UInt_UInt_RetUInt, r(K) K K, K))
592+
DEFINE_METASIG(SM(RefULong_ULong_ULong_RetULong, r(L) L L, L))
589593

590594
DEFINE_METASIG_T(SM(RefCleanupWorkListElement_RetVoid, r(C(CLEANUP_WORK_LIST_ELEMENT)), v))
591595
DEFINE_METASIG_T(SM(RefCleanupWorkListElement_SafeHandle_RetIntPtr, r(C(CLEANUP_WORK_LIST_ELEMENT)) C(SAFE_HANDLE), I))

‎src/libraries/Common/src/System/Net/StreamBuffer.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -292,15 +292,15 @@ private sealed class ResettableValueTaskSource : IValueTaskSource
292292

293293
private ManualResetValueTaskSourceCore<bool> _waitSource; // mutable struct, do not make this readonly
294294
private CancellationTokenRegistration _waitSourceCancellation;
295-
private int _hasWaiter;
295+
private bool _hasWaiter;
296296

297297
ValueTaskSourceStatus IValueTaskSource.GetStatus(short token) => _waitSource.GetStatus(token);
298298

299299
void IValueTaskSource.OnCompleted(Action<object?> continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _waitSource.OnCompleted(continuation, state, token, flags);
300300

301301
void IValueTaskSource.GetResult(short token)
302302
{
303-
Debug.Assert(_hasWaiter == 0);
303+
Debug.Assert(!_hasWaiter);
304304

305305
// Clean up the registration. This will wait for any in-flight cancellation to complete.
306306
_waitSourceCancellation.Dispose();
@@ -312,7 +312,7 @@ void IValueTaskSource.GetResult(short token)
312312

313313
public void SignalWaiter()
314314
{
315-
if (Interlocked.Exchange(ref _hasWaiter, 0) == 1)
315+
if (Interlocked.Exchange(ref _hasWaiter, false))
316316
{
317317
_waitSource.SetResult(true);
318318
}
@@ -322,21 +322,21 @@ private void CancelWaiter(CancellationToken cancellationToken)
322322
{
323323
Debug.Assert(cancellationToken.IsCancellationRequested);
324324

325-
if (Interlocked.Exchange(ref _hasWaiter, 0) == 1)
325+
if (Interlocked.Exchange(ref _hasWaiter, false))
326326
{
327327
_waitSource.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken)));
328328
}
329329
}
330330

331331
public void Reset()
332332
{
333-
if (_hasWaiter != 0)
333+
if (_hasWaiter)
334334
{
335335
throw new InvalidOperationException("Concurrent use is not supported");
336336
}
337337

338338
_waitSource.Reset();
339-
Volatile.Write(ref _hasWaiter, 1);
339+
Volatile.Write(ref _hasWaiter, true);
340340
}
341341

342342
public void Wait()

‎src/libraries/Common/tests/System/Net/WebSockets/WebSocketStream.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ public class WebSocketStream : Stream
2121
// Used by the class to indicate that the stream is writable.
2222
private bool _writeable;
2323

24-
// Whether Dispose has been called. 0 == false, 1 == true
25-
private int _disposed;
24+
// Whether Dispose has been called.
25+
private bool _disposed;
2626

2727
public WebSocketStream(WebSocket socket)
2828
: this(socket, FileAccess.ReadWrite, ownsSocket: false)
@@ -140,7 +140,7 @@ public void Close(int timeout)
140140

141141
protected override void Dispose(bool disposing)
142142
{
143-
if (Interlocked.Exchange(ref _disposed, 1) != 0)
143+
if (Interlocked.Exchange(ref _disposed, true))
144144
{
145145
return;
146146
}
@@ -269,7 +269,7 @@ public override void SetLength(long value)
269269

270270
private void ThrowIfDisposed()
271271
{
272-
ObjectDisposedException.ThrowIf(_disposed != 0, this);
272+
ObjectDisposedException.ThrowIf(_disposed, this);
273273
}
274274

275275
private static IOException WrapException(string resourceFormatString, Exception innerException)

0 commit comments

Comments
 (0)