Runtime: Proposal: Hide Thread.VolatileRead and Thread.VolatileWrite

[carlreinke]

Rationale

Thread.VolatileRead and Thread.VolatileWrite delegate to Volatile.Read and Volatile.Write, but it is an incomplete delegation in that Volatile has support for bool whereas Thread does not.

I always reach for Thread.VolatileRead before Volatile.Read. Hiding it would help break this habit.

Proposal

 namespace System.Threading
 {
     public sealed class Thread : CriticalFinalizerObject
     {
         ...
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static byte VolatileRead(ref byte address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static double VolatileRead(ref double address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static short VolatileRead(ref short address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static int VolatileRead(ref int address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static long VolatileRead(ref long address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static IntPtr VolatileRead(ref IntPtr address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static object VolatileRead(ref object address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static sbyte VolatileRead(ref sbyte address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static float VolatileRead(ref float address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static ushort VolatileRead(ref ushort address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static uint VolatileRead(ref uint address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static ulong VolatileRead(ref ulong address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static UIntPtr VolatileRead(ref UIntPtr address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref byte address, byte value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref double address, double value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref short address, short value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref int address, int value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref long address, long value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref IntPtr address, IntPtr value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref object address, object value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref sbyte address, sbyte value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref float address, float value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref ushort address, ushort value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref uint address, uint value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref ulong address, ulong value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref UIntPtr address, UIntPtr value) => Volatile.Write(ref address, value);
         ...
     }
 }

[carlreinke]

cc: @kouvel

[carlreinke]

Is there something I need to do to get someone to take a look at this? It ought to be a simple thumbs up or down.

[danmosemsft]

Cc @kouvel

This does not seem a compelling reason on the face of it. It can cause more confusion eg if you are used to using these methods and no longer see them in Intellisense. I doubt we would change this. You might consider a Roslyn analyzer or simple grep script.

[carlreinke]

Would a proposal to add Thread.VolatileRead and Thread.VolatileWrite overloads for bool be more acceptable?

If a developer discovers Thread.VolatileRead/Thread.VolatileWrite before the Volatile class then they're going to conclude that volatile operations can't be performed on bool, as was the case for a long time in Framework.

[kouvel]

I would prefer to deprecate the Thread versions over adding new overloads there. They are effectively deprecated APIs. @stephentoub, what is the proper way to deprecate APIs in .NET Core? Hiding them in the editor seems reasonable to me since we would like people to switch over to the Volatile versions.

[stephentoub]

We generally don't Obsolete things; there are a variety of problems with that.

@terrajobst has been exploring an alternate analyzer-based mechanism; he can provide more details.

[carlreinke]

Can this be ready-for-review or does something else need to happen first?

[terrajobst]

Video

• This seems like a good candidate for a code fixer that replaces the call site.
• Usage of these APIs is low.
• We should consider this API in our plan for obsoletion.
• Hiding these APIs seems fine.

[sharwell]

I would love to see a volatile read with in parameters. I don't think either Thread or Volatile offers these today.

[stephentoub]

I don't see the benefit of hiding these. Why is it valuable, @terrajobst?

[sharwell]

I also lean against hiding these. It would be preferable to allow someone to reference them, followed by a diagnostic analyzer reporting the alternative and providing a code fix for it.

[danmosemsft]

Why not just add the bool overloads? Is it a simple oversight that there are all the others?

[paulomorgado]

@sharwell, I would mix both. The obsolete attribute could have an overload with the replacement API and a generic analyzer/fixer for this attribute would offer the proper fix.

[stephentoub]

Moving back to ready for review to discuss again.

[alexrp]

I would just like to add some arguments in favor of hiding and obsoleting these methods. (The linked video is currently failing to play for me, so I am not sure if these have been brought up already.)

---

There is an important bit of history that needs to be considered. Before .NET Core was a thing, these methods were implemented differently on .NET Framework. They used to just call MemoryBarrier and read or write the value from the given location in pure C# code.

So, why was that implementation problematic?

In Ecma 335 §I.12.6.5, it is mentioned:

Volatile reads and writes. The CIL includes a prefix, volatile., that specifies that
the subsequent operation is to be performed with the cross-thread visibility constraints
described in §I.12.6.7. In addition, the class library provides methods to perform
explicit volatile reads (System.Thread.VolatileRead) and writes
(System.Thread.VolatileWrite), as well as barrier synchronization
(System.Thread.MemoryBarrier).

Next, §I.12.6.7 says:

The volatile. prefix on certain instructions shall guarantee cross-thread memory ordering rules.
They do not provide atomicity, other than that guaranteed by the specification of §I.12.6.6.

A volatile read has “acquire semantics” meaning that the read is guaranteed to occur prior to any
references to memory that occur after the read instruction in the CIL instruction sequence. A
volatile write has “release semantics” meaning that the write is guaranteed to happen after any
memory references prior to the write instruction in the CIL instruction sequence. 

As I mentioned above, the way these methods were implemented actually resulted in stronger ordering semantics. So users quite possibly came to rely on those semantics, even though they're not equivalent to what Ecma 335 claims.

Let's address atomicity next. The above section says that volatile reads/writes are not necessarily atomic. It would make sense that this applies to these methods as well, considering that the .NET Framework implementation linked above is indeed not atomic. Only the 32-bit overloads are atomic as a result of §I.12.6.6:

A conforming CLI shall guarantee that read and write access to properly aligned memory
locations no larger than the native word size (the size of type native int) is atomic
(see §I.12.6.2) when all the write accesses to a location are the same size. Atomic writes shall
alter no bits other than those written. Unless explicit layout control (see
Partition II (Controlling Instance Layout)) is used to alter the default behavior, data elements no
larger than the natural word size (the size of a native int) shall be properly aligned. Object
references shall be treated as though they are stored in the native word size.

But, unfortunately, things get murky when we start to look at MSDN documentation:

On a multiprocessor system, VolatileRead obtains the very latest value written to a memory location by any processor. This might require flushing processor caches.

Consider the case of using the 64-bit overloads of these methods on a 32-bit system. Memory operations involving 64-bit quantities will often be implemented as two 32-bit operations on such platforms since, depending on architecture version, there simply might not be any instructions available to do 64-bit loads/stores. In this case, a simple non-atomic read of a 64-bit quantity could obtain a garbled value because another thread has just written the first half of the value to the location.

I think it's safe to say that most people reading this text would conclude that these methods are atomic. The situation I just described could not reasonably be said to have obtained the "very latest value" in any useful sense. At the very least, if one wants to argue otherwise, I think one would have to concede that the documentation should have mentioned the 32-bit on 64-bit issue.

The MSDN documentation actually commits another offense:

In C#, using the volatile modifier on a field guarantees that all access to that field uses VolatileRead or VolatileWrite.

This is just flat out wrong. The C# volatile keyword compiles to the CIL volatile. prefix opcode which, as we've established, does not have the same memory ordering semantics as these methods. This further adds to the pile of confusion.

In .NET Core today, these methods are forwarded to the Volatile class. Even though I actually think this is the way they should have worked from day one, this makes the situation even worse. The Volatile class actually provides proper acquire/release semantics. This means that users who came to rely on the stronger barriers these methods had in the .NET Framework will be in for a not-fun surprise when they migrate to .NET Core. (But at least they'll get proper atomicity for the 64-bit overloads. Silver lining? 😄)

---

In conclusion, the semantics of these methods were never clear. The documentation and implementation are all over the place. Between all the possible expectations when using them, no one can really say what a user can, will, and should expect. Therefore, to my mind, these methods have been sufficiently poisoned that it makes no sense to even passively encourage their use any longer. There are better replacements with clearer semantics, both in terms of documentation and actual implementation. These methods have to go. (And someone should probably consider what to do about the .NET Framework/.NET Core incompatibility.)

---

Side note: I was trying to find out when/why .NET Core started forwarding these methods to Volatile, but this required me to go back to the old coreclr/corefx repos... which have now been archived, with all history seemingly removed. That's not great.

[stephentoub]

Side note: I was trying to find out when/why .NET Core started forwarding these methods to Volatile, but this required me to go back to the old coreclr/corefx repos... which have now been archived, with all history seemingly removed. That's not great.

History was not removed. a) All of the history is in this repo, and b) in the archived repos you just need to look at the right branch, e.g. release/3.1.

[alexrp]

Thanks, I hadn't noticed the release branches were still there.

FWIW, the incompatibility seems to have been introduced in 5cfbd066343338c696b4a5345c7bd188c87b0096 when Thread was moved over to CoreFX. Prior to that, it still had the .NET Framework-compatible implementations in CoreCLR (as seen in 0d99e34c932ba91440665335e8e7d48260c4e0be).

Worth noting that a comment in Volatile's source code used to say that this change should not have happened: https://github.com/dotnet/runtime/commit/3be954ff66a5e9102286f126d407a9a6a20434a8#diff-81ab4bfc4390e265641b170c145aa347L21

    // (We cannot change the implementations of Thread.VolatileRead/VolatileWrite without breaking code
    // that relies on their overly-strong ordering guarantees.)

There's unfortunately not enough context in any of these commits to say why the change did happen.

[bartonjs]

Approved as marking as obsolete and EditorBrowsable(Never), contingent upon first creating the fixer to rewrite the calls automatically.

 namespace System.Threading
 {
     public sealed class Thread : CriticalFinalizerObject
     {
         ...
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static byte VolatileRead(ref byte address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static double VolatileRead(ref double address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static short VolatileRead(ref short address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static int VolatileRead(ref int address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static long VolatileRead(ref long address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static IntPtr VolatileRead(ref IntPtr address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static object VolatileRead(ref object address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static sbyte VolatileRead(ref sbyte address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static float VolatileRead(ref float address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static ushort VolatileRead(ref ushort address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static uint VolatileRead(ref uint address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static ulong VolatileRead(ref ulong address) => Volatile.Read(ref address);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static UIntPtr VolatileRead(ref UIntPtr address) => Volatile.Read(ref address);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref byte address, byte value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref double address, double value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref short address, short value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref int address, int value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref long address, long value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref IntPtr address, IntPtr value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref object address, object value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref sbyte address, sbyte value) => Volatile.Write(ref address, value);
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref float address, float value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref ushort address, ushort value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref uint address, uint value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref ulong address, ulong value) => Volatile.Write(ref address, value);
         [CLSCompliant(false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public static void VolatileWrite(ref UIntPtr address, UIntPtr value) => Volatile.Write(ref address, value);
         ...
     }
 }

[GSPP]

Did the historic versions really call MemoryBarrier? I remember seeing that call in the source code but I also seem to recall that they really were JIT intrinsics and the source code body was never called.

[stephentoub]

Did the historic versions really call MemoryBarrier? I remember seeing that call in the source code but I also seem to recall that they really were JIT intrinsics and the source code body was never called.

If you look at the disassembly for VolatileRead, e.g. as I've done here with benchmarkdotnet's disassembly diagnoser:
```C#
[Benchmark]
[Arguments(1)]
public int M(int i) => Thread.VolatileRead(ref i);

you'll see something like this:
```assembly
; .NET Framework 4.8 (4.8.4200.0), X64 RyuJIT

; Program.M(Int32)
       sub       rsp,28
       mov       [rsp+38],edx
       lea       rcx,[rsp+38]
       call      System.Threading.Thread.VolatileRead(Int32 ByRef)
       nop
       add       rsp,28
       ret
; Total bytes of code 24

; System.Threading.Thread.VolatileRead(Int32 ByRef)
       nop       dword ptr [rax+rax]
       mov       eax,[rcx]
       lock or   dword ptr [rsp],0
       ret
; Total bytes of code 13

so while there's no actual call to Thread.MemoryBarrier, there's still a lock or in there (and Thread.VolatileRead doesn't get inlined).

In contrast, changing the Thread.VolatileRead to Volatile.Read, you'll see something like this:

; ## .NET Framework 4.8 (4.8.4200.0), X64 RyuJIT

; Program.M(Int32)
       mov       eax,edx
       ret
; Total bytes of code 3

[stephentoub]

@bartonjs, @terrajobst, the approval information says "Approved as marking as obsolete and EditorBrowsable(Never)", but the API diff only shows EditorBrowsable(Never), not Obsolete and what diagnostic ID should be used with it, etc. Which is correct, the text or the API diff? Presumably the intent is to have Obsolete with a unique diagnostic ID that can be used to trigger a fixer?

[gfoidl]

to Volatile.Read, you'll see something like this:

Hm, for .NET 5.0 the stack is used. So the JIT from .NET 4.8 produces better code?

; ## .NET Core 5.0.0 (CoreCLR 5.0.20.36411, CoreFX 5.0.20.36411), X64 RyuJIT
; ConsoleApp2.Program.M(Int32)
       mov       [rsp+10],edx
       mov       eax,[rsp+10]
       ret
; Total bytes of code 9

[stephentoub]

Hm, for .NET 5.0 the stack is used. So the JIT from .NET 4.8 produces better code?

Potentially (@AndyAyersMS), but my example was also not particularly realistic: there's really no reason to use Volatile.Read like this on a parameter. I expect if you changed the code to, for example, access a static field instead, that the extra mov wouldn't be there.

[gfoidl]

Yep, looks as expected (for static and instance field)

; ConsoleApp2.Program.StaticField()
       mov       eax,[7FFC5EB13914]
       ret
; Total bytes of code 7

; ConsoleApp2.Program.InstanceField()
       add       rcx,8
       mov       eax,[rcx]
       ret
; Total bytes of code 7

As the code with reading the argument itself isn't realistic, I'd say there is no action to be taken on this.

[bartonjs]

Presumably the intent is to have Obsolete with a unique diagnostic ID that can be used to trigger a fixer?

Yeah. I think that in June we didn't have a 100% solid plan on new obsoletions yet, so I didn't have anything to include in the ref code snippet.

[AndyAyersMS]

Hm, for .NET 5.0 the stack is used. So the JIT from .NET 4.8 produces better code?

Yes, we're arguably too conservative now when we see some volatile accesses (the V flag on the IND node below):

LocalAddressVisitor visiting statement:
STMT00001 (IL   ???...  ???)
               [000004] --C---------              *  RETURN    int   
               [000012] V--XGO-N----              \--*  IND       int   
               [000010] ------------                 \--*  ADDR      long  
               [000011] -------N----                    \--*  LCL_VAR   int    V01 arg1         

Local V01 should not be enregistered because: it is address exposed

This bit of code could also be checking that the base of the indirection is not on the stack.
https://github.com/dotnet/runtime/blob/de6f9114a54587ea1c3e13c1aba2dbaff3c39fc5/src/coreclr/src/jit/lclmorph.cpp#L487-L492

cc @dotnet/jit-contrib